search

panoptix-za / web-bundler

Bundles a Rust WebAssembly frontend application for publishing
Apache License 2.0
11 stars 3 forks source link

Directly depend on wasm-pack instead of CLI #3

Closed edmellum closed 3 years ago

edmellum commented 3 years ago

Fixes #1

I tried using extra_options to pass --target-dir to cargo but it fails with wasm_bindgen which is why the environment variable is set.

JWorthe commented 3 years ago

Hi @edmellum.

Thank you for the contribution!

Regarding the environment variable, do you know how this interacts with Cargo for the non-web parts of the workspace? For example, if the workspace has other crates being built in parallel while the bundler is running, I'm worried that changing the environment variable might change the target directory Cargo uses for those other crates.

The previous approach spawned a new process with Command, so conveniently the environment variables were scoped only to that process. I suspect that Cargo is going to run our build.rs script in its own process so it could be fine, but I haven't found any docs confirming this. Do you maybe have any insights on this, and any knock on effects we should be aware of?

Cheers, Justin

edmellum commented 3 years ago

Thanks for the great review, I appreciate it a lot! 😄

Considering build.rs is compiled into its own executable and specifically given certain environment variables, I don't think anything is allowed to leak.

I've taken a cursory glance at the cargo source starting at where I believe the custom build script executable is built into a Command to run and it looks to me like there should be no leakage, although there is a bit of code between std::process::Command and where the build script executable is run, following it isn't too hard and I didn't spot anything strange.