fix(bundle): prevent autoremove from removing Brewfile packages during cleanup #21351
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #21350 by preventing autoremove from removing Brewfile packages during cleanup operations. The solution adds protection for Brewfile formulae when running brew bundle cleanup.
Key changes:
- Adds a
protected_formulaeparameter toUtils::Autoremove.removable_formulaeto exclude specific formulae from removal - Implements environment variable-based protection by setting
HOMEBREW_NO_CLEANUP_FORMULAEduring formula uninstall operations - Ensures Brewfile formulae are preserved even if they have
installed_on_request: false
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Library/Homebrew/utils/autoremove.rb | Adds optional protected_formulae parameter to removable_formulae method to exclude specific formulae from removal |
| Library/Homebrew/test/utils/autoremove_spec.rb | Adds comprehensive test coverage for the new protected_formulae parameter functionality |
| Library/Homebrew/bundle/commands/cleanup.rb | Implements with_brewfile_formulae_protected helper method to temporarily set HOMEBREW_NO_CLEANUP_FORMULAE environment variable during formula uninstall |
| Library/Homebrew/test/bundle/commands/cleanup_spec.rb | Adds tests verifying that Brewfile formulae are protected via environment variable during uninstall operations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
MikeMcQuaid
left a comment
There was a problem hiding this comment.
Thanks for the PR!
I'd prefer Option 2 in #21350.
brew bundle cleanup already has side-effects and there will be other issues with having things in a Brewfile that are not marked as installed-on-request so having cleanup handle this seems appropriate. brew bundle (install) should also handle this, if it doesn't already.
Happy to rereview after that.
Not at all a big deal but for future reference, no need to open an issue if you're going to soon afterwards open a PR to fix your own issue: skip straight to opening a PR in that case.
Thanks again!
dc29d97 to
03e27a1
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
MikeMcQuaid
left a comment
There was a problem hiding this comment.
Thanks! Few style tweaks.
Co-authored-by: Mike McQuaid <mike@mikemcquaid.com>
|
Thanks for the quick feedback @MikeMcQuaid! I've implemented Option 2 and addressed your comments. Let me know if there's anything else you'd like changed.
Noted, will skip straight to PRs next time. 👍 |
MikeMcQuaid
left a comment
There was a problem hiding this comment.
Thanks! Looking great so far. Few more comments.
Co-authored-by: Mike McQuaid <mike@mikemcquaid.com>
- added early returns to `mark_as_installed_on_request!` - moved the call inside `if formulae.any?` in cleanup.rb - removed unnecessary mocks from cleanup_spec.rb contexts - refactored bundle_spec.rb with subject/let/before
|
Good points @MikeMcQuaid, all addressed. Let me know if there's anything else we can do better. |
MikeMcQuaid
left a comment
There was a problem hiding this comment.
Great work here, thanks @AbdelrahmanHafez!
Fixes #21350
brew lgtm(style, typechecking and tests) with your changes locally?