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

keg: fix fallback dependencies with multiple kegs #1752

Merged
merged 2 commits into from
Dec 31, 2016

Conversation

alyssais
Copy link
Contributor

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

We didn't remove formulae to be uninstalled from the list of installed formulae to check for dependencies, but only in the fallback. This is what caused #1524, and made it difficult to reproduce (because one had to realise that it was only in the fallback).

f.installed_kegs.any? { |k| Tab.for_keg(k).runtime_dependencies.nil? }
installed_kegs = f.installed_kegs

# All installed kegs are going to be removed anyway,
Copy link
Member

Choose a reason for hiding this comment

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

Can you elaborate a bit more on this? The "removed anyway" bit makes sense in context of the PR but I'm not sure it does in the context of the function itself which isn't necessarily relating specifically to uninstall.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. The purpose of Keg.find_some_installed_dependents is to take some kegs and see if there are any kegs outside of that array that depend on those kegs. (That's why it takes an array.) This is what it does currently when the install receipts are reliable.

But, in the fallback, I forgot to remove the passed kegs from the list of dependents, so now I've fixed that.

Copy link
Member

Choose a reason for hiding this comment

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

Cool, makes sense. I think the comment could be expanded on a bit to make this clearer e.g. why they will be "removed"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do 👍

@MikeMcQuaid MikeMcQuaid merged commit d85a83c into Homebrew:master Dec 31, 2016
@MikeMcQuaid
Copy link
Member

Perfect. Great comments.

@MikeMcQuaid
Copy link
Member

Thanks @alyssais!

@alyssais alyssais deleted the uninstalling_dependencies branch December 31, 2016 17:49
@Homebrew Homebrew locked and limited conversation to collaborators May 3, 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.

2 participants