-
Notifications
You must be signed in to change notification settings - Fork 411
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
fix canonical paths windows #389
fix canonical paths windows #389
Conversation
@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 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 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:
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 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: I followed the same steps:
Running
I'm not sure how I could make this in a unit test. Ideas? |
b8d90c8
to
ef68e5b
Compare
So! I can reproduce this now, but it's only the crc crate
Compiling crc locally, both work:
(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 Calling This PR implements a solution that follows a strategy suggested by a few others in that rust-lang issue. |
That issue of mine has nothing to do with canonicalization, it has to do with an extra layer of escaping.
… On Oct 5, 2018, at 11:09 PM, Mackenzie Clark ***@***.***> wrote:
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.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
Agreed. A separate issue is the display as a UNC path. If it were un-escaped, it would look like this: 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! |
That comment gives us some kind of reproduction but it doesn’t translate into a test that we could use to ensure that it won’t regress, and it also is “too big” to really sort out what the root issue actually is. In my understanding a unit test is what’s being sought here.
… On Oct 13, 2018, at 10:53 AM, Mackenzie Clark ***@***.***> wrote:
@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!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
src/command/utils.rs
Outdated
} | ||
} | ||
else { | ||
/// Strips UNC from canonical path on Windows. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this comment is missplaced
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 |
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? |
87c9ae0
to
a2a3520
Compare
tests/all/utils/fixture.rs
Outdated
|
||
[dependencies] | ||
wasm-bindgen = "=0.2.21" | ||
crc = "*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is extremely fragile
I'm not sure about this strategy overall
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah i will need to talk a look at this a bit closer but i am not in favor of * deps in general, and i would prefer a more general use case test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I also agree, and I'm glad you both point it out.
In this case, I was having a problem installing crc = "1.8.1"
. I left it as * because pinning the version makes sense today, but if there was a change tomorrow in crc, we wouldn't catch it.
I can pin the version if that is preferred 😄
EDIT:
I think I may have misunderstood Steve's concern. Is the concern with the strategy of testing a specific crate? If I need to revise this test to pin point the exact problem that crc introduces, I can try and investigate. I have some ideas. Just will be more time.
Cargo.toml
Outdated
@@ -21,6 +21,7 @@ indicatif = "0.9.0" | |||
lazy_static = "1.1.0" | |||
openssl = { version = '0.10.11', optional = true } | |||
parking_lot = "0.6" | |||
path-absolutize = "1.1.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was there a particular reason for using path-absolutize
, which implements its own custom logic, rather than just calling GetFullPathName
on Windows? I expect the latter would be more robust.
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! |
👍
… On Nov 7, 2018, at 8:25 AM, Alex Crichton ***@***.***> wrote:
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!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
0a9c770
to
0c045c2
Compare
0c045c2
to
b3d62e1
Compare
Thanks @steveklabnik @alexcrichton! I have removed the test. |
👍 |
This PR addresses problems with canonical paths on windows that were reported in #380. I tested this by building a wasm app with a few dependencies on windows.
I would love somebody to check the work and try running it on another windows box targeting another wasm app 😄 .
Implementation:
Remove the usage of std::fs::canonicalize.
Unit test:
I added a unit test based on reported failures: