-
-
Notifications
You must be signed in to change notification settings - Fork 176
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add dynamically linked musl distributions #541
Conversation
I got the
I presumed we just didn't package the .so, but it is there
So I futzed with patchelf
Huh
I guess I'll try harder?
Seems like |
|
This was very opaque in the error message in CI
This is what it is available on ubuntu-22.04
Hm okay something is wrong with Python 3.11 — peachy otherwise! edit: It's loading the libpython from the system, which is 3.11. |
This is loosely ready for review. I have some minor clean-ups and documentation to do. |
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.
I strongly prefer retaining the ability to produce a statically linked musl distribution. There are some users of PyOxidizer who really like single file binaries. And you only get that if you statically link musl.
I would prefer standing up a new target variant with dynamic linked musl which is ABI compatible with the musl Python platform/target.
I can look into that. Will require #542. |
# Conflicts: # cpython-unix/build-cpython.sh # cpython-unix/build.py # pythonbuild/cpython.py
# TODO should the musl target be normalized? | ||
.replace("-unknown-linux-musl", "-unknown-linux-gnu") |
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.
See xproto
and xextproto
build changes
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 answer to this todo seems to be: no it should not generally be normalized like this because it causes problems. But then there are a couple specific builds where it does need to be normalized because there isn't support for -musl
targets.
See also, the tcl
build change.
"url": "https://musl.libc.org/releases/musl-1.2.2.tar.gz", | ||
"size": 1055220, | ||
"sha256": "9b969322012d796dc23dda27a35866034fa67d8fb67e0e2c45c913c3d43219dd", | ||
"version": "1.2.2", |
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.
We use an older version for dynamic builds, for compatibility with more systems. For example, the GitHub runners we're using did not have 1.2.5.
We retain the latest version for static builds, since compatibility isn't a concern.
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.
LGTM. At first glance I'm okay changing the default musl build to be dynamic and to make the static variant an explicit option, and I would assume that works fine with the release process since it at most adds more artifacts, some of which might get missed, but the existing set of artifacts will still be present (with a semantics change for the musl ones). But we should probably double-check whether uv and Rye are chill with the change before shipping. Are there any other high-profile users we should check, especially ones we know are using the musl builds? (PyOxidizer?)
mipsel-unknown-linux-musl) | ||
EXTRA_FLAGS="${EXTRA_FLAGS} --enable-malloc0returnsnull" | ||
;; |
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.
I'm assuming we were previously implicitly doing this for musl?
allowed_libraries.push(format!("libpython{}t.so.1.0", python_major_minor)); | ||
allowed_libraries.push(format!("libpython{}td.so.1.0", python_major_minor)); | ||
} | ||
|
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 changes from line 921 to here seem a little inconsistent, are all the changes here intentional?
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.
Oh thanks the changes around 924 are confusing? I'll revert that.
I can also probably make this bit conditional on the static
build option.
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 comment was marked as duplicate.
This comment was marked as duplicate.
I want to ack this, but I don't have a good answer. @ofek comes to mind as an automated consumer outside Astral. |
Changing the default to something that actually works is desirable in my opinion. It doesn't matter if there is new naming because in my projects that consume (PyApp & Hatch) there is no ability to use newer non-custom builds without a new release because PyApp has a script that generates the hardcoded source of truth, which Hatch then uses in a script to hardcode its source of truth. So I would just have to change the former script. |
I considered a transition plan like
However, users still need to encode a specific version of python-build-standalone in which the default changed. After thinking about it more, I'm still leaning towards just changing the defaut. |
@ofek by "actually works" I assume you mean "able to dynamically load stuff"? Then I think we should ship this and call it a bugfix and make sure we ship it at a time that people will be around on Discord etc. to help downstream folks that are broken by the change if any are actually broken by it. We could also figure out how to make a static musl include a working libdl, which is probably easier than it would be for glibc because musl is generally less complex, but would still be a little bit of a research project. |
The diff is not too complicated, so I'll just merge this and close #546 |
Follows #541 Tested locally with `just release-dry-run` and the artifacts on the latest commit to `main`. Then used for https://github.com/astral-sh/python-build-standalone/releases/tag/20250311
As a consequence of #540 I was playing with these concepts and decided to explore it.
This includes #546 (could be merged separately or together), which separates "static" builds from the "musl" triple specifically in favor of a dedicated build option.
The main implementation downside here is that
$ORIGIN
doesn't work with DT_NEEDED so we need to use RUNPATH instead, which can cause the wrong library to be loaded if LD_LIBRARY_PATH is set. Given the current builds aren't usable at all, I think this is a fine trade-off. We can explore alternatives in the future, like statically linking just libpython.Another caveat here: for consistency with the glibc builds, we're changing the "default" musl build to be dynamically linked. This is a breaking change in the release artifacts. The statically linked musl build will include a
+static
suffix. We could do something for backwards compatibility here, but I think this probably makes sense in the long term. My primary concern is that consumers that combine releases (such as uv) would need to encode this change (e.g., toggle the expectation based on the python-build-standalone version tag).It's challenging to test changes to the release artifact handling. Regardless of approach, this will need a follow-up to adjust that accordingly.