search

astral-sh / uv

An extremely fast Python package and project manager, written in Rust.
https://docs.astral.sh/uv
Apache License 2.0
24.46k stars 708 forks source link

Revisit LTO for ppc64le binaries #5986

Open zanieb opened 2 months ago

zanieb commented 2 months ago

In #5904, LTO was added to our release profile. However, in #5984, this failed for just the ppc64le musl cross build (which uses the nightly toolchain): 

error: failed to get bitcode from object file for LTO (could not find requested section)

error: could not compile `uv` (bin "uv") due to 1 previous error
💥 maturin failed
  Caused by: Failed to build a native library through cargo
  Caused by: Cargo build finished with "exit status: 101": `env -u CARGO "cargo" "rustc" "--features" "self-update" "--target" "powerpc64le-unknown-linux-musl" "--message-format" "json-render-diagnostics" "--locked" "--manifest-path" "/home/runner/work/uv/uv/crates/uv/Cargo.toml" "--release" "--bin" "uv" "--" "-C" "link-arg=-s"`

I unsuccessfully tried using -Cembed-bitcode=yes (https://github.com/rust-osdev/cargo-xbuild/pull/73) to workaround this, before creating a separate release profile (release-no-lto) with LTO disabled and using it for just this build (https://github.com/astral-sh/uv/pull/5984/commits/f0ffda4039f80d49810dd37ac067c373bb26c7f8).

I'm not anywhere near an expert at Rust compilation options. Someone else should validate this approach and explore alternatives.

davfsa commented 2 months ago

Hey!

First of all, sorry for making this an issue right at the time of release. Wish I could have caught this earlier.

Now, to the issue at hand, it seems like your attempts to pass -Cembed-bitcode=yes using RUSTFLAGS didn't make it through. I dont think docker containers inherit the environment of the runner. Something that could be tried is adding 

maturin_docker_options: -e RUSTFLAGS="-Cembed-bitcode=yes"

to the matrix task for ppc64le, which would pass the environment flag to the container.

I am unable to test it until late night today or potentially tomorrow (I'm CEST), so dropping it here in case someone can give it a shot. Otherwise, will attempt to look more into it myself by running the docker containers locally.

Again, sorry for the issues this has caused!

zanieb commented 2 months ago

First of all, sorry for making this an issue right at the time of release. Wish I could have caught this earlier.

That's not your fault, we should have tested the release builds with your change :)

I dont think docker containers inherit the environment of the runner.

I think Maturin specifically uses -e RUSTFLAGS to pass the variable to the container, I can see this passed to docker run in the logs — that said, I'm not a Maturin expert and cannot confirm the flag was used.

Thanks for following up!

davfsa commented 2 months ago

You were right!

It seems like it does get passed correctly, but maturin does seem to ignore them, as the way to specify them is in the toml (which doesn't allow specifying per type flags)

I'll keep investigating. Getting the docker image to run locally is being quite the struggle

zanieb commented 2 months ago

Might be easiest to run the action in a fork and just comment out the rest of the jobs.