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

Support compiling on wasm targets #1068

Closed
wants to merge 4 commits into from

Conversation

jozanza
Copy link
Contributor

@jozanza jozanza commented May 9, 2024

In this PR, I added target_family = "wasm" to the compile_error! exceptions list in a few modules.

For more context, in a wasm game engine SDK I maintain (turbo), I'm pulling in a crate (solana-sdk). However, cc-rs is deeply buried in dependencies of dependencies of that crate. This wouldn't be a terrible problem since cc-rs (especially the parallel feature stuff) is far from any code path that runs in wasm and optimization can get rid of a unused code, however, the compile_error! macro prevents cc-rs prevents builds from ever completing in the first place.

So the only option in my case (where cc-rs is buried deep in the dependency tree and I'm targeting wasm) currently is to fork and patch. This gets brittle fairly quickly since other dependencies can have version conflicts and force me to create more branches with version bumps and other minor tweaks over time.

I'm hoping we can land this patch and carve out exceptions for wasm with compile_error! moving forward 🙏🏽

@madsmtm
Copy link
Contributor

madsmtm commented May 9, 2024

Wait... These configs are for the host machine - why are you compiling cc itself to run on WASM?

Anyhow, I'd rather suggest that you fix the dependencies of solana-sdk that rely on cc, namely blake3 and solana-program, of which the last one is already cfg-guarded, so it should just work on WASM.

Copy link
Collaborator

@NobodyXu NobodyXu left a comment

Choose a reason for hiding this comment

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

(Deleted)

Copy link
Collaborator

@NobodyXu NobodyXu left a comment

Choose a reason for hiding this comment

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

(Sorry I'm bit sleepy today.)

Is cc-rs used in the build-script?
If so then it would be build for your host OS instead of wasm.

@jozanza
Copy link
Contributor Author

jozanza commented May 11, 2024

Wait... These configs are for the host machine - why are you compiling cc itself to run on WASM?

Anyhow, I'd rather suggest that you fix the dependencies of solana-sdk that rely on cc, namely blake3 and solana-program, of which the last one is already cfg-guarded, so it should just work on WASM.

Yeah that's a great suggestion, and it is fixed in the latest version of solana-sdk as you noticed. The problem was originally caused by that dependency not being cfg-guarded in older versions. Unfortunately, those older versions are often still in use and will always need a patch.

It's very easy for library authors to not cfg-guard their deps with wasm exclusions (or even realize they should) or to include a dependency that fails to do so. So if another dependency of a dependency makes the same mistake (not cfg-guarding cc) even after such a fix, we'll wind up with the same problem in the future, and once again need to fork/patch. So that's my train of thought here. I'd also suggest that perhaps the compile error being thrown may not be particularly useful for the wasm platform target in practice.

@NobodyXu
Copy link
Collaborator

@jozanza cc is used as a build-dep right?

If it is a build-dep, then I can't imagine it causing your wasm build to fail, because your host OS is still unix/windows.

Is cc used as a normal dependency in your crate?

@jozanza
Copy link
Contributor Author

jozanza commented May 13, 2024

You're right @NobodyXu. The issue is it's not my crate that directly uses cc. If it was, using build-deps or cfg-guarding would work just fine. In my situation, cc is deep in the dependency tree, so I can't control these things by any other means than patching.

@NobodyXu
Copy link
Collaborator

Could you do a cargo tree -i cc?

I wonder what is their use case for this, if it's a valid use case (not a mistake) I'm willing to merge this PR.

Usually nobody uses cc on wasm, because wasm does not support spawning any process, which means you can't really use cc to compile anything on wasm.

@thomcc thomcc changed the title Support compiling to wasm targets Support compiling on wasm targets Jul 3, 2024
@NobodyXu
Copy link
Collaborator

Hello, now that I think about it again, we actually should support wasm/wasi.

There's ongoing effort to support sandboxing (likely using wasi), so we should plan ahead.

cc @thomcc @the8472

@NobodyXu
Copy link
Collaborator

cc @jozanza sorry for the delay, I now think it makes sense to accept this PR, just a minor question.

@jozanza
Copy link
Contributor Author

jozanza commented Jul 12, 2024

Oh great to hear @NobodyXu! What's the question?

@NobodyXu
Copy link
Collaborator

Oh great to hear @NobodyXu! What's the question?

There's a comment on the command_helper, just a minor one

@NobodyXu
Copy link
Collaborator

Can you also update CI to run cargo check for wasm targets?

Thanks

@NobodyXu
Copy link
Collaborator

Thanks, I've opened #1160 which runs the wasm in CI

NobodyXu added a commit that referenced this pull request Jul 14, 2024
* Don't throw compiler error for wasm targets

* Don't throw compiler error for wasm targets

* Fix attr syntax error

* Add unimplemented path for pipe error match for wasm target

* Replace `unimplemented!` with `panic!`

Signed-off-by: Jiahao XU <[email protected]>

* Add CI check for wasm

Signed-off-by: Jiahao XU <[email protected]>

* Fix CI

Signed-off-by: Jiahao XU <[email protected]>

* Fix clippy warning on wasm

Signed-off-by: Jiahao XU <[email protected]>

---------

Signed-off-by: Jiahao XU <[email protected]>
Co-authored-by: Josiah Savary <[email protected]>
@github-actions github-actions bot mentioned this pull request Jul 14, 2024
@jozanza
Copy link
Contributor Author

jozanza commented Jul 14, 2024

Awesome! Thanks @NobodyXu

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.

3 participants