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

Fix uninstall not working when version changes. #23328

Closed
wants to merge 12 commits into from
Closed

Fix uninstall not working when version changes. #23328

wants to merge 12 commits into from

Conversation

reitermarkus
Copy link
Member

@reitermarkus reitermarkus commented Aug 1, 2016

Changes to the core


Todo

  • Add test for message when other versions remain installed.
  • Add test for uninstalling multiple versions consecutively.

#2988
#14058

@reitermarkus reitermarkus added awaiting maintainer feedback Issue needs response from a maintainer. core Issue with Homebrew itself rather than with a specific cask. labels Aug 1, 2016
timestamped_versions.map(&:first)
.reverse
.uniq
.reverse
Copy link
Contributor

Choose a reason for hiding this comment

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

Why reverse twice? Isn't .uniq.reverse sufficient?

Copy link
Member Author

Choose a reason for hiding this comment

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

Consider this:

timestamped_versions = [['a', '1'], ['b', '2'], ['a', '3']]

timestamped_versions.map(&:first) --> ['a', 'b', 'a']
                    .uniq         --> ['a', 'b']

Now, .last doesn't actually return the version which was installed last.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, gotcha.

@jawshooah
Copy link
Contributor

I think a change of this magnitude warrants some new tests. Once those are added and the currently failing tests are fixed, I'm all for merging this 👍

@reitermarkus
Copy link
Member Author

@jawshooah, what exactly is PERSISTENT_METADATA_SUBDIRS? Is the gpg sub-directory used for anything else than signature.asc?

According to fetch_sig in gpg.rb, this is only used for caching. Wouldn't Hbc.cache be a better place for signature.asc files?

@jawshooah
Copy link
Contributor

@reitermarkus I'm not sure. Looks like it was added in #6117 "to provide persistent per-distribution locations to for GPG data". I'd be fine with moving the signature file to Hbc.cache

@reitermarkus
Copy link
Member Author

reitermarkus commented Aug 8, 2016

Ah, I had an error in thinking:

I thought the gpg directory would be atcaskroom/cask/.metadata/gpg, which would result in a conflict when globbing the .metadata directory to retreive timestamped versions.

But actually it's stored at caskroom/cask/.metadata/version/timestamp/gpg, so there's no conflict.

Still, there's not much use keeping persistent data in a timestamped directory.

@reitermarkus
Copy link
Member Author

@jawshooah, could you take a look at the test I have added (and the one I have altered)?

Also, not sure what would be the best way to reuse the code from the uninstalls one version at a time. MiniTest doesn't have a before(:each), right?

Or should I simply add the test for the cask 1.2.3 is still installed message into the existing one?

@reitermarkus
Copy link
Member Author

reitermarkus commented Aug 11, 2016

Ok, apparently after and before are equivalent to teardown and setup. Strange thing is, after and before are working, teardown and setup are not.

@jawshooah
Copy link
Contributor

@reitermarkus LGTM, merge when ready 👍

@reitermarkus
Copy link
Member Author

This pull request was merged in 36a98c5...1835d6d.

reitermarkus added a commit that referenced this pull request Aug 13, 2016
reitermarkus added a commit that referenced this pull request Aug 13, 2016
@reitermarkus reitermarkus deleted the uninstall branch August 13, 2016 12:08
jedahan pushed a commit to jedahan/homebrew-cask that referenced this pull request Sep 24, 2016
jedahan pushed a commit to jedahan/homebrew-cask that referenced this pull request Sep 24, 2016
jedahan pushed a commit to jedahan/homebrew-cask that referenced this pull request Sep 24, 2016
jedahan pushed a commit to jedahan/homebrew-cask that referenced this pull request Sep 24, 2016
jedahan pushed a commit to jedahan/homebrew-cask that referenced this pull request Sep 24, 2016
jedahan pushed a commit to jedahan/homebrew-cask that referenced this pull request Sep 24, 2016
jedahan pushed a commit to jedahan/homebrew-cask that referenced this pull request Sep 24, 2016
jedahan pushed a commit to jedahan/homebrew-cask that referenced this pull request Sep 24, 2016
jedahan pushed a commit to jedahan/homebrew-cask that referenced this pull request Sep 24, 2016
jedahan pushed a commit to jedahan/homebrew-cask that referenced this pull request Sep 24, 2016
jedahan pushed a commit to jedahan/homebrew-cask that referenced this pull request Sep 24, 2016
jedahan pushed a commit to jedahan/homebrew-cask that referenced this pull request Sep 24, 2016
@miccal miccal removed the awaiting maintainer feedback Issue needs response from a maintainer. label Dec 15, 2016
@miccal miccal removed the core Issue with Homebrew itself rather than with a specific cask. label Dec 23, 2016
@Homebrew Homebrew locked and limited conversation to collaborators May 9, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants