Recurse up the path looking for the Cargo.toml #624

gameldar · 2019-04-12T15:34:58Z

PR for #620 Walk up parent directories to find the current crate

When setting the crate path, if the default path of the current working directory is defined pass back a flag indicating the default was used
If the default is used, and a path is not specified, then walk up the path to find the Cargo.toml file

I'm relatively new to Rust and figured this was a good first step to getting to know the project and wet my feet, but I'm not sure I'm doing things the idiomatic way - so I'm happy to have all and any feedback!

fitzgen

Looks good to me modulo the one suggestion below. Thanks @gameldar! (Sorry for the delayed review!)

fitzgen · 2019-04-24T21:00:28Z

src/manifest/mod.rs

+        let full_path = crate_path.canonicalize()?;
+        let full_path = full_path.as_path();
+        let mut manifest_path = crate_path.join("Cargo.toml");
+        if recurse {


I think this logic should be in set_crate_path (which should probably be renamed get instead of set) so that we don't have to deal with a bool part in a returned tuple nor a bool parameter. It would be cleaner without that, IMO.

That was the part that I thought was quite clunky but I was worried about the scope of the refactoring. As it turns of it was the opposite of what I thought - it is the smaller refactoring and is a lot cleaner and obvious. Thanks!

fitzgen

Looks good; moving the logic into get_crate_path was a nice clean up. Will merge with comments below addressed (sorry if these were existing things I just didn't notice in the first round of review!)

Thanks again!

fitzgen · 2019-04-26T16:41:22Z

src/command/utils.rs

+/// to provide the appropriate error
+fn find_manifest_from_cwd() -> Result<PathBuf, failure::Error> {
+    let cwd = PathBuf::from(".");
+    let parent_path = cwd.canonicalize()?;


I'm a little hesitant to add canonicalize calls here, since we have historically had issues with canonicalized paths on windows (and in non-english locales?) that I never fully understood the root causes for.

If this isn't required for correctness (I don't think any APIs we use require canonical paths, and work just fine with relative paths), then I'd like to remove the canonicalize call. Or maybe @ashleygwilliams or @alexcrichton understood the bug we hit better than I do.

Sorry I missed this in the first round of review!

I think this shoudl be fixable by basically just using env::current_dir() rather than canonicalize which should return an appropriate absolute path

Before refactoring I did need to use canonicalize - the problem was that a relative path coming into the function (e.g. PathBuff::from(".") that was the default) - isn't absolute so parent_path.parent() returns None even if you are not in the root directory. I needed to canonicalize it to ensure I had an absolute path.

However the logic changed after migrating the code into the the get_crate_path as now we take the current working directory and then work back from there. So thanks @alexcrichton using env::current_dir() gives the absolute path needed. Looking back at the output on Windows of the cwd after it was canonicalized it had a whole bunch of escaping included - compared to the absolute path.

fitzgen · 2019-04-26T16:46:24Z

src/command/utils.rs

+fn find_manifest_from_cwd() -> Result<PathBuf, failure::Error> {
+    let cwd = PathBuf::from(".");
+    let parent_path = cwd.canonicalize()?;
+    let mut parent_path = parent_path.as_path();


This call should be unnecessary, and I think we can wholesale remove this line. Since PathBuf derefs to Path the .join call below should Just Work(tm).

Ah yes I see now - after more digging due to conflicting type errors while trying to address this I actually found the pop method on PathBuf... which is exactly what I was doing but better!

fitzgen · 2019-04-26T16:48:21Z

src/command/utils.rs

+                None => break Ok(cwd),
+            };
+        } else {
+            break Ok(parent_path.to_path_buf());


These breaks should be return.

When it is break, I double check if there is a let foo = loop { ... }; but there isn't, and then I realize it is the last expression in the function, so it is equivalent to return... and we can avoid readers doing that same little bit of confusion+understanding in the future by using return.

I agree - this was my attempt to do things the 'idiomatic' way - it seems from what I've been reading rustwise the preferred thing is to not have explicit return calls. I personally prefer explicit use of return because it makes it clear that is the intention (although some of that is that half the time I automatically add the semi colon and wonder why the compiler is saying I forgot to return a value from the function)

fitzgen · 2019-04-26T16:51:35Z

AppVeyor failure:

error: package collision in the lockfile: packages project_b v0.1.0 (C:\projects\wasm-pack-071k0\target\t\.tmpFEj1Z4\wasm-pack\project_b) and project_b v0.1.0 (C:\projects\wasm-pack-071k0\target\t\.tmpFEj1Z4\wasm-pack\main\../project_b) are different, but only one can be written to lockfile unambiguously

Maybe related to canonicalize or are we manually add "/" path separators somewhere? Double checking the PR...

tests/all/manifest.rs

fitzgen · 2019-04-26T16:56:19Z

So the it_should_build_nested_transitive_dependencies test has hard coded file paths:

            [dependencies]
            wasm-bindgen = "0.2"
            project_a = { path = "../project_a" }
            project_b = { path = "../project_b" }

and I think this is fine and cargo understands how to translate unix separators into windows separators so long as the path is not canonical(/absolute/has windows separators?). So I think this test failure will be fixed by removing the call to canonicalize which will in turn make our underlying cargo invocation do the Right Thing Automatically For Us(tm).

gameldar · 2019-04-27T08:58:37Z

I asked this on discord but it disappeared up into the ether:
What is the policy is for cleaning up the PR? E.g. should I squash and do a force push to my fork to bring it back down to one commit?

fitzgen · 2019-04-29T16:08:52Z

What is the policy is for cleaning up the PR? E.g. should I squash and do a force push to my fork to bring it back down to one commit?

I don't think we have an official policy for wasm-pack, but my rule of thumb is that if the follow up commits stand on their own (make sense outside the context of the PR, pass CI without relying on commits that come after, etc) then I leave them be, otherwise I squash them.

* Renamed set_crate_path to get_crate_path * If the path isn't specified in the call then use the current_dir and walk up the path try to find the manifest file. * Specify the cwd for the build test so it doesn't walk up the path. The test needs to specify the path to build, otherwise because fixtures are run in a subdirectory of the source tree it will find the Cargo.toml from the wasm-pack source repository.

ashleygwilliams · 2019-07-16T17:47:26Z

this is great! sorry for the delay and thanks for your patience. this will go out in 0.9.0!

ashleygwilliams added the needs review label Apr 12, 2019

ashleygwilliams added this to the 0.9.0 milestone Apr 12, 2019

ashleygwilliams added the changelog - feature label Apr 12, 2019

drager requested a review from fitzgen April 24, 2019 13:38

fitzgen reviewed Apr 24, 2019

View reviewed changes

fitzgen reviewed Apr 26, 2019

View reviewed changes

tests/all/manifest.rs Outdated Show resolved Hide resolved

gameldar force-pushed the master branch from 8f98637 to d44a104 Compare April 27, 2019 15:31

gameldar force-pushed the master branch from d44a104 to e880a33 Compare April 30, 2019 02:07

ashleygwilliams merged commit 127c72a into rustwasm:master Jul 16, 2019

ashleygwilliams removed the needs review label Jul 16, 2019

ashleygwilliams mentioned this pull request Jul 16, 2019

Walk up parent directories to find the current crate #620

Closed

EverlastingBugstopper mentioned this pull request Jan 7, 2020

Allow wrangler to be run in subdirectories cloudflare/wrangler-legacy#989

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Recurse up the path looking for the Cargo.toml #624

Recurse up the path looking for the Cargo.toml #624

gameldar commented Apr 12, 2019 •

edited

Loading

fitzgen left a comment

fitzgen Apr 24, 2019

gameldar Apr 25, 2019

fitzgen left a comment

fitzgen Apr 26, 2019

alexcrichton Apr 26, 2019

gameldar Apr 27, 2019

fitzgen Apr 26, 2019

gameldar Apr 27, 2019

fitzgen Apr 26, 2019

gameldar Apr 27, 2019

fitzgen commented Apr 26, 2019

fitzgen commented Apr 26, 2019

gameldar commented Apr 27, 2019

fitzgen commented Apr 29, 2019

ashleygwilliams commented Jul 16, 2019

Recurse up the path looking for the Cargo.toml #624

Recurse up the path looking for the Cargo.toml #624

Conversation

gameldar commented Apr 12, 2019 • edited Loading

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

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

fitzgen commented Apr 26, 2019

fitzgen commented Apr 26, 2019

gameldar commented Apr 27, 2019

fitzgen commented Apr 29, 2019

ashleygwilliams commented Jul 16, 2019

gameldar commented Apr 12, 2019 •

edited

Loading