Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cleanup: allow users to specify formulae to skip cleaning #11931

Merged
merged 1 commit into from Aug 30, 2021

Conversation

carlocab
Copy link
Member

  • 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. -- I tried, but I really don't grok RSpec.
  • 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?

This allows users to set HOMEBREW_NO_CLEANUP_FORMULAE to a
comma-separated list of formula that brew will refuse to clean with
brew cleanup.

We currently allow a less granular version of this with
HOMEBREW_NO_INSTALL_CLEANUP. All this changes is how much control
users have over what is and isn't cleaned.

Fixes #11924.

@BrewTestBot
Copy link
Member

Review period will end on 2021-08-30 at 10:05:52 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Aug 27, 2021
@carlocab carlocab changed the title cleanup: allow users to specify formula to skip cleaning cleanup: allow users to specify formulae to skip cleaning Aug 27, 2021
This allows users to set `HOMEBREW_NO_CLEANUP_FORMULAE` to a
comma-separated list of formulae that `brew` will refuse to clean with
`brew cleanup`.

We currently allow a less granular version of this with
`HOMEBREW_NO_INSTALL_CLEANUP`. All this changes is how much control
users have over what is and isn't cleaned.

Fixes Homebrew#11924.
@BrewTestBot
Copy link
Member

Review period ended.

@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Aug 30, 2021
Copy link
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 good! One optional comment/thought.

def skip_clean_formula?(f)
return false if Homebrew::EnvConfig.no_cleanup_formulae.blank?

skip_clean_formulae = Homebrew::EnvConfig.no_cleanup_formulae.split(",")
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it's worth adding this behaviour into EnvConfig itself under the type? We also seem to alternate between comma and space separated on our configuration; might be worth doubling down on one?

Copy link
Member Author

Choose a reason for hiding this comment

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

We actually also have an option that takes a path to a file that specifies the list. We should probably only pick one approach, but this will require a deprecation cycle.

Copy link
Member

Choose a reason for hiding this comment

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

Or: we support both space-separated and comma-separated in the cases where we can use both (e.g. environment variables), document consistently and only use comma-separated in the cases where only it works (e.g. arguments)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, fine with me. I'll try to look into it.

@carlocab carlocab merged commit 430bd71 into Homebrew:master Aug 30, 2021
@carlocab carlocab deleted the cleanup-formulae branch August 30, 2021 15:09
@github-actions github-actions bot added the outdated PR was locked due to age label Sep 30, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

brew cleanup should continue even if it wasn't permitted to remove a file
3 participants