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.

Sign up for GitHub

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

Jump to bottom

Insert -Zbuild-std when building rust packages #40

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

Insert -Zbuild-std when building rust packages #40

wants to merge 3 commits into from
+12 −0

Conversation

hoodmane
Copy link
Member

@hoodmane hoodmane commented Oct 11, 2024 •
edited
Loading

Otherwise we are linking a copy of the standard library built with some particular version of emscripten selected by the rust compiler that does not match the version we are using. This can lead to ABI mismatch.
rust-lang/rust#131467

cc @messense @davidhewitt perhaps it would be good for maturin to also insert this flag when targeting emscripten.

@hoodmane
Insert -Zbuild-std when building rust packages
a84379d 
Otherwise we are linking a copy of the standard library built with some
particular version of emscripten selected by the rust compiler that does not
match the version we are using. This can lead to an ABI mismatch.

See rust-lang/rust#131467
hoodmane added a commit to hoodmane/pyodide that referenced this pull request Oct 11, 2024
@hoodmane
Add test for rust stat64 abi mismatch
d110c7f 
This tests the fix for rust-lang/rust#131467 in pyodide/pyodide-build#40.
The problem is that rust cannot correctly read file metadata because the ABI of
fstat64 changed between the version of emscripten that the rust compiler built
the standard library against and our version of the rust compiler.

By passing `-Zbuild-std` we rebuild the rust standard library with the correct
abi. I also added a test that works only if we pass `-Zbuild-std`.
@hoodmane hoodmane mentioned this pull request Oct 11, 2024
Open
@hoodmane
Copy link
Member Author

@juntyr says:

When building the Rust crates, you could pass --sysroot <PATH> to a prebuilt sysroot. To each crate, it would look again just normal (no build-std anymore).

I'm not sure how to prepare the sysroot itself, but there is a crate https://crates.io/crates/cargo-sysroot that supports building with custom sysroots, so it might be useful to look at.

This would have the advantage of not requiring nightly for this feature, but it has the disadvantage that none of the tooling looks very maintained...

ryanking13
ryanking13 approved these changes Oct 12, 2024
View reviewed changes
Copy link
Member

@ryanking13 ryanking13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Well this is very tricky problem.

pyodide_build/pywasmcross.py
Comment on lines +528 to +530
elif cmd == "cargo":
line.insert(1, "-Zbuild-std")
return line
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a test for this (test_pywasmcross.py)

@davidhewitt
Copy link

There is also https://github.com/RalfJung/rustc-build-sysroot which is maintained by a Rust project member but doesn't have a CLI; you'd need to wrap it.

I think both those tools do still need a nightly compiler to generate the sysroot though.

@ryanking13
Copy link
Member

@davidhewitt Thanks for the info! rustc-build-sysroot looks very intersesting. I am not sure patching rust packages to include that crate is the right way to go for us, though, as we are not package maintainers.

Anyway, for now, we are having an issue that the build-std flag is incompatible with the cargo vendor, and rustc-build-sysroot can be an alternative option.

@hoodmane
Copy link
Member Author

I think for now we can bump to a rust nightly that contains rust-lang/rust#131533 and that will probably fix the trouble without -Zbuild-std. But in general, we have a pretty serious problem considering that we both must pass -Zbuild-std in order for compilation to be sound but also cannot pass it in order to compile packages with frozen crates. But we can add an option to suppress it and pass it for all other packages. And then just pray.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryanking13 ryanking13 ryanking13 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hoodmane @davidhewitt @ryanking13