refactor(build): use existing -I and -r flags from cmd/link#1804
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modifies the go build command in src/go-build-wrapper to use Go's -I and -r linker flags instead of passing -Wl,-dynamic-linker and -Wl,-rpath via -extldflags. However, the reviewer points out that using -I does not work when -linkmode external is enabled, as the host linker will ignore it and use the default dynamic linker instead. The reviewer suggests reverting this change to ensure the correct dynamic linker and rpath are set.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
The Go cmd/link linker (ie., 'go tool link') natively supports setting the path of the dynamic linker (ie., PT_INTERP), and RPATHs or RUNPATHs. Therefore, there's no need to rely on its external linking mode and the external linker flags for these. The Go cmd/link linker was originally switched to external mode to use the GNU ld linker's --wrap flag [1]. Later, the --wrap flag was dropped, but the external linking mode was retained to minimize the amount of changes [2]. Currently, the external mode is needed by the --export-dynamic and --unresolved-symbols flags [3]. Reducing the reliance on the external linking mode makes the build more flexible to future changes, and simplifies go-build-wrapper by removing one layer of quoting. [1] Commit 6ad9c63 containers@6ad9c631806961f3 containers#534 containers#529 [2] Commit 6063eb2 containers@6063eb27b9893994 containers#897 containers#821 [3] Commit 66280a6 containers@66280a617ae7eaa2 containers#1548 containers#1804
debarshiray
left a comment
debarshiray
left a comment
There was a problem hiding this comment.
Thanks for working on this, @xandris ! The changes look correct to me.
I tested with:
$ patchelf --print-interpreter ./builddir/src/toolbox
/run/host/usr/lib64/ld-linux-x86-64.so.2
$ patchelf --print-interpreter /usr/bin/toolbox
/run/host/usr/lib64/ld-linux-x86-64.so.2
$ readelf --program-headers ./builddir/src/toolbox | grep interpreter
[Requesting program interpreter: /run/host/usr/lib64/ld-linux-x86-64.so.2]
$ readelf --program-headers /usr/bin/toolbox | grep interpreter
[Requesting program interpreter: /run/host/usr/lib64/ld-linux-x86-64.so.2]
$ readelf --dynamic ./builddir/src/toolbox | grep PATH
0x000000000000001d (RUNPATH) Library runpath: [/run/host/usr/lib64]
$ readelf --dynamic /usr/bin/toolbox | grep PATH
0x000000000000001d (RUNPATH) Library runpath: [/run/host/usr/lib64]Let me try to rebase it on main.
debarshiray
force-pushed
the
refactor/simplify-linker-args
branch
from
1f0ff33 to
0851179
Compare
Done. I added some links to the commit message to track the history of the external linking mode. |
|
I have always wondered if we should add some tests to introspect the generated Ideally, the tests would be independent from the slower and I/O intensive system tests in @xandris any thoughts or ideas? |
I found some tests hidden in #1031 Let's see if I can rescue them. |
2 participants
Make cmd/link pass these flags to the compiler driver instead of doing it manually.
Here you go. Naturally this will change after the other pull request is merged. RPATH is set correctly from what I can tell.