packaging: use local patchelf #2519
Conversation
Pull patchelf from the latest stable tag from the upstream repository, which contains patches critical for snapcraft to work properly with Go ELF binaries. Noteworthy patches and issues fixed: - Avoid inflating file sizes needlessly and allow binaries to be stripped - patchelf --set-interpreter fails with `cannot find section` (canonical#66) - --set-interpreter gives "cannot find section" for go binary (canonical#138) LP: #1805205 Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
There was a problem hiding this comment.
Hi, thanks for this, there a few things to consider:
tests/integration/__init__.pysets abspaths for patchelf that would need theusrpart stripped out.- if this really is a fix, the workaround that strips notes from binaries and retries in
snapcraft/internal/elf.pyshould be.removed.
|
This will require additional work. Checking with readelf, we see the section layout in the gotty binary changing from: to which will still cause problems with cgo binaries. |
Patchelf 0.10 is able to correctly set the interpreter, so the workaround put in place for previous versions is no longer necessary. Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
When building for classic confinement, relink dynamic go executables so that patchelf will succeed when setting rpath. Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
Codecov Report
@@ Coverage Diff @@
## master #2519 +/- ##
==========================================
+ Coverage 89.17% 89.19% +0.02%
==========================================
Files 201 201
Lines 13615 13606 -9
Branches 2057 2057
==========================================
- Hits 12141 12136 -5
+ Misses 1039 1035 -4
Partials 435 435
Continue to review full report at Codecov.
|
|
Instead of fixing patchelf to be able to set rpath in Go binaries, we changed the strategy to relink dynamic executables with the system linker so that patchelf will be able to handle them. Patchelf 0.10 is still needed to set the interpreter without the workarounds put in place for patchelf 0.9. This solves the rpath setting problem for binaries we build (but not for pre-built dynamic Go binaries already linked with the Go linker). |
|
Looks good! Just wondering if you can add a spread test. Feel free to use the current cgo snap with a different spread task to make it classic, you can use the tooling to set a base and reset the yaml on restore as a template for setting |
Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
…nto use-local-patchelf
Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
| fi | ||
|
|
||
| # Ensure binaries are properly patched | ||
| patchelf --print-rpath prime/bin/go-cgo | MATCH "/snap/core[0-9]*/current" |
There was a problem hiding this comment.
make this /snap/snapcraft/current/bin/patchelf
There was a problem hiding this comment.
and if you really wanted some independence here, you you install binutils and use readelf for this :-)
Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
snapcraft/internal/elf.py
Outdated
| logger.debug("ELFFile({}) failed: {}".format(self.path, elf_error)) | ||
| return False | ||
| else: | ||
| return "PT_DYNAMIC" in [s.header.p_type for s in ef.iter_segments()] |
There was a problem hiding this comment.
just for reference, we also have https://github.com/snapcore/snapcraft/pull/2519/files#diff-f7cf307285f042f35434bbe680606c77R258 and those are all set as properties here https://github.com/snapcore/snapcraft/pull/2519/files#diff-f7cf307285f042f35434bbe680606c77R219 so it might be good to just do it on load, we can leave as is for now if we have a bug for this and a code comment TODO with the LP: # here
Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
cmatsuoka
force-pushed
the
use-local-patchelf
branch
from
4096352 to
3e1289a
Compare
Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
cmatsuoka
force-pushed
the
use-local-patchelf
branch
from
3e1289a to
1789b46
Compare
Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
…onical#2519) Pull patchelf from the latest stable tag from the upstream repository, which contains patches critical for snapcraft to work properly with Go ELF binaries. Patchelf 0.10 is able to correctly set the interpreter, so the workaround put in place for previous versions is no longer necessary. When building for classic confinement, relink dynamic go executables so that patchelf will succeed when setting rpath. Noteworthy issues fixed in patchelf 0.10 are: - Avoid inflating file sizes needlessly and allow binaries to be stripped - patchelf --set-interpreter fails with `cannot find section` (canonical#66) - --set-interpreter gives "cannot find section" for go binary (canonical#138) LP: #1805205 Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
Pull patchelf from the latest stable tag from the upstream repository,
which contains patches critical for snapcraft to work properly with
Go ELF binaries.
Noteworthy patches and issues fixed:
cannot find section(catkin plugin catches the wrong exception #66)LP: #1805205
Signed-off-by: Claudio Matsuoka claudio.matsuoka@canonical.com
./runtests.sh static?./runtests.sh tests/unit?