Skip to content

sync installed_as_dependency when changing installed_on_request#21755

Open
ooye-sanket wants to merge 2 commits intoHomebrew:mainfrom
ooye-sanket:fix/tab-sync-installed-as-dependency
Open

sync installed_as_dependency when changing installed_on_request#21755
ooye-sanket wants to merge 2 commits intoHomebrew:mainfrom
ooye-sanket:fix/tab-sync-installed-as-dependency

Conversation

@ooye-sanket
Copy link
Copy Markdown
Contributor

@ooye-sanket ooye-sanket commented Mar 18, 2026

When marking a formula as not installed on request, also set
installed_as_dependency to true, to ensure brew bundle dump
correctly excludes it.

Previously, brew tab --no-installed-on-request had no effect on
brew bundle dump because the dump logic in brew.rb checks both fields:

f[:installed_on_request?] || !f[:installed_as_dependency?]

Setting installed_on_request to false alone was not enough if
installed_as_dependency was also false — the second condition would
still evaluate to true, causing the formula to be included in the dump.

Fixes #21751

@MikeMcQuaid can you please review it!!

  • 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 lgtm (style, typechecking and tests) with your changes locally?

  • AI was used to generate or assist with generating this PR. Please specify below how you used AI to help you, and what steps you have taken to manually verify the changes.

When marking a formula as not installed on request, also set
installed_as_dependency to true, to ensure brew bundle dump
correctly excludes it. Previously, brew tab --no-installed-on-request
had no effect on bundle dump because the dump logic checks both

Fixes Homebrew#21751
@MikeMcQuaid
Copy link
Copy Markdown
Member

@ooye-sanket thanks for the PR! I actually think the bigger change we should make is get rid of installed_as_dependency altogether; replace all call sites with installed_on_request and stop storing it in new Tabs for both formulae and casks.

@ooye-sanket
Copy link
Copy Markdown
Contributor Author

Thanks @MikeMcQuaid!

I'd love to take on the bigger refactor. Before I start, could you clarify the approach?

  • Should installed_on_request become the single source of truth (i.e., !installed_on_request replaces every installed_as_dependency check)?
  • Should we keep reading the old installed_as_dependency from existing tab JSON files for backward compatibility, or drop that as well?
  • Should this be a separate PR from the current one, or should I update this PR?

@MikeMcQuaid
Copy link
Copy Markdown
Member

  • Should installed_on_request become the single source of truth (i.e., !installed_on_request replaces every installed_as_dependency check)?

Yep!

  • Should we keep reading the old installed_as_dependency from existing tab JSON files for backward compatibility, or drop that as well?

No, think we can skip/drop that too.

  • Should this be a separate PR from the current one, or should I update this PR?

Can update this PR!

…ce of truth

Replace all uses of installed_as_dependency with !installed_on_request.
Stop storing installed_as_dependency in new Tabs for both formulae and casks.
installed_on_request is now the single source of truth.

Fixes Homebrew#21751
@ooye-sanket
Copy link
Copy Markdown
Contributor Author

Hi @MikeMcQuaid, I've updated the PR with the full refactor as you suggested!

Here's what was done:

  • Removed installed_as_dependency entirely from AbstractTab and Tab (no longer stored in new tab JSON files)
  • Replaced all call sites with !installed_on_request installed_on_request is now the single source of truth
  • Updated formula_installer.rb, cask/installer.rb, install.rb, reinstall.rb, upgrade.rb, bundle/brew.rb, cmd/leaves.rb, cmd/list.rb, formula.rb, cask/tab.rb and all related test files and fixtures
  • Kept the --installed-as-dependency CLI flag in brew list and brew leaves as-is since it's a user-facing option

Could you confirm if this approach looks correct, or if there's anything I may have missed or handled incorrectly? Happy to make any adjustments!

Copy link
Copy Markdown
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Looks great, nice work @ooye-sanket! One more tiny suggestion.

statuses = []
statuses << "installed on request" if args.installed_on_request? && tab.installed_on_request
statuses << "installed as dependency" if args.installed_as_dependency? && tab.installed_as_dependency
statuses << "installed as dependency" if args.installed_as_dependency? && !tab.installed_on_request
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could add a commented-out odeprecated for the args.installed_as_dependency? so we can deprecate it in a future release.

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.

brew tab --no-installed-on-request does not work as expected

2 participants