Skip to content
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

OUT_DIR env var Behaviour discrepancy between testing a whole crate and individual test #13127

Closed
ShiqiHe000 opened this issue Dec 6, 2023 · 5 comments · Fixed by #13204
Closed
Assignees
Labels
A-build-scripts Area: build.rs scripts A-environment-variables Area: environment variables C-bug Category: bug S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review

Comments

@ShiqiHe000
Copy link

ShiqiHe000 commented Dec 6, 2023

Problem

Hi! I want to run the integration tests of a lib crate (my_crate_2) in my rust project. The crate has such structure:

my_project/
├── Cargo.toml
└── src/
    └── main.rs
my_crate_2/
├── Cargo.toml
|-- build.rs
└── src/
    └── lib.rs
|__ tests/ 
    |__my_crate_2.rs

In tests/my_crate_2.rs, I need to extract artifacts output path of the package, i.e., the OUT_DIR env var, which should be set by Cargo.

I observed a cargo test behaviour discrepancy on OUT_DIR env var between running tests against the whole package and a single/specific test. I created this sandbox to demo: here.

The test: test_add() will fail if OUT_DIR env var is not set:

#[cfg(test)]
pub mod tests {
    use my_crate_2::*;

    #[test]
    fn test_add() {
        // I need to use env var `OUT_DIR` here
        // err: called `Result::unwrap()` on an `Err` value: NotPresent
        // note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
        let out_dir = std::env::var("OUT_DIR").unwrap();
        println!("OUT_DIR is {}", out_dir);

        assert_eq!(add(2, 5), 7);
    }
}

If you run the whole test package (cargo test -- --nocapture) in my_crate_2/, the test can pass without a problem.

cargo test -- --nocapture
    Finished test [unoptimized + debuginfo] target(s) in 0.00s
     Running unittests src/lib.rs (target/debug/deps/my_crate_2-e7e58afb09a93b31)

running 1 test
test tests::it_works ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running tests/my_crate_2.rs (target/debug/deps/my_crate_2-9892d21b51d30121)

running 1 test
OUT_DIR is /workspaces/workspace/my_crate_2/target/debug/build/my_crate_2-443e542c04636da9/out
test tests::test_add ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

   Doc-tests my_crate_2

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

But if I only run a specific test: cargo test --package my_crate_2 --test my_crate_2 -- tests::test_add --nocapture, the test would fail with OUT_DIR env var not found.

cargo test --package my_crate_2 --test my_crate_2 -- tests::test_add --nocapture
    Finished test [unoptimized + debuginfo] target(s) in 0.00s
     Running tests/my_crate_2.rs (target/debug/deps/my_crate_2-9892d21b51d30121)

running 1 test
thread 'tests::test_add' panicked at tests/my_crate_2.rs:10:48:
called `Result::unwrap()` on an `Err` value: NotPresent
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
test tests::test_add ... FAILED

Looks like cargo test --package <package_name> -- <test_name> lose the OUT_DIR env var? Thank you for your help.

Steps

Provided in the description

Possible Solution(s)

No response

Notes

No response

Version

cargo 1.71.1 (7f1d04c00 2023-07-29)
release: 1.71.1
commit-hash: 7f1d04c0053083b98fa50b69b6f56e339b0556a8
commit-date: 2023-07-29
host: x86_64-unknown-linux-gnu
libgit2: 1.6.4 (sys:0.17.1 vendored)
libcurl: 8.0.1-DEV (sys:0.4.61+curl-8.0.1 vendored ssl:OpenSSL/1.1.1t)
ssl: OpenSSL 1.1.1t  7 Feb 2023
os: Ubuntu 20.04 (focal) [64-bit]
@ShiqiHe000 ShiqiHe000 added C-bug Category: bug S-triage Status: This issue is waiting on initial triage. labels Dec 6, 2023
@weihanglo weihanglo added A-build-scripts Area: build.rs scripts A-environment-variables Area: environment variables S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. and removed S-triage Status: This issue is waiting on initial triage. labels Dec 6, 2023
@epage
Copy link
Contributor

epage commented Dec 6, 2023

We limit what build targets get processed when specifying a test as an optimization, see https://github.com/rust-lang/cargo/blob/master/src/bin/cargo/commands/test.rs#L100

I'm guessing we don't set OUT_DIR for tests but we have leakage between targets, making it available.

By looking at the code, I'm guessing cargo test --tests will also reproduce this.

The question is which behavior is more correct.

@weihanglo
Copy link
Member

From https://doc.rust-lang.org/nightly/cargo/reference/environment-variables.html#environment-variables-cargo-sets-for-crates:

Note that this applies for running binaries with cargo run and cargo test as well.

My interpretation of "running binaries" are the binary targets (i.e. [[bin]]) being run in a integration tests, though it is not necessary to be the case.

@epage
Copy link
Contributor

epage commented Dec 7, 2023

Hadn't had a chance to check the docs yet. The only way to set it for running the bins is if we set it for running the tests. Looks like we aren't lining up with the documentation.

@ehuss
Copy link
Contributor

ehuss commented Dec 8, 2023

I believe this was an unintentional regression in 1.15 via #3310.

The code has moved around a bit, but it now looks like this:

// If the unit has a build script, add `OUT_DIR` to the
// environment variables.
if unit.target.is_lib() {
for dep in &self.bcx.unit_graph[unit] {
if dep.unit.mode.is_run_custom_build() {
let out_dir = self
.files()
.build_script_out_dir(&dep.unit)
.display()
.to_string();
let script_meta = self.get_run_build_script_metadata(&dep.unit);
self.compilation
.extra_env
.entry(script_meta)
.or_insert_with(Vec::new)
.push(("OUT_DIR".to_string(), out_dir));
}
}
}

The problem is that it is only including OUT_DIR in extra_env if the lib target is one of the roots. That's...not how it should work. It's tricky to explain, but Context::compile should do something like:

  • Gather a list of root units that have a build script.
  • Deduplicate that list based on unit.pkg
  • For each remaining unit in the deduplicated list, set the extra_env the same way the current code does (essentially everything inside the if unit.target.is_lib() block).

I can try to explain it further if that isn't clear.

@ehuss ehuss added S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review and removed S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. labels Dec 8, 2023
@Rustin170506
Copy link
Member

@rustbot claim

I will try to fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-build-scripts Area: build.rs scripts A-environment-variables Area: environment variables C-bug Category: bug S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants