Skip to content
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 support for musl Python toolchain fetches #4160

Closed
wants to merge 1 commit into from
Closed

Conversation

zanieb
Copy link
Member

@zanieb zanieb commented Jun 8, 2024

Previously, we always assumed GNU.

I considered restoring some of the platform-host crate which was removed in #2381. I'm not sure we actually need to do the whole ELF inspection thing though since we don't need version information and target-lexicon seems to have what we need. I presume since target-lexicon::HOST is a const, that the information is based on the uv's target host not the runtime host. This seems okay for toolchain download assumptions? If not, we can go back to the ELF inspection technique but I'm not fully comfortable modifying that code.

@zanieb zanieb added the preview Experimental behavior label Jun 8, 2024
@zanieb zanieb requested a review from konstin June 8, 2024 14:10
@charliermarsh
Copy link
Member

This seems okay for toolchain download assumptions?

I'm not sure... Wouldn't this always download the musl Pythons on our Linux builds now, since we only ship musl? My guess is we do need to determine this dynamically.

@zanieb
Copy link
Member Author

zanieb commented Jun 8, 2024

Eek that might be true. Okay, back to the goblin elf approach. I'll need to chat with Konsti about that code.

@charliermarsh
Copy link
Member

I'm actually a bit worried about the use of std::env::consts::ARCH too -- is it granular enough to map to the set of Pythons that we ship? That's what #3857 was about.

@zanieb
Copy link
Member Author

zanieb commented Jun 9, 2024

Oh interesting thanks for the link I didn't see #3857 or #3855. I'll need to look into that further. These were not intended to be fully "production ready" when written. I certainly wouldn't be surprised if we need to do something more robust or restore more of the implementation we were using before we moved platform information queries to rely on a Python implementation.

@konstin
Copy link
Member

konstin commented Jun 10, 2024

tl;dr: We need to go back to the libc detection from #2381.

There are three different libc configurations supported by rust: glibc dynamically linked (x86_64-unknown-linux-gnu), statically linked with musl (x86_64-unknown-linux-musl), musl dynamically linked (-crt-static, rust-lang/compiler-team#422). To load a dynamic library such as native python modules, the libc configuration of the loading binary must match the one of the loaded library. Python encodes this as manylinux and musllinux tags. Statically linked binaries can't load dynamic libraries (by the mechanism we need).

The libc that uv itself uses doesn't matter since we don't load libraries ourself, but the python we download and start needs to match the libc the user expects. Finding the platform default is non-trivial since you can install glibc on alpine (https://wiki.alpinelinux.org/wiki/Running_glibc_programs) and musl on ubuntu (apt install musl). As proxy, we check /bin/ls or /bin/sh for its libc and use it platform default. Alternatively, we could switch to shipping dynamically linked glibc and musl builds of uv and enforce using the right one in the installer (removing the glibc-too-old to musl fallback).

On that note, python-build-standalone is currently statically linked for musl and fails to loads any compiled native module (#2382). This is a blocker for deploying python-build-standalone on alpine targets.

Base automatically changed from zb/toolchain-v to main June 10, 2024 14:10
@zanieb zanieb marked this pull request as draft June 10, 2024 14:12
@zanieb
Copy link
Member Author

zanieb commented Jun 10, 2024

Moving this back to draft as it will require significant revisions.

@zanieb
Copy link
Member Author

zanieb commented Jun 11, 2024

I'll open an issue for proper musl detection.

This pull request is superseded by #4236

@zanieb zanieb closed this Jun 11, 2024
zanieb added a commit that referenced this pull request Jun 12, 2024
Closes #3857

Instead of using custom `Arch`, `Os`, and `Libc` types I just use
`target-lexicon`'s which enumerate way more variants and implement
display and parsing. We use a wrapper type to represent a couple special
cases to support the "x86" alias for "i686" and "macos" for "darwin".
Alternatively we could try to use our `platform-tags` types but those
capture more information (like operating system versions) that we don't
have for downloads.

As discussed in #4160, this is not
sufficient for proper libc detection but that work is larger and will be
handled separately.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview Experimental behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants