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

keg: correctly cleanup old aliases. #2923

Merged
merged 3 commits into from Jul 30, 2017

Conversation

Projects
None yet
3 participants
@MikeMcQuaid
Copy link
Member

MikeMcQuaid commented Jul 21, 2017

Cleanup old, versioned aliases and tap aliases folders when unlinking or uninstalling a keg.

Also:

  • Include aliases in the tab to allow e.g. brew switch to correctly handle switching between different keg’s aliases.
  • Ensure aliases don’t include full names (aliases that include /s end up creating directories and this is bad).

Fixes #2937

MikeMcQuaid added some commits Jul 21, 2017

formula: ensure aliases don’t include full names.
Aliases that include `/`s end up creating directories and this is bad.
tab: include aliases.
Including aliases in the tab allows e.g. `brew switch` to correctly
handle switching between different keg’s aliases.
FileUtils.rm_rf bad_tap_opt if bad_tap_opt.directory?

Pathname.glob("#{opt_record}@*").each do |a|
next if aliases.include?(a.basename)

This comment has been minimized.

@ilovezfs

ilovezfs Jul 21, 2017

Contributor

may want to skip if it's an alias for a different rack

@MikeMcQuaid MikeMcQuaid force-pushed the MikeMcQuaid:alias-fixes branch from 20b0e73 to f86014f Jul 27, 2017

@MikeMcQuaid MikeMcQuaid force-pushed the MikeMcQuaid:alias-fixes branch 2 times, most recently from 7aff27f to daeb6bb Jul 27, 2017

@MikeMcQuaid MikeMcQuaid force-pushed the MikeMcQuaid:alias-fixes branch from daeb6bb to e9e5b7c Jul 28, 2017

keg: correctly cleanup old aliases.
Cleanup old, versioned aliases and tap aliases folders when unlinking
or uninstalling a keg.

@MikeMcQuaid MikeMcQuaid force-pushed the MikeMcQuaid:alias-fixes branch from e9e5b7c to 1651647 Jul 28, 2017

@MikeMcQuaid MikeMcQuaid merged commit e8c4989 into Homebrew:master Jul 30, 2017

3 checks passed

codecov/patch 72.72% of diff hit (target 66.55%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +6.17% compared to ac2cbd2
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@MikeMcQuaid MikeMcQuaid deleted the MikeMcQuaid:alias-fixes branch Jul 30, 2017

@soonho-tri

This comment has been minimized.

Copy link

soonho-tri commented on Library/Homebrew/keg.rb in 1651647 Mar 31, 2018

Dear @MikeMcQuaid ,

I think I experience a problem because of this then-branch.

Symptom

After upgrading a formula (say, dreal), running brew cleanup dreal removes /usr/local/opt/dreal even if the new version is still installed.

Reproduce steps:

brew tap dreal/dreal    # Hosted in https://github.com/dreal/homebrew-dreal

# Install an old version
brew install https://raw.githubusercontent.com/dreal/homebrew-dreal/feaa4e1c18e7e8395480bbe34c4903c4f2fe05c9/dreal.rb 
brew update
brew upgrade dreal

# Up to this point, /usr/local/opt/dreal exists.

brew cleanup dreal

# `brew cleanup` removes /usr/local/opt/dreal

Note

My understanding is that I'm in a bad combination 1) My formula is in a Tap, 2) The tap is hosted in dreal/homebrew-dreal where the user part is also dreal which is the name of formula dreal.

Could you confirm that my understanding is correct? Is there a naming convention such that a formula name should be different from its user name? Please let me know if you think I should open an issue. Thank you.

This comment has been minimized.

Copy link
Member

MikeMcQuaid replied Mar 31, 2018

Install an old version

brew install https://raw.githubusercontent.com/dreal/homebrew-dreal/feaa4e1c18e7e8395480bbe34c4903c4f2fe05c9/dreal.rb

This is not a supported way to install old versions.

This comment has been minimized.

Copy link

soonho-tri replied Mar 31, 2018

I see. I mainly wanted to set up an environment where this branch is triggered.

I guess something like the following is better? (found from S&O).

brew tap dreal/dreal

# Edit the formula to install an old version
brew edit dreal # Change the `url` part to have `4.18.03.2` (line 7)
                # Also comment out sha256                   (line 8)

brew install dreal  # It will install 4.18.03.2

brew edit dreal # Change line 7 to have `4.18.03.3`
brew upgrade dreal

# Check if /usr/local/opt/dreal exists
ls /usr/local/opt/dreal

brew cleanup dreal

# Check if /usr/local/opt/dreal exists
ls /usr/local/opt/dreal

I've tried these locally and am still able to reproduce the problem.

This comment has been minimized.

Copy link
Member

MikeMcQuaid replied Mar 31, 2018

Can you reproduce it with a formula in Homebrew/homebrew-core? What aliases does dreal have, if any?

This comment has been minimized.

Copy link

soonho-tri replied Mar 31, 2018

I think the issue is that the fullname of my formula is dreal/dreal/dreal. As you know the format is <user>/<repo>/<formula>. In my case, I have user = dreal, formula = dreal. I believe this is why when brew cleanup removes outdated version(s) of dreal, it removes /usr/local/opt/dreal.

I've added a renamed formula dreal/dreal/dreal4 (no change in the .rb file) to see if it gives me a different behavior. I confirm that dreal4 does not show the problem (i.e. /usr/local/opt/dreal4 still exists after brew cleanup dreal4).

I think to make simplest reproducible step, one has to create either a github user or github org named <A> and create a formula <A> (i.e. <A>/something/<A>). Please let me know if you want me to set things up. Also, please let me know if you want me to open an issue for it.

This comment has been minimized.

Copy link

soonho-tri replied Mar 31, 2018

Can you reproduce it with a formula in Homebrew/homebrew-core?

I don't think so unless I am allowed to add a formula homebrew.rb there.

What aliases does dreal have, if any?

None.

This comment has been minimized.

Copy link
Member

MikeMcQuaid replied Mar 31, 2018

Ah, I see what you mean now. I'll look at a fix thanks.

This comment has been minimized.

Copy link

soonho-tri replied Mar 31, 2018

Thank you!!

@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018

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