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.

Sign up for GitHub

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

Jump to bottom

Skip build deps to avoid downloading bottles #13065

Merged
merged 2 commits into from
Apr 22, 2022
Merged

Skip build deps to avoid downloading bottles #13065

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
193d6d3
Skip build dependencies to avoid downloading bottles
xxyzz Mar 30, 2022
990b612
Get gcc version from API to avoid downloading gcc bottles on Linux
xxyzz Mar 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Library/Homebrew/dependency.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,8 @@ def expand(dependent, deps = dependent.deps, cache_key: nil, ignore_missing: fal

deps.each do |dep|
next if dependent.name == dep.name
# avoid downloading build dependency bottles
next if dep.build? && dependent.pour_bottle? && Homebrew::EnvConfig.install_from_api?
Comment on lines +115 to +116
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems odd to me that it only happens in the fetch stage and only for API installs. Something must be done differently somewhere for this inconsistency to exist.

Copy link
Contributor Author

@xxyzz xxyzz Mar 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the correct fix should be creating the Formula object from json directly instead of from the downloaded bottle file IIUC, but that needs more work as the formula json API doesn't have any Linux dependencies.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah so #12936 basically.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, exactly. Should I remove the first commit or close this pull request?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could cherry-pick #12936 into this PR and address the feedback from there here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with at least the second commit as a short-term fix - it's consistent to what we have elsewhere.

I wouldn't object to the first commit - but I feel like there's something wrong elsewhere if it doesn't happen at install time and only at fetch time.

Copy link
Contributor Author

@xxyzz xxyzz Mar 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dependent.deps at here contains all the dependencies including build dependencies, this happens also when core tap is cloned

brew/Library/Homebrew/dependency.rb

Line 101 in b533b38

def expand(dependent, deps = dependent.deps, cache_key: nil, ignore_missing: false, &block)

then at this line to_formula will cause an error

brew/Library/Homebrew/dependency.rb

Line 128 in b533b38

expanded_deps.concat(expand(dep.to_formula, cache_key: cache_key, ignore_missing: ignore_missing, &block))

then start downloading bottle and retry

brew/Library/Homebrew/formula_installer.rb

Lines 221 to 228 in b533b38

def verify_deps_exist
begin
compute_dependencies
rescue CoreTapFormulaUnavailableError => e
raise unless Homebrew::API::Bottle.available? e.name
Homebrew::API::Bottle.fetch_bottles(e.name)
retry

I'd like to apply the commit from the draft pr and work on that. It's just these json files lack Linux dependencies, should work fine on mac.

But why not download the zip file of homebrew-core and use that as if the repo is cloned? It's about the same size of formula.json(14M).

Copy link
Member

@Bo98 Bo98 Mar 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok that makes sense. I'm fine with the PR then as a fix until we can get rid of the API fetch layer everywhere.

In regards to Linux dependencies, there's an issue open for that: Homebrew/formulae.brew.sh#566. It is likely a prerequisite before #12936 can ship.


case action(dependent, dep, ignore_missing: ignore_missing, &block)
when :prune
Expand Down
11 changes: 9 additions & 2 deletions Library/Homebrew/extend/os/linux/keg_relocate.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,9 +84,16 @@ def elf_files
def self.bottle_dependencies
@bottle_dependencies ||= begin
formulae = relocation_formulae
gcc = Formulary.factory(CompilerSelector.preferred_gcc)
if Homebrew::EnvConfig.install_from_api?
gcc_hash = Homebrew::API::Formula.fetch(CompilerSelector.preferred_gcc)
preferred_gcc_version = Version.new gcc_hash["versions"]["stable"]
else
gcc = Formulary.factory(CompilerSelector.preferred_gcc)
preferred_gcc_version = gcc.version
end
if !Homebrew::EnvConfig.simulate_macos_on_linux? &&
DevelopmentTools.non_apple_gcc_version("gcc") < gcc.version.to_i
DevelopmentTools.non_apple_gcc_version("gcc") < preferred_gcc_version
gcc = Formulary.factory(CompilerSelector.preferred_gcc) if Homebrew::EnvConfig.install_from_api?
formulae += gcc.recursive_dependencies.map(&:name)
formulae << gcc.name
end
Expand Down