@xmclark thanks so much! looks like the cargo fmt check is failing.

@steveklabnik would you be willing to give this a go on your windows box?

@xmclark what's an easy way to reproduce the bug?

Compiler errors on linux/mac. I think I am not correctly conditionally compiling on windows. I am welcome to suggestions, otherwise I will dig into this tonight. I was pretty sure I tried this on my mac after I built it on my windows...but maybe I made a mistake somewhere 🙃

@steveklabnik the easiest way is creating a wasm-pack app that has rust/crate dependencies. Simply calling wasm-pack build might produce a build error because windows doesn't do well with UNC paths.

I will dig into it later this evening after I get over with my day job. Thanks for jumping on this everyone 😂

I spun up a new app via cargo-generate, added semver-parser to its Cargo.toml, modified the code to this:

extern crate cfg_if;
extern crate wasm_bindgen;
extern crate semver_parser;

mod utils;

use wasm_bindgen::prelude::*;

#[wasm_bindgen]
extern {
    fn alert(s: &str);
}

#[wasm_bindgen]
pub fn greet() {
    alert(&format!("{:?}", semver_parser::range::parse("1.0.0").unwrap()));
}

and here's the output:

> wasm-pack build

  [1/9] Checking `rustc` version...
  [2/9] Checking crate configuration...
  [3/9] Adding WASM target...
  [4/9] Compiling to WASM...
  [5/9] Creating a pkg directory...
  [6/9] Writing a package.json...
  :-) [WARN]: Field 'description' is missing from Cargo.toml. It is not necessary, but recommended
  :-) [WARN]: Field 'repository' is missing from Cargo.toml. It is not necessary, but recommended
  :-) [WARN]: Field 'license' is missing from Cargo.toml. It is not necessary, but recommended
  [7/9] Copying over your README...
  [8/9] wasm-bindgen already installed...
  [9/9] Running WASM-bindgen...
  :-) Done in 0 seconds
| :-) Your wasm pkg is ready to publish at "\\\\?\\C:\\Users\\Steve Klabnik\\tmp\\wasm-bug\\pkg".

So, I can't reproduce...

ok so- taking a look at this PR i'm starting to have the suspicion that this is not a wasm-pack bug but potentially something that needs to be fixed either in the plugin or in parcel itself.

the first step to sorting this that i would want to see is a test for wasm-pack that can reproduce the bug you've found. we have our tests run on appveyor and there may very well be a gap in our tests, but fundamentally i think the approach we have for paths on windows is reasonable.

to say a bit more here, we have to dig into the differences between canonicalize and canonicalize_path. my understanding (again could be wrong) is that canonicalize asks your OS for the path and here canonicalize will, on a windows machine, return a UNC path (e.g. \\? which u can see in Steve's output above). canonicalize_path will return a non UNC path, which is to say it will use C:. So, my understanding is that this non UNC path could be wrong, which is why canonicalize is what we use here.

my main curiosity is exactly why the UNC path doesn't work in some cases. my assumption is that there are more constraints be added in the tooling you've layered here, but i genuinely am not sure.

hopefully this was vaguely helpful? again, starting with a test that can reproduce this issue is the best step for moving forward i think. i also would like to dive into the plugin and Parcel code, so if you have links to particular bits of those code bases that could help illuminate how these paths are being used that'd be great!

@steveklabnik @ashleygwilliams thanks for the detailed example. I will be a bit more explicit in my example next time and provide a similar example.

I have tried a few more things on my windows box with both git bash and cmd:

I also was not able to reproduce the error using your example. The default dependencies in the template seem fine on windows.

I am able to reproduce a problem by installing a different package: crc.

I followed the same steps:

1. Install latest wasm-pack with git url,
2. cargo generate
3. Add crc = "1.8.1" to Cargo.toml
4. wasm-pack build

Mac@DESKTOP-NNBM40V MINGW64 ~/dev/reproduce-wasm-pack-pr-389 (master)
$ wasm-pack build

  [1/9] Checking `rustc` version...
  [2/9] Checking crate configuration...
  [3/9] Adding WASM target...
/ [4/9] Compiling to WASM...
Process exited with exit code: 101: Compilation of your program failed. stderr:

   Compiling crc v1.8.1
error: couldn't read "\\\\?\\C:\\Users\\Mac\\dev\\reproduce-wasm-pack-pr-389\\target\\wasm32-unknown-unknown\\release\\build\\crc-087346432c34fa56\\out/crc16_constants.rs": The filename, directory name, or volume label syntax is incorrect. (os error 123)
 --> C:\Users\Mac\.cargo\registry\src\github.aaakk.us.kg-1ecc6299db9ec823\crc-1.8.1\src\crc16.rs:8:1
  |
8 | include!(concat!(env!("OUT_DIR"), "/crc16_constants.rs"));
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error

error: Could not compile `crc`.

To learn more, run the command again with --verbose.

Running cargo build --target wasm32-unknown-unknown --release --lib will complete successfully.

Mac@DESKTOP-NNBM40V MINGW64 ~/dev/reproduce-wasm-pack-pr-389 (master)
$ cargo build --target wasm32-unknown-unknown --release --lib
   Compiling crc v1.8.1
   Compiling reproduce-wasm-pack-pr-389 v0.1.0 (C:\Users\Mac\dev\reproduce-wasm-pack-pr-389)
warning: function is never used: `set_panic_hook`
  --> src\utils.rs:11:9
   |
11 |         pub fn set_panic_hook() {}
   |         ^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: #[warn(dead_code)] on by default

    Finished release [optimized] target(s) in 0.76s 

I'm not sure how I could make this in a unit test. Ideas?

So! I can reproduce this now, but it's only the crc crate

> wasm-pack build

  [1/9] Checking `rustc` version...
  [2/9] Checking crate configuration...
  [3/9] Adding WASM target...
- [4/9] Compiling to WASM...
Compilation of your program failed. stderr:

    Updating crates.io index
   Compiling build_const v0.2.1
   Compiling crc v1.8.1
error: couldn't read "\\\\?\\C:\\Users\\Steve Klabnik\\tmp\\wasm-bug\\target\\wasm32-unknown-unknown\\release\\build\\crc-d224ab13f0f73b57\\out/crc16_constants.rs": The filename, directory name, or volume label syntax is incorrect. (os error 123)
 --> C:\Users\Steve Klabnik\.cargo\registry\src\github.aaakk.us.kg-1ecc6299db9ec823\crc-1.8.1\src\crc16.rs:8:1
  |
8 | include!(concat!(env!("OUT_DIR"), "/crc16_constants.rs"));
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Compiling crc locally, both work:

~\.cargo\registry\src\github.aaakk.us.kg-1ecc6299db9ec823\crc-1.8.1> cargo +nightly build --target=wasm32-unknown-unknown
   Compiling build_const v0.2.1
   Compiling crc v1.8.1 (C:\Users\Steve Klabnik\.cargo\registry\src\github.aaakk.us.kg-1ecc6299db9ec823\crc-1.8.1)
    Finished dev [unoptimized + debuginfo] target(s) in 1.48s
~\.cargo\registry\src\github.aaakk.us.kg-1ecc6299db9ec823\crc-1.8.1> cargo +nightly build
   Compiling crc v1.8.1 (C:\Users\Steve Klabnik\.cargo\registry\src\github.aaakk.us.kg-1ecc6299db9ec823\crc-1.8.1)
    Finished dev [unoptimized + debuginfo] target(s) in 0.36s

(They also do in release mode, no interesting output though, so I didn't bother including it)

I have no idea what's going on here.

Yeah this is an especially odd one!

I think the main reason is how the std canonicalize function works on windows. It is pretty well documented in this issue.

Calling fs::canonicalize on windows produces a path with a few extra leading characters e.g. \\?C:\blah. This kind of path is not understood by cargo (and many other programs on windows). It is also the same cause for another issue reported recently about the pkg path not being printed correctly.

This PR implements a solution that follows a strategy suggested by a few others in that rust-lang issue.

Agreed. A separate issue is the display as a UNC path.

If it were un-escaped, it would look like this:
| :-) Your wasm pkg is ready to publish at "\\?\C:\Users\Steve Klabnik\tmp\wasm-bug\pkg"

I would expect it to be printed without the leading \\?\.

Wow! This comment is starting to sound a lot like an issue report... I made this fix as well in this PR. If the project would prefer that be a separate PR, I'm up for removing that extra revision.

hey! so- this issue PR convo is indeed getting long. @xmclark in order to accept this PR, i need a test case that demonstrates this bug. let me know if you need help or have questions about that!

@xmclark following up- are you around on a sync channel anywhere? would it be helpful to discuss a bit?

@ashleygwilliams hey sorry for late reply. Long week.

I apologize if there has been some miscommunication on my part. I would like to do whatever I need to help reviewers. I was under the impression that I had provided all the needed information for this to be reviewed. The description references the issue number that this PR fixes. This comment demonstrates how to reproduce the bug.

@technetos was super helpful and got me onto the rust-lang and the rust programming language discords last week. Are those good channels for discussion? Thanks!

Maybe the repro of #413 could be made into a unit test since it will also get fixed by this PR. The underlying issue there is that when you call cargo from a std::process::Command with a UNC Path cargo cannot handle it.

@xmclark you can find me on most rust platforms as @ag_dubs if you'd like to talk sync- and no worries on the delay- it's important to fix this but i have also been traveling :)

I'm not sure what's up with the appveyor build. This code is up for review! I have decided on a different implementation which avoids some of the problems with std::canonicalize. I also added two tests that capture the reported fail cases.

I'm not sure what's up with the appveyor build.

If you rebase on master, the appveyor builds should be fixed now. See #417 for details.

This PR looks good to me. I'm unfamiliar with the absolutize crate, but if it fixes the problem then that sgtm. Do either of @ashleygwilliams or @alexcrichton want to verify that this is the correct fix?

I'm unfortunately away for windows for awhile now so I can't verify this, but sorry if this has already been asked by why is canonicalization done? Could the original path just be plumbed around?

@alexcrichton that is a good point. I removed absolutize (and canonicalize) and it just works. It seems that we do not need to canonicalize the path at all.

Ok great!

Is this ready to go in that case? Or are there final points to take care of?

Thanks!

I think I could use some feedback on how to fix my unit test that uses the crc-rs crate. The test is valid, but I have received feedback that not pinning the dependency version is bad. I am also a little concerned that we are NOT testing for the underlying problem. I think I understand that fundamental issue, but recreating it in a test fixture would could be really complicated. The crc-rs crate has a custom build step that generates some rust code in the out_dir. When included, the path to the generated file is in the windows UNC path format, and cargo chokes on it. This only happens when building with wasm-pack and this PR fixes that issue.

I am interested in any suggestions on this!

I think it's probably ok to not include that test and we can mostly wing it for now. This is a particularly difficult aspect to test, but we've got enough users that it should crop up quickly again if it comes up!

Thanks @steveklabnik @alexcrichton! I have removed the test.

👍