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

v1.0.102 breaks wasm builds with trunk #1113

Closed
Kuuuube opened this issue Jun 29, 2024 · 9 comments
Closed

v1.0.102 breaks wasm builds with trunk #1113

Kuuuube opened this issue Jun 29, 2024 · 9 comments

Comments

@Kuuuube
Copy link
Contributor

Kuuuube commented Jun 29, 2024

I depend on cc through zstd-safe (zstd-safe -> zstd-sys -> cc). v1.0.102 causes wasm builds through trunk (ex: trunk serve) to display the following error when accessing the wasm webpage.

Uncaught TypeError: The specifier “env” was a bare specifier, but was not remapped to anything. Relative module specifiers must start with “./”, “../” or “/”.

I've tested manually reverting cc through Cargo.lock and a clean build. In my testing, cc v1.0.101 does not have this issue.

Possibly related to #1105 and building with wasm32-unknown-unknown target.

Repo and build steps for reference (I've committed the cargo lock with v1.0.101 to mitigate this issue currently):
https://github.com/Kuuuube/hentaigana_input
https://github.com/Kuuuube/hentaigana_input/blob/main/.github/workflows/pages.yml

@NobodyXu
Copy link
Collaborator

Thank you for reporting!

cc @antaalt It seems that for wasm-unknown-unknown and wasmp1, mentioned in #1109 , we don't have to explicitly pass the wasi root and the compiler (clang) can infer it

@NobodyXu
Copy link
Collaborator

NobodyXu commented Jun 30, 2024

Both use ubuntu, so I guess clang on Linux can find the wasi root itself?

Or maybe clang on Linux already has the wasi libraries required.

Or perhaps it's just that Ubuntu-latest runner already has it installed and configured.

NobodyXu added a commit that referenced this issue Jun 30, 2024
On ubuntu-latest github action runner, it used to work without the environment variable.

Fixed #1109 #1113
NobodyXu added a commit that referenced this issue Jun 30, 2024
* Fix compilation for wasm: env WASI_SYSROOT should be optional

On ubuntu-latest github action runner, it used to work without the environment variable.

Fixed #1109 #1113

* Add wasm32-unknown targets (#1115)

---------

Co-authored-by: Kuuuube <61125188+Kuuuube@users.noreply.github.com>
@antaalt
Copy link
Contributor

antaalt commented Jun 30, 2024

Thank you for reporting!

cc @antaalt It seems that for wasm-unknown-unknown and wasmp1, mentioned in #1109 , we don't have to explicitly pass the wasi root and the compiler (clang) can infer it

The issue here is probably that wasm32-u known-unknown is web assembly and not WASI, so we don't need WASI sysroot at all I think, not sure how to compile webassembly correctly though, it does not support exception either so these changes should be working I think

@NobodyXu
Copy link
Collaborator

The issue here is probably that wasm32-u known-unknown is web assembly and not WASI, so we don't need WASI sysroot at all I think

Yeah but it also happened for wasip1, so I think we should make the WASI_SYSROOT optional.

@antaalt
Copy link
Contributor

antaalt commented Jun 30, 2024

Yes, that's probably best

@dhedey
Copy link

dhedey commented Jun 30, 2024

This might be totally unrelated, and I don't know anything about the motivation for these changes, but in #1105 the tool match statement changed the match logic for wasm32-unknown-unknown. Was this intentional @antaalt ?

Specifically:

                } else if target == "wasm32-wasi"
                    || target == "wasm32-unknown-wasi"
                    || target == "wasm32-unknown-unknown"
                {

was replaced by Build::is_wasi_target(target) which doesn't pass for wasm32-unknown-unknown anymore

@antaalt
Copy link
Contributor

antaalt commented Jun 30, 2024

This was an issue on my side, but it is now fixed with #1115

@NobodyXu
Copy link
Collaborator

cc @Kuuuube Can you try 1.0.103 to see if it fixes your issue?

@Kuuuube
Copy link
Contributor Author

Kuuuube commented Jun 30, 2024

It works! Thanks for getting to this so fast.

@Kuuuube Kuuuube closed this as completed Jun 30, 2024
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

No branches or pull requests

4 participants