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 nightly toolchain to commands #172

Closed
wants to merge 4 commits into from
Closed

Conversation

FreeMasen
Copy link

To address issue #171

This change will check if rustc +nightly -V returns an error, if it does it will run rustup toolchain add nightly. It then updated the previous process to run the following.

rustup target add wasm32-unknown-unknown --toolchain nightly
cargo +nightly build --target wasm32-unknown-unknown

This should make things a little bit easier for new rust users

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! 😄 ✨✨

This flag skips:
    adding target
    compiling to wasm
    installing wasm-bindgen
    running wasm-bindgen
Copy link
Contributor

@mgattozzi mgattozzi left a comment

Choose a reason for hiding this comment

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

This is a great start and I'm real glad you took the time to do this as it'll help reduce user friction which is great 😄

I don't have too much to say besides you're going to run into some rebase issues soon depending on what order we merge things in!

src/build.rs Outdated
@@ -11,10 +11,30 @@ pub fn rustup_add_wasm_target() -> Result<(), Error> {
emoji::TARGET
);
let pb = PBAR.message(&step);
let nightly_check = Command::new("rustc").arg("+nightly").arg("-V").output()?;
Copy link
Contributor

Choose a reason for hiding this comment

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

This whole part should be it's own section/fn as there's no sub bar really. It should just be part of the process.

Copy link
Author

Choose a reason for hiding this comment

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

Just pushed an update with this change

src/build.rs Outdated
@@ -11,10 +11,30 @@ pub fn rustup_add_wasm_target() -> Result<(), Error> {
emoji::TARGET
);
let pb = PBAR.message(&step);
let nightly_check = Command::new("rustc").arg("+nightly").arg("-V").output()?;
if !nightly_check.status.success() {
let sub_bar = PBAR.message(&format!(
Copy link
Contributor

Choose a reason for hiding this comment

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

There's another PR where this is going to change a little so be prepared to rebase!

Specifically here:
https://github.com/ashleygwilliams/wasm-pack/pull/151

You'll need to add this function to the vec that way it knows to call it and how to calculate how many steps there are etc.

Copy link
Author

Choose a reason for hiding this comment

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

Let me know when I need to perform the rebase

@ashleygwilliams
Copy link
Member

hey! thanks so much for this- i think this is great, and mostly echo @mgattozzi 's comments re: rebasing, particularly from #151 which i think we intend to land first- if you could rebase this off of that i think it'd be great to try and get this into today's release.

if you're not up for that- don't worry, we can also do it for you- but i always like to ask if folks would like to do it themselves :)

@ashleygwilliams
Copy link
Member

oh also- would you mind fixing up into a single commit message? thanks so much!

@FreeMasen FreeMasen force-pushed the master branch 2 times, most recently from 7b8a55f to acd8fe8 Compare June 13, 2018 19:04
@FreeMasen
Copy link
Author

This should be 1 commit and it was re-based against the 151 branch

@ashleygwilliams
Copy link
Member

thank you so much!!

@ashleygwilliams
Copy link
Member

closed by #175 ! thanks so much!!

@ashleygwilliams ashleygwilliams mentioned this pull request Jun 13, 2018
@ashleygwilliams ashleygwilliams added this to the 0.4.0 milestone Jun 18, 2018
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.

4 participants