Use package root as `cargo check` working directory #8819

bcully · 2021-05-13T02:55:20Z

Cargo commands are affected by the .cargo/config files above
their working directory. If cargo is invoked from above the directory
holding Cargo.toml, it may not pick up important settings like
registry replacements, causing it to behave differently or even fail.

Most cargo invocations are currently setting their working directories
to the directory containing Cargo.toml, but a couple of paths remain
in which cargo is invoked from the default workspace root instead.

This change fixes that, resolving some cargo check failures that I
experienced in a multi-root workspace in which packages used different
registries.

Cargo commands are affected by the `.cargo/config` files above their working directory. If cargo is invoked from above the directory holding `Cargo.toml`, it may not pick up important settings like registry replacements, causing it to behave differently or even fail. Most cargo invocations are currently setting their working directories to the directory containing `Cargo.toml`, but a couple of paths remain in which cargo is invoked from the default workspace root instead. This change fixes that, resolving some cargo check failures that I experienced in a multi-root workspace in which packages used different registries.

lf-

OOOH YES! I like this change! I too have a cursed workspace, in my case, it's because the package wide cargo check is plain wrong due to the package specific cargo configs and targets. Thanks for doing it.

matklad · 2021-05-13T05:37:18Z

Great idea! Let’s remove the —manifest-path argument then!

bors d+

bcully · 2021-05-13T15:38:54Z

Great idea! Let’s remove the —manifest-path argument then!

Will do.

matklad · 2021-05-13T18:18:44Z

bors r+

And this also is a negative diff, splendid!

8819: Use package root as `cargo check` working directory r=matklad a=bcully Cargo commands are affected by the `.cargo/config` files above their working directory. If cargo is invoked from above the directory holding `Cargo.toml`, it may not pick up important settings like registry replacements, causing it to behave differently or even fail. Most cargo invocations are currently setting their working directories to the directory containing `Cargo.toml`, but a couple of paths remain in which cargo is invoked from the default workspace root instead. This change fixes that, resolving some cargo check failures that I experienced in a multi-root workspace in which packages used different registries. Co-authored-by: Brendan Cully <brendan@cully.org>

bors · 2021-05-13T18:23:33Z

Build failed:

Rust (macos-latest)

bcully · 2021-05-13T18:36:51Z

I'm not sure what to make of the test failure. It looks like it's just cargo test on a mac, and that works on mine.

flodiebold · 2021-05-13T18:55:30Z

Try RUN_SLOW_TESTS=true cargo test.

bcully · 2021-05-13T18:56:09Z

I'm not sure what to make of the test failure. It looks like it's just cargo test on a mac, and that works on mine.

TestDir::new looks racy to me, seems like there might be a TOCTOU between the is_dir check and create_dir_all. One simple fix might be to use create_dir_all on the parent directory, and then attempt create_dir on the directory itself, continuing if it fails because the directory already exists.

bcully · 2021-05-13T18:59:02Z

Try RUN_SLOW_TESTS=true cargo test.

Ah, this does reproduce 10 of the 11 failures on my mac. I'll take a closer look.

bcully · 2021-05-13T20:12:44Z

Seems like some kind of path canonicalization issue. Here's a diff on mac of the metadata command in the test suite, comparing the call without an explicit --manifest-path to one with it:

--- noman	2021-05-13 13:06:02.000000000 -0700
+++ man	2021-05-13 13:06:08.000000000 -0700
@@ -3,7 +3,7 @@
     {
       "name": "foo",
       "version": "0.0.0",
-      "id": "foo 0.0.0 (path+file:///private/var/folders/dg/nkftf0j15hq7g2c4c_jvtfddnyp9v9/T/testdir/81433_0)",
+      "id": "foo 0.0.0 (path+file:///var/folders/dg/nkftf0j15hq7g2c4c_jvtfddnyp9v9/T/testdir/81433_0)",
       "license": null,
       "license_file": null,
       "description": null,
@@ -18,7 +18,7 @@
             "lib"
           ],
           "name": "foo",
-          "src_path": "/private/var/folders/dg/nkftf0j15hq7g2c4c_jvtfddnyp9v9/T/testdir/81433_0/src/lib.rs",
+          "src_path": "/var/folders/dg/nkftf0j15hq7g2c4c_jvtfddnyp9v9/T/testdir/81433_0/src/lib.rs",
           "edition": "2015",
           "doc": true,
           "doctest": true,
@@ -26,7 +26,7 @@
         }
       ],
       "features": {},
-      "manifest_path": "/private/var/folders/dg/nkftf0j15hq7g2c4c_jvtfddnyp9v9/T/testdir/81433_0/Cargo.toml",
+      "manifest_path": "/var/folders/dg/nkftf0j15hq7g2c4c_jvtfddnyp9v9/T/testdir/81433_0/Cargo.toml",
       "metadata": null,
       "publish": null,
       "authors": [],
@@ -41,21 +41,21 @@
     }
   ],
   "workspace_members": [
-    "foo 0.0.0 (path+file:///private/var/folders/dg/nkftf0j15hq7g2c4c_jvtfddnyp9v9/T/testdir/81433_0)"
+    "foo 0.0.0 (path+file:///var/folders/dg/nkftf0j15hq7g2c4c_jvtfddnyp9v9/T/testdir/81433_0)"
   ],
   "resolve": {
     "nodes": [
       {
-        "id": "foo 0.0.0 (path+file:///private/var/folders/dg/nkftf0j15hq7g2c4c_jvtfddnyp9v9/T/testdir/81433_0)",
+        "id": "foo 0.0.0 (path+file:///var/folders/dg/nkftf0j15hq7g2c4c_jvtfddnyp9v9/T/testdir/81433_0)",
         "dependencies": [],
         "deps": [],
         "features": []
       }
     ],
-    "root": "foo 0.0.0 (path+file:///private/var/folders/dg/nkftf0j15hq7g2c4c_jvtfddnyp9v9/T/testdir/81433_0)"
+    "root": "foo 0.0.0 (path+file:///var/folders/dg/nkftf0j15hq7g2c4c_jvtfddnyp9v9/T/testdir/81433_0)"
   },
-  "target_directory": "/private/var/folders/dg/nkftf0j15hq7g2c4c_jvtfddnyp9v9/T/testdir/81433_0/target",
+  "target_directory": "/var/folders/dg/nkftf0j15hq7g2c4c_jvtfddnyp9v9/T/testdir/81433_0/target",
   "version": 1,
-  "workspace_root": "/private/var/folders/dg/nkftf0j15hq7g2c4c_jvtfddnyp9v9/T/testdir/81433_0",
+  "workspace_root": "/var/folders/dg/nkftf0j15hq7g2c4c_jvtfddnyp9v9/T/testdir/81433_0",
   "metadata": null
 }

When given a --manifest-path, the returned values match it. When --manifest-path is omitted, it seems cargo canonicalizes the path more fully than rust-analyzer does, which confuses it later.

I'm inclined to just restore --manifest-path to the metadata command, and debug the difference in canonicalization separately.

bcully · 2021-05-13T20:17:46Z

Actually, I'd like to just drop the second commit. Two tests still fail when only metadata gets --manifest-path, and I'm assuming it's the same issue. I think we can work on the difference in path canonicalization separately.

bcully · 2021-05-13T20:46:35Z

As an added data point, this also gets the test suite to pass:

diff --git a/crates/rust-analyzer/tests/rust-analyzer/testdir.rs b/crates/rust-analyzer/tests/rust-analyzer/testdir.rs
index 36271344b..ec48d4363 100644
--- a/crates/rust-analyzer/tests/rust-analyzer/testdir.rs
+++ b/crates/rust-analyzer/tests/rust-analyzer/testdir.rs
@@ -11,7 +11,7 @@ pub(crate) struct TestDir {

 impl TestDir {
     pub(crate) fn new() -> TestDir {
-        let base = std::env::temp_dir().join("testdir");
+        let base = std::env::temp_dir().canonicalize().unwrap().join("testdir");
         let pid = std::process::id();

         static CNT: AtomicUsize = AtomicUsize::new(0);

but I doubt that is the correct fix, real users might also be using symlinks in their package directories. We should probably be doing the same normalization cargo is doing when we create the workspace roots.

bcully · 2021-05-13T21:08:14Z

Alternatively perhaps we should update our config with the manifest_path and workspace_root that cargo metadata returns.

matklad · 2021-05-13T21:33:24Z

I think canonicalization tempdir is fine

bcully · 2021-05-13T21:51:51Z

I think canonicalization tempdir is fine

I guess something else is already canonicalizing the workspace root before giving it to the server, outside test code? I couldn't seem to trip up rust-analyzer on the command line:

❯ target/release/rust-analyzer diagnostics $HOME/symlink-to-dev/rust-analyzer/
processing crate: vfs, module: /Users/brendan/dev/rust-analyzer/crates/vfs/src/lib.rs
...

I'll canonicalize tempdir. It seems like the test code ought to pass through the same canonicalization that the real code uses though.

bcully · 2021-05-14T05:16:38Z

... actually, I don't think we should just canonicalize tempdir. I installed this on a linux server where my homedir was a symlink, and it broke. I'm going to roll back these last two commits again and work on the path canonicalization issue separately.

matklad · 2021-05-14T17:15:07Z

bors r+

bors · 2021-05-14T17:23:45Z

Build succeeded:

lf- approved these changes May 13, 2021

View reviewed changes

bors bot merged commit e7b8e6f into rust-lang:master May 14, 2021

Use package root as cargo check working directory #8819

Use package root as cargo check working directory #8819

Uh oh!

Conversation

bcully commented May 13, 2021

Uh oh!

lf- left a comment

Choose a reason for hiding this comment

Uh oh!

matklad commented May 13, 2021

Uh oh!

bcully commented May 13, 2021

Uh oh!

matklad commented May 13, 2021

Uh oh!

bors bot commented May 13, 2021

Uh oh!

bcully commented May 13, 2021

Uh oh!

flodiebold commented May 13, 2021

Uh oh!

bcully commented May 13, 2021

Uh oh!

bcully commented May 13, 2021

Uh oh!

bcully commented May 13, 2021

Uh oh!

bcully commented May 13, 2021

Uh oh!

bcully commented May 13, 2021

Uh oh!

bcully commented May 13, 2021

Uh oh!

matklad commented May 13, 2021

Uh oh!

bcully commented May 13, 2021

Uh oh!

bcully commented May 14, 2021

Uh oh!

matklad commented May 14, 2021

Uh oh!

bors bot commented May 14, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Use package root as `cargo check` working directory #8819

Use package root as `cargo check` working directory #8819