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

Revamp installed_on_request handling #18768

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+7 −14
Open

Revamp installed_on_request handling #18768

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 0 additions & 2 deletions Library/Homebrew/cmd/reinstall.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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

force_bottle: args.force_bottle?,
build_from_source_formulae: args.build_from_source_formulae,
interactive: args.interactive?,
Expand All @@ -156,7 +155,6 @@ def run
Upgrade.check_installed_dependents(
formulae,
flags: args.flags_only,
installed_on_request: args.named.present?,
force_bottle: args.force_bottle?,
build_from_source_formulae: args.build_from_source_formulae,
interactive: args.interactive?,
Expand Down
2 changes: 0 additions & 2 deletions Library/Homebrew/cmd/upgrade.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,6 @@ def upgrade_outdated_formulae(formulae)
formulae_to_install,
flags: args.flags_only,
dry_run: args.dry_run?,
installed_on_request: args.named.present?,
force_bottle: args.force_bottle?,
build_from_source_formulae: args.build_from_source_formulae,
interactive: args.interactive?,
Expand All @@ -237,7 +236,6 @@ def upgrade_outdated_formulae(formulae)
formulae_to_install,
flags: args.flags_only,
dry_run: args.dry_run?,
installed_on_request: args.named.present?,
force_bottle: args.force_bottle?,
build_from_source_formulae: args.build_from_source_formulae,
interactive: args.interactive?,
Expand Down
2 changes: 1 addition & 1 deletion Library/Homebrew/formula_installer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ def initialize(
formula,
link_keg: false,
installed_as_dependency: false,
installed_on_request: true,
installed_on_request: false,
show_header: false,
build_bottle: false,
skip_post_install: false,
Expand Down
4 changes: 3 additions & 1 deletion Library/Homebrew/install.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ def install_formula?(

keg = Keg.new(formula.opt_prefix.resolved_path)
tab = keg.tab
unless tab.installed_on_request
if tab.installed_on_request.nil?
Copy link
Member

@cho-m cho-m Nov 14, 2024 •
edited
Loading

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

tab.installed_on_request = true
tab.write
end
Expand Down Expand Up @@ -259,6 +259,8 @@ def install_formulae(
formula_installer = FormulaInstaller.new(
formula,
options: build_options.used_options,
installed_on_request: true,
installed_as_dependency: false,
build_bottle:,
force_bottle:,
bottle_arch:,
Expand Down
3 changes: 1 addition & 2 deletions Library/Homebrew/reinstall.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ module Reinstall
def self.reinstall_formula(
formula,
flags:,
installed_on_request: false,
force_bottle: false,
build_from_source_formulae: [],
interactive: false,
Expand Down Expand Up @@ -41,7 +40,7 @@ def self.reinstall_formula(
options:,
link_keg: keg_had_linked_opt ? keg_was_linked : nil,
installed_as_dependency: tab&.installed_as_dependency,
installed_on_request: installed_on_request || tab&.installed_on_request,
installed_on_request: tab&.installed_on_request,
build_bottle: tab&.built_bottle?,
force_bottle:,
build_from_source_formulae:,
Expand Down
2 changes: 1 addition & 1 deletion Library/Homebrew/test/support/helper/spec/shared_context/integration_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ class #{Formulary.class_s(name)} < Formula

def install_test_formula(name, content = nil, build_bottle: false)
setup_test_formula(name, content)
fi = FormulaInstaller.new(Formula[name], build_bottle:)
fi = FormulaInstaller.new(Formula[name], build_bottle:, installed_on_request: true)
fi.prelude
fi.fetch
fi.install
Expand Down
6 changes: 1 addition & 5 deletions Library/Homebrew/upgrade.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ def self.upgrade_formulae(
formulae_to_install,
flags:,
dry_run: false,
installed_on_request: false,
force_bottle: false,
build_from_source_formulae: [],
dependents: false,
Expand Down Expand Up @@ -55,7 +54,6 @@ def self.upgrade_formulae(
fi = create_formula_installer(
formula,
flags:,
installed_on_request:,
force_bottle:,
build_from_source_formulae:,
interactive:,
Expand Down Expand Up @@ -114,7 +112,6 @@ def self.upgrade_formulae(
private_class_method def self.create_formula_installer(
formula,
flags:,
installed_on_request: false,
force_bottle: false,
build_from_source_formulae: [],
interactive: false,
Expand Down Expand Up @@ -148,7 +145,7 @@ def self.upgrade_formulae(
options:,
link_keg: keg_had_linked_opt ? keg_was_linked : nil,
installed_as_dependency: tab&.installed_as_dependency,
installed_on_request: installed_on_request || tab&.installed_on_request,
installed_on_request: tab&.installed_on_request,
Comment on lines 147 to +148
Copy link
Member

@cho-m cho-m Nov 14, 2024 •
edited
Loading

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

build_bottle: tab&.built_bottle?,
force_bottle:,
build_from_source_formulae:,
Expand Down Expand Up @@ -338,7 +335,6 @@ def self.check_installed_dependents(
upgrade_formulae(
upgradeable_dependents,
flags:,
installed_on_request:,
force_bottle:,
build_from_source_formulae:,
dependents: true,
Expand Down
Loading