Coordinate wasm-bindgen versions. #244

data-pup · 2018-08-01T22:48:54Z

Fixes #146. This aims to solve the issue of making sure that there is (a) a locally installed wasm-bindgen and (b) that it is the same version as specified in the Cargo.toml file.

Rebasing the previous PR ended up being too much effort after some intensive changes were made in the meantime, so I re-did the work off the current master branch.

While doing so, I fixed most of the issues that @fitzgen mentioned in their review. There is one remaining detail to take care of tomorrow, regarding how the output of wasm-bindgen --version is parsed. I thought it would be useful to get this new PR open before I take care of that, so the rest of this can be reviewed :)

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

data-pup · 2018-08-01T22:52:22Z

src/bindgen.rs

+        Ok(installed_version == dep_version)
+    } else {
+        // FIXUP: This error message can be improved.
+        let message = String::from("Could not find version of local wasm-bindgen");


As the "FIXUP" comment mentions, I think this error message can be improved. Parsing the output of the wasm-bindgen --version command will also be some fixup work I can get finished tomorrow.

EDIT: On second thought, @fitzgen mentioned that we probably shouldn't be propagating errors when we call this function. I can edit this so that it returns a plain boolean value, rather than a Result<bool, Error>

data-pup · 2018-08-01T22:55:02Z

src/bindgen.rs

+    if let Ok(path) = which("wasm-bindgen") {
+        Some(path)
+    } else {
+        None


Other details I know I need to take care of: These functions can be collapsed into a single function, and should be named wasm_bindgen_path rather than being suffixed path_str, since these return PathBuf values rather than strings now.

data-pup · 2018-08-01T22:56:05Z

src/manifest.rs

@@ -199,6 +199,9 @@ fn check_wasm_bindgen(path: &Path) -> Result<(), Error> {
        "Ensure that you have \"{}\" as a dependency in your Cargo.toml file:\n[dependencies]\nwasm-bindgen = \"0.2\"",
        style("wasm-bindgen").bold().dim()
    ))
+    // FIXUP: Use the `get_wasm_bindgen_version` function for this?
+    // let _ = get_wasm_bindgen_version(path)?;
+    // Ok(())


This also needs to get cleaned up. Pardon!

data-pup · 2018-08-01T22:59:33Z

src/bindgen.rs

+        wasm_bindgen_version_check(&bindgen_path, version).unwrap_or_else(|_| false)
+    } else {
+        false
+    };


This is a little wonky right now, because the version check returns a result. As mentioned below, I think I can edit that so that it returns a plain boolean value, which should make this much nicer.

data-pup · 2018-08-01T23:00:21Z

I added some various nits that I already see, but if I missed anything do let me know! I should have this cleaned up tomorrow.

data-pup · 2018-08-02T16:19:02Z

This is all cleaned up and ready for review now 😸 Let me know if there are any changes that I should make!

mgattozzi · 2018-08-06T13:59:04Z

Cargo.toml

@@ -23,6 +23,7 @@ slog-term = "2.4"
 slog-async = "2.3"
 structopt = "0.2"
 toml = "0.4"
+which = "2.0.0"


I didn't know this existed and now I'm happy to see it does.

mgattozzi

I think you did a really good job! I'm gonna have to test locally before I approve or deny but wanted to write down my first pass thoughts :D

mgattozzi · 2018-08-06T13:59:48Z

src/bindgen.rs

+        .map(|bindgen_path| wasm_bindgen_version_check(&bindgen_path, version))
+        .unwrap_or(false)
+    {
+        let msg = format!("{}WASM-bindgen already installed...", emoji::DOWN_ARROW);


WASM should be wasm both here and the message below :)

mgattozzi · 2018-08-06T14:03:03Z

src/bindgen.rs

        .arg("--force")
+        .arg("wasm-bindgen-cli")
+        .arg("--version")
+        .arg(version)


My understanding is that this will keep them pinned to the same version as the package, but if they use a new one in a different project it'll overwrite it?

How do we account for versions such as "0.2" which allows for any version from ">= 0.2 && < 0.3"? which version should be chosen in that case? The one in the lockfile? I only realized we'd have to answer this question just now unfortunately :/

Oh I see now further below you're specifying to install it locally and to use that one like was discussed. Awesome :D I still wonder about the versioning in the second part of my comment.

@mgattozzi Regarding the second part of your first comment, would this crate be a sensible solution? https://crates.io/crates/semver

Still thinking about how its VersionReq struct would be used to find the most recent version that satisfies a requirement like >= 0.2 && < 0.3, but it seems like a promising library for this.

That is the canonical semver crate. I'm just thinking like:

I build a program using "0.2" as my version
Cargo.lock has it at 0.2.8

A new release is out and wasm-bindgen is at 0.2.9

Which one do you use? The lockfile or new crates.io? I think because it's a library the newest one on crates.io that fits the description, but that also means running cargo update which may not be desired behavior. I'm more asking this question to you and also @ashleygwilliams and whoever else might have a stake in answering this.

I think we should always follow the lock file. Since we do (or will do) a cargo build before getting the wasm-bindgen CLI, the lock file will always have the exact wasm-bindgen used to build the .wasm binary.

Following the lock file makes a lot of sense to me! If it makes sense to everybody else, I could try adjusting the get_wasm_bindgen_version function so that it looks through the Cargo.lock file rather than the .toml file.

If I do go this route, should get_wasm_bindgen_version end up in a new file, i.e. lockfile.rs given that we wouldn't be checking the manifest itself?

mgattozzi · 2018-08-06T14:09:05Z

src/bindgen.rs

-    } else {
-        Ok(())
-    }
+    local_bindgen_path(crate_path).or_else(global_bindgen_path)


This is a pretty clever usage of closures I really like it :)

mgattozzi · 2018-08-06T14:09:40Z

src/command/build.rs

@@ -71,21 +72,27 @@ impl Build {
    pub fn try_from_opts(build_opts: BuildOptions) -> Result<Self, Error> {
        let crate_path = set_crate_path(build_opts.path);
        let crate_name = manifest::get_crate_name(&crate_path)?;
+        let mode = match build_opts.mode.as_str() {
+            "no-install" => BuildMode::Noinstall,
+            "normal" => BuildMode::Normal,


Is there a reason to have this if it always defaults to normal?

Not really! I had included both cases for aesthetic reasons only, but I can clean that up.

mgattozzi · 2018-08-06T14:10:03Z

src/command/build.rs

@@ -213,8 +220,19 @@ impl Build {
    }

    fn step_install_wasm_bindgen(&mut self, step: &Step, log: &Logger) -> Result<(), Error> {
+        info!(&log, "Identifying WASM-bindgen dependency...");


s/WASM/wasm/g

data-pup · 2018-08-23T20:09:21Z

@mgattozzi, this is ready for review now!

Update: AppVeyor failed because of this error, is that related to something I should fix myself?

cargo test --locked -- --test-threads 1
    Updating registry `https://github.com/rust-lang/crates.io-index`
 Downloading which v2.0.0
error: unable to get packages from source
Caused by:
  [35] SSL connect error (schannel: next InitializeSecurityContext failed: Unknown error (0x80092013) - The revocation function was unable to check revocation because the revocation server was offline.)

fitzgen

This looks wonderful to me! I think we can merge this and if @mgattozzi notices anything that I missed, we can file follow up issues for those things :)

Thanks for sticking with this @data-pup!

data-pup · 2018-08-24T18:26:05Z

Yay! 🎉 Really happy to hear :)

mgattozzi · 2018-08-25T22:36:57Z

@data-pup Thanks for this. I've opened up an issue in #270

xmclark · 2018-08-26T16:56:27Z

src/manifest.rs

+/// Get the version of `wasm-bindgen` specified as a dependency.
+pub fn get_wasm_bindgen_version(path: &Path) -> Result<String, Error> {
+    if let Some(deps) = read_cargo_toml(path)?.dependencies {
+        match deps.get("wasm_bindgen") {


Is this correct? I thought it would be "wasm-bindgen". This line is giving me problems locally.

data-pup mentioned this pull request Aug 1, 2018

Coordinating wasm-bindgen versions #192

Closed

3 tasks

data-pup commented Aug 1, 2018

View reviewed changes

data-pup force-pushed the wasm-bindgen-coord branch from 489ffce to e582d08 Compare August 2, 2018 15:35

mgattozzi reviewed Aug 6, 2018

View reviewed changes

Coordinate wasm-bindgen versions.

8301613

data-pup force-pushed the wasm-bindgen-coord branch from e095fca to 8301613 Compare August 23, 2018 20:11

fitzgen approved these changes Aug 24, 2018

View reviewed changes

fitzgen merged commit d07ef5a into rustwasm:master Aug 24, 2018

xmclark reviewed Aug 26, 2018

View reviewed changes

ashleygwilliams added this to the 0.5.0 milestone Sep 4, 2018

data-pup mentioned this pull request Sep 6, 2018

Error constructor methods should return Self instead of Result<(), Self> #279

Closed

data-pup deleted the wasm-bindgen-coord branch October 13, 2018 18:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Coordinate wasm-bindgen versions. #244

Coordinate wasm-bindgen versions. #244

data-pup commented Aug 1, 2018

data-pup Aug 1, 2018 •

edited

Loading

data-pup Aug 1, 2018

data-pup Aug 1, 2018

data-pup Aug 1, 2018

data-pup commented Aug 1, 2018

data-pup commented Aug 2, 2018

mgattozzi Aug 6, 2018

mgattozzi left a comment

mgattozzi Aug 6, 2018

data-pup Aug 6, 2018

mgattozzi Aug 6, 2018

mgattozzi Aug 6, 2018

data-pup Aug 6, 2018

mgattozzi Aug 6, 2018

fitzgen Aug 6, 2018

data-pup Aug 7, 2018

mgattozzi Aug 6, 2018

mgattozzi Aug 6, 2018

data-pup Aug 6, 2018

mgattozzi Aug 6, 2018

data-pup commented Aug 23, 2018 •

edited

Loading

fitzgen left a comment

data-pup commented Aug 24, 2018

mgattozzi commented Aug 25, 2018 •

edited

Loading

xmclark Aug 26, 2018

fitzgen Aug 27, 2018

Coordinate wasm-bindgen versions. #244

Coordinate wasm-bindgen versions. #244

Conversation

data-pup commented Aug 1, 2018

data-pup Aug 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

data-pup commented Aug 1, 2018

data-pup commented Aug 2, 2018

Choose a reason for hiding this comment

mgattozzi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

data-pup commented Aug 23, 2018 • edited Loading

fitzgen left a comment

Choose a reason for hiding this comment

data-pup commented Aug 24, 2018

mgattozzi commented Aug 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

data-pup Aug 1, 2018 •

edited

Loading

data-pup commented Aug 23, 2018 •

edited

Loading

mgattozzi commented Aug 25, 2018 •

edited

Loading