Coordinating wasm-bindgen versions #192

data-pup · 2018-06-19T00:10: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.

As of now, there are still some rough edges that I want to hone down here. It seemed like a good idea to check in the existing work on this issue that I got done today, to make sure that I am headed in the right direction on this. Any feedback is totally welcome!

I'll also close #81 now, since I believe this will also take care of #51.

I should be able to get this all wrapped up tomorrow, had to spend a little more time than expected today getting up to speed with the changes that have occurred since then. Loving all the stuff people have contributed! 😄

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-06-19T00:12:45Z

src/bindgen.rs

+        let stderr = String::from_utf8_lossy(&output.stderr).to_string();
+        let e = Error::Cli { message, stderr };
+        Err(e)
+    }


Not a giant fan of this ☝️ Creating the Error:Cli manually, rather than through the cli(..) function seems a little hacky, but the other approach caused a return type error, since this function returns a `Result<bool, Error>

Odd. What was the error?

If I use the Error::cli function here I end up with the following error:

expected type `std::result::Result<bool, _>` found type `std::result::Result<(), _>`

This is happening because that function wraps the error up into a result, which is fine for functions that also return a Result<(), Error>, but means that it won't work for functions that return something else in the Ok case :sad`

pub fn cli(message: &str, stderr: Cow<str>) -> Result<(), Self> { Err(Error::Cli{ // ... }) }

data-pup · 2018-06-19T00:14:56Z

src/bindgen.rs

+
+pub fn wasm_bindgen_locally_installed(_path: &str) -> bool {
+    fs::metadata(LOCAL_BINDGEN_PATH).is_ok()
+}


Should I create an absolute path here? Or is the relative path in LOCAL_BINDGEN_PATH enough? I think I saw an issue regarding what happens when wasm-pack is run from within the pkg directory, so I was thinking that using the crate path could make this a little less fragile.

If relative path is satisfactory, I can remove the parameter here, otherwise an absolute path can be formed with format! :)

data-pup · 2018-06-19T00:16:26Z

src/command/init.rs

+            info!(&log, "Installing wasm-bindgen-cli...");
+            bindgen::cargo_install_wasm_bindgen(&self.crate_path, &bindgen_version, step)?;
+            info!(&log, "Installing wasm-bindgen-cli was successful.");
+        }


The control flow here is the main thing I'd like to clean up, not especially happy with these nested if statements.

data-pup · 2018-06-19T20:05:30Z

I think this is ready for review now 😄 Let me know if you have any feedback!

ashleygwilliams · 2018-06-19T20:28:04Z

@data-pup awesome! i'll take a look soon- in the meantime it looks like rust fmt needs appeasing lolsob. thanks for putting so much effort into this! it's super appreciated. as mentioned previously- i think i'm down to sneak this in in a point release because it's gonna make everything way more awesome :)

data-pup · 2018-06-19T20:36:00Z

Ahh pardon! I ran fmt before I rebased this, that's fixed now :) Happy I could help!

mgattozzi

Definitely getting there but there's still some things that need to be addressed. You've done a great job so far! :D You'll also need to make sure the part that calls wasm-bindgen in the build step uses the local one!

mgattozzi · 2018-06-20T00:22:46Z

src/bindgen.rs

 use PBAR;

-pub fn cargo_install_wasm_bindgen(step: &Step) -> Result<(), Error> {
+static LOCAL_BINDGEN_PATH: &str = "bin/wasm-bindgen";


You should cfg this so that windows is "bin\wasm-bindgen". While windows can handle it (sort of? windows pathing is wild technically the yen symbol is a valid path separator) it's best to have an option for it.

mgattozzi · 2018-06-20T00:23:23Z

src/bindgen.rs

+static LOCAL_BINDGEN_PATH: &str = "bin/wasm-bindgen";
+
+fn wasm_bindgen_exists_locally(crate_path: &str) -> bool {
+    let path_str = format!("{}/{}", crate_path, LOCAL_BINDGEN_PATH);


Same reasoning here for the path.

mgattozzi · 2018-06-20T00:24:35Z

src/bindgen.rs

+        let stderr = String::from_utf8_lossy(&output.stderr).to_string();
+        let e = Error::Cli { message, stderr };
+        Err(e)
+    }


Odd. What was the error?

mgattozzi · 2018-06-20T00:25:47Z

src/command/init.rs

-        bindgen::cargo_install_wasm_bindgen(step)?;
-        info!(&log, "Installing wasm-bindgen-cli was successful.");
+        let bindgen_version =
+            manifest::get_wasm_bindgen_version(&self.crate_path)?.ok_or(Error::CrateConfig {


We should just make a crate_config function for Error and use that here.

mgattozzi · 2018-06-20T00:25:55Z

src/command/init.rs

+            })?;
+
+        if bindgen_version.is_empty() {
+            let e = Error::CrateConfig {


then you can do return Err(Error::crate_config("wasm-bindgen version dependency was empty"))

mgattozzi · 2018-06-20T00:28:59Z

src/manifest.rs

@@ -148,6 +148,13 @@ pub fn get_crate_name(path: &str) -> Result<String, Error> {
    Ok(read_cargo_toml(path)?.package.name)
 }

+pub fn get_wasm_bindgen_version(path: &str) -> Result<Option<String>, Error> {
+    let version: Option<String> = read_cargo_toml(path)?


you shouldn't need to specify Option<String> here. The type is inferred as the return type

data-pup · 2018-06-20T16:07:24Z

Added some more commits to fix the issues mentioned in @mgattozzi 's review. Thanks for the feedback! I added some platform specific path strings, and changed the get_wasm_bindgen_version function to return a Result<String, Error>.

then you can do return Err(Error::crate_config("wasm-bindgen version dependency was empty"))

Regarding this point, I realized that there was already a crate_config error, but did run into the same issue I'd seen with the other cli helper function. These were returning a Result<(), Error>, which meant that they aren't usable in a function that does return a value in the success case.

I altered these so that they return Self, so that we can use these methods elsewhere too. This felt a little opinionated on my part, but I don't think it affects readability.

data-pup · 2018-06-20T16:08:18Z

You'll also need to make sure the part that calls wasm-bindgen in the build step uses the local one!

Still need to take care of this part, I'll do that right now though! 😃

data-pup · 2018-06-20T18:05:43Z

Ready for another round of review, whenever you have time. Thanks for the help with this!

ashleygwilliams

a few comments.

curious how this works if i have wasm-bindgen-cli installed on my computer but run wasm-pack init --no-installs?

overall, i want to have a clearer story about how we want this work, and how people can override it to get it to potentially work in other ways. perhaps a flag to not use a local version? or point to another? still thinking on it myself, but am curious your thoughts!

ashleygwilliams · 2018-06-20T19:56:49Z

src/bindgen.rs

+    dep_version: &str,
+    step: &Step,
+) -> Result<bool, Error> {
+    let msg = format!("{}Checking WASM-bindgen dependency...", emoji::CHECK);


not sure if this should be a step displayed to users- i think we should just do it in the install wasm-bindgen step

ashleygwilliams · 2018-06-20T19:57:33Z

src/bindgen.rs

 use PBAR;

-pub fn cargo_install_wasm_bindgen(step: &Step) -> Result<(), Error> {
+#[cfg(not(target_family = "windows"))]
+static LOCAL_BINDGEN_PATH: &str = "bin/wasm-bindgen";


this is local to the crate- correct? if so we'll want to add it to the .gitignore

ashleygwilliams · 2018-06-20T19:58:57Z

src/bindgen.rs

+        let installed_version = s.trim();
+        Ok(installed_version == dep_version)
+    } else {
+        let error_msg = "Could not find version of local wasm-bindgen";


we may want to give some sort of instruction about how to handle this if it happens. feels a little like a dead end right now :)

data-pup · 2018-06-21T13:14:59Z

I like your points regarding the --no-installs flag, that's an important detail I don't think my changes currently account for. Would it make sense in that case to search through the locations in the $PATH environment variable for wasm-bingen?

Overriding the local behavior with a flag makes sense to me, but right now I'm not sure if I have a solid answer to the story about how this works either. I will think about this too, but feel free to follow up if you come to a decision :)

ashleygwilliams · 2018-06-22T20:20:52Z

@data-pup i also don't know exactly what we should do! very open to suggestions. i can take some time this weekend to think on it :)

mgattozzi · 2018-06-25T13:53:48Z

Doesn't no installs imply using a local one anyways? From my understanding it won't be installing a global version. In fact wouldn't it make more sense to just use a sandboxed version anyways rather than relying on a global one? In tools like stack/cabal for Haskell it uses sandboxed versions of everything unless specified to use a global one which seems like the better choice, unless I'm misunderstanding what you mean by --no-installs @ashleygwilliams

data-pup · 2018-07-02T16:53:36Z

Just wanted to follow up on this, do you happen to have any advice for next steps on my end? If there might still be some thought required regarding the interface etc., I'm happy to hold off on this!

I know I can definitely add some extra logic to handle the --no-installs flag, but a little unsure what that should generally look like at the moment.

Btw, thanks for being nice maintainers! Appreciate the feedback so far :)

data-pup · 2018-07-20T16:00:25Z

I saw that #146 has been marked as Blocking Rust 2018, so I rebased this off of master to prepare for further work on this. At this point I think all of the changes requested by Michael have been taken care of, and I can take care of some of the minor details mentioned in Ashley's review today.

My current plan is to look for a globally installed version of wasm-bindgen that matches the dependency version if --no-installs is provided. If that cannot be found in the path, then it will fail rather than installing a new version.

If I should take a different route, let me know!

data-pup · 2018-07-20T23:17:40Z

Took a swing at this today :) Let me know if you spot any problems!

mgattozzi · 2018-07-24T14:33:10Z

@data-pup I'll have to take a look now that I'm back when I get the chance

data-pup · 2018-07-24T14:43:37Z

Sounds good to me! I can also rebase this again, now that the path things from #220 have landed 👍

fitzgen

I have a whole bunch of comments inline below, but ultimately I think this is a huge step forward for wasm-pack, that we should merge this now, and then we can do a whole bunch of mini PRs addressing my inline comments as follow ups.

Thanks @data-pup! :)

fitzgen · 2018-07-31T20:06:18Z

src/bindgen.rs

+    #[cfg(not(target_family = "windows"))]
+    let local_path = format!("{}/{}", crate_path, "bin/wasm-bindgen");
+    #[cfg(target_family = "windows")]
+    let local_path = format!("{}\\{}", crate_path, "bin\\wasm-bindgen");


If these functions were using std::path::Path[Buf], then we wouldn't need these different code paths. Is there a reason we aren't using them?

No reason, I hadn't known about those. I can fix this :)

fitzgen · 2018-07-31T20:09:47Z

src/bindgen.rs

+    // Determine if the `wasm-bindgen` dependency is already met for the given mode.
+    let dependency_met = if let Some(bindgen_path) = wasm_bindgen_path_str(path, install_permitted)?
+    {
+        wasm_bindgen_version_check(&bindgen_path, version)?


I think we don't want to propagate errors here, but instead treat them as "false, dependency not met".

fitzgen · 2018-07-31T20:13:47Z

src/bindgen.rs

+    crate_path: &str,
+    install_permitted: bool,
+) -> Result<Option<String>, Error> {
+    if install_permitted {


If we are not permitting installs, we should still allow using a local install, shouldn't we? Just not installing the local version? Or is it more of "force use of the global version and don't install it if missing"?

fitzgen · 2018-07-31T20:15:34Z

src/bindgen.rs

+    let path = env::var("PATH")?;
+    for path_dir in path.split(path_sep) {
+        let prog_str = format!("{}/wasm-bindgen", path_dir);
+        if path::Path::new(&prog_str).is_file() {


I think we should just use https://crates.io/crates/which instead of rolling our own implementation.

fitzgen · 2018-07-31T20:32:09Z

src/bindgen.rs

+    if output.status.success() {
+        let s = String::from_utf8_lossy(&output.stdout);
+        let installed_version = s.trim();
+        Ok(installed_version == dep_version)


Running wasm-bindgen --version doesn't produce just the version number:

$ wasm-bindgen --version wasm-bindgen 0.2.15

So I think we need to do a little bit more parsing here than just .trim(), since we are getting the expected version from Cargo.toml which will just be the version number.

Ah! Good catch. I can take care of that.

fitzgen · 2018-07-31T20:36:14Z

src/error.rs

@@ -44,23 +49,24 @@ pub enum Error {

 impl Error {
    /// Construct a CLI error.
-    pub fn cli(message: &str, stderr: Cow<str>) -> Result<(), Self> {
-        Err(Error::Cli {
+    pub fn cli(message: &str, stderr: Cow<str>) -> Self {


These changes to the Error constructors are making this PR a bit noisier than it otherwise would be; unless this was requested for this PR, I'd have personally left them for a separate PR.

I agree that these are a little noisy, I can totally remove these changes. The tldr of why they are there is that these methods were causing type errors when I tried to use them from within a function that didn't also return a Result<(), Error>, but you are right that making that change separately is probably wiser :)

data-pup · 2018-08-01T15:12:11Z

Thanks for the feedback @fitzgen! I can resolve the merge conflicts today, and take care of some of your inline comments as well. I can have something EOD, since this relates to a 'Blocking Rust 2018' issue 🦀

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

Continuing this work in #244, because the merge conflicts got a little too drastic.

data-pup commented Jun 19, 2018

View reviewed changes

data-pup force-pushed the coordinate-bindgen-vers-fix-146 branch 2 times, most recently from c002256 to 2060191 Compare June 19, 2018 20:00

data-pup changed the title ~~Coordinating wasm-bindgen versions (WIP)~~ Coordinating wasm-bindgen versions Jun 19, 2018

ashleygwilliams requested review from ashleygwilliams and mgattozzi June 19, 2018 20:26

ashleygwilliams added the needs review label Jun 19, 2018

ashleygwilliams added this to the 0.4.1 milestone Jun 19, 2018

ashleygwilliams added the needs cargo fmt label Jun 19, 2018

data-pup force-pushed the coordinate-bindgen-vers-fix-146 branch from 2060191 to 5a3d723 Compare June 19, 2018 20:34

mgattozzi suggested changes Jun 20, 2018

View reviewed changes

data-pup force-pushed the coordinate-bindgen-vers-fix-146 branch from 506e9cf to 0ba6e5e Compare June 20, 2018 17:54

ashleygwilliams removed the needs cargo fmt label Jun 20, 2018

ashleygwilliams requested changes Jun 20, 2018

View reviewed changes

ashleygwilliams added question Further information is requested blocked and removed needs review labels Jun 22, 2018

ashleygwilliams removed this from the 0.4.1 milestone Jul 13, 2018

ashleygwilliams added this to the 0.5.0 milestone Jul 13, 2018

Solves rustwasm#146. Check locally installed wasm-bindgen dependency.

8eb0555

data-pup force-pushed the coordinate-bindgen-vers-fix-146 branch from 0ba6e5e to 8eb0555 Compare July 20, 2018 15:44

data-pup added 3 commits July 20, 2018 14:34

Add mode to the Init struct

ef5790b

Added more logic to the bindgen file, for --no-installs.

488f0af

Added logic for handling wasm-bindgen versions.

37ac9e3

fitzgen approved these changes Jul 31, 2018

View reviewed changes

data-pup closed this Aug 1, 2018

fitzgen mentioned this pull request Aug 29, 2018

if target/wasm-bindgen already exists don't attempt to re-add/install it #51

Closed

data-pup deleted the coordinate-bindgen-vers-fix-146 branch October 13, 2018 18:20

Coordinating wasm-bindgen versions #192

Coordinating wasm-bindgen versions #192

Conversation

data-pup commented Jun 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

data-pup Jun 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

data-pup commented Jun 19, 2018

ashleygwilliams commented Jun 19, 2018

data-pup commented Jun 19, 2018

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

data-pup commented Jun 20, 2018

data-pup commented Jun 20, 2018

data-pup commented Jun 20, 2018 • edited Loading

ashleygwilliams 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

data-pup commented Jun 21, 2018

ashleygwilliams commented Jun 22, 2018

mgattozzi commented Jun 25, 2018

data-pup commented Jul 2, 2018

data-pup commented Jul 20, 2018 • edited Loading

data-pup commented Jul 20, 2018

mgattozzi commented Jul 24, 2018

data-pup commented Jul 24, 2018

fitzgen 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

data-pup commented Aug 1, 2018

data-pup commented Aug 1, 2018

data-pup Jun 20, 2018 •

edited

Loading

data-pup commented Jun 20, 2018 •

edited

Loading

data-pup commented Jul 20, 2018 •

edited

Loading