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

Implement the HOMEBREW_INSTALL_CLEANUP env to trigger cleanup on reinstall/install/upgrade #5165

Merged
merged 4 commits into from Nov 6, 2018

Conversation

Projects
None yet
2 participants
@Wojonatior
Copy link
Contributor

Wojonatior commented Oct 23, 2018

  • 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 tests with your changes locally?

This PR is thethe implementation of the HOMEBREW_INSTALL_CLEANUP ENV and associated features as discussed in #4760.

setup_test_formula "testball"
# allow(ENV).to receive(:[]).and_call_original
# allow(ENV).to receive(:[]).with("HOMEBREW_INSTALL_CLEANUP").and_return("1")
ENV["HOMEBREW_INSTALL_CLEANUP"] = "1"

This comment has been minimized.

@Wojonatior

Wojonatior Oct 23, 2018

Contributor

I've tried both manually setting into ENV and mocking it out as shown in the 2 commented lines. I wasn't able to get either working even though I could confirm the functionality manually.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Oct 24, 2018

Member

@Wojonatior Ideally we wouldn't add another integration test for this (they are really slow). Either add a unit test or omit one entirely.

@@ -3,7 +3,7 @@
#:
#: Options for the `install` command are also valid here.
#:
#: If `--cleanup` is specified or `HOMEBREW_UPGRADE_CLEANUP` is set then remove
#: If `--cleanup` is specified, `HOMEBREW_INSTALL_CLEANUP` or `HOMEBREW_UPGRADE_CLEANUP` is set then remove

This comment has been minimized.

@Wojonatior

Wojonatior Oct 23, 2018

Contributor

As a clarification around the original feature, is the intent to keep both ENVs or to enhance what the old one did as well a rename it?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Oct 24, 2018

Member

@Wojonatior Good question 👍 Let's remove the documentation for HOMEBREW_UPGRADE_CLEANUP in favour of HOMEBREW_INSTALL_CLEANUP but keep the HOMEBREW_UPGRADE_CLEANUP variable functional for people who are still using it.

@MikeMcQuaid
Copy link
Member

MikeMcQuaid left a comment

Nice start here! I'd like to see cmd/install changed similarly before this is merged.

setup_test_formula "testball"
# allow(ENV).to receive(:[]).and_call_original
# allow(ENV).to receive(:[]).with("HOMEBREW_INSTALL_CLEANUP").and_return("1")
ENV["HOMEBREW_INSTALL_CLEANUP"] = "1"

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Oct 24, 2018

Member

@Wojonatior Ideally we wouldn't add another integration test for this (they are really slow). Either add a unit test or omit one entirely.

@@ -3,7 +3,7 @@
#:
#: Options for the `install` command are also valid here.
#:
#: If `--cleanup` is specified or `HOMEBREW_UPGRADE_CLEANUP` is set then remove
#: If `--cleanup` is specified, `HOMEBREW_INSTALL_CLEANUP` or `HOMEBREW_UPGRADE_CLEANUP` is set then remove

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Oct 24, 2018

Member

@Wojonatior Good question 👍 Let's remove the documentation for HOMEBREW_UPGRADE_CLEANUP in favour of HOMEBREW_INSTALL_CLEANUP but keep the HOMEBREW_UPGRADE_CLEANUP variable functional for people who are still using it.

@Wojonatior

This comment has been minimized.

Copy link
Contributor

Wojonatior commented Oct 29, 2018

I've added the formula cleanup to the install and reinstall commands. I wasn't sure if it was supposed to be the formula cleanup or the full/regular cleanup. The issue seems to discuss the formula cleanup, but I'm not sure what would need to be cleaned up from the formula on an install/reinstall

@@ -73,6 +73,9 @@
#:
#: If `--git` (or `-g`) is passed, Homebrew will create a Git repository, useful for
#: creating patches to the software.
#:
#: If `HOMEBREW_INSTALL_CLEANUP` is set then remove previously installed versions
#: of upgraded <formulae>.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Oct 30, 2018

Member

This also will clean up the HOMEBREW_CACHE for that formula

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Oct 30, 2018

@Wojonatior After fixing the docs please run brew man and commit the results.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Oct 30, 2018

I wasn't sure if it was supposed to be the formula cleanup or the full/regular cleanup.

Formula cleanup 👍. Could have regular cleanup occurring every e.g. 100 installs or something, though? Thoughts?

The issue seems to discuss the formula cleanup, but I'm not sure what would need to be cleaned up from the formula on an install/reinstall

Nothing specifically but there may be leftover cache and cellar entries that it's worth cleaning up at this stage.

@Wojonatior

This comment has been minimized.

Copy link
Contributor

Wojonatior commented Oct 30, 2018

@MikeMcQuaid I updated the docs and the manpage. I like the idea of the occasional full clean. Would it make sense to count that in the formula clean or specifically for the install? Is there a pattern established somewhere for how I would persist the number of installs?

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Oct 31, 2018

Would it make sense to count that in the formula clean or specifically for the install? Is there a pattern established somewhere for how I would persist the number of installs?

I was thinking that rather than being by installs it could be by days. brew cleanup could write (or touch if it already exists) a file like HOMEBREW_CACHE/.cleaned which the mtime is checked if HOMEBREW_INSTALL_CLEANUP is set and a full, non-formula cleanup could be run at that time if it hasn't been run for e.g. 30 days (that's how often git gc is run). If we want to be safer still that brew cleanup could also only remove files/directories that would be removed anyway and are 30 days old (but that may be overkill).

@MikeMcQuaid MikeMcQuaid merged commit 6b1d439 into Homebrew:master Nov 6, 2018

3 checks passed

codecov/patch 100% of diff hit (target 70.62%)
Details
codecov/project Absolute coverage decreased by -0.02% but relative coverage increased by +29.38% compared to 18bac4f
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Nov 6, 2018

Thanks so much for your first contribution! Without people like you submitting PRs we couldn't run this project. You rock, @Wojonatior! This is useful as-is so figured it was worth merging. Would still love to see the stuff we talked about in a follow-up PR!

@Wojonatior

This comment has been minimized.

Copy link
Contributor

Wojonatior commented Nov 6, 2018

@MikeMcQuaid Thanks! Sorry about the lack of communication on my part. I definitely want to make the follow-up PR for this, you should hopefully see something around it in the next week or so.

@Wojonatior Wojonatior deleted the Wojonatior:feature/homebrew_install_cleanup branch Nov 6, 2018

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Nov 7, 2018

@Wojonatior No worries, looking forward to it!

@lock lock bot added the outdated label Dec 7, 2018

@lock lock bot locked as resolved and limited conversation to collaborators Dec 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.