Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Download all resources before installation #1666

Closed
alexchandel opened this issue Dec 13, 2016 · 8 comments
Closed

Download all resources before installation #1666

alexchandel opened this issue Dec 13, 2016 · 8 comments
Labels
outdated PR was locked due to age stale No recent activity

Comments

@alexchandel
Copy link

After spending 30 minutes building wine 2.0-rc1, my network connection fluctuated briefly and the curl of the gecko package failed. Homebrew then decided the entire build failed and threw it out. Now I need to spend another 30 minutes rebuilding what I just had.

This shouldn't happen. Homebrew should be able to either: retry a broken curl; resume a build where it left off; or ignore the curl error and skip the resource.

@MikeMcQuaid
Copy link
Member

I agree. I think we should at the very least retry curl automatically or perhaps ensure all resources are downloaded before installation, even if they aren't actually used.

@MikeMcQuaid MikeMcQuaid added the help wanted We want help addressing this label Dec 18, 2016
@squarepantz
Copy link

squarepantz commented Dec 29, 2016

@alexchandel
Actually, Homebrew does retry urls, and it does so at two different levels.
First, at curl call with resume option "-C".

  def _fetch
    url = @url

    if ENV["HOMEBREW_ARTIFACT_DOMAIN"]
      url = url.sub(%r{^((ht|f)tps?://)?}, ENV["HOMEBREW_ARTIFACT_DOMAIN"].chomp("/") + "/")
      ohai "Downloading from #{url}"
    end

    urls = actual_urls(url)
    unless urls.empty?
      ohai "Downloading from #{urls.last}"
      if !ENV["HOMEBREW_NO_INSECURE_REDIRECT"].nil? && url.start_with?("https://") &&
         urls.any? { |u| !u.start_with? "https://" }
        puts "HTTPS to HTTP redirect detected & HOMEBREW_NO_INSECURE_REDIRECT is set."
        raise CurlDownloadStrategyError, url
      end
      url = urls.last
    end

    curl url, "-C", downloaded_size, "-o", temporary_path
  end

Second, retry once if _fetch failed, unless it's due to resume error (exit code 33).

  def fetch
    ohai "Downloading #{@url}"

    if cached_location.exist?
      puts "Already downloaded: #{cached_location}"
    else
      had_incomplete_download = temporary_path.exist?
      begin
        _fetch
      rescue ErrorDuringExecution
        # 33 == range not supported
        # try wiping the incomplete download and retrying once
        unless $?.exitstatus == 33 && had_incomplete_download
          raise CurlDownloadStrategyError, @url
        end

        ohai "Trying a full download"
        temporary_path.unlink
        had_incomplete_download = false
        retry
      end
      ignore_interrupts { temporary_path.rename(cached_location) }
    end
  rescue CurlDownloadStrategyError
    raise if mirrors.empty?
    puts "Trying a mirror..."
    @url = mirrors.shift
    retry
  end

I think that's exactly what happened to you, curl tried to resume but couldn't because the Gecko server didn't support resume, and then, Homebrew decided not to retry because curl exited with 33. So the issue really is whether Homebrew should support retry on servers that don't support resume.

@MikeMcQuaid
Copy link
Member

There's been on activity on this for over a year so going to close this out.

@ghost ghost removed the help wanted We want help addressing this label Jun 1, 2018
@lock lock bot added the outdated PR was locked due to age label Jul 1, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jul 1, 2018
@MikeMcQuaid MikeMcQuaid reopened this Feb 16, 2019
@Homebrew Homebrew unlocked this conversation Feb 16, 2019
@MikeMcQuaid MikeMcQuaid changed the title Build fails completely when peripheral post-build resources fail to download Download all resources before installation Feb 16, 2019
@orgh0
Copy link

orgh0 commented Feb 23, 2019

The install_dependency method currently creates an object of the FormulaInstaller class for the dependency and installs it.

Ideas:

  • A download_dependencies method that computes all dependencies recursively and downloads the files and stores them somewhere(I'm not sure what is the right place to be storing the downloaded bottles, comments?). This method would be a part of the FormulaInstaller Class.

In the install method of FormulaInstaller, a change similar to this:

unless ignore_deps?
      deps = compute_dependencies
      check_dependencies_bottled(deps) if pour_bottle? && !DevelopmentTools.installed?
      downloaded_path = download_dependencies(deps)
      install_dependencies(dowloaded_path)
    end

Hence a call to the install method for the FormulaInstaller would:

  • first, compute all dependencies: compute_depency
  • download them download_dependency
  • Install them using their downloaded paths (Not sure if we should make instances of FormulaInstaller for this, FormulaInstaller.install would recurse)
  • call the pour method to install the formula in concern.

Thoughts?

@MikeMcQuaid
Copy link
Member

@arghyatiger The files are already stored in $(brew --cache). You can examine the behaviour of brew fetch; that's what you should aim to replicate.

@orgh0
Copy link

orgh0 commented Feb 24, 2019

@MikeMcQuaid

Just looked at brew fetch, It doesn't seem to download the dependencies though.
In that case, something like this should suffice?

-> Compute all dependencies for a package A.
-> brew fetch dependencies, (not run the command, but implement similar functionality)
-> FormulaInstaller.install method needn't go through changes as it detects the bottles stored in the cache

I'll try to make a PR next.
Any suggestions/changes to the idea?

@MikeMcQuaid
Copy link
Member

@arghyatiger Yep, sounds right. Probably want to put the behaviour behind an option or ENV until it's ready to be the default. Look forward to the PR!

@stale
Copy link

stale bot commented Mar 17, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale No recent activity label Mar 17, 2019
@stale stale bot closed this as completed Mar 24, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Apr 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age stale No recent activity
Projects
None yet
Development

No branches or pull requests

4 participants