Add CARGO_WORKSPACE_DIR env var for integration tests and benchmarks #12158

yerke · 2023-05-19T04:06:12Z

What does this PR try to resolve?

This is step towards #3946

Add CARGO_WORKSPACE_DIR env var for integration tests and benchmarks

How should we test and review this PR?

Added tests for projects with and without workspace.

Additional information

Big thank you to @epage for helping with this PR.

rustbot · 2023-05-19T04:06:17Z

r? @epage

(rustbot has picked a reviewer for you, use r? to override)

weihanglo

Thanks for the pull request! Looks generally reasonable. Some suggestions:

A variable with a relative path is not usual. If you could, I'd like to see the rationale of being relative in the PR description and/or inline in code.
Can you add docmentation for it? Should start from one line below this.

src/cargo/core/compiler/mod.rs

weihanglo · 2023-05-19T09:16:39Z

tests/testsuite/build.rs

@@ -1563,6 +1578,208 @@ fn crate_env_vars() {
    }
 }

+#[cargo_test]
+fn crate_env_vars_with_workspace() {


Thank you for setting up this new test case. Given it is largely the same as crate_env_vars_without_workspace, should we test the part we need (CARGO_WORKSPACE_DIR) instead of duplicating it?

Or alternatively we could find a way to avoid the copy.

epage · 2023-05-19T13:36:35Z

A variable with a relative path is not usual. If you could, I'd like to see the rationale of being relative in the PR description and/or inline in code.

@yerke the rationale to put in is "Use a relative path and limits to tests in hopes that it conveys to the user that the meaning of this value is a bit fuzzy (very different meaning in the original repo vs once published)"

epage · 2023-05-19T13:42:11Z

src/cargo/core/compiler/mod.rs

+        let cargo_workspace_dir_rel_path =
+            pathdiff::diff_paths(cargo_workspace_dir, cargo_manifest_dir)
+                .expect("both paths are absolute");


Can you add a test where this is being used in a published package?

How that would work

Publish a package with a test using the variable

Create a package that depends on the published package

cargo test -p <published-package-name> (since cargo lets you test dependencies)

For the published package, CARGO_WORKSPACE_DIR should be the same as CARGO_MANIFEST_DIR.

From #3946 (comment):

There was the question of whether this is for the "workspace" of what is being built now or the final artifact. We felt that it would best be associated with what is being built right now, either exposing macros for dependents to use or have them pass paths down into what is being compiled. That has clearer semantics and doesn't run into inverted dependencies where the crate knows what it is being compiled fir. This means that for the registry crates, the path would end up being the same as CARGO_MANIFEST_DIR

weihanglo · 2023-05-26T09:48:44Z

@rustbot author

yerke · 2023-05-30T02:00:25Z

@rustbot ready

yerke · 2023-05-30T02:31:25Z

tests/testsuite/build.rs

+            #[test]
+            fn env() {
+                assert!(Path::new(option_env!("CARGO_WORKSPACE_DIR").unwrap()).join(file!()).exists());
+                // assert_eq!(std::fs::canonicalize(option_env!("CARGO_WORKSPACE_DIR").unwrap()).unwrap().display().to_string(), option_env!("CARGO_MANIFEST_DIR").unwrap());


@epage you suggested that for published crates, we should get the same directory as CARGO_MANIFEST_DIR. This is not the case here.

thread 'env' panicked at 'assertion failed: `(left == right)` left: `"/Users/yerke/dev/rust/cargo/target/tmp/cit/t0/foo"`, right: `"/Users/yerke/dev/rust/cargo/target/tmp/cit/t0/home/.cargo/registry/src/-c29b6f5e91792042/bar-0.1.0"`', /Users/yerke/dev/rust/cargo/target/tmp/cit/t0/home/.cargo/registry/src/-c29b6f5e91792042/bar-0.1.0/tests/env.rs:7:17

CARGO_MANIFEST_DIR from inside the published crate's test still seems to point to the published crate's manifest directory, whereas CARGO_WORKSPACE_DIR does point to the our current package's workspace directory.

So I guess for a published crate CARGO_WORKSPACE_DIR works as you expected, but CARGO_MANIFEST_DIR doesn't?

I think I might be misunderstanding something.

CARGO_WORKSPACE_DIR should be pointing to the CARGO_MANIFEST_DIR for what is being built. We should not be exposing details of the dependent. I was concerned that this was not being followed and was trying to point out that this was a potential failure case of the current implementation,.

Got it. Yes, I did misunderstand what you meant by it.

I updated the PR and it should now behave how you envisioned it (I think).

src/doc/src/reference/environment-variables.md

epage · 2023-05-30T16:19:11Z

tests/testsuite/build.rs

+                    // Check that CARGO_WORKSPACE_DIR isn't set for unit tests
+                    assert!(option_env!("CARGO_WORKSPACE_DIR").is_none());
+                    env::var("CARGO_WORKSPACE_DIR").unwrap_err();


Should we be making unit tests work?

Do you also want to update the behavior of CARGO_BIN_EXE_{name} and CARGO_TARGET_TMPDIR? They currently also only work with integration tests and benchmarks. Otherwise I think the behavior of those env vars would be inconsistent.

If we decide to do it, perhaps we should do it in a separate PR?

I'd recommend leaving this as an open question (like the name) for us to hash out during stabilization. If you haven't updated for unit tests, maybe we leave it as-is for now?

I said that but...

It looks like CARGO_BIN_EXE_ and CARGO_TARGET_TMPDIR are for integration tests because, in theory, unit tests wouldn't be needing to do operations like this. However, it perfectly makes sense to have basic snapshots in unit tests, so I would lean towards supporting this in unit tests. I'd also be in favor of re-evaluating CARGO_TARGET_TMPDIR so it can be used in unit tests but thats another topic.

epage · 2023-05-30T18:29:16Z

src/cargo/core/compiler/mod.rs

+            // Use a relative path and limit to integration tests and benchmarks in hopes
+            // that it conveys to the user that the meaning of this value is a bit fuzzy
+            // (very different meaning in the original repo vs once published).
+            let cargo_workspace_dir = if is_primary && is_workspace {


Does is_primary work?

This environment variable will be set if the package being built is primary. Primary packages are the ones the user selected on the command-line, either with -p flags or the defaults based on the current directory and the default workspace members. This environment variable will not be set when building dependencies. This is only set when compiling the package (not when running binaries or tests).

I can run cargo test -p <transitve-dep>. This would make it primary but not something we should give my workspace dir to

I'm unsure if there are cases for local use where something isn't primary but is part of my workspace. I'm guessing not since this is for top-level tests but if we relied on this, its indirect and we'd want to document it.

yerke · 2023-06-15T23:11:43Z

tests/testsuite/build.rs

+                dbg!(std::env::current_dir().unwrap());
+                dbg!(option_env!("CARGO_MANIFEST_DIR").unwrap());
+                assert!(Path::new(option_env!("CARGO_WORKSPACE_DIR").unwrap()).join(file!()).exists());
+                // thread 'env' panicked at 'assertion failed: `(left == right)`


This test fails. I added debug info above. Essentially, I would expect CARGO_MANIFEST_DIR to be /Users/yerke/dev/rust/cargo/target/tmp/cit/t0/foo/baz, but it's /Users/yerke/dev/rust/cargo/target/tmp/cit/t0/foo instead.
So in a situation where a member of workspace depends on path dependency crate, which is in another workspace and we test path dependency crate, we get the first workspace instead of the second one.

Just to avoid any dusty corners of cargo, try moving baz to /Users/yerke/dev/rust/cargo/target/tmp/cit/t0/foo/baz. I think the way to do this is with project().at("baz').

My concern is that without baz_member having package.workspace or workspace, cargo is not seeing baz/Cargo.toml to realize baz_member is within a workspace. Since baz_member is underneath foo, it may automatically become a member. By splitting it out, this breaks the auto-workspace behavior. You can go a step further by also setting package.workspace.

If that still doesn't work, have we verified that we are getting anything but the top-level workspace to work with?

yerke · 2023-06-15T23:14:48Z

tests/testsuite/build.rs

+
+    println!("test");
+    // fails because it's running tests in baz/baz_member/src/lib.rs
+    // p.cargo("test -v").masquerade_as_nightly_cargo(&[]).run();


Test failure is described above.

yerke · 2023-06-15T23:15:04Z

tests/testsuite/build.rs

+
+    // path dependency crate with workspace
+    // this test fails
+    // p.cargo("test -p baz_member")


Test failure is described above.

yerke · 2023-06-15T23:16:42Z

tests/testsuite/build.rs

+                dbg!(file!());
+                dbg!(std::env::current_dir().unwrap());
+                dbg!(option_env!("CARGO_MANIFEST_DIR").unwrap());
+                assert!(Path::new(option_env!("CARGO_WORKSPACE_DIR").unwrap()).join(file!()).exists());


This test fails. I added debug info above. In short, we are not recognizing that path dependency is a part of its own workspace.

yerke · 2023-06-15T23:17:05Z

tests/testsuite/build.rs

+    p.cargo("bench -v").masquerade_as_nightly_cargo(&[]).run();
+
+    // This test fails
+    // p.cargo("test -p baz_member")


Test failure is described above.

yerke · 2023-06-15T23:18:56Z

@epage When you have time, do you mind taking a look at the 2nd commit? The current implementation doesn't work correctly with path dependency that is part of its own workspace. Thanks.

weihanglo · 2023-08-10T18:10:50Z

@yerke me again. Are you still interested in this? I am happy to have a look and iron out those issue together :)

weihanglo · 2023-09-18T01:10:57Z

@rustbot author

Ygg01 · 2023-09-28T11:58:52Z

@weihanglo I'll take a look, but I haven't worked on Cargo in years.

This is an alternative to rust-lang#12158's `CARGO_WORKSPACE_DIR` that was implementing the solution to rust-lang#3946 that previously discussed in the cargo team meeting. `CARGO_WORKSPACE_DIR` is a bit awkward to document / describe because its the effective workspace directory of the thing being built. If the thing being built doesn't have a workspace, it falls back to `CARGO_MANIFEST_DIR`. It would also be hard to take into account what the `CARGO_WORKSPACE_DIR` would be for path dependencies into foreign workspaces *and* it wouldn't solve the problem the user is having. What the user really wants is the CWD of rustc when it is invoked. This is much simpler to describe and is accurate when using a path dependency to a foreign package. Because the CWD is a much simpler mechanism to talk about, I figured we could diverge from our prior consensus and make it always present, rather than limiting it to tests. Remaining work for rust-lang#3946: get this stabilized

feat: Add `CARGO_RUSTC_CURRENT_DIR` (unstable) ### What does this PR try to resolve? This is an alternative to #12158's `CARGO_WORKSPACE_DIR` that was implementing the solution to #3946 that previously discussed in the cargo team meeting. `CARGO_WORKSPACE_DIR` is a bit awkward to document / describe because its the effective workspace directory of the thing being built. If the thing being built doesn't have a workspace, it falls back to `CARGO_MANIFEST_DIR`. It would also be hard to take into account what the `CARGO_WORKSPACE_DIR` would be for path dependencies into foreign workspaces *and* it wouldn't solve the problem the user is having. What the user really wants is the CWD of rustc when it is invoked. This is much simpler to describe and is accurate when using a path dependency to a foreign package. Because the CWD is a much simpler mechanism to talk about, I figured we could diverge from our prior consensus and make it always present, rather than limiting it to tests. ### How should we test and review this PR? The preparatory refactor commits have explanation for why they were to help ### Additional information Remaining work for #3946: get this stabilized

bors · 2023-11-24T16:55:27Z

☔ The latest upstream changes (presumably #12996) made this pull request unmergeable. Please resolve the merge conflicts.

weihanglo · 2023-11-24T16:58:24Z

Thank you for the effort. Close in favor of #12996, and looking forward to feedback!

rustbot assigned epage May 19, 2023

rustbot added A-build-execution Area: anything dealing with executing the compiler S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 19, 2023

weihanglo reviewed May 19, 2023

View reviewed changes

epage reviewed May 19, 2023

View reviewed changes

rustbot added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 26, 2023

yerke force-pushed the yerke/cargo_manifest_dir branch from 512d2f5 to c3122ce Compare May 30, 2023 01:58

rustbot added the A-documenting-cargo-itself Area: Cargo's documentation label May 30, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. labels May 30, 2023

yerke force-pushed the yerke/cargo_manifest_dir branch from c3122ce to bebbe6b Compare May 30, 2023 02:18

yerke commented May 30, 2023

View reviewed changes

yerke force-pushed the yerke/cargo_manifest_dir branch from bebbe6b to 645e323 Compare May 30, 2023 02:33

epage reviewed May 30, 2023

View reviewed changes

src/doc/src/reference/environment-variables.md Outdated Show resolved Hide resolved

epage reviewed May 30, 2023

View reviewed changes

yerke force-pushed the yerke/cargo_manifest_dir branch 2 times, most recently from 5f7a056 to 3699f8c Compare May 30, 2023 18:10

epage reviewed May 30, 2023

View reviewed changes

yerke commented Jun 15, 2023

View reviewed changes

rustbot added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 18, 2023

weihanglo mentioned this pull request Sep 18, 2023

Environment variable for Cargo Workspace #3946

Open

yerke added 2 commits November 16, 2023 13:05

add CARGO_WORKSPACE_DIR env var for integration tests and benchmarks

00c4697

WIP: add test cases with published crates and path deps

fb88580

epage force-pushed the yerke/cargo_manifest_dir branch from e166a51 to fb88580 Compare November 16, 2023 19:21

epage mentioned this pull request Nov 17, 2023

feat: Add CARGO_RUSTC_CURRENT_DIR (unstable) #12996

Merged

weihanglo closed this Nov 24, 2023

epage mentioned this pull request Mar 1, 2024

Need a reliable way to get the target dir from the build script #9661

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CARGO_WORKSPACE_DIR env var for integration tests and benchmarks #12158

Add CARGO_WORKSPACE_DIR env var for integration tests and benchmarks #12158

yerke commented May 19, 2023 •

edited by epage

Loading

rustbot commented May 19, 2023

weihanglo left a comment

weihanglo May 19, 2023

epage commented May 19, 2023

epage May 19, 2023

weihanglo commented May 26, 2023

yerke commented May 30, 2023

yerke May 30, 2023 •

edited

Loading

epage May 30, 2023

yerke May 30, 2023

yerke May 30, 2023

epage May 30, 2023

yerke May 30, 2023

yerke May 30, 2023

epage May 30, 2023

epage May 30, 2023

epage May 30, 2023

yerke Jun 15, 2023

epage Jun 19, 2023

yerke Jun 15, 2023

yerke Jun 15, 2023

yerke Jun 15, 2023

yerke Jun 15, 2023

yerke commented Jun 15, 2023 •

edited

Loading

weihanglo commented Aug 10, 2023

weihanglo commented Sep 18, 2023

Ygg01 commented Sep 28, 2023

bors commented Nov 24, 2023

weihanglo commented Nov 24, 2023

Add CARGO_WORKSPACE_DIR env var for integration tests and benchmarks #12158

Add CARGO_WORKSPACE_DIR env var for integration tests and benchmarks #12158

Conversation

yerke commented May 19, 2023 • edited by epage Loading

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rustbot commented May 19, 2023

weihanglo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epage commented May 19, 2023

Choose a reason for hiding this comment

weihanglo commented May 26, 2023

yerke commented May 30, 2023

yerke May 30, 2023 • edited Loading

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

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

yerke commented Jun 15, 2023 • edited Loading

weihanglo commented Aug 10, 2023

weihanglo commented Sep 18, 2023

Ygg01 commented Sep 28, 2023

bors commented Nov 24, 2023

weihanglo commented Nov 24, 2023

yerke commented May 19, 2023 •

edited by epage

Loading

yerke May 30, 2023 •

edited

Loading

yerke commented Jun 15, 2023 •

edited

Loading