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

Skip build deps to avoid downloading bottles #13065

Merged
merged 2 commits into from
Apr 22, 2022

Conversation

xxyzz
Copy link
Contributor

@xxyzz xxyzz commented Mar 30, 2022

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

This should fixes #13040.

Dependency.to_formula() in the following line will cause a
CoreTapFormulaUnavailableError then download all dependency bottles in FormulaInstaller.verfi_deps_exist()
Comment on lines +115 to +116
# avoid downloading build dependency bottles
next if dep.build? && dependent.pour_bottle? && Homebrew::EnvConfig.install_from_api?
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

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

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

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

then at this line to_formula will cause an error

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

then start downloading bottle and retry

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

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.

@github-actions
Copy link

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

@github-actions github-actions bot added the stale No recent activity label Apr 22, 2022
@xxyzz
Copy link
Contributor Author

xxyzz commented Apr 22, 2022

Could this pull request be merged as a temporary fix?

@github-actions github-actions bot removed the stale No recent activity label Apr 22, 2022
Copy link
Member

@Bo98 Bo98 left a comment

Choose a reason for hiding this comment

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

Yes, indeed. Thanks! Sorry for forgetting to merge this!

@Bo98 Bo98 merged commit b2a896e into Homebrew:master Apr 22, 2022
@xxyzz xxyzz deleted the skip_build_deps branch April 22, 2022 00:47
@github-actions github-actions bot added the outdated PR was locked due to age label May 23, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 23, 2022
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

brew install downloads build dependencies if $HOMEBREW_INSTALL_FROM_API is set
3 participants