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

Make runtime dependencies handling more consistent #3979

Merged
merged 10 commits into from Mar 28, 2018

Conversation

Projects
None yet
3 participants
@MikeMcQuaid
Copy link
Member

MikeMcQuaid commented Mar 25, 2018

Previously runtime dependencies were stored in the tab but these values were only used by brew uninstall. Confusingly this would result in various commands (e.g. brew leaves, brew missing, brew doctor, brew uses --installed) contradicting brew uninstall in what the dependencies are for a formula.

This refactoring means that Formula#runtime_dependencies will now look at the formula's tab if possible (and if not specifically requested not to). After this, Formula#runtime_dependencies has been able to be used in more places so all the above commands can be consistent.

While I was here a few other tweaks have been made:

  • diagnostic and leaves didn't need changes but, while looking there, I was able to clean them up a bit for more readable and simple code
  • Tab now defaults runtime_dependencies to nil rather than an empty array. This is useful in differentiating between runtime_dependencies being not present in a Tab vs. there being no runtime dependencies. The nil case already needed handled because of returning nil for old, invalid runtime_dependencies anyway.

@MikeMcQuaid MikeMcQuaid requested review from alyssais and ilovezfs Mar 25, 2018

@MikeMcQuaid MikeMcQuaid referenced this pull request Mar 25, 2018

Merged

Undeclared dependencies in runtime_dependencies #3789

4 of 6 tasks complete

@MikeMcQuaid MikeMcQuaid force-pushed the MikeMcQuaid:runtime_dependencies_changes branch from eec3637 to e5403b4 Mar 25, 2018

@alyssais
Copy link
Contributor

alyssais left a comment

I think that the fact that we'd have to introduce an option here hints at a bigger abstraction problem in Homebrew's code, that this would continue to make worse:

It seems off to me that asking a formula for its runtime dependencies would cause it to check its tab, because that isn't information about the formula, but about a specific installation of the formula (ie a keg). This ends up with us having to introduce options to specify whether we actually mean a formula itself, or its installed keg, which continues to reduce the separation of concerns and complicate the code with conditionals and options.

Something worth thinking about anyway, I think. I'm not against this change – in fact I think it would be good for usability. I vaguely remember bringing up this inconsistency a while ago, and the consensus at the time seemed to be that we didn't want to change the behaviour of leaves, uses etc, but if that's changed that's more than fine by me.

puts f.full_name unless deps_of_installed.include? f.full_name
end
leaves = installed.map(&:full_name) - deps_of_installed
leaves.each(&method(:puts))

This comment has been minimized.

@alyssais

alyssais Mar 26, 2018

Contributor

Is this any different to puts leaves?

This comment has been minimized.

@MikeMcQuaid

This comment has been minimized.

@alyssais

alyssais Mar 26, 2018

Contributor

Reckon that would be a little clearer

@MikeMcQuaid MikeMcQuaid force-pushed the MikeMcQuaid:runtime_dependencies_changes branch from e5403b4 to 688fc7a Mar 26, 2018

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Mar 26, 2018

It seems off to me that asking a formula for its runtime dependencies would cause it to check its tab, because that isn't information about the formula, but about a specific installation of the formula (ie a keg). This ends up with us having to introduce options to specify whether we actually mean a formula itself, or its installed keg, which continues to reduce the separation of concerns and complicate the code with conditionals and options.

I agree with this in principle but not in practice, I think. That said: yes, this could potentially be refactored so that every time you want Formula#runtime_dependencies to return the value from the Tab you actually want to query the relevant Keg but this ends up meaning that all the callers that are currently already asking a formula what the dependencies are now need to deal with a Keg instead.

Something worth thinking about anyway, I think. I'm not against this change – in fact I think it would be good for usability. I vaguely remember bringing up this inconsistency a while ago, and the consensus at the time seemed to be that we didn't want to change the behaviour of leaves, uses etc, but if that's changed that's more than fine by me.

Cool thanks 👍

@alyssais

This comment has been minimized.

Copy link
Contributor

alyssais commented Mar 26, 2018

this ends up meaning that all the callers that are currently already asking a formula what the dependencies are now need to deal with a Keg instead.

I don't think that would be a bad thing, although the effort to get there isn't necessarily worth it.

@MikeMcQuaid MikeMcQuaid force-pushed the MikeMcQuaid:runtime_dependencies_changes branch from 688fc7a to 8975bf0 Mar 27, 2018

@MikeMcQuaid MikeMcQuaid merged commit d1b8381 into Homebrew:master Mar 28, 2018

3 checks passed

codecov/patch 75.34% of diff hit (target 70.16%)
Details
codecov/project Absolute coverage decreased by -0.02% but relative coverage increased by +5.18% compared to 0851f96
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@MikeMcQuaid MikeMcQuaid deleted the MikeMcQuaid:runtime_dependencies_changes branch Mar 28, 2018

@lock lock bot added the outdated label May 4, 2018

@lock lock bot locked as resolved 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.