Fixes `openjdk_dep_name_if_applicable` when not using `CurlGitHubPackagesDownloadStrategy` #16439

arianf · 2024-01-06T16:34:42Z

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?

Fixes openjdk_dep_name_if_applicable when not using CurlGitHubPackagesDownloadStrategy

When installing a formula, FormulaInstaller calls #pour, which in turn calls:

brew/Library/Homebrew/formula_installer.rb

Line 1260 in 6f20c03

tab = Utils::Bottles.load_tab(formula)

This tab is expected to have #runtime_dependencies, and it typically will because most packages come from http://ghcr.io

brew/Library/Homebrew/utils/bottles.rb

Line 111 in 6f20c03

if (tab_attributes = formula.bottle_tab_attributes.presence)

Any DownloadStrategy that does not match CurlGitHubPackagesDownloadStrategy will lead here:

brew/Library/Homebrew/software_spec.rb

Line 463 in 6f20c03

return if @resource.download_strategy != CurlGitHubPackagesDownloadStrategy

Causing this branch to be executed for creating the tab, without #runtime_dependencies:

brew/Library/Homebrew/utils/bottles.rb

Line 119 in 6f20c03

Tab.for_keg(keg)

This causes a slight issue because openjdk_dep_name_if_applicable calls keg.runtime_dependencies when it's still nil.

brew/Library/Homebrew/keg_relocate.rb

Lines 134 to 140 in 6f20c03

    
           def openjdk_dep_name_if_applicable 
        
             deps = runtime_dependencies 
        
             return if deps.blank? 
        
             dep_names = deps.map { |d| d["full_name"] } 
        
             dep_names.find { |d| d.match? Version.formula_optionally_versioned_regex(:openjdk) } 
        
           end

And if it's blank, it won't do the regex replacement on @@HOMEBREW_JAVA@@, resulting in the following error when running Kafka:

$ tail -f /opt/homebrew/var/log/kafka/kafka_output.log
/opt/homebrew/Cellar/kafka/3.6.0/libexec/bin/kafka-run-class.sh: line 346: /opt/homebrew/@@HOMEBREW_JAVA@@/bin/java: No such file or directory
/opt/homebrew/Cellar/kafka/3.6.0/libexec/bin/kafka-run-class.sh: line 346: exec: /opt/homebrew/@@HOMEBREW_JAVA@@/bin/java: cannot execute: No such file or directory

As mentioned by: https://github.com/orgs/Homebrew/discussions/2530#discussioncomment-2002374

Installing Java-dependent formulae from bottle mirrors doesn't work properly at the moment. The issue is that brew needs the manifest in order to correctly replace @@HOMEBREW_JAVA@@ but brew only knows how to fetch manifests from ghcr.io.
Pull requests to fix this welcome.

This should fix this issue, by getting the runtime_dependencies directly from the formula for those cases that it can't get it from https://ghcr.io or tabfile

f_runtime_deps = formula.runtime_dependencies(read_from_tab: false)
tab.runtime_dependencies = Tab.runtime_deps_hash(formula, f_runtime_deps)

apainintheneck · 2024-01-06T20:08:01Z

Thanks for taking a look at this.

It's interesting that we already update the runtime dependency field in the tab when finishing up the install and this is essentially just moving that work earlier in the process so that keg relocation works here.

brew/Library/Homebrew/formula_installer.rb

Lines 820 to 825 in 3821fab

    
           # Update tab with actual runtime dependencies 
        
           tab = Tab.for_keg(keg) 
        
           Tab.clear_cache 
        
           f_runtime_deps = formula.runtime_dependencies(read_from_tab: false) 
        
           tab.runtime_dependencies = Tab.runtime_deps_hash(formula, f_runtime_deps) 
        
           tab.write

Does this have to wait until after the keg is installed or can we just do it for all of them earlier on in the install process?

MikeMcQuaid

Thanks for great PR @arianf and comprehensive explanation, wonderful work. Have a few questions and interested in answers to @apainintheneck's too.

MikeMcQuaid · 2024-01-08T14:00:20Z

Library/Homebrew/test/utils/bottles/bottles_spec.rb

+
+  describe ".load_tab" do
+    context "when tab_attributes and tabfile are missing" do
+      it "includes runtime_dependencies", :integration_test do


Are you sure this needs to be an :integration_test? Would like to avoid adding new ones whenever possible because they are so slow 🐌

I added :integration_test, because I needed install_test_formula and setup_test_formula to perform the tests with the formulas. I'd be open to other ways of testing this.

Search for temporarily_install_bottle or when it can't remove a keg or eligible_kegs_for_cleanup for some examples of tests that do installation without an integration test.

@MikeMcQuaid sorry for the delay, I have fixed this and made the test a non :integration_test

Library/Homebrew/utils/bottles.rb

arianf · 2024-01-09T19:03:47Z

Thank you both for reviewing this PR.

It's interesting that we already update the runtime dependency field in the tab when finishing up the install and this is essentially just moving that work earlier in the process so that keg relocation works here.

brew/Library/Homebrew/formula_installer.rb

Lines 820 to 825 in 3821fab

# Update tab with actual runtime dependencies

tab = Tab.for_keg(keg)

Tab.clear_cache

f_runtime_deps = formula.runtime_dependencies(read_from_tab: false)

tab.runtime_dependencies = Tab.runtime_deps_hash(formula, f_runtime_deps)

tab.write

Does this have to wait until after the keg is installed or can we just do it for all of them earlier on in the install process?

Yea, that's something I was wondering as well. To be honest, I'm not sure why the runtime dependency are always overwritten in the #finish step. My assumption was there might be some edge conditions that I'm unaware of, where this is needed.. so I was hesitant to modify it.

That block above is reason why @carlocab observed the correct value within runtime_dependencies. It's just happening much later than relocate tries to use them.

https://github.com/orgs/Homebrew/discussions/2530#discussioncomment-1696893

The problem is here:

brew/Library/Homebrew/keg_relocate.rb

Line 135 in 3ab140e

deps = runtime_dependencies

runtime_dependencies is evaluating to nil for some reason. I don't really understand why, because the tab/install receipt indicates a runtime dependency on openjdk@11. Instantiating a Keg after installing your formula also has the correct value for runtime_dependencies. @Bo98, any ideas here?
— @carlocab

MikeMcQuaid · 2024-01-10T13:00:25Z

To be honest, I'm not sure why the runtime dependency are always overwritten in the #finish step.

We always want to calculate the actual formula linkage based on current dependencies rather than using the one in the tab.

…agesDownloadStrategy` When installing a formula, `FormulaInstaller` calls `#pour`, which in turn calls: https://github.com/Homebrew/brew/blob/6f20c0300aee2ab0feae3132d13f859d91cf295b/Library/Homebrew/formula_installer.rb#L1260 This `tab` is expected to have `#runtime_dependencies`, and it typically will because most packages come from http://ghcr.io https://github.com/Homebrew/brew/blob/6f20c0300aee2ab0feae3132d13f859d91cf295b/Library/Homebrew/utils/bottles.rb#L111 Any `DownloadStrategy` that does not match `CurlGitHubPackagesDownloadStrategy` will lead here: https://github.com/Homebrew/brew/blob/6f20c0300aee2ab0feae3132d13f859d91cf295b/Library/Homebrew/software_spec.rb#L463 Causing this branch to be executed for creating the `tab`: https://github.com/Homebrew/brew/blob/6f20c0300aee2ab0feae3132d13f859d91cf295b/Library/Homebrew/utils/bottles.rb#L119 This causes a slight issue because `openjdk_dep_name_if_applicable` calls `keg.runtime_dependencies` when it's still `nil`. https://github.com/Homebrew/brew/blob/6f20c0300aee2ab0feae3132d13f859d91cf295b/Library/Homebrew/keg_relocate.rb#L134-L140 And if it's blank, it won't do the regex replacement on `@@HOMEBREW_JAVA@@`, resulting in the following error when running `Kafka`: ```console $ tail -f /opt/homebrew/var/log/kafka/kafka_output.log /opt/homebrew/Cellar/kafka/3.6.0/libexec/bin/kafka-run-class.sh: line 346: /opt/homebrew/@@HOMEBREW_JAVA@@/bin/java: No such file or directory /opt/homebrew/Cellar/kafka/3.6.0/libexec/bin/kafka-run-class.sh: line 346: exec: /opt/homebrew/@@HOMEBREW_JAVA@@/bin/java: cannot execute: No such file or directory ``` As mentioned by: https://github.com/orgs/Homebrew/discussions/2530#discussioncomment-2002374 > Installing Java-dependent formulae from bottle mirrors doesn't work properly at the moment. The issue is that brew needs the manifest in order to correctly replace @@HOMEBREW_JAVA@@ but brew only knows how to fetch manifests from ghcr.io. > Pull requests to fix this welcome. This should fix this issue, by getting the `runtime_dependencies` directly from the formula for those cases that it can't get it from https://ghcr.io or tabfile ```ruby f_runtime_deps = formula.runtime_dependencies(read_from_tab: false) tab.runtime_dependencies = Tab.runtime_deps_hash(formula, f_runtime_deps) ```

Library/Homebrew/utils/bottles.rb

MikeMcQuaid

Thanks so much for your first contribution! Without people like you submitting PRs we couldn't run this project. You rock, @arianf!

MikeMcQuaid reviewed Jan 8, 2024

View reviewed changes

arianf force-pushed the fix-non-ghcr branch from 1ea1fe9 to 940f7ec Compare January 9, 2024 19:24

arianf requested a review from MikeMcQuaid January 9, 2024 23:27

MikeMcQuaid removed their request for review January 11, 2024 09:01

arianf force-pushed the fix-non-ghcr branch from 940f7ec to 9088cf9 Compare January 29, 2024 23:58

arianf requested a review from MikeMcQuaid January 29, 2024 23:59

MikeMcQuaid reviewed Jan 30, 2024

View reviewed changes

Library/Homebrew/utils/bottles.rb Outdated Show resolved Hide resolved

utils/bottles: tweak syntax.

176095a

MikeMcQuaid approved these changes Jan 30, 2024

View reviewed changes

MikeMcQuaid merged commit 3707c90 into Homebrew:master Jan 31, 2024
24 checks passed

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes `openjdk_dep_name_if_applicable` when not using `CurlGitHubPackagesDownloadStrategy` #16439

Fixes `openjdk_dep_name_if_applicable` when not using `CurlGitHubPackagesDownloadStrategy` #16439

arianf commented Jan 6, 2024 •

edited

apainintheneck commented Jan 6, 2024

MikeMcQuaid left a comment

MikeMcQuaid Jan 8, 2024

arianf Jan 9, 2024 •

edited

MikeMcQuaid Jan 10, 2024

arianf Jan 29, 2024

arianf commented Jan 9, 2024

MikeMcQuaid commented Jan 10, 2024

MikeMcQuaid left a comment

	def openjdk_dep_name_if_applicable
	deps = runtime_dependencies
	return if deps.blank?

	dep_names = deps.map { \|d\| d["full_name"] }
	dep_names.find { \|d\| d.match? Version.formula_optionally_versioned_regex(:openjdk) }
	end

Fixes openjdk_dep_name_if_applicable when not using CurlGitHubPackagesDownloadStrategy #16439

Fixes openjdk_dep_name_if_applicable when not using CurlGitHubPackagesDownloadStrategy #16439

Conversation

arianf commented Jan 6, 2024 • edited

apainintheneck commented Jan 6, 2024

MikeMcQuaid left a comment

Choose a reason for hiding this comment

MikeMcQuaid Jan 8, 2024

Choose a reason for hiding this comment

arianf Jan 9, 2024 • edited

Choose a reason for hiding this comment

MikeMcQuaid Jan 10, 2024

Choose a reason for hiding this comment

arianf Jan 29, 2024

Choose a reason for hiding this comment

arianf commented Jan 9, 2024

MikeMcQuaid commented Jan 10, 2024

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Fixes `openjdk_dep_name_if_applicable` when not using `CurlGitHubPackagesDownloadStrategy` #16439

Fixes `openjdk_dep_name_if_applicable` when not using `CurlGitHubPackagesDownloadStrategy` #16439

arianf commented Jan 6, 2024 •

edited

arianf Jan 9, 2024 •

edited