Use patchelf.rb to set interpreter instead of patchelf. First step towards shelless elftools #7213
+36
−12
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I believe you'll need to run |
MikeMcQuaid
reviewed
Mar 24, 2020
|
Nice work so far here @rmNULL! |
thanks, i'll push. |
rmNULL
force-pushed
the
linux-shelless-elftools
branch
2 times, most recently
from
e403927 to
737a809
Compare
Thanks, i appreciate it. |
|
Looks like a great start to me, thanks @rmNULL! Will do some initial review tonight (ET). |
|
Would be good to see if it's possible to partially vendor any of the gems. >8000 lines added isn't great for a Linux-only dependency (but we'll suck it up if need be). |
most of the lines come from |
woodruffw
reviewed
Mar 25, 2020
Cool, that would be good. I ask because what I've done with some gems in the past is try to remove all files until it breaks so we can figure out what the bare minimum files (or at least directories) we need to include. This may be overkill in this case, though. |
sjackman
reviewed
Mar 25, 2020
sjackman
suggested changes
Mar 25, 2020
|
Nice work! I'm curious if it's faster or slower than before. It might be difficult to get good timing results, but could you try giving it a shot? Find a package with a lot of ELF files that need to be patched, and try timing it before and after. This isn't critical, but would be cool to see if it's actually faster. |
|
Could you please report the output of |
Note: When I did this my brew installation was at Imagespatch
master
Above shell interaction as text |
rmNULL
force-pushed
the
linux-shelless-elftools
branch
from
737a809 to
7148e9d
Compare
@sjackman I'm afraid it gets slower in some cases. benchmarkhello and ocaml
GCC and Boost
benchmark snippetrequire "benchmark"
class Keg
def relocate_dynamic_linkage(relocation)
# Patching the dynamic linker of glibc breaks it.
return if name == "glibc"
::Benchmark.bm(8) do |b|
b.report("PATCH") do
elf_files.each do |file|
file.ensure_writable do
change_rpath(file, relocation.old_prefix, relocation.new_prefix)
end
end
end
b.report("MASTER") do
elf_files.each do |file|
file.ensure_writable do
master_change_rpath(file, relocation.old_prefix, relocation.new_prefix)
end
end
end
end
end
def change_rpath(file, old_prefix, new_prefix)
# patch
end
def master_change_rpath(file, old_prefix, new_prefix)
# original code
end
# ....
endopencv install log |
|
Hi I'm the maintainer of patchelf.rb |
@david942j 😎. |
|
can u try with other browser, if that doesn't work, i'll reupload. |
@rmNULL Could you instead just drag and drop the images into the GitHub comments so they can be seen by everyone (regardless of browser)? Thanks! |
didn't know i could drag and drop, nice. The comment is updated |
|
@rmNULL I believe the time issue is a bug of your benchmark code :p ::Benchmark.bm(8) do |b|
b.report("PATCH") do
elf_files.each do |file|
file.ensure_writable do
change_rpath(file, relocation.old_prefix, relocation.new_prefix)
end
end
end
b.report("MASTER") do
elf_files.each do |file|
file.ensure_writable do
master_change_rpath(file, relocation.old_prefix, relocation.new_prefix)
end
end
end
endSince it's an in-place patch, the input file will be modified at the first time of patching. I simply swapped the order of "PATCH" and "MASTER" and got a totally different result: |
@david942j i had doubt about my bench marking code, thanks for pointing it out. Thanks |
|
NP. rollback = proc do
elf_files.each do |file|
file.ensure_writable do
master_change_rpath(file, relocation.new_prefix, relocation.old_prefix)
end
end
end
::Benchmark.bm(8) do |b|
b.report("PATCH") do
elf_files.each do |file|
file.ensure_writable do
change_rpath(file, relocation.old_prefix, relocation.new_prefix)
end
end
end
rollback.call
b.report("MASTER") do
elf_files.each do |file|
file.ensure_writable do
master_change_rpath(file, relocation.old_prefix, relocation.new_prefix)
end
end
end
rollback.call
endresult: So the Ruby ver is around ~4 times slower than patchelf. I feel it's a reasonable and acceptable difference since string manipulations in Ruby is expected to be much slower than pure C in this scale. |
sjackman
reviewed
Aug 11, 2020
sjackman
reviewed
Aug 11, 2020
sjackman
reviewed
Aug 11, 2020
rmNULL
force-pushed
the
linux-shelless-elftools
branch
from
c1ce2b7 to
7a8de09
Compare
sjackman
reviewed
Aug 16, 2020
sjackman
reviewed
Aug 16, 2020
sjackman
suggested changes
Aug 16, 2020
There was a problem hiding this comment.
Submodule patchelf.rb-3a4fe5e8d35c added at 3a4fe5
No Git submodules, please. We don't use submodules in Homebrew/brew.
$ git submodule init
fatal: No url found for submodule path 'Library/Homebrew/vendor/bundle/ruby/2.6.0/bundler/gems/patchelf.rb-3a4fe5e8d35c' in .gitmodules.gitmodules is also missing.
woodruffw
reviewed
Aug 24, 2020
Library/Homebrew/Gemfile
Outdated
| @@ -21,7 +21,7 @@ gem "tapioca" | |||
| gem "activesupport" | |||
| gem "concurrent-ruby" | |||
| gem "mechanize" | |||
| gem "patchelf" | |||
| gem "patchelf", git: "https://github.com/rmnull/patchelf.rb", ref: "3a4fe5e8" | |||
There was a problem hiding this comment.
This needs to be bumped to the latest work on david942j/patchelf.rb#28, right?
There was a problem hiding this comment.
yep, working on it, ditching few commits, plus i'm not sure whether we wait for upstream to merge.
and then vendor it.
i think we better for @sjackman
There was a problem hiding this comment.
yep, working on it, ditching few commits, plus i'm not sure whether we wait for upstream to merge.
and then vendor it.
Let's keep this PR up-to-date with your branch. Once upstream merges it, we can vendor here.
rmNULL
force-pushed
the
linux-shelless-elftools
branch
4 times, most recently
from
b05b7a9 to
05fd281
Compare
|
@MikeMcQuaid Could you please give this PR a re-review when you have a chance? This PR will be changed to use vendored Gems once PR david942j/patchelf.rb#28 is merged. |
Instructions to test the bottling of PR
Once you are in docker running this command should be do the trick. ReferenceQuirksthe script my stop on first run, if that happens, please run it again. |
@sjackman I will this time but, in general, I don't think there's a huge amount of point reviewing PRs with failing CI. |
MikeMcQuaid
reviewed
Aug 27, 2020
Library/Homebrew/Gemfile
Outdated
| @@ -21,7 +21,7 @@ gem "tapioca" | |||
| gem "activesupport" | |||
| gem "concurrent-ruby" | |||
| gem "mechanize" | |||
| gem "patchelf" | |||
| gem "patchelf", git: "https://github.com/rmnull/patchelf.rb", branch: "for-brew" | |||
There was a problem hiding this comment.
Note to self: don't merge until this is merged upstream.
rmNULL
force-pushed
the
linux-shelless-elftools
branch
from
1757a50 to
649e02b
Compare
MikeMcQuaid
approved these changes
Aug 28, 2020
|
Thanks again @rmNULL! |
|
Good show! Congratulations on completing this work, rmNull! 🏆 |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add a new gem requirement to patchelf.rb,
patchelfcommand was used to update the name of interpreter and replace DT_RPATH.Same features of patchelf.rb, was used as a substitute.
brew stylewith your changes locally?brew testswith your changes locally?