-
Notifications
You must be signed in to change notification settings - Fork 522
feat: Add WASI Preview 2 support and enhanced target coverage #3507
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
Conversation
7249b22 to
d834ab8
Compare
|
This looks very useful, thanks. I'm not sure what the policy for this repo regarding how focused changes are but this looks like at least 3 distinct things are included here:
|
|
Maybe you could add |
|
Thanks for the feedback @cameron-martin! You're absolutely right about this covering multiple areas - I'm still learning the contribution patterns here so really appreciate the guidance. For the DEFAULT_EXTRA_TARGET_TRIPLES suggestion, that makes perfect sense for consistency. I'll add that. Regarding splitting this up - I'm totally open to that if it would be better for the project. I was trying to get all the WASI P2 pieces working together but I realize that might not align with how changes are typically structured here. Would you recommend breaking it into separate PRs? Happy to follow whatever approach works best. Thanks for taking the time to review - still getting familiar with the codebase so feedback like this is really helpful. |
ef700fc to
28f4d92
Compare
That'll be a question for @illicitonion and @UebelAndre. I was just suggesting it may be easier to review that way. |
UebelAndre
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable to me, thanks!
|
looks like there are formatting issues though. |
28f4d92 to
06eea3e
Compare
- Add WASI version constraint settings and platform configurations for wasip2 - Add support for wasm32-wasip2 and wasm32-wasip1-threads targets - Fix WASI toolchain and allocator library issues for cross-compilation - Add comprehensive unit tests and documentation for WASI platform functionality
06eea3e to
0d583ae
Compare
|
Thanks @UebelAndre for catching the formatting issues! I've now fixed all the buildifier and clang-format issues that were causing the CI to fail. All pre-commit hooks are now passing and the code-format-checks should be green. |
|
Howdy, I think by default we should use https://github.com/bazelbuild/platforms for constrant_settings & values. Is there a reason not to do it here? cc @krasimirgg |
|
|
|
Thanks for the heads-up! I wasn’t aware that bazelbuild/platforms is the default place for these constraints — that makes sense in hindsight. After looking into it, I saw there’s already an open issue about adding support for wasip1 and wasip2 (bazelbuild/platforms#123), so I went ahead and submitted a patch there to help move things forward. Happy to follow up once that’s merged so we can align fully. Appreciate the pointer! |
|
Thanks so much for a quick response!! ❤️ |
juankyc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pendiente
Summary
Add
wasm32-wasip2toDEFAULT_EXTRA_TARGET_TRIPLESfollowing the same pattern aswasm32-wasip1. This ensures WASI Preview 2 toolchains are generated out-of-the-box alongside Preview 1, providing consistency for users working with both WASI versions.Changes
wasm32-wasip2toDEFAULT_EXTRA_TARGET_TRIPLESinrust/private/repository_utils.bzlThis addresses the feedback from @cameron-martin to include
wasm32-wasip2in the default extra target triples for automatic toolchain generation.