Virtualenv relocation fixes #2442
Conversation
When we're assessing whether a bottle is relocatable, we shouldn't have to descend into symlink paths we encounter. This is supposed to be the default behavior but it doesn't appear to be (perhaps because we pass a symlink to the keg on the command line?). All of the switches that control this behavior differ between BSD and GNU grep, so sniff the grep flavor first.
ln_sf does the right thing when `dest` is a symlink pointing to a file: the symlink gets overwritten with a link pointing to the new src. But when dest points to a directory, we create a new symlink inside the folder dest points to, which doesn't help us at all.
Library/Homebrew/keg_relocate.rb
Outdated
| @@ -97,7 +99,12 @@ def detect_cxx_stdlibs(_options = {}) | |||
| end | |||
|
|
|||
| def each_unique_file_matching(string) | |||
| Utils.popen_read("/usr/bin/fgrep", "-lr", string, to_s) do |io| | |||
| bsd = `/usr/bin/fgrep -V`.include?("BSD grep") | |||
There was a problem hiding this comment.
Favour Utils.popen_read over backticks.
There was a problem hiding this comment.
Also I reckon BSD grep logic should live in https://github.com/Homebrew/brew/blob/master/Library/Homebrew/extend/os/mac/keg_relocate.rb or the converse in os/linux
There was a problem hiding this comment.
I think I'd have to copy the body of the method into both os/mac/keg_relocate.rb and os/linux/keg_relocate.rb and just change the arguments that are being passed to grep. Is that right? It feels repetitive.
Why not backticks, out of curiosity? Consistency or something more subtle?
There was a problem hiding this comment.
(or do I want to write e.g. a private each_unique_file_matching_grep_args method for each OS?)
There was a problem hiding this comment.
Why not backticks, out of curiosity? Consistency or something more subtle?
I dunno, it's just a pattern we've been following here for a while. I think we handle them slightly differently. I don't care enormously, to be honest.
I think I'd have to copy the body of the method into both os/mac/keg_relocate.rb and os/linux/keg_relocate.rb and just change the arguments that are being passed to grep. Is that right? It feels repetitive.
Ideally use a separate grep_arguments method for just getting the arguments which should make it pretty small. Up to you to decide what the generic_grep_arguments version should be but I think there's only a need for a single override e.g. assume BSD here and override in os/linux or assume GNU here and override in os/mac.
Library/Homebrew/keg_relocate.rb
Outdated
| FileUtils.ln_sf(link.relative_path_from(file.parent), file) | ||
| new_src = link.relative_path_from(file.parent) | ||
| file.unlink | ||
| FileUtils.ln_s(new_src, file) |
There was a problem hiding this comment.
How does this differ from before? Not disagreeing, just wanting to understand.
There was a problem hiding this comment.
Oh, I mentioned in the commit message; shoulda copied it into the PR body:
ln_sf does the right thing when `dest` is a symlink pointing to a file:
the symlink gets overwritten with a link pointing to the new src. But
when dest points to a directory, we create a new symlink inside the
folder dest points to, which doesn't help us at all.
|
Need to look at the tests: |
Library/Homebrew/keg_relocate.rb
Outdated
| if link.to_s.start_with?(HOMEBREW_CELLAR.to_s) || link.to_s.start_with?(HOMEBREW_PREFIX.to_s) | ||
| FileUtils.ln_sf(link.relative_path_from(file.parent), file) | ||
| end | ||
| next unless link.to_s.start_with?(HOMEBREW_CELLAR.to_s) || link.to_s.start_with?(HOMEBREW_PREFIX.to_s) |
There was a problem hiding this comment.
Can you make this two next unlesss for line length and for stupid Mike who can't handle unless ||.
Last thing and otherwise looks 👍
There was a problem hiding this comment.
I don't think I can; next if a || b can be written next if a; next if b and next unless a && b can be written next unless a; next unless b but I don't see what to do here. My boolean logic or my imagination might be failing me.
There was a problem hiding this comment.
next if !a && !b would work and is maybe easier
There was a problem hiding this comment.
My boolean logic or my imagination might be failing me.
No but mine was which is why I had unless || 😁
Yeh, next if !a && !b and perhaps putting them into variables for length is a little easier to follow.
|
Thanks @tdsmith! |
Fix a couple of issues discovered in Homebrew/homebrew-core#9424 that affect the relocatability of virtualenv formulae.