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

Revamp installed_on_request handling #18768

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MikeMcQuaid
Copy link
Member

  • reinstall and upgrade no longer mark as installed on request, with or without names specified, but preserve the version from the tab instead
  • default install_on_request to false rather than true
  • only set installed in request in a tab if it's missing rather than false

@cho-m feel free to push any changes/test fixes directly to this PR as desired

Fixes #18754

- `reinstall` and `upgrade` no longer mark as installed on request,
  with or without names specified, but preserve the version from the
  tab instead
- default `install_on_request` to `false` rather than `true`
- only set installed in request in a tab if it's missing rather than
  false
Copy link
Member

@cho-m cho-m left a comment

Choose a reason for hiding this comment

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

Okay with changes avoiding excess installed-on-request (given we often recommend brew [reinstall | upgrade] ... as a way of fixing some issues)

I added comments on behavior differences that may need to be verified as intentional.

@@ -138,7 +138,6 @@ def run
Homebrew::Reinstall.reinstall_formula(
formula,
flags: args.flags_only,
installed_on_request: args.named.present?,
Copy link
Member

@cho-m cho-m Nov 14, 2024

Choose a reason for hiding this comment

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

There is a scenario of brew reinstall <not-installed-formula>. Previously, this would behave like brew install. With change, the installed formula will be neither installed on request nor a dependency, so will be deleted on next brew autoremove

unless tab.installed_on_request
if tab.installed_on_request.nil?
Copy link
Member

@cho-m cho-m Nov 14, 2024

Choose a reason for hiding this comment

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

Previous logic may be preferred so that brew install <already-installed-formula> has same tab as brew install <not-installed-formula>

e.g. the installed-on-request status of openssl@3 after:

$ brew install ruby
$ brew install openssl@3

And result would be different by changing order:

$ brew install openssl@3
$ brew install ruby

Comment on lines 147 to +148
installed_as_dependency: tab&.installed_as_dependency,
installed_on_request: installed_on_request || tab&.installed_on_request,
installed_on_request: tab&.installed_on_request,
Copy link
Member

@cho-m cho-m Nov 14, 2024

Choose a reason for hiding this comment

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

Doesn't have to be in this PR, but one way of preserving original tab in case of migration via alias (not rename) is adding a fallback for keg

brew(main):001> Formula["pkg-config"].opt_prefix
=> #<Pathname:/usr/local/opt/pkgconf>
brew(main):002> Formula["pkg-config"].optlinked?
=> false
brew(main):003> Formula["pkg-config"].installed_kegs.find(&:optlinked?)
=> #<Keg:/usr/local/Cellar/pkg-config/0.29.2_3>
brew(main):004> Formula["pkg-config"].installed_kegs.find(&:optlinked?).tab.installed_as_dependency
=> true
brew(main):005> Formula["pkg-config"].installed_kegs.find(&:optlinked?).tab.installed_on_request
=> false

So maybe:

keg = if formula.optlinked?
  Keg.new(formula.opt_prefix.resolved_path)
else
  formula.installed_kegs.find(&:optlinked?)
end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Formula dependency incorrectly marked as installed_on_request upon upgrade/migration
2 participants