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

Make formula upgrades more liberal based on bottle #15927

Merged
merged 2 commits into from
Sep 3, 2023
Merged

Make formula upgrades more liberal based on bottle #15927

merged 2 commits into from
Sep 3, 2023

Conversation

MikeMcQuaid
Copy link
Member

When we're installing a formula from a bottle, we currently always upgrade all dependencies in the dependency tree to be safe.

However, if we're installing a bottle and the runtime_dependencies within that bottle's tab all have older or equal versions to those already installed: we do not need to upgrade these dependencies.

This should help a lot of upgrading a lot of the time, at least for users using bottles (which is the huge majority).

The only downside or other noticeable change is that this requires us to download or attempt to download the bottle tab before we compute the dependencies at installation time.

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.

Thanks!

Copy link
Contributor

@apainintheneck apainintheneck left a comment

Choose a reason for hiding this comment

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

Would it be possible to add a test for this change since it's in an important piece of code? If it's too hard, don't worry about it.

Library/Homebrew/formula_installer.rb Outdated Show resolved Hide resolved
Library/Homebrew/formula_installer.rb Outdated Show resolved Hide resolved
Library/Homebrew/formula_installer.rb Show resolved Hide resolved
@MikeMcQuaid
Copy link
Member Author

Would it be possible to add a test for this change since it's in an important piece of code? If it's too hard, don't worry about it.

I tried and failed to add a test for this because of how buried it is. It is at least covered by existing tests, though.

Copy link
Contributor

@apainintheneck apainintheneck left a comment

Choose a reason for hiding this comment

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

I'm getting the same type of errors showing up in the tests locally when trying to install packages where one of the dependencies doesn't need to be updated based on the new criteria.

It looks like the UsesFromMacOSDependency#installed? method needs to be updated.

MikeMcQuaid and others added 2 commits September 3, 2023 15:07
When we're installing a formula from a bottle, we currently always
upgrade all dependencies in the dependency tree to be safe.

However, if we're installing a bottle and the `runtime_dependencies`
within that bottle's tab all have older or equal versions to those
already installed: we do not need to upgrade these dependencies.

This should help a lot of upgrading a lot of the time, at least for
users using bottles (which is the huge majority).

The only downside or other noticeable change is that this requires us
to download or attempt to download the bottle tab before we compute
the dependencies at installation time.

Co-authored-by: Kevin <[email protected]>
@MikeMcQuaid MikeMcQuaid merged commit b535088 into Homebrew:master Sep 3, 2023
24 checks passed
@MikeMcQuaid MikeMcQuaid deleted the liberal_upgrade_bottle_version branch September 3, 2023 19:28
@MikeMcQuaid
Copy link
Member Author

Whoops, sorry for auto-merging the WIP commit 🙈

@github-actions github-actions bot added the outdated PR was locked due to age label Oct 4, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 4, 2023
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.

3 participants