cmd/uninstall: Add env variable that runs autoremove after uninstalls #13532

apainintheneck · 2022-07-08T18:35:33Z

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?

When HOMEBREW_UNINSTALL_AUTOREMOVE is set, brew autoremove is run after every successful call to brew uninstall. If there are no formulae or casks to uninstall, it exits early (cmd/uninstall.rb:53).

Resolves #13362.

BrewTestBot · 2022-07-08T18:35:49Z

Review period will end on 2022-07-11 at 18:35:33 UTC.

dawidd6

LGTM

Library/Homebrew/cmd/uninstall.rb

MikeMcQuaid

I love this, great work so far!

Library/Homebrew/cmd/uninstall.rb

BrewTestBot · 2022-07-11T21:02:14Z

Review period ended.

apainintheneck · 2022-07-12T08:17:25Z

Okay, I took a stab at moving the autoremove logic out into other files so that we don't have to shell out to brew autoremove from inside of brew uninstall. This needs more testing but I just wanted to get your thoughts on the refactor first. I moved the current logic into formula.rb and uninstall.rb but it could just as easily go into its own file too.

MikeMcQuaid

Looks great so far! Would love some unit tests for the Formula methods.

MikeMcQuaid · 2022-07-12T08:42:10Z

Library/Homebrew/env_config.rb

@@ -343,6 +343,10 @@ module EnvConfig
        default_text: "macOS: `/private/tmp`, Linux: `/tmp`.",
        default:      HOMEBREW_DEFAULT_TEMP,
      },
+      HOMEBREW_UNINSTALL_AUTOREMOVE:             {
+        description: "If set, `brew autoremove` is run after every successful call to `brew uninstall`.",


Oh, I misread this before. I wonder if a HOMEBREW_INSTALL_AUTOREMOVE makes sense to have as well that is run on each install/upgrade/reinstall or whether a HOMEBREW_AUTOREMOVE should do all of these. Thoughts?

I have no opinion on when exactly we should do this, but I have a strong opinion against having multiple environment variables.

If we do it after multiple commands then it should just be HOMEBREW_AUTOREMOVE.

Works for me @carlocab. I'll personally enable HOMEBREW_AUTOREMOVE as soon as it's around and will primarily rely on it being used at install time.

Thinking about this more, though: we automatically run brew cleanup at install time so I wonder whether running it at uninstall or cleanup time would be the most sensible implementation of HOMEBREW_AUTOREMOVE?

Running it as part of/after brew cleanup makes sense to me 👍

To be clear you're suggesting running autoremove after install, uninstall, upgrade and reinstall, is that right?

I just took a look at how cleanup is handled internally and it makes me think that a separate autoremove.rb file might be the way to go with this.

Also, HOMEBREW_AUTOREMOVE makes sense to me too.

To be clear you're suggesting running autoremove after install, uninstall, upgrade and reinstall, is that right?

Yes, but I (now) think the thing that should run it after install/upgrade/reinstall is that brew cleanup should run the autoremove logic if HOMEBREW_AUTOREMOVE is set (and also perhaps if --autoremove is passed if people think that makes sense 🤔)? I could see --autoremove being useful for brew bundle cleanup.

Library/Homebrew/formula.rb

When HOMEBREW_UNINSTALL_AUTOREMOVE is set, `brew autoremove` is run after every successful call to `brew uninstall`.

This allows us to autoremove formulae in the autoremove and uninstall commands without having to shell out to brew.

This allows us to call that logic internally in other brew commands instead of having to shell out.

apainintheneck · 2022-07-14T00:20:33Z

Okay, HOMEBREW_UNINSTALL_AUTOREMOVE has been changed to HOMEBREW_AUTOREMOVE as already discussed.

Beyond that all the logic for brew autoremove has been moved into autoremove.rb so that it can easily be run after other commands. I was not able to attach the behavior directly to brew cleanup because that runs multiple times, once for each formula; brew autoremove runs once for all installed formulae.

Also, it seems like there are currently no tests that exercise the autoremove code right now (the only thing being tested before was the arg parser). It's kind of a tricky bit of code to test though. If we're going to be running this after a bunch of other commands, I'd like to at least have a few tests in place just to catch obvious bugs.

apainintheneck · 2022-07-14T00:22:43Z

Ah, and one more thing. It now runs after brew install, brew uninstall, brew upgrade and brew reinstall if HOMEBREW_AUTOREMOVE is set.

MikeMcQuaid · 2022-07-14T07:59:42Z

Library/Homebrew/autoremove.rb

+    module_function
+
+    def remove_unused_formulae(dry_run: false)
+      removable_formulae = unused_formulae_with_no_dependents


Would be good to have some basic unit tests to cover this module. This can wait until after we're agreed on design but would be good to have before PR lands.

Yep, I agree.

Library/Homebrew/cmd/reinstall.rb

MikeMcQuaid · 2022-07-14T08:02:29Z

docs/Manpage.md

@@ -1945,6 +1945,9 @@ example, run `export HOMEBREW_NO_INSECURE_REDIRECT=1` rather than just

  *Default:* `300`.

+- `HOMEBREW_AUTOREMOVE`
+  <br>If set, calls to `brew install`, `brew upgrade`, `brew reinstall` and `brew uninstall` will automatically remove unused formula dependents.


Suggested change

<br>If set, calls to `brew install`, `brew upgrade`, `brew reinstall` and `brew uninstall` will automatically remove unused formula dependents.

<br>If set, calls to `brew cleanup` and `brew uninstall` will automatically remove unused formula dependents. Unless HOMEBREW_NO_INSTALL_CLEANUP is set, `brew cleanup` will automatically remove unused formula dependents every 30 days.

I thought we were going to test this out for a while first before making it the default. This would make running brew autoremove every 30 days the default. Is that what we want?

Sorry, what I was intending with this messaging is "if you set HOMEBREW_AUTOREMOVE and not HOMEBREW_NO_INSTALL_CLEANUP: brew cleanup will start doing brew autoremove periodically".

Cleanup.rb: - Added #autoremove method - #autoremove is called in clean when HOMEBREW_AUTOREMOVE is set Formula.rb: - Added #unused_formulae_with_no_dependents and helpers Removed old autoremove.rb module.

MikeMcQuaid

This is great! A few tiny optional style nits and some unit tests and I think this is good to go!

Library/Homebrew/cleanup.rb

Library/Homebrew/cmd/uninstall.rb

Library/Homebrew/formula.rb

Also, made sure to clear formula cache before and after autoremoving packages.

Updated docs for: HOMEBREW_AUTOREMOVE HOMEBREW_NO_CLEANUP_FORMULAE

apainintheneck · 2022-07-17T16:33:16Z

Okay, I added a few tests to make sure dependent formulae get filtered out before removing them. Would an integration test make sense here or do you think that's enough?

The docs were updated as well for two environment variables.

HOMEBREW_AUTOREMOVE : If set, calls to brew cleanup and brew uninstall will automatically remove unused formula dependents and if HOMEBREW_NO_INSTALL_CLEANUP is not set, brew cleanup will start running brew autoremove periodically.
HOMEBREW_NO_CLEANUP_FORMULAE : A comma-separated list of formulae. Homebrew will refuse to clean up or autoremove a formula if it appears on this list.

The other main thing that was changed is that two calls to Formula#clear_cache were added to Cleanup#autoremove. These are needed because the cached list of installed formula will get invalidated by a preceding call to brew uninstall and may get invalidated by a preceding call to brew install, brew reinstall or brew upgrade when run periodically after brew cleanup. The formulae cache also gets cleared after autoremoving because the list of installed formula will be invalid again. This is probably not essential (I couldn't find any code that relied on that cache that would run after Cleanup#autoremove) but better safe than sorry, right?

Library/Homebrew/cleanup.rb

Library/Homebrew/cmd/autoremove.rb

apainintheneck · 2022-07-19T06:24:31Z

I've also been wondering if we should update the tab associated with a formula if it was originally installed as a dependency but then the user tries to install it explicitly later on (installed-as-dependency -> installed-on-request). In theory, that would mean that the user would probably want it to not get included in an autoremove cycle but I'm not sure if changing that behavior would break something else. Just an idea.

MikeMcQuaid · 2022-07-19T08:47:27Z

I've also been wondering if we should update the tab associated with a formula if it was originally installed as a dependency but then the user tries to install it explicitly later on

I think we already do? Also, note that "installed as dependency" does not imply "not installed as request"; both can be true.

apainintheneck · 2022-07-30T04:12:15Z

I think we already do? Also, note that "installed as dependency" does not imply "not installed as request"; both can be true.

You're absolutely right. I had just missed it.

MikeMcQuaid

Great work here! Happy for this to ship as-is or with tiny suggested changed.

MikeMcQuaid · 2022-08-01T13:09:36Z

Library/Homebrew/test/cmd/autoremove_spec.rb

+      expect(testball2.any_version_installed?).to be true
+
+      # When there are no unused dependencies
+      expect { brew "autoremove" }


Ideally I'd say only have one brew invocation in this test, even if it reduces the coverage a bit, as it's super slow.

On my machine there's only like a 5 second difference between running one and two tests and I'm still using this old iMac. Not sure how important that 5 seconds is. But yeah, I can take one out if you want.

I think important enough to omit tbh, sorry!

Got it. Changed it to only one brew invocation in the integration test.

MikeMcQuaid

Great work! Merge when ready.

BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Jul 8, 2022

dawidd6 approved these changes Jul 8, 2022

View reviewed changes

carlocab approved these changes Jul 9, 2022

View reviewed changes

carlocab reviewed Jul 9, 2022

View reviewed changes

Library/Homebrew/cmd/uninstall.rb Show resolved Hide resolved

apainintheneck force-pushed the add-uninstall-autoremove branch from 997b4a0 to 80d9d37 Compare July 9, 2022 16:46

MikeMcQuaid reviewed Jul 11, 2022

View reviewed changes

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

BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Jul 11, 2022

BrewTestBot approved these changes Jul 11, 2022

View reviewed changes

apainintheneck force-pushed the add-uninstall-autoremove branch from 0e947e7 to c686739 Compare July 12, 2022 08:16

MikeMcQuaid reviewed Jul 12, 2022

View reviewed changes

apainintheneck added 4 commits July 13, 2022 16:28

cmd/uninstall: Add env variable that runs autoremove after uninstalls

f8ae5f3

When HOMEBREW_UNINSTALL_AUTOREMOVE is set, `brew autoremove` is run after every successful call to `brew uninstall`.

autoremove.rb: moved logic out into formula.rb, and uninstall.rb

74f1eca

This allows us to autoremove formulae in the autoremove and uninstall commands without having to shell out to brew.

Changed env variable to HOMEBREW_AUTOREMOVE

74f7b60

Moved autoremove logic into autoremove.rb

fa43418

This allows us to call that logic internally in other brew commands instead of having to shell out.

apainintheneck force-pushed the add-uninstall-autoremove branch from c686739 to fa43418 Compare July 13, 2022 23:57

MikeMcQuaid reviewed Jul 14, 2022

View reviewed changes

Moved autoremove logic into cleanup.rb and formula.rb

fe83500

Cleanup.rb: - Added #autoremove method - #autoremove is called in clean when HOMEBREW_AUTOREMOVE is set Formula.rb: - Added #unused_formulae_with_no_dependents and helpers Removed old autoremove.rb module.

MikeMcQuaid reviewed Jul 15, 2022

View reviewed changes

Library/Homebrew/cleanup.rb Outdated Show resolved Hide resolved

Library/Homebrew/cmd/uninstall.rb Show resolved Hide resolved

carlocab reviewed Jul 15, 2022

View reviewed changes

Library/Homebrew/formula.rb Show resolved Hide resolved

apainintheneck added 2 commits July 17, 2022 00:39

Add tests for retrieving formula based on dependents

cb91f8b

Also, made sure to clear formula cache before and after autoremoving packages.

Updated env variable docs on manpage

85cf0ca

Updated docs for: HOMEBREW_AUTOREMOVE HOMEBREW_NO_CLEANUP_FORMULAE

apainintheneck mentioned this pull request Jul 18, 2022

Run periodic cleanup after installing all packages #13569

Merged

7 tasks

carlocab reviewed Jul 18, 2022

View reviewed changes

Library/Homebrew/cleanup.rb Show resolved Hide resolved

MikeMcQuaid reviewed Jul 18, 2022

View reviewed changes

Library/Homebrew/cmd/autoremove.rb Show resolved Hide resolved

apainintheneck added the in progress Maintainers are working on this label Jul 23, 2022

apainintheneck force-pushed the add-uninstall-autoremove branch 2 times, most recently from 54bd5d2 to 7bd06f6 Compare July 30, 2022 20:37

apainintheneck removed the in progress Maintainers are working on this label Jul 31, 2022

MikeMcQuaid approved these changes Aug 1, 2022

View reviewed changes

Add integration tests for autoremove cmd

d04051a

apainintheneck force-pushed the add-uninstall-autoremove branch from 7bd06f6 to d04051a Compare August 3, 2022 01:21

MikeMcQuaid approved these changes Aug 3, 2022

View reviewed changes

apainintheneck merged commit 93bf9e5 into Homebrew:master Aug 4, 2022

github-actions bot added the outdated PR was locked due to age label Sep 4, 2022

github-actions bot locked as resolved and limited conversation to collaborators Sep 4, 2022

apainintheneck deleted the add-uninstall-autoremove branch January 3, 2023 00:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/uninstall: Add env variable that runs autoremove after uninstalls #13532

cmd/uninstall: Add env variable that runs autoremove after uninstalls #13532

apainintheneck commented Jul 8, 2022

BrewTestBot commented Jul 8, 2022

dawidd6 left a comment

MikeMcQuaid left a comment

BrewTestBot commented Jul 11, 2022

apainintheneck commented Jul 12, 2022

MikeMcQuaid left a comment

MikeMcQuaid Jul 12, 2022

carlocab Jul 12, 2022

MikeMcQuaid Jul 12, 2022

carlocab Jul 12, 2022

apainintheneck Jul 13, 2022

MikeMcQuaid Jul 13, 2022

apainintheneck commented Jul 14, 2022 •

edited

apainintheneck commented Jul 14, 2022

MikeMcQuaid Jul 14, 2022

apainintheneck Jul 14, 2022

MikeMcQuaid Jul 14, 2022

apainintheneck Jul 14, 2022

MikeMcQuaid Jul 14, 2022

MikeMcQuaid left a comment

apainintheneck commented Jul 17, 2022

apainintheneck commented Jul 19, 2022 •

edited

MikeMcQuaid commented Jul 19, 2022

apainintheneck commented Jul 30, 2022

MikeMcQuaid left a comment

MikeMcQuaid Aug 1, 2022

apainintheneck Aug 2, 2022

MikeMcQuaid Aug 2, 2022

apainintheneck Aug 3, 2022

MikeMcQuaid left a comment

	<br>If set, calls to `brew install`, `brew upgrade`, `brew reinstall` and `brew uninstall` will automatically remove unused formula dependents.
	<br>If set, calls to `brew cleanup` and `brew uninstall` will automatically remove unused formula dependents. Unless HOMEBREW_NO_INSTALL_CLEANUP is set, `brew cleanup` will automatically remove unused formula dependents every 30 days.

cmd/uninstall: Add env variable that runs autoremove after uninstalls #13532

cmd/uninstall: Add env variable that runs autoremove after uninstalls #13532

Conversation

apainintheneck commented Jul 8, 2022

BrewTestBot commented Jul 8, 2022

dawidd6 left a comment

Choose a reason for hiding this comment

MikeMcQuaid left a comment

Choose a reason for hiding this comment

BrewTestBot commented Jul 11, 2022

apainintheneck commented Jul 12, 2022

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apainintheneck commented Jul 14, 2022 • edited

apainintheneck commented Jul 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid left a comment

Choose a reason for hiding this comment

apainintheneck commented Jul 17, 2022

apainintheneck commented Jul 19, 2022 • edited

MikeMcQuaid commented Jul 19, 2022

apainintheneck commented Jul 30, 2022

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid left a comment

Choose a reason for hiding this comment

apainintheneck commented Jul 14, 2022 •

edited

apainintheneck commented Jul 19, 2022 •

edited