-
Notifications
You must be signed in to change notification settings - Fork 170
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
homebrew: accept keg-only packages as installed #799
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #799 +/- ##
==========================================
- Coverage 74.93% 74.87% -0.07%
==========================================
Files 42 42
Lines 3280 3283 +3
==========================================
Hits 2458 2458
- Misses 822 825 +3
Continue to review full report at Codecov.
|
@HesselM thanks for opening this PR and proposing changes to improve the situation with homebrew. My main concern with the change as proposed is keg-only dependencies will require additional setup to be usable and rosdep may mask that fact by reporting keg-only packages as installed. I think we definitely need keg-only packages to be recognized as installed but does it make sense to print an info message when a dependency is found as keg-only? |
I think it's important to have some notice to the user that the formula is keg-only and that rosdep hasn't resolved that issue, but maybe the caveat output of Homebrew during the rosdep install is sufficient? I don't feel strongly about it. |
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.
lgtm once the conceptual question is resolved.
Using keg-only formulae often requires adding some extra paths to environment variables (see this bit of code from the scripts for build.osrfoundation.org for an example). I was going to suggest the caveats printed by Homebrew as well, which can be viewed with
|
return False | ||
keg_only = pkg_info['keg_only'] | ||
if keg_only: | ||
return True |
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 code will return True
for any keg-only formula, even if it's not actually installed. Do you want to use the installed
field instead?
note that the caveats
field in the json does not match the full output printed by brew info
. I believe running brew ruby -e 'require "caveats"; puts Caveats.new("qt@5".f).keg_only_text'
can generate the full caveats string
fixes #764 #513 #490 (and others)
As suggested by @NikolausDemmel in his comment :
The issue is that qt5 is keg-only and rosdep marks it as "not installed", when it in fact, it is installed yet not symlinked as mentioned by brew during (re)installation:
As a result, Rosdep fails:
By checking if a package is keg-only when no linked keg is found, packages like Qt are accepted and rosdep finished clean.