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

Wasm redux #1190

Merged
merged 3 commits into from
Feb 23, 2018
Merged

Wasm redux #1190

merged 3 commits into from
Feb 23, 2018

Conversation

stevepentland
Copy link

@stevepentland stevepentland commented Feb 22, 2018

This is a complete rebase of the existing changes in #1187 to clean up the history

cc: @kbknapp


This change is Reviewable

@mention-bot
Copy link

@stevepentland, thanks for your PR! By analyzing the history of the files in this pull request, we identified @kbknapp to be a potential reviewer.

@kbknapp
Copy link
Member

kbknapp commented Feb 23, 2018

Sorry it took me so long, since we can't really run CI against this, I wanted to set up a local wasm test to ensure it all works.

Turns out there are still quite a few things that need to happen, as it wouldn't build just yet. I was able to get it building, and have the changes locally, but there may be a better way so I'll just tell you roughly what I did in case you have more ideas.

  • Added some cfg directives to the ansi_term dep in Cargo.toml so that ansi_term won't build for target_arch="wasm32"
  • Moved the atty dep down to not compile with target_arch="wasm32" similar to ansi_term
  • Added more cfg directives to not have extern crate atty with target_arch="wasm32"
  • Changed all the cfg directives src/fmt.rs to not use atty when target_arch="wasm32"

I'm assuming we should also not compile ansi_term for wasm, so that could also be something to add, and may actually simplify what I did above.

@stevepentland
Copy link
Author

stevepentland commented Feb 23, 2018

@kbknapp It builds without any problem against wasm32-unknown-unknown. Were you trying against emscripten?

image
Edit: I also have an external project referencing my changes that uses clap and builds to wasm that has no issues either. Emscripten is (iirc) older and uses an external toolchain. Since recent nodejs and browsers can load wasm files now, I'm not sure that supporting that particular target is worth the effort.

Edit 2: The original target linked in #1132 bug is also wasm32-unknown-unknown. In the README changes I documented only compatibility with this particular target, omitting emscripten intentionally.

@kbknapp
Copy link
Member

kbknapp commented Feb 23, 2018

Ah yes, I should have mentioned I built against both wasm32-unknown-unknown and emscripten 😜

So also, feel free to correct me because I'm not a web/wasm guy at all but is there no advantage to building for emscripten and unknown-unknown? I know unknown-unknown is included by default now in nightly, but is there the possibility of people still using emscripten?

@kbknapp
Copy link
Member

kbknapp commented Feb 23, 2018

Edit 2: The original target linked in #1132 bug is also wasm32-unknown-unknown. In the README changes I documented only compatibility with this particular target, omitting emscripten intentionally.

👍 gotcha I'm good with this

@stevepentland
Copy link
Author

emscripten makes for much larger code and is slower from what I understand as it was using some interpreter. The newer unknown-unknown is the rust 'native' one and I believe is what we're going ahead with in rust core.

@kbknapp
Copy link
Member

kbknapp commented Feb 23, 2018

Ok, sounds good, thanks for explaining and for all the work on this!

@kbknapp kbknapp merged commit 2bcd6c6 into clap-rs:master Feb 23, 2018
@stevepentland
Copy link
Author

No problem! And thanks for helping me figure out more about clap! It was really helpful to get your input.

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