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
bottle: Skip matches to files in build deps #5366
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.
Seems like a good approach, nice work 👍
Library/Homebrew/dev-cmd/bottle.rb
Outdated
@@ -178,6 +178,18 @@ def keg_contain?(string, keg, ignores) | |||
offset, match = str.split(" ", 2) | |||
next if linked_libraries.include? match # Don't bother reporting a string if it was found by otool | |||
|
|||
# Do not report matches to directories that do not exist. |
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.
Could this just check for paths that don't exist? Seems like it might be more specific and therefore more likely to find cellar :any
relocations.
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.
Sure
Library/Homebrew/dev-cmd/bottle.rb
Outdated
|
||
# Do not report matches to build dependencies. | ||
begin | ||
keg_name = Keg.for(dir).name |
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.
Does this match cases where it's a random path inside a Keg
rather than the actual keg?
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.
Yes
Library/Homebrew/dev-cmd/bottle.rb
Outdated
# Do not report matches to build dependencies. | ||
begin | ||
keg_name = Keg.for(dir).name | ||
next unless @runtime_deps.include? keg_name |
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.
Could this be passed through as an argument (to keg_contain?
) instead? More obvious to me than relying on a instance variable declared elsewhere.
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.
Done.
2c21d35
to
47d43d8
Compare
Thanks, Mike! |
Library/Homebrew/dev-cmd/bottle.rb
Outdated
next unless File.exist? match | ||
|
||
# Do not report matches to build dependencies. | ||
if runtime_deps && !runtime_deps.empty? |
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.
Make runtime_deps = []
by default or use if runtime_deps.present?
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 didn't know about .present?
. Nice!
Library/Homebrew/dev-cmd/bottle.rb
Outdated
@@ -272,6 +285,7 @@ def bottle_formula(f) | |||
|
|||
ohai "Bottling #{filename}..." | |||
|
|||
runtime_deps = [f.name] + f.runtime_dependencies.map(&:name) |
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.
formula_and_runtime_deps_names
?
Files in build dependencies are not required at run time.
@MikeMcQuaid Good to merge? |
Merged. Thanks for the review, Mike! |
Files in build dependencies are not required at run time.
brew style
with your changes locally?brew tests
with your changes locally?This PR addresses the issue that
topgrade
is detected as being non-relocatable on macOS, when it ought to becellar :any_skip_relocation
.For details see #5365 (comment)
cc @iMichka