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

add an option to pass an arbitrary set of arguments to cargo build #461

Merged
merged 3 commits into from
Jan 15, 2019

Conversation

torkve
Copy link
Contributor

@torkve torkve commented Dec 16, 2018

Fixes #455.
Example: wasm-pack build -- --features "feature-a feature-b"

Make sure these boxes are checked! πŸ“¦βœ…

  • You have the latest version of rustfmt installed and have your
    cloned directory set to nightly
$ rustup override set nightly
$ rustup component add rustfmt-preview --toolchain nightly
  • You ran rustfmt on the code base before submitting
  • You reference which issue is being closed in the PR text

✨✨ πŸ˜„ Thanks so much for contributing to wasm-pack! πŸ˜„ ✨✨

@ashleygwilliams
Copy link
Member

@torkve thanks so much for this! would you mind adding a test? and potentially an example to the documention? thanks so much!

@torkve
Copy link
Contributor Author

torkve commented Dec 18, 2018

I've added a test and a bit of docs, however English is not my native language, so feel free to fix it.

@torkve
Copy link
Contributor Author

torkve commented Jan 13, 2019

Are there any news with this PR? :-)

@ashleygwilliams
Copy link
Member

hey @torkve - thank you for this and sorry for the lack of and delay of reply. i'm currently a little blocked on getting 0.6.0 out. my only concern with this PR is that we may also want to pass extra unsupported arguments to wasm-bindgen and this interface poses a bit of a conflict with that. if it were gated by a flag (perhaps --cargo-args) that might work better? i need to perhaps think on exactly what that interface should be. does that concern make any sense? if you have thoughts i'd be happy to hear them!

@ashleygwilliams ashleygwilliams added needs design and removed needs docs please add docs to this PR needs tests please add tests to this PR labels Jan 13, 2019
@torkve
Copy link
Contributor Author

torkve commented Jan 14, 2019

I suppose you are correct from the POV that options should be similar. --cargo-args and --wasm-bindgen-args would be more intuitive than my implementation.
The reason I wrote it this very way, is the special additional quotation required otherwise. For example, two ways to specify feature set:

$ wasm-pack ... -- --features "feature-a feature-b"
$ wasm-pack ... --cargo-args "--features 'feature-a feature-b'"

Second variant is not pretty enough. And if one would need to escape some characters, it'll become even more complicated.

While currently I do not see any possible complex values in wasm-bindgen arguments, I think it would not be the best option to make it look just like wasm-pack ... --wasm-bindgen-args "..." -- --cargo-args...

May be every wasm-bindgen option just should be exposed in wasm-pack?

@danwilhelm
Copy link
Member

Would setting env vars to pass arguments possibly be a solution? That might make it somewhat consistent with cargo, since RUSTFLAGS could be set as well to send some flags to rustc.

Perhaps @alexcrichton might have ideas? I believe he contributed to passing arguments to rustc from cargo.

@alexcrichton
Copy link
Contributor

I'd personally advocate for a design that:

  • Like this PR, accepts arbitrary arguments after -- to pass to Cargo
  • Adds most first-class options from Cargo to wasm-pack build, like --features, --test, etc.
  • Also accepts arbitrary arguments to wasm-bindgen, but as first-class options to wasm-pack. The wasm-bindgen tool is basically an internal implementation detail of wasm-pack, whereas cargo is a first-class feature. I'd much rather prominently support Cargo than wasm-bindgen

Copy link
Member

@ashleygwilliams ashleygwilliams left a comment

Choose a reason for hiding this comment

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

after thinking about it- i'm inclined to agree with @alexcrichton! i think this looks great- i'll work to get it into 0.6.0

@ashleygwilliams ashleygwilliams merged commit 5258d42 into rustwasm:master Jan 15, 2019
@fitzgen fitzgen added the TWiRaWA Nominate this PR for inclusion in the next issue of This Week in Rust and WebAssembly label Feb 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - feature TWiRaWA Nominate this PR for inclusion in the next issue of This Week in Rust and WebAssembly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow passing extra command-line flags to cargo
5 participants