Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork249
POC: Add a relocatable build flag#861
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
63daa7f toc2e856dComparec2e856d tod715fa3Compare
geofft left a comment
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.
Unfortunately I think there's a good amount of stuff missing compared to our current implementation, and this is a hairy enough spot that I don't think we should have any regressions in 3.15 compared to what we do on older versions.
On glibc/Linux, we add aDT_NEEDED for bin/python to$ORIGIN/../lib/libpython3.x.so.1.0 (and for lib/libpython3.so to$ORIGIN/libpython3.x.so.1.0) , to avoid issues withLD_LIBRARY_PATH taking precedence. I'm relatively sure this is annoying to do without a separate patchelf step, but maybe it can be done.
We also set anDT_RPATH for bin/python on Linux (required on musl, compatibility on glibc). This is not aDT_RUNPATH, so this needs-Wl,--disable-new-dtags.
I don't see either of those in the Makefile diff, in addition to the other things noted in the comments.
If you really wanted, you could start by upstreaming this gated to macOS only, which might be a little less code to write.
Also speaking of platform support, the dynamic linkers on the BSDs and Solaris works much like glibc, so most of our Linux support should apply there too in the version that gets upstreamed.
In either case, I would error out if--enable-relocatable is passed on another platform.
| Author: Zanie Blue <contact@zanie.dev> | ||
| Date: Fri Nov 21 15:44:42 2025 -0600 | ||
| Set linker arguments when `--enabled-relocatable` is used |
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.
| Set linker arguments when `--enabled-relocatable` is used | |
| Set linker arguments when `--enable-relocatable` is used |
| -change /install/lib/${LIBPYTHON_SHARED_LIBRARY_BASENAME} @executable_path/${LIBPYTHON_SHARED_LIBRARY_BASENAME} \ | ||
| ${ROOT}/out/python/install/lib/${LIBPYTHON_SHARED_LIBRARY_BASENAME} | ||
| # We also normalize /tools/deps/lib/libz.1.dylib to the system location. |
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.
How come you didn't need to preserve this?
| # At the moment, python3 and libpython don't have shared-library | ||
| # dependencies, but at some point we will want to run this for | ||
| # them too. | ||
| formodulein${ROOT}/out/python/install/lib/python*/lib-dynload/*.so;do | ||
| install_name_tool -add_rpath @loader_path/../.."$module" | ||
| done |
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.
This almost certainly needs to be preserved, this fixes up _tkinter, right? That is, you need this to go into the CPython Makefile rule for building shared-library extension modules (... which we might not be using, to make things more exciting).
| @@ -1025,10 +1025,19 @@ libpython$(LDVERSION).so: $(LIBRARY_OBJS) $(DTRACE_OBJS) | ||
| fi | ||
| libpython3.so:libpython$(LDVERSION).so |
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.
You need to patch thelibpython$(LDVERSION).so target too. (LINKFORSHARED is only used when building the interpreter.)
| +LINKFORSHARED="-Xlinker -export-dynamic" | ||
| +# Add rpath for relocatable builds | ||
| +if test "x$enable_relocatable" = xyes; then | ||
| +LINKFORSHARED="$LINKFORSHARED -Wl,-rpath=\$ORIGIN/../lib" |
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.
The rpath here is not escaped correctly resulting in the the RPATH in the binary being set toRIGIN/../lib
This causes among other issues,_tkinter not to be importable.
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.
| +LINKFORSHARED="$LINKFORSHARED -Wl,-rpath=\$ORIGIN/../lib" | |
| +LINKFORSHARED="$LINKFORSHARED -Wl,-rpath,'\$\$ORIGIN/../lib'" |
| +LINKFORSHARED="-Xlinker -export-dynamic" | ||
| +# Add rpath for relocatable builds | ||
| +if test "x$enable_relocatable" = xyes; then | ||
| +LINKFORSHARED="$LINKFORSHARED -Wl,-rpath=\$ORIGIN/../lib" |
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.
| +LINKFORSHARED="$LINKFORSHARED -Wl,-rpath=\$ORIGIN/../lib" | |
| +LINKFORSHARED="$LINKFORSHARED -Wl,-rpath,'\$\$ORIGIN/../lib'" |
jjhelmus commentedDec 3, 2025
A question that I suspect will get asked when upstreaming this, is what should be done to accommodate third-party libraries such as libtcl, libtk, libz in relocatable builds. With the exception of libtcl/tk these are statically linked but this patch should support cases where these are dynamically linked. Adding a RPATH entry to the extensions with a relative path to I could also see an opportunity to hash the SO name of third-party libraries on Linux to protect against clashes with system libraries. This approach is used by |
Uh oh!
There was an error while loading.Please reload this page.
The goal here is to construct two upstreamable patches
--enable-relocatableconfigure flag to gate new behaviorAs a PoC, we apply these patches here for Python 3.15 and skip our post-build relocatability patchelf invocations.