Refine ask‐option dependency resolution and strengthen tests #20033

tyuwags · 2025-05-31T17:11:01Z

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?

These changes resolve an error a finding the whole hierarchy for the ask option, pruning the build dependencies and be able to find the whole dependants hierarchy of the formula.
These changes also create and update tests to make sure the whole hierarchy is displayed in the ask prompt.

…_spec.rb

…h dependencies and dependants is upgraded as expected

tyuwags · 2025-05-31T17:17:34Z

I saw a problem, with this example, I don't know if it's the expected behaviour.
Here the deps of fonttools
fonttools
└── python@3.13
├── mpdecimal
├── openssl@3
│ └── ca-certificates
├── sqlite
│ └── readline
├── xz
└── expat
Fonttools and sqlite are outdated but not python@3.13
My function found correctly that if I do brew upgrade --ask fonttools that fonttools and sqlite need to be upgraded as I look into the recursive dependencies.
But as python@3.13 is up-to-date, sqlite isn't upgraded by brew, I don't know if it's a normal behaviour and I should modify my function in consequence or if I should open an issue for the upgrade function.

MikeMcQuaid · 2025-06-02T07:35:01Z

look into the recursive dependencies.
But as python@3.13 is up-to-date, sqlite isn't upgraded by brew, I don't know if it's a normal behaviour and I should modify my function in consequence or if I should open an issue for the upgrade function.

@tyuwags Not sure I understand the question. It's expected that some dependencies may need to be upgraded and others may not. Does that answer your question?

tyuwags · 2025-06-02T13:54:29Z

Thanks for the reply! Let me clarify the issue a bit:

In this example:

fonttools is outdated
sqlite is also outdated, and it's a dependency of python@3.13
python@3.13 itself is not outdated
When I run brew upgrade --ask fonttools, my function detects both fonttools and sqlite as outdated by looking through recursive dependencies — which seems logical.

However, during the actual upgrade, Homebrew skips sqlite because its direct parent (python@3.13) is not being rebuilt.

So my actual question is:

Should my function consider only the outdated recursive dependencies whose parent will also be rebuilt?
Or is it fine (or even expected) that brew upgrade skips outdated indirect deps when their parent is not being upgraded?
I'm trying to align my implementation logic with how brew upgrade behaves internally. Thanks again!

MikeMcQuaid · 2025-06-02T14:06:28Z

Or is it fine (or even expected) that brew upgrade skips outdated indirect deps when their parent is not being upgraded?

This is expected.

I'm trying to align my implementation logic with how brew upgrade behaves internally.

Thanks for this. Ideally this would be done by using the same code rather than having two code paths that have to mirror each other.

tyuwags · 2025-06-02T14:59:46Z

Thanks for this. Ideally this would be done by using the same code rather than having two code paths that have to mirror each other.

That was my initial thought as well. However, from what I’ve seen, brew doesn’t seem to know upfront which formulae will need to be upgraded. Instead, it determines the necessary dependencies and dependents dynamically using:

Upgrade.upgrade_formulae formulae_to_install
Upgrade.check_installed_dependents formulae_to_install

What I'd like is for these functions to return the full list of formulae that need to be upgraded, without performing the upgrade yet. Then, I could pass that list to a separate function that handles the actual upgrade logic.

This way, I could reuse the existing resolution logic and keep everything consistent, without duplicating or mirroring code paths.

MikeMcQuaid · 2025-06-02T15:06:43Z

@tyuwags I'm open to seeing PR(s) to modify those functions to the logic can be shared. Let me know when this one is ready for review and maybe make it draft until then?

# Conflicts: # Library/Homebrew/test/cmd/upgrade_spec.rb

Copilot

Pull Request Overview

This PR refactors how upgrade, install, and reinstall commands resolve and act on dependencies when --ask is used, and strengthens integration tests to cover full dependency hierarchies.

Split dependency resolution from execution by introducing get_formulae_dependencies, get_dependants, upgrade_dependents, and get_hierarchy.
Refactored Reinstall and Install modules to parallel Upgrade’s two-phase flow.
Enhanced test helpers and added end-to-end integration tests covering nested formula dependency trees.

Reviewed Changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
Library/Homebrew/upgrade.rb	Extracted dependency-gathering into `get_formulae_dependencies` and `get_dependants`; separated upgrade execution into `upgrade_formulae` and `upgrade_dependents`.
Library/Homebrew/install.rb	Renamed first `install_formulae` to `get_formulae_dependencies`; added two-phase flow and `get_hierarchy` for ask-mode.
Library/Homebrew/reinstall.rb	Introduced `get_formula_to_reinstall` and a `Formula_keg` struct; moved prelude/fetch into new `reinstall_formula`.
Library/Homebrew/cmd/upgrade.rb	Updated CLI to use new dependency resolution methods and only call `ask_formulae` when appropriate.
Library/Homebrew/cmd/install.rb	Mirrored `upgrade` changes: using `get_formulae_dependencies`, `ask_formulae`, `upgrade_dependents`.
Library/Homebrew/cmd/reinstall.rb	Adjusted to call `get_formula_to_reinstall` and adopt two-phase ask-mode.
Library/Homebrew/test/support/helper/spec/shared_context/integration_test.rb	Extended `setup_test_formula` to handle bottled testballs and generalized program names.
Library/Homebrew/test/cmd/upgrade_spec.rb	Updated descriptions, test setup, and added a new integration test for full dependant hierarchy.
Library/Homebrew/test/cmd/install_spec.rb	Aligned expected output regex and test formulas to new ask-mode behavior.

Comments suppressed due to low confidence (2)

Library/Homebrew/reinstall.rb:10

[nitpick] The constant Formula_keg does not follow Ruby’s CamelCase naming convention. Consider renaming it to FormulaKeg for consistency with typical Ruby style.

Formula_keg = Struct.new(:formula_installer, :keg, :formula, :options)

Library/Homebrew/upgrade.rb:98

In the rescue block you reference an undefined variable formula. You should reference the installer’s formula, for example fi.formula.full_name, to report the correct formula name.

ofail "#{formula}: #{e}"

MikeMcQuaid

Thanks @tyuwags! Looking good so far.

Library/Homebrew/cmd/install.rb

MikeMcQuaid · 2025-06-11T15:44:01Z

Library/Homebrew/cmd/install.rb

-          dry_run:                    args.dry_run?,
-        )
+        if args.ask?
+          dependants = Upgrade.get_dependants(


Is it possible to have a single call to this rather than two? Does Install.install_formulae change the output here always/sometimes/maybe/never?

Yeah, I have to make two calls or change the behaviour of this method
In the flow, I have this line

installed_formulae = (dry_run ? formulae : FormulaInstaller.installed.to_a).dup

which used formulae or FormulaInstaller.installed.to_a
the problem is that I have to get the dependants to print them out before installing
so I must call this function before installing or upgrading and use formulae rather than FormulaInstaller.installed.to_a as its empty for now
I could refactor to only use formulae but the original flow was to install formulae and get the dependants of the freshly installed formulae so I keep both flow depending of the option

the problem is that I have to get the dependants to print them out before installing

Yeh, makes sense. I guess the question I have is: can you get these dependents and use them regardless of whether ask? is used. If not: can you explain why not and what needs to be different/breaks/etc.?

I can but I will get the dépendants based on formulae that I want to install instead of which formulae brew really installed. Normally, it shouldn't have any difference but im not sure about that

@tyuwags Can you try it out and see? Thanks!

tests are all running as expected after just using formulae.dup and I didn't see any problems when using brew locally without ask

Library/Homebrew/cmd/reinstall.rb

Library/Homebrew/cmd/install.rb

Library/Homebrew/test/cmd/upgrade_spec.rb

Library/Homebrew/test/support/helper/spec/shared_context/integration_test.rb

Library/Homebrew/upgrade.rb

MikeMcQuaid · 2025-06-11T15:51:30Z

Library/Homebrew/reinstall.rb:10

[nitpick] The constant Formula_keg does not follow Ruby’s CamelCase naming convention. Consider renaming it to FormulaKeg for consistency with typical Ruby style.
Formula_keg = Struct.new(:formula_installer, :keg, :formula, :options)
Library/Homebrew/upgrade.rb:98

In the rescue block you reference an undefined variable formula. You should reference the installer’s formula, for example fi.formula.full_name, to report the correct formula name.
ofail "#{formula}: #{e}"

@tyuwags ☝🏻 these both seem correct

tyuwags · 2025-06-18T13:20:46Z

@MikeMcQuaid, everything is ready, everything is resolved

MikeMcQuaid

A few suggested style tweaks but the bulk of this looks great, thanks @tyuwags! Feel like we're really close now. Gonna ask a few other maintainers to review.

Library/Homebrew/cmd/install.rb

Bo98

Looks good overall. Mostly some naming comments here, though with a couple of logic comments at the end which are more important.

Library/Homebrew/reinstall.rb

Library/Homebrew/cmd/reinstall.rb

Library/Homebrew/install.rb

Library/Homebrew/upgrade.rb

Bo98 · 2025-06-19T02:44:16Z

Library/Homebrew/test/cmd/install_spec.rb

@@ -95,34 +95,4 @@ def install

    expect(HOMEBREW_CELLAR/"testball1/0.1/bin/test").to be_a_file
  end
-
-  it "installs with asking for user prompts with installed dependent checks", :integration_test do


Why are these tests deleted? If they're redundant, it's probably better to handle in a separate PR.

I created those tests to make sure all the new methods created ran correctly. As @MikeMcQuaid said before, those tests are too long so I deleted them

Bo98 · 2025-06-19T02:48:00Z

Library/Homebrew/upgrade.rb

+          if !dry_run && dependents && fi.bottle_tab_runtime_dependencies.presence&.all? do |dependency, hash|
+            minimum_version = Version.new(hash["version"]) if hash["version"].present?
+            Dependency.new(dependency).installed?(minimum_version:, minimum_revision: hash["revision"])
+          end
            # Don't need to install this bottle if all of the runtime
            # dependencies have the same or newer version already installed.
-            next if dependents && fi.bottle_tab_runtime_dependencies.presence&.all? do |dependency, hash|
-              minimum_version = Version.new(hash["version"]) if hash["version"].present?
-              Dependency.new(dependency).installed?(minimum_version:, minimum_revision: hash["revision"])
-            end
-
-            fi.fetch
+            next
          end


The double end here is a bit confusing. I'd move things about a bit so the first end doesn't look like it's ending the if block.

Ideally go for next if if possible.

Library/Homebrew/upgrade.rb

Bo98 · 2025-06-19T02:56:43Z

Library/Homebrew/upgrade.rb

-            fi.prelude
-
+          if !dry_run && dependents && fi.bottle_tab_runtime_dependencies.presence&.all? do |dependency, hash|


I don't think this logic is correct. fi.bottle_tab_runtime_dependencies requires fi.fetch_bottle_tab to have run first, which was previously called by fi.prelude that we're removing here.

Bo98 · 2025-06-19T03:11:05Z

Library/Homebrew/upgrade.rb

-      installed_formulae = (dry_run ? formulae : FormulaInstaller.installed.to_a).dup
+      installed_formulae = formulae.dup
      installed_formulae.reject! { |f| f.core_formula? && f.versioned_formula? }
      return if installed_formulae.empty?


This sounds like it'll now upgrade dependents of things that failed to upgrade.

We may need to group dependents by formula and then filter out those that didn't actually install later.

Co-authored-by: Bo Anderson <mail@boanderson.me>

tyuwags added 4 commits May 31, 2025 12:52

adding tarball for the different tests in upgrade_spec.rb and install…

3963af7

…_spec.rb

modifying ask function as it didn't find the whole hierarchy for parents

3f1341e

making tests more robust with bottles to verify the whole hierarchy o…

19134b3

…h dependencies and dependants is upgraded as expected

Merge remote-tracking branch 'tyuwags/master'

fbd1320

tyuwags added 3 commits May 31, 2025 13:25

updating tarball to remove attributes from MacOS

960b434

updating name tarball for linux

b0b29f9

adding tarball parent for linux

b1c8567

tyuwags closed this May 31, 2025

tyuwags force-pushed the master branch from b1c8567 to 54c8b12 Compare May 31, 2025 21:12

tyuwags added 2 commits June 1, 2025 03:25

Removing attributes from macOS

326376e

double quotes for brew style

ab3b273

tyuwags reopened this Jun 1, 2025

tyuwags changed the title ~~Fixing dependants and dependencies research in ask option and making tests stronger~~ Refine ask‐option dependency resolution and strengthen tests Jun 1, 2025

tyuwags added 2 commits May 31, 2025 23:02

double quotes for brew style

01ad6b5

Merge remote-tracking branch 'tyuwags/master'

7c3281f

tyuwags added 9 commits June 8, 2025 11:31

refactoring homebrew upgrade for ask option

27f7f28

updating test for upgrade

628d265

updating ask function

fd159ed

refactoring install

a5251b2

updating upgrade to work

8222b19

putting up to date install test

a863827

refactoring code

aa47105

Merge branch 'refactor-install'

d39bded

# Conflicts: # Library/Homebrew/test/cmd/upgrade_spec.rb

resolving error typecheck

32a53ab

MikeMcQuaid requested a review from Copilot June 11, 2025 15:41

Copilot AI reviewed Jun 11, 2025

View reviewed changes

MikeMcQuaid reviewed Jun 11, 2025

View reviewed changes

tyuwags and others added 10 commits June 11, 2025 22:35

resolving discussions

7e876e4

resolving discussions

fb5ddde

resolving discussions

d525e25

trying to remove FormulaInstaller.installed to formulae

8c203df

refactoring as we can get dependants before installing

971a8b4

removing longer tests and putting back integration_test.rb

fc18956

removing fixture

ea7d441

brew style error

1b0e791

Merge branch 'master' into master

27a040c

refactoring install.rb and reinstall.rb after removing Formula.installed

fe06e04

MikeMcQuaid approved these changes Jun 18, 2025

View reviewed changes

Library/Homebrew/cmd/install.rb Outdated Show resolved Hide resolved

Library/Homebrew/cmd/install.rb Outdated Show resolved Hide resolved

refactoring style to resolve the last discussions

19901b5

Bo98 reviewed Jun 19, 2025

View reviewed changes

tyuwags and others added 11 commits June 20, 2025 11:10

Update Library/Homebrew/install.rb

bcc7f0f

Co-authored-by: Bo Anderson <mail@boanderson.me>

Update Library/Homebrew/upgrade.rb

0800faf

Co-authored-by: Bo Anderson <mail@boanderson.me>

adding parameter to fetch quietly if wanted

052c0ae

fetching tab and renaming variables

0db36b5

renaming parameter, method and struct to make it more understandable

90b6979

renaming variable

2fb6105

Merge remote-tracking branch 'tyuwags/master'

7d7d04b

fixing method refactoring

6d0a77f

new line to make it easier to understand

a16129f

comment

f674a3a

making flow easier to understand

2d39c7f

Anki-code1 mentioned this pull request Jun 20, 2025

formula: allow excluding deprecate! reason when disable! exists #18661

Draft

7 tasks

		fi.prelude

		if !dry_run && dependents && fi.bottle_tab_runtime_dependencies.presence&.all? do \|dependency, hash\|

Uh oh!

Refine ask‐option dependency resolution and strengthen tests #20033

Are you sure you want to change the base?

Refine ask‐option dependency resolution and strengthen tests #20033

Conversation

tyuwags commented May 31, 2025

Uh oh!

tyuwags commented May 31, 2025

Uh oh!

MikeMcQuaid commented Jun 2, 2025

Uh oh!

tyuwags commented Jun 2, 2025

Uh oh!

MikeMcQuaid commented Jun 2, 2025

Uh oh!

tyuwags commented Jun 2, 2025

Uh oh!

MikeMcQuaid commented Jun 2, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MikeMcQuaid commented Jun 11, 2025

Uh oh!

tyuwags commented Jun 18, 2025

Uh oh!

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Bo98 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Bo98 Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Bo98 Jun 19, 2025 •

edited

Loading