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

Cargo does not provide a .wasm file in the correct location when building for wasm32-unknown-emscripten #4535

Closed
Herschel opened this issue Sep 24, 2017 · 16 comments
Labels
C-bug Category: bug

Comments

@Herschel
Copy link

Herschel commented Sep 24, 2017

When building a project with the wasm32-unknown-emscripten target:
cargo build --target=wasm32-unknown-emscripten
a .js file is output in the target/wasm32-unknown-emscripten/debug folder, but not a corresponding .wasm file.

Instead, the .wasm file appears in the target/wasm32-unknown-emscripten/debug/deps folder, named with the metadata suffix.

The expected behavior would be to output both the .js and .wasm in the debug folder. The .js loader file is also trying to load .wasm file from its own folder with the metadata suffix.

If you run rustc --target=wasm32-unknown-emscripten --print=file-names main.rs, you'll see that rustc only prints main.js as an output file, even though emscripten will produce both main.js and main.wasm.

cargo 0.23.0-nightly (8118b02ac 2017-09-14)
rustc 1.22.0-nightly (26015da01 2017-09-23)
x86_64-pc-windows-msvc
emcc 1.37.21

@alexcrichton alexcrichton added the C-bug Category: bug label Sep 25, 2017
@derekdreery
Copy link
Contributor

I'm having the same problem. I'd be interested in helping to fix it - but would need to be pointed to the right area of cargo src.

@alexcrichton
Copy link
Member

I think this is the same case as #4500 (comment), neither Cargo nor rustc unfortunately knows about this output file.

@derekdreery
Copy link
Contributor

How would you go about telling rustc/cargo about this file? Would it be better to do it in rustc, or cargo? In cargo, I've found cargo::opt::cargo_rustc::Context::calc_target_filenames, which seems to be one place it could be added.

@alexcrichton
Copy link
Member

@derekdreery indeed! So up to this point we've avoided encoding anything target-specific in Cargo, it relies solely on rustc for this sort of information. That being said I think it's time to break that rule! If Cargo were to handle this then I believe yeah, that location is where the fix would go.

@derekdreery
Copy link
Contributor

Yeah I was worried that I wasn't allowed to do anything target-related, but yes this file doesn't really have anything to do with rustc.

@alexcrichton
Copy link
Member

I think ideally rustc would inform Cargo about this as it tends to have more target-specific information, but for the time being I think we can go ahead and implement it in Cargo (in a conservative fashion) and we can move it around later if need be

@derekdreery
Copy link
Contributor

OK cool.

The situation will change anyway if emscripten comes out of the build chain, and llvm is compiled directly to wasm.

tatsuya6502 added a commit to tatsuya6502/cargo that referenced this issue Oct 3, 2017
Fixes rust-lang#4500, rust-lang#4535

Until now, Cargo does not have any knowledge about target-specific
output files. It relies solely on rustc for this sort of information
(`rustc --print=file-names ...`).

As a result, Cargo does not place some build artifacts (files) to
target/{debug,release} directory. These files include *.wasm
for wasm32-unknown-emscripten target and *.dll.lib for
*-pc-windows-msvc cdylib target.

This commit will add such knowledge to Cargo so that *.wasm and
*.dll.lib will be placed in target/{debug,release} directory.
@tatsuya6502
Copy link
Contributor

@Herschel @derekdreery I opened a PR #4570 to address the issue. If you have time, can you please check if it works for you? I do not have environment with emscripten installed.

$ git clone -b target-specific-artifacts https://github.com/tatsuya6502/cargo.git
$ cd cargo
$ cargo build --release

cargo binary will be found in target/release directory.

Thanks!

@derekdreery
Copy link
Contributor

Cool! Could you submit it as a PR?

@tatsuya6502
Copy link
Contributor

tatsuya6502 commented Oct 3, 2017

Yes, I submitted it as a PR (#4570). But before it gets merged, I think one of us needs to verify that it actually solves the problem. The PR contains fixes for wasm and Windows DLL (#4500), and I verified the latter. I cannot verify the earlier because I do not have emscripten now.

@Herschel
Copy link
Author

Herschel commented Oct 4, 2017

Thanks for your work, @tatsuya6502!

The .wasm file now appears in the correct folder. However, the .js file is still trying to load the .wasm file with the incorrect name including the metadata suffix. (for example, web_test-4d1ad28c85a2ee45.wasm instead of web-test.wasm).

See this gist showing the incorrect line in the .js:
https://gist.github.com/Herschel/df22af2b04d69c0ecb6c447a54765d54#file-web-test-js-L1625

Ideally cargo would generate the correct filename in the .js file. You can either:
a) Replace (module_name)-(suffix).wasm with (module-name).wasm in the .js file.
or b) Append var Module = {wasmBinaryFile: "web-test.wasm"}; to the top of the js file.
or c) Maybe there is some way to get emscripten to generate the js with the correct filename?

@tatsuya6502
Copy link
Contributor

@Herschel Thank you for testing it!

Hmm, I think another option could be:
d) Do not ask rustc to add the metadata (-C metadata) for wasm bin target, like this for cdylib.

@alexcrichton Do you have any preference/suggestions?

@alexcrichton
Copy link
Member

@tatsuya6502 sorry I'm not sure what the question is about, I'll try to read the PR in more detail tomorrow

@tatsuya6502
Copy link
Contributor

I took option d) and added another commit f9a541b to PR #4570. I installed emsdk (incoming) to Fedora and tested wasm32-unknown-emscripten target.

I will test x86_64-pc-windows-msvc target after work. (It is almost 10am now in my timezone.)

@Herschel
Copy link
Author

Herschel commented Oct 5, 2017

I tested x86_64-pc-windows-msvc, works great now! Thank you!

@tatsuya6502
Copy link
Contributor

@alexcrichton Sorry for asking an unclear question. My original question was "is it OK to remove metadata suffix from wasm target files (js, wasm) in deps directory?" The wasm file name was embedded in js file, so we should avoid to copy wasm file something like "debug/deps/wasm_test-0f5ba9409d843569.wasm" to "debug/wasm_test.wasm".

I added a summary of changes on the PR. Hope this helps.

bors added a commit that referenced this issue Oct 9, 2017
…ichton

Handle target specific outputs such as .wasm or .dll.lib

Fixes #4500, #4535

Until now, Cargo does not have any knowledge about target-specific output files. It relies solely on rustc for this sort of information (`rustc --print=file-names ...`).

As a result, Cargo does not place some build artifacts (files) to target/{debug,release} directory. These files include *.wasm for wasm32-unknown-emscripten target and *.dll.lib for *-pc-windows-msvc cdylib target.

This commit will add such knowledge to Cargo so that *.wasm and *.dll.lib will be placed in target/{debug,release} directory.

**EDIT**: I added [a summary of changes](#4570 (comment)). Please read it for more details of changes.

**IMPORTANT**
Although I added test cases for both wasm32-unknown-emscripten and *-pc-windows-msvc cdylib targets, ~I did not do manual testing on wasm32-unknown-emscripten target as I do not have an environment with emscripten installed. It will be appreciated if anybody tests this change for wasm32-unknown-emscripten target.~  **EDIT**: Tested for both wasm32-unknown-emscripten and x86_64-pc-windows-msvc. Thanks @Herschel for the help.
@bors bors closed this as completed in f9a541b Oct 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: bug
Projects
None yet
Development

No branches or pull requests

4 participants