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
formula_installer: fix :test requirement expansion. #7613
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not fully awake so hopefully I followed the logic correctly.
keep ||= req.test? && include_test? && dependent == f | ||
keep ||= req.build? && !install_bottle_for_dependent | ||
keep ||= (dep = formula_deps_map[dependent.name]) && !dep.build? | ||
Requirement.prune unless keep |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the case keep
is true, I think we want to fallthrough to the else
case. At this point, the requirment has already been established as not satisfied (line 458).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeh, good call. Pushed a tweak.
607e1fa
to
89a0984
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is good. There's probably still edge cases like if formula_deps_map[dependent.name]
is a test dependency, but we would probably need a set of test cases to discover them all.
Thanks @Bo98. |
CC @Bo98 as we talked about this.
brew style
with your changes locally?brew tests
with your changes locally?