Fix support of formulae aliases in taps #16637

sjorek · 2024-02-11T00:04:02Z

Resolves: #16636

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?
- ** Yes in Support of formulae aliases in taps is broken #16636 **
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?
- … but I have the same failures in master and my PR (49 failures)

apainintheneck

In general, this fix looks correct. Thanks for taking the initiative to open a PR. However the regex should not get moved as mentioned in my comments.

Library/Homebrew/formulary.rb

reitermarkus · 2024-02-11T18:11:00Z

Library/Homebrew/formulary.rb

@@ -949,6 +950,7 @@ def self.tap_loader_for(tapped_name, warn:)
      end
    end

+    name = name.split("/").last if name != :nil && type == :alias


As mentioned in #16623 (comment), this is the exact same fix. Can you move the split into the conditional above to avoid checking another condition here?

Yeah, sorry, your suggestion is simpler than what I proposed earlier. Using String#split should avoid the nil type errors entirely too.

@apainintheneck I'm not sure who is targeted by @reitermarkus's wish to move the split - shall I do it?
And if yes; I'm not sure where exactly to move it to?

So like this?

diff --git a/Library/Homebrew/formulary.rb b/Library/Homebrew/formulary.rb index 9a1a7cc2c..cabc1f74a 100644 --- a/Library/Homebrew/formulary.rb +++ b/Library/Homebrew/formulary.rb @@ -909,7 +909,7 @@ module Formulary alias_name = tap.core_tap? ? name : "#{tap}/#{name}" if (possible_alias = tap.alias_table[alias_name].presence) - name = possible_alias + name = tap.core_tap? ? possible_alias : possible_alias.split("/").last type = :alias elsif (new_name = tap.formula_renames[name].presence) old_name = name @@ -950,7 +950,6 @@ module Formulary end end - name = name.split("/").last if name != :nil && type == :alias path = find_formula_in_tap(name, tap) TapLoader.new(name, path, tap: tap) end

Or like this?

diff --git a/Library/Homebrew/formulary.rb b/Library/Homebrew/formulary.rb index 9a1a7cc2c..952ee50fd 100644 --- a/Library/Homebrew/formulary.rb +++ b/Library/Homebrew/formulary.rb @@ -948,9 +948,10 @@ module Formulary elsif Homebrew::API::Formula.all_formulae.key?(name) return FormulaAPILoader.new(name) end + elsif name != :nil && type == :alias + name = name.split("/").last end - name = name.split("/").last if name != :nil && type == :alias path = find_formula_in_tap(name, tap) TapLoader.new(name, path, tap: tap) end

The latter seems to be error-prone … eg. when tap.core_tap? && !Homebrew::EnvConfig.no_install_from_api? is true and the API-lookup Homebrew::API::Formula.all_formulae.key?(name) is false.

@apainintheneck I'm not sure who is targeted by @reitermarkus's wish to move the split - shall I do it? And if yes; I'm not sure where exactly to move it to?

So like this?

diff --git a/Library/Homebrew/formulary.rb b/Library/Homebrew/formulary.rb index 9a1a7cc2c..cabc1f74a 100644 --- a/Library/Homebrew/formulary.rb +++ b/Library/Homebrew/formulary.rb @@ -909,7 +909,7 @@ module Formulary alias_name = tap.core_tap? ? name : "#{tap}/#{name}" if (possible_alias = tap.alias_table[alias_name].presence) - name = possible_alias + name = tap.core_tap? ? possible_alias : possible_alias.split("/").last type = :alias elsif (new_name = tap.formula_renames[name].presence) old_name = name @@ -950,7 +950,6 @@ module Formulary end end - name = name.split("/").last if name != :nil && type == :alias path = find_formula_in_tap(name, tap) TapLoader.new(name, path, tap: tap) end

This is pretty much what I was thinking. I'll update it now and test it locally to make sure it works.

apainintheneck · 2024-02-11T19:24:40Z

I'm also not sure why this test didn't catch this problem.

brew/Library/Homebrew/test/formulary_spec.rb

Lines 239 to 244 in 88b5e50

    
           it "returns a Formula from an Alias path" do 
        
             alias_dir = tap.path/"Aliases" 
        
             alias_dir.mkpath 
        
             FileUtils.ln_s formula_path, alias_dir/"bar" 
        
             expect(described_class.factory("bar")).to be_a(Formula) 
        
           end

sjorek · 2024-02-11T19:31:27Z

I'm also not sure why this test didn't catch this problem.

brew/Library/Homebrew/test/formulary_spec.rb

Lines 239 to 244 in 88b5e50

it "returns a Formula from an Alias path" do

alias_dir = tap.path/"Aliases"

alias_dir.mkpath

FileUtils.ln_s formula_path, alias_dir/"bar"

expect(described_class.factory("bar")).to be_a(Formula)

end

… I think because the spec just asks for formula-name and not for a tap/formula-name.

apainintheneck · 2024-02-11T20:54:32Z

I'm also not sure why this test didn't catch this problem.

brew/Library/Homebrew/test/formulary_spec.rb

Lines 239 to 244 in 88b5e50

it "returns a Formula from an Alias path" do

alias_dir = tap.path/"Aliases"

alias_dir.mkpath

FileUtils.ln_s formula_path, alias_dir/"bar"

expect(described_class.factory("bar")).to be_a(Formula)

end

… I think because the spec just asks for formula-name and not for a tap/formula-name.

That's exactly it. I added a regression test that checks for that and it fails on the master branch and works on this one.

apainintheneck · 2024-02-11T21:28:36Z

/u/l/Homebrew (issue-16636-fix-support-of-formulae-aliases-in-taps|✚2) $ brew info yaml@8.3
==> shivammathur/extensions/yaml@8.3: stable 2.2.3, HEAD
Yaml PHP extension
https://github.com/php/pecl-file_formats-yaml
Not installed
From: https://github.com/shivammathur/homebrew-extensions/blob/HEAD/Formula/yaml@8.3.rb
License: PHP-3.01
==> Dependencies
Build: autoconf ✔, pkg-config ✔, shivammathur/php/php@8.3 ✘
Required: libyaml ✔
==> Options
--HEAD
	Install HEAD version
==> Caveats
To finish installing yaml for PHP 8.3:
  * /usr/local/etc/php/8.3/conf.d/20-yaml.ini was created,"
    do not forget to remove it upon extension removal."
  * Validate installation by running php -m

/u/l/Homebrew (issue-16636-fix-support-of-formulae-aliases-in-taps|✔) $ brew info shivammathur/extensions/yaml@8.3
==> shivammathur/extensions/yaml@8.3: stable 2.2.3, HEAD
Yaml PHP extension
https://github.com/php/pecl-file_formats-yaml
Not installed
From: https://github.com/shivammathur/homebrew-extensions/blob/HEAD/Formula/yaml@8.3.rb
License: PHP-3.01
==> Dependencies
Build: autoconf ✔, pkg-config ✔, shivammathur/php/php@8.3 ✘
Required: libyaml ✔
==> Options
--HEAD
	Install HEAD version
==> Caveats
To finish installing yaml for PHP 8.3:
  * /usr/local/etc/php/8.3/conf.d/20-yaml.ini was created,"
    do not forget to remove it upon extension removal."
  * Validate installation by running php -m

It seems to work now.

apainintheneck

Thanks for the help with fixing this bug @sjorek!

Closes #16636

reitermarkus · 2024-02-12T06:21:37Z

Thanks for adding the test here, @apainintheneck!

MikeMcQuaid · 2024-02-12T15:33:12Z

Thanks for this @sjorek and for the review and merge @apainintheneck and @reitermarkus!

Fix support of formulae aliases in taps

1570a4b

Resolves: Homebrew#16636

sjorek mentioned this pull request Feb 11, 2024

Support of formulae aliases in taps is broken #16636

Closed

3 tasks

remove accidentally added newline

863654e

apainintheneck reviewed Feb 11, 2024

View reviewed changes

Library/Homebrew/formulary.rb Show resolved Hide resolved

Library/Homebrew/formulary.rb Show resolved Hide resolved

apainintheneck mentioned this pull request Feb 11, 2024

Refactor Formulary::loader_for. #16623

Merged

7 tasks

refactor bugfix to satify brew audit

d01de75

sjorek force-pushed the issue-16636-fix-support-of-formulae-aliases-in-taps branch from 38f3ea7 to d01de75 Compare February 11, 2024 10:41

reitermarkus reviewed Feb 11, 2024

View reviewed changes

formulary: update logic and add regression test

70ffc31

apainintheneck approved these changes Feb 11, 2024

View reviewed changes

apainintheneck added the critical Critical change which should be shipped as soon as possible. label Feb 11, 2024

apainintheneck merged commit ac7e759 into Homebrew:master Feb 11, 2024
25 checks passed

github-actions bot added the outdated PR was locked due to age label Mar 14, 2024

github-actions bot locked as resolved and limited conversation to collaborators Mar 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix support of formulae aliases in taps #16637

Fix support of formulae aliases in taps #16637

sjorek commented Feb 11, 2024 •

edited

apainintheneck left a comment

reitermarkus Feb 11, 2024 •

edited

apainintheneck Feb 11, 2024

sjorek Feb 11, 2024

sjorek Feb 11, 2024

sjorek Feb 11, 2024

apainintheneck Feb 11, 2024

apainintheneck commented Feb 11, 2024

sjorek commented Feb 11, 2024

apainintheneck commented Feb 11, 2024

apainintheneck commented Feb 11, 2024

apainintheneck left a comment

reitermarkus commented Feb 12, 2024

MikeMcQuaid commented Feb 12, 2024

Fix support of formulae aliases in taps #16637

Fix support of formulae aliases in taps #16637

Conversation

sjorek commented Feb 11, 2024 • edited

apainintheneck left a comment

Choose a reason for hiding this comment

reitermarkus Feb 11, 2024 • edited

Choose a reason for hiding this comment

apainintheneck Feb 11, 2024

Choose a reason for hiding this comment

sjorek Feb 11, 2024

Choose a reason for hiding this comment

sjorek Feb 11, 2024

Choose a reason for hiding this comment

sjorek Feb 11, 2024

Choose a reason for hiding this comment

apainintheneck Feb 11, 2024

Choose a reason for hiding this comment

apainintheneck commented Feb 11, 2024

sjorek commented Feb 11, 2024

apainintheneck commented Feb 11, 2024

apainintheneck commented Feb 11, 2024

apainintheneck left a comment

Choose a reason for hiding this comment

reitermarkus commented Feb 12, 2024

MikeMcQuaid commented Feb 12, 2024

sjorek commented Feb 11, 2024 •

edited

reitermarkus Feb 11, 2024 •

edited