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

Fix nightly WASM target #5799

Merged
merged 1 commit into from
Nov 8, 2024
Merged

Fix nightly WASM target #5799

merged 1 commit into from
Nov 8, 2024

Conversation

robertbastian
Copy link
Member

No description provided.

@robertbastian robertbastian requested review from Manishearth and removed request for echeran November 8, 2024 14:25
@Manishearth Manishearth merged commit 221979f into unicode-org:main Nov 8, 2024
28 checks passed
Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

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

Context, please?

Older nightlies, including the one we were using in CI when I last checked a few months ago, do not support the wasip1 target.

@robertbastian
Copy link
Member Author

@robertbastian robertbastian deleted the wasi branch November 8, 2024 18:14
@robertbastian
Copy link
Member Author

@Manishearth
Copy link
Member

@sffc Rustc has been working on renaming wasm-wasi to wasm-wasip1 for a while (there's a wasip2 target being introduced as well, hence the rename). Our build has warned on this for a while but we haven't noticed since it's in CI.

@sffc
Copy link
Member

sffc commented Nov 8, 2024

Okay, I understand "wasip1" and why we want to use it; I just don't know "why now". The pinned nightly that we had been using for a long time, about a year old, did not support "wasip1" last I checked. I don't know how big the window was of nightlies that supported both "wasi" and "wasip1". I guess the latest nightly out last night dropped "wasi" backwards compatibility support?

@Manishearth
Copy link
Member

Yes.

@Manishearth
Copy link
Member

Manishearth commented Nov 8, 2024

See rust-lang/rust#132562: wasip1 introduced in January, and wasi removed a few days ago.

@sffc
Copy link
Member

sffc commented Nov 8, 2024

That seems like a fairly aggressive schedule, especially considering wasm32-wasi is a target on stable...

Currently our LLVM_COMPATIBLE_NIGHTLY ?= "nightly-2024-01-01" which is still too old. I think we picked that version because it was the latest version that was compatible with LLVM in Xcode; I assume it is a coincidence that it is January 1.

So this is a bit unfortunate of timing. But I guess if CI works, we can ignore it and hope things are okay

@Manishearth
Copy link
Member

@sffc I think it would be fine to mention the Xcode thing on that PR, people who care about LLVM versions have heavy overlap with people doing wasm stuff. I agree that the schedule feels aggressive, it felt aggressive to me as well.

@Manishearth
Copy link
Member

Surprised it didn't show up with deny(warnings) though

Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

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

I think this PR might not break things in the short term because I believe our WASM LLVM size tests use wasm32-unknown-unknown, not wasm32-wasi. However, I did notice one bug in one of the changes, which should have been caught earlier.

@@ -30,7 +30,7 @@ wasm_obj/icu4c/%.o: $(ICU4C_SOURCE)/%.cpp
wasm_obj/ucptrie_wrap.o: ucptrie_wrap.cpp
mkdir -p wasm_obj
$(CXX) --target=wasm32-unknown-wasi \
-I/usr/include/wasm32-wasi \
-I/usr/include/wasm32-wasip1 \
Copy link
Member

Choose a reason for hiding this comment

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

Issue: This is impacting the include path of clang++, not Rust

@sffc
Copy link
Member

sffc commented Nov 8, 2024

Well we do link C++ together with Rust on a WASI target in the codepointtrie builder code. So it may be difficult to rebuild the codepointtrie builder WASM with this change. Hopefully we won't need to do that again soon.

@bjorn3
Copy link

bjorn3 commented Nov 8, 2024

Surprised it didn't show up with deny(warnings) though

#[deny] only denies lints and this warning is not a lint. If it was a lint, you did see a note with the name of the lint and the current lint level attached to the warning.

@riking
Copy link

riking commented Nov 15, 2024

@sffc Target availability is explicitly excluded from the stability promises, due to it needing to match timing decisions of other organizations outside of Rust's control https://doc.rust-lang.org/rustc/target-tier-policy.html?highlight=hard%20stability%20guarantee#general

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.

5 participants