fix(toolchain): exclude libpython from runtime in recent releases - #4091
fix(toolchain): exclude libpython from runtime in recent releases#4091aignas wants to merge 5 commits into
Conversation
Use the already existing auto-detection and exclude libpython and friends as suggested in the upstream ticket. Fixes bazel-contrib#3534
|
Re-read the comment and realized that this should be config flag gated, will PoC the interface |
|
Re: interface: I had three thoughts One: a coarse/vague string flag like minimal/full/some-use-case-name Two: a string flag of pieces to include/exclude, similar to how output group flag works. -libs,+pip,-headers,-tcl,etc Three: a flag to pick the runtime flavor, ie install_only (which is a 26M install compared to the other tars) (this flag will be added regardless, eventually) |
|
I am done for the day so I'll quickly respond:
I chose to be very specific here with respect to what we are including. I chose to have a runtime inclusion behaviour because we already have the thing extracted, so we can support various configurations given the extracted contents and what is present.
This is a nice idea, but could feel clunky? Maybe not... The nice thing here is that we could trampoline off of what user supplies and use FeatureFlagInfo to get derived flags for what we need. I like this because it keeps the API surface small and extendible.
This can be already achieved via what is provided to the machinery as the URLs. I thought the Feel free to hack on this and mold it to your liking. |
rickeylev
left a comment
rickeylev
left a comment
There was a problem hiding this comment.
Needs test. I think a simple py_runtime analysis test that verifies libpython isn't in the files is sufficient.
| (toolchain) users can now request libpython to be not implemented via | ||
| {target}`//python/config_settings:py_runtime_include_libpython=no`. | ||
| The default is to include it, but it may change in the future. Ensure | ||
| that you include `libpython` via the `<toolchain_repo>//:libpython` if |
There was a problem hiding this comment.
We shouldn't tell people to reference the toolchain repos directly, as those are implementation details.
The py_cc_libs target is the one to depend on if build-time linking. I suppose that could be used to get libpython into runtime data deps with some extra steps. (depending on py_cc_libs might already do that? not sure).
What's the use case for a target to have libpython at runtime when the toolchain says to not included it?
| ) | ||
|
|
||
| config_setting( | ||
| name = "_is_py_runtime_include_libpython_auto", |
There was a problem hiding this comment.
Move the private config setting definitions into the bzl code; there's a helper in there that defines all the various private config settings
| if release and release >= 20250517: | ||
| # Starting with 20250517 we have python3 linked statically | ||
| # https://github.com/astral-sh/python-build-standalone/issues/941 | ||
| python3_statically_links_libpython = False |
There was a problem hiding this comment.
This logic looks backwards? The conditional doesn't match the comment
| elif rctx.attr.distutils_content: | ||
| rctx.file(distutils_path, rctx.attr.distutils_content) | ||
|
|
||
| # Support not including libraries into runtime |
There was a problem hiding this comment.
nit: remove comment. The release is used for other things, too.
| # https://github.com/astral-sh/python-build-standalone/releases/tag/20240224 | ||
| rctx.delete("share/terminfo") | ||
|
|
||
| if release and release >= 20250517: |
There was a problem hiding this comment.
Hrm. I don't like this way of deciding it, but not sure what options we have. I'm willing to live with this, but that said:
It would be best to check if the executable has dt_needed for libpython somehow. A build rule that looks at python3 and then conditionally emits the libpython file or...not sure what the "or" is. error or empty file? Maybe a params file of link opts?
If there was a prebuilt patchelf or some such available, we could use that during the repo phase. Or just call out to system patchelf, if available.
I've really been wanting to create some of our own repo-phase tools for stuff like this.
| files_exclude += extra_files_glob_exclude | ||
|
|
||
| native.filegroup( | ||
| native.alias( |
There was a problem hiding this comment.
I'd say remove the alias. The extra level of indirection for the slightly clearer "_all" name doesn't seem worth it.
2 participants
Use the already existing auto-detection and exclude libpython and
friends as suggested in the upstream ticket.
Fixes #3534