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

chore: switch from wasm32-unknown-unknown to wasm32-wasip1 #373

Closed

Conversation

greenhat
Copy link
Contributor

As a workaround for our old rustc version (fails in fib tests). See rust-lang/libm#214 (comment)
Besides, that I think we should stick to wasm32-wasi everywhere.

workaround for our old rustc version (fails in fib tests).
See rust-lang/libm#214 (comment)
Besides, that I think we should stick to `wasm32-wasi` everywhere.
@greenhat greenhat requested a review from bitwalker January 14, 2025 14:08
@greenhat
Copy link
Contributor Author

@bitwalker Although I stacked this PR on the #372, the merging order is not important here.

@bitwalker
Copy link
Contributor

I didn't need to make this change to get our build working, I think only the old wasm32-wasi target was removed, so that was causing tests to fail, but removing that target via rustup was sufficient to get things working again (although I combined it with an update of the stable/nightly toolchains, so there may be some fixes I picked up that I am not aware of).

I think we should continue to use wasm32-unknown-unknown for tests which are not Component Model based, since it is technically the appropriate target for pure Rust projects that don't target the rollup, and it doesn't hurt to have a check on any unintentional assumptions we make due to the nature of wasm32-wasip1 or wasm32-wasip2 targets.

Given that, I'm going to close this for now I think, if you disagree, feel free to reopen and let's discuss further.

@bitwalker bitwalker closed this Jan 17, 2025
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.

2 participants