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

support for musl environment #619

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

support for musl environment #619

wants to merge 8 commits into from

Conversation

dsp
Copy link
Contributor

@dsp dsp commented Feb 8, 2024

This is an experimental pull request aimed to support environments.

Problem

Linux and in rare cases windows binaries can be built with different environments. At the moment rye assumes we will be always using a gnu environment on linux. This is not true for distributions like Alpine, which use musl instead of glibc. Hence, rye currently does not support Alpine Linux.

Approach

This PR introduces the notion of an environment to our internal handling of python versions and python version requests. It extends the version matching to include environments, and modifies our download link creators for uv and cypthon builds to include the environment. Lastly, it changes how we link libcurl to make builds for Alpine linux working.

Once the PR is merged, we can build and use rye on alpine.

Notable, there are some caveats to the current implementation. I decided to do this in multiple PRs to not overload the PR with more.

Status of implementation

  • We introduce the notion of a "environment", which can either be unset (None) or set to a string.
  • We changed rye-devtools to include the environment for all uv and cpython builds.
  • We modified match_versions to correctly resolve toolchain.
  • We define the default_toolchain based on the target_env, so based on the target triple rye was build for. This is deliberate, as we have no easy way to find the target_env at runtime (e.g. a system that provides both gnu and musl, is completely valid on Linux).
  • We add a cargo feature to select the features of the curl crate we are using, to allow linking to system openssl on alpine.
  • Added tests to bootstrap on different environments (gnu and alpine). We ran manual tests to compile and run on Alpine, see: https://asciinema.org/a/REzOhL77l95VsqaBSuOxf9pH5

Caveats

Missing CI integration

There is currently not CI integration for Alpine linux or one that links against musl on Ubuntu. However, we did manually test it.

Python Storage Paths

We currently still save python into [RYE_HOME]/py/cpython@X.Y.Z. This can lead to problems where a user would have a both a musl and a gnu version. It is something that we should support, particularly when we allow users to more easily select their environments within a project. However it is out of scope of the current PR.

User selected environments

Users cannot easily select a python version for a specific environment. For example rye pin cpython-linux-musl doesn't work. we should make this work in the future.

@dsp dsp force-pushed the main branch 2 times, most recently from 602407c to 45f2cc7 Compare February 8, 2024 12:22
@mitsuhiko
Copy link
Collaborator

The script that generates this out is this one here: https://github.com/mitsuhiko/rye/blob/main/rye/find-downloads.py

I would like to avoid the term "toolchain" here as this term is already used to refer to the python installations itself. I'm not sure what a good term is, but toolchain probably not.

@rdbo
Copy link

rdbo commented Feb 8, 2024

Rust uses the term target_env in Conditional Compilation (https://doc.rust-lang.org/reference/conditional-compilation.html#target_env) to disambiguate between targets. So I guess "env" or "environment" could be candidates for a different term (even though it is not exactly the same thing, since the target_env can be empty).

@mitsuhiko
Copy link
Collaborator

mitsuhiko commented Feb 8, 2024

This appears to be a common structure for target triples: https://docs.rs/target-lexicon/latest/target_lexicon/struct.Triple.html

pub struct Triple {
    pub architecture: Architecture,
    pub vendor: Vendor,
    pub operating_system: OperatingSystem,
    pub environment: Environment,
    pub binary_format: BinaryFormat,
}

What this refers to is in fact called environment.

@dsp
Copy link
Contributor Author

dsp commented Feb 8, 2024

I'll change it to environment.

@dsp dsp force-pushed the main branch 4 times, most recently from efb8627 to db63433 Compare February 12, 2024 09:15
@dsp dsp changed the title [experimental] sources: Add support for toolchain [experimental] sources: Add support for environment Feb 12, 2024
@dsp
Copy link
Contributor Author

dsp commented Feb 12, 2024

rebased

@dsp dsp changed the title [experimental] sources: Add support for environment Sources: Add support for environment Feb 13, 2024
@dsp
Copy link
Contributor Author

dsp commented Feb 16, 2024

rebased again

@mitsuhiko
Copy link
Collaborator

Playing with this still. Pretty determined to merge but I need to figure out how this actually plays out in practice still.

rye/src/sources.rs Outdated Show resolved Hide resolved
@dsp dsp force-pushed the main branch 2 times, most recently from bff21bc to d76c72f Compare February 18, 2024 17:34
@mitsuhiko
Copy link
Collaborator

I'm temporarily holding on until I have the test system in place.

Refs #687

@dsp dsp marked this pull request as draft February 21, 2024 09:51
@dsp
Copy link
Contributor Author

dsp commented Feb 21, 2024

I am adding tests for this. There is a problem that currently makes using Indygreg's python3 musl packages unusable for rye:

  1. rye uses python3 -mvenv --upgrade-deps to bootstrap.
  2. This will call ensurepip, which bootstraps pip from a bundled wheel and calls pip.
  3. pip fails when trying to detect libc version. It asks for the libc version to set it as a user-agent for any request it makes. This uses ctypes.CDLL, which fails for indygregs python3 with "Dynamic loading not supported".

There are patches for this in indygregs repo, but they don't affected the bundled wheel which ensurepip uses during bootstrapping.

Standard alpine musl python allows dynamic loading and hence is not affected.

Putting this on hold until it's resolved upstream, or someone has a good idea how to go about it.

@mitsuhiko
Copy link
Collaborator

@dsp it would be interesting to know if uv has the same issue. Because one option would be to do away with pip to bootstrap but manually fetch a matching uv release and bootstrap via that one.

@dsp dsp changed the title Sources: Add support for environment support for musl environment Mar 1, 2024
@dsp dsp force-pushed the main branch 6 times, most recently from 1b510d5 to 264a4c7 Compare March 4, 2024 15:14
@dsp dsp force-pushed the main branch 3 times, most recently from 827a652 to c6c3909 Compare March 5, 2024 14:28
@dsp dsp marked this pull request as ready for review March 5, 2024 14:28
@dsp
Copy link
Contributor Author

dsp commented Mar 5, 2024

Okay, I hope this is finally ready. I updated the description. Manually tested on Alpine Linux, but not tested on a GNU system with musl installed.

@dsp dsp force-pushed the main branch 2 times, most recently from 6cc3f41 to 0a88940 Compare March 27, 2024 10:27
@mitsuhiko
Copy link
Collaborator

I would like to push out the current main branch of rye as a release, and then schedule this to go into the release after. The current changes are mostly bugfixes.

@dsp
Copy link
Contributor Author

dsp commented Mar 29, 2024

@mitsuhiko Thank you! Looking forward to having it in! I'll rebase it one more time today.

Note that likely the merge will fail if you update uv releases or python releases in between. You would have to regenerate them.

dsp added 8 commits March 29, 2024 15:27
Linux and in rare cases windows binaries can be built with different
environments. At the moment rye assumes we will be always using a gnu
environment on linux. This is not true for distributions like Alpine,
which use musl instead of glibc.

Indygreg offers musl builds, but we don't pull them in just yet.

1. We introduce the notion of a "environment", which can either be unset (None)
   or set to a string.
2. We set the environment string to Some("gnu") for all existing linux downloads
3. We set the environment string to None for all others
4. We modified match_versions to correctly resolve environment or fallback
   to the default environment (Some("gnu") for linux, None for all others).
5. We added tests.
We are using the target environment to select the requested cpython
version. Since `std::env::consts` does not offer a way to select
the target os on runtime, we are having to find a suitable way.

This implies that a rye version build for musl will pull
a musl toolchain by default and a rye version for gnu will will
pull a gnu toolchain. In general this should work. On a fully
musl system such as alpine, you wouldn't have a system that is compatible
with gnu anyway and on a GNU system you would just always pull
a musl toolchain which will work.
Alpine Linux builds will fail for a `static-ssl`. Instead we must link
against system openssl. We now introduce a feature set that allows to
link against system ssl using `--no-default-features --features system-ssl`.

For a musl system, you also must specify
RUSTFLAGS="-C target-feature=-crt-static".

The alternative approach is to use using a
`[target.'cfg(target_env="musl")'.dependencies]` specification, that
selects features based on the `target_env`. However since we
are currently using the Cargo resolver 1, this does not work. We would
need to switch to resolver 2. In general the feature selection is more
versatile since we ask users to select the appropriate set for their
platform.
@tuananh
Copy link

tuananh commented Apr 15, 2024

Thanks @dsp for working on this

@tuananh
Copy link

tuananh commented May 2, 2024

olps, this one doesnt make it in 0.33.0 release :(

@dsp
Copy link
Contributor Author

dsp commented May 2, 2024

If someone wants to pick it up, rebase and check why the test is failing, please go ahead. I still think this is valuable but won't have time to work on it. Since I am personally not running Alpine, it's not too relevant for me (although a bit sad since I poured quite a bit of time into it)

@dsp
Copy link
Contributor Author

dsp commented May 2, 2024

I also assume we have it wait for @mitsuhiko to be back. I think it's quite a big and scary feature, so I totally see why it is not merged currently while @charliermarsh is helping out with more straight forward PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants