Add CARGO_TARGET_TMPDIR env var for integration tests & benches #9375

vojtechkral · 2021-04-18T22:05:18Z

Hi.
Recently I ran into the problem that integration tests don't have a good way to figure out where target dir is.

Knowing where target is is useful for integration tests that need to setup some filesystem structure for test cases. In fact cargo itself does this too (and figures out the path rather clumsily).

Another popular way of doing this is to create a directory in /tmp (or quivalent on other systems), however, I believe using subdirectory in target is better as testcases are easier to debug that way and temporary locations aren't polluted.

I think this would also address some concerns in #2841

Another solution might be to provide a dedicated subdirectory in target for this, something like target/scratchpad, but I'm not convinced this is warranted... Edit: That's what was decided to do, see below...

Let me know what you think 🙂

rust-highfive · 2021-04-18T22:05:20Z

r? @alexcrichton

(rust-highfive has picked a reviewer for you, use r? to override)

alexcrichton · 2021-04-20T15:13:15Z

This seems reasonable to me! We've been hesitant to set this for things like build scripts in the past but integration tests are much more "top level" and feels more appropriate to set this information for them.

@rfcbot fcp merge

rfcbot · 2021-04-20T15:13:17Z

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

~~concerns~~ resolved by Add CARGO_TARGET_TMPDIR env var for integration tests & benches #9375 (comment)

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

ehuss · 2021-04-21T16:42:37Z

@rfcbot concern concerns

I'm thinking it may be better to set this at compiletime instead of runtime. That would make it behave more similarly to CARGO_BIN_EXE_. The concern about making it runtime is if you want to run a debugger against the executable, then it requires setting up environment variables which can be a pain. Does that make sense?
I'm mildly concerned about forward compatibility with name collisions. Let's say some project decides to create a target/build directory, and then some future cargo version decides to add a build profile, there will be a collision. I'm not sure if my concern is strong enough to want to do something different, but it might make it riskier to make any changes.
The documentation says this is set for benchmarks, but that does not appear to be the case?

vojtechkral · 2021-04-26T21:56:33Z

@ehuss

I'm thinking it may be better to set this at compiletime instead of runtime. That would make it behave more similarly to CARGO_BIN_EXE_. The concern about making it runtime is if you want to run a debugger against the executable, then it requires setting up environment variables which can be a pain. Does that make sense?

Yeah, I suppose. But would that be ok for cargo itself too?
I'm not sure how to do this wrt. test suite backcompat, ie. running those tests with an older cargo. Or is it ok to require the current cargo to run its tests?

I'm mildly concerned about forward compatibility with name collisions. Let's say some project decides to create a target/build directory, and then some future cargo version decides to add a build profile, there will be a collision. I'm not sure if my concern is strong enough to want to do something different, but it might make it riskier to make any changes.

Yeah, this is what I mean with the sratchpad remark in the opening post. I don't have a strong feeling one way or another. But I guess providing a subdir name cargo knows is not used for something else could work.

The documentation says this is set for benchmarks, but that does not appear to be the case?

Oh, that should be fixed then, I'll look into it tomorrow...

ehuss · 2021-04-27T20:40:54Z

Yeah, I suppose. But would that be ok for cargo itself too?

Sorry, I'm not sure I full understand the question here. Is this in reference to the GLOBAL_ROOT? If so, I think that would just change to option_env!. It looks like it is already backwards compatible, it would just hit the else clause.

vojtechkral · 2021-04-29T20:13:02Z

@ehuss I'm having a hard time making the variable compile time. Setting it for compilation of integration tests/benches means it's really only available in the test/bench binaries themselves, but for example the cargo-test-support (where the path resolution is implemented) can't see the variable that way, as that's normal library code.
I'd have to set it for all code / regular code as well and I'm not sure if that's acceptable.
What do you think?

ehuss · 2021-05-01T15:56:25Z

Hm, that is unfortunate. I guess we could keep the runtime behavior of the current PR.

Let me know if you need any help with the benchmarks.

vojtechkral · 2021-05-01T16:32:39Z

@ehuss Ok. No that's ok, it's easy enough to fix, I was just dealing with other stuff... I'll update the PR shortly...

vojtechkral · 2021-05-01T21:10:51Z

Updated

ehuss · 2021-05-04T20:56:04Z

I had a chat with the rest of the team members, and we reconsidered the two issues that were brought up here, and we think we would prefer the following:

Use a more constrained directory. There are concerns that letting free-form access to the target directory would make it more difficult to change things later. By giving a more targeted option (specifically for, here's a scratch pad to place things for tests), it gives a little higher-level indication of what it is being used for. We were thinking just naming it tmp in the target/debug or target/release directory, and use an environment variable name to match. Maybe something like CARGO_TARGET_TMPDIR? Not sure if others have opinions on the exact name.
Make it a build-time environment variable. There are concerns about making it harder to work with tests (such as debugging, code coverage, launching via directly, etc.). If the test author really wants to make it a runtime thing, they can write something like set_var("CARGO_TARGET_TMPDIR", env!("CARGO_TARGET_TMPDIR")). For Cargo's own testsuite, which uses a separate dependency, we're fine with not necessarily changing it right away. An option for that scenario is to change the cargo_test attribute to relay the env!("CARGO_TARGET_TMPDIR") to the appropriate initialization. (Not necessary for this PR, though.)

Would that work for you? It seemed from your description that you were primarily interested in a scratch directory for convenience in a test.

If so, let me know if you are OK with that, and if you need or want any help.

vojtechkral · 2021-05-04T21:19:06Z

@ehuss Glad you had a chat & came to a decision on this, wouldn't want to rush things. And yeah, that sounds reasonable.

I'll update the code to match. Probably don't need help, will ask if anything comes up 🙂 Thanks!

The variable is set to $target_dir/$config/tmp This is a directory where tests & benches can place testcasei-related data for use by the tests. cargo makes sure the directory exists when building tests/benches.

vojtechkral · 2021-05-06T23:03:22Z

Ok, should be ready for the next review round I think...

ehuss · 2021-05-07T19:33:37Z

Oh, nice, thanks!

@rfcbot resolve concerns

ehuss · 2021-05-07T19:38:06Z

@rust-lang/cargo I have restarted the FCP since this changed a bit, and I wanted to give you a chance to re-review. Specifically:

This is now CARGO_TARGET_TMPDIR, which is a path to target/$PROFILE/tmp, set only when compiling integration tests or benchmarks, and thus requires env!("CARGO_TARGET_TMPDIR").

Please re-check your boxes up at #9375 (comment) unless you have any concerns (such as the particular variable name, which we didn't really discuss).

rfcbot · 2021-05-07T19:56:59Z

🔔 This is now entering its final comment period, as per the review above. 🔔

ehuss · 2021-05-07T21:29:47Z

@bors r+

bors · 2021-05-07T21:29:48Z

📌 Commit 53343bc has been approved by ehuss

bors · 2021-05-07T21:29:55Z

⌛ Testing commit 53343bc with merge e51522a...

bors · 2021-05-07T21:58:03Z

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing e51522a to master...

Update cargo 7 commits in f3e13226d6d17a2bc5f325303494b43a45f53b7f..e51522ab3db23b0d8f1de54eb1f0113924896331 2021-04-30 21:50:27 +0000 to 2021-05-07 21:29:52 +0000 - Add CARGO_TARGET_TMPDIR env var for integration tests & benches (rust-lang/cargo#9375) - Bump to 0.55.0, update changelog (rust-lang/cargo#9464) - Some updates to the unstable documentation (rust-lang/cargo#9457) - Add CARGO_PROFILE_<name>_SPLIT_DEBUGINFO to env docs. (rust-lang/cargo#9456) - Add `report` subcommand. (rust-lang/cargo#9438) - Respect Cargo.toml `[package.exclude]` even not in a git repo. (rust-lang/cargo#9186) - Document the other crates in the codebase in the contrib guide. (rust-lang/cargo#9439)

Pkgsrc changes: * Bump bootstrap requirements to 1.53.0. * Adjust patches, adapt to upstream changes, adjust cargo checksums * If using an external llvm, require >= 10.0 Upsteream changes: Version 1.54.0 (2021-07-29) ============================ Language ----------------------- - [You can now use macros for values in built-in attribute macros.][83366] While a seemingly minor addition on its own, this enables a lot of powerful functionality when combined correctly. Most notably you can now include external documentation in your crate by writing the following. ```rust #![doc = include_str!("README.md")] ``` You can also use this to include auto-generated modules: ```rust #[path = concat!(env!("OUT_DIR"), "/generated.rs")] mod generated; ``` - [You can now cast between unsized slice types (and types which contain unsized slices) in `const fn`.][85078] - [You can now use multiple generic lifetimes with `impl Trait` where the lifetimes don't explicitly outlive another.][84701] In code this means that you can now have `impl Trait<'a, 'b>` where as before you could only have `impl Trait<'a, 'b> where 'b: 'a`. Compiler ----------------------- - [Rustc will now search for custom JSON targets in `/lib/rustlib/<target-triple>/target.json` where `/` is the "sysroot" directory.][83800] You can find your sysroot directory by running `rustc --print sysroot`. - [Added `wasm` as a `target_family` for WebAssembly platforms.][84072] - [You can now use `#[target_feature]` on safe functions when targeting WebAssembly platforms.][84988] - [Improved debugger output for enums on Windows MSVC platforms.][85292] - [Added tier 3\* support for `bpfel-unknown-none` and `bpfeb-unknown-none`.][79608] \* Refer to Rust's [platform support page][platform-support-doc] for more information on Rust's tiered platform support. Libraries ----------------------- - [`panic::panic_any` will now `#[track_caller]`.][85745] - [Added `OutOfMemory` as a variant of `io::ErrorKind`.][84744] - [ `proc_macro::Literal` now implements `FromStr`.][84717] - [The implementations of vendor intrinsics in core::arch have been significantly refactored.][83278] The main user-visible changes are a 50% reduction in the size of libcore.rlib and stricter validation of constant operands passed to intrinsics. The latter is technically a breaking change, but allows Rust to more closely match the C vendor intrinsics API. Stabilized APIs --------------- - [`BTreeMap::into_keys`] - [`BTreeMap::into_values`] - [`HashMap::into_keys`] - [`HashMap::into_values`] - [`arch::wasm32`] - [`VecDeque::binary_search`] - [`VecDeque::binary_search_by`] - [`VecDeque::binary_search_by_key`] - [`VecDeque::partition_point`] Cargo ----- - [Added the `--prune <spec>` option to `cargo-tree` to remove a package from the dependency graph.][cargo/9520] - [Added the `--depth` option to `cargo-tree` to print only to a certain depth in the tree ][cargo/9499] - [Added the `no-proc-macro` value to `cargo-tree --edges` to hide procedural macro dependencies.][cargo/9488] - [A new environment variable named `CARGO_TARGET_TMPDIR` is available.][cargo/9375] This variable points to a directory that integration tests and benches can use as a "scratchpad" for testing filesystem operations. Compatibility Notes ------------------- - [Mixing Option and Result via `?` is no longer permitted in closures for inferred types.][86831] - [Previously unsound code is no longer permitted where different constructors in branches could require different lifetimes.][85574] - As previously mentioned the [`std::arch` instrinsics now uses stricter const checking][83278] than before and may reject some previously accepted code. - [`i128` multiplication on Cortex M0+ platforms currently unconditionally causes overflow when compiled with `codegen-units = 1`.][86063] [85574]: rust-lang/rust#85574 [86831]: rust-lang/rust#86831 [86063]: rust-lang/rust#86063 [86831]: rust-lang/rust#86831 [79608]: rust-lang/rust#79608 [84988]: rust-lang/rust#84988 [84701]: rust-lang/rust#84701 [84072]: rust-lang/rust#84072 [85745]: rust-lang/rust#85745 [84744]: rust-lang/rust#84744 [85078]: rust-lang/rust#85078 [84717]: rust-lang/rust#84717 [83800]: rust-lang/rust#83800 [83366]: rust-lang/rust#83366 [83278]: rust-lang/rust#83278 [85292]: rust-lang/rust#85292 [cargo/9520]: rust-lang/cargo#9520 [cargo/9499]: rust-lang/cargo#9499 [cargo/9488]: rust-lang/cargo#9488 [cargo/9375]: rust-lang/cargo#9375 [`BTreeMap::into_keys`]: https://doc.rust-lang.org/std/collections/struct.BTreeMap.html#method.into_keys [`BTreeMap::into_values`]: https://doc.rust-lang.org/std/collections/struct.BTreeMap.html#method.into_values [`HashMap::into_keys`]: https://doc.rust-lang.org/std/collections/struct.HashMap.html#method.into_keys [`HashMap::into_values`]: https://doc.rust-lang.org/std/collections/struct.HashMap.html#method.into_values [`arch::wasm32`]: https://doc.rust-lang.org/core/arch/wasm32/index.html [`VecDeque::binary_search`]: https://doc.rust-lang.org/std/collections/struct.VecDeque.html#method.binary_search [`VecDeque::binary_search_by`]: https://doc.rust-lang.org/std/collections/struct.VecDeque.html#method.binary_search_by [`VecDeque::binary_search_by_key`]: https://doc.rust-lang.org/std/collections/struct.VecDeque.html#method.binary_search_by_key [`VecDeque::partition_point`]: https://doc.rust-lang.org/std/collections/struct.VecDeque.html#method.partition_point

ehuss · 2021-08-12T23:43:08Z

This has caused a regression in beta, see #9783.

Move `tmp` test directory. The `tmp` directory added in #9375 was placed within the profile directory (such as `target/debug/tmp` or `target/release/tmp`). This causes problems for any cargo target (binary, test, etc.) with the name `tmp` as there is a name collision. This PR attempts to address that by moving the `tmp` directory to the root of the target directory (`target/tmp`), and reserving the profile name "tmp". Fixes #9783

Move `tmp` test directory. The `tmp` directory added in rust-lang#9375 was placed within the profile directory (such as `target/debug/tmp` or `target/release/tmp`). This causes problems for any cargo target (binary, test, etc.) with the name `tmp` as there is a name collision. This PR attempts to address that by moving the `tmp` directory to the root of the target directory (`target/tmp`), and reserving the profile name "tmp". Fixes rust-lang#9783

vojtechkral · 2021-08-23T08:55:35Z

Thanks for having a look, I was AFK for a week so I missed this issue...

rust-highfive assigned alexcrichton Apr 18, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 18, 2021

vojtechkral force-pushed the tests-target-dir branch from fda79ff to 4753720 Compare April 18, 2021 22:33

alexcrichton added the T-cargo Team: Cargo label Apr 20, 2021

rfcbot added proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge labels Apr 20, 2021

vojtechkral force-pushed the tests-target-dir branch 2 times, most recently from 6658741 to fa41075 Compare May 1, 2021 20:17

vojtechkral force-pushed the tests-target-dir branch from fa41075 to 67fe2fe Compare May 6, 2021 21:28

vojtechkral added 2 commits May 6, 2021 23:47

Add CARGO_TARGET_TMPDIR env var for integration tests & benches

64bfe7f

The variable is set to $target_dir/$config/tmp This is a directory where tests & benches can place testcasei-related data for use by the tests. cargo makes sure the directory exists when building tests/benches.

Use CARGO_TARGET_TMPDIR in integration tests if available

53343bc

vojtechkral force-pushed the tests-target-dir branch from 67fe2fe to 53343bc Compare May 6, 2021 22:03

vojtechkral changed the title ~~Set CARGO_TARGET_DIR for integration tests~~ Add CARGO_TARGET_TMPDIR env var for integration tests & benches May 6, 2021

rfcbot added the final-comment-period FCP — a period for last comments before action is taken label May 7, 2021

rfcbot removed the proposed-final-comment-period An FCP proposal has started, but not yet signed off. label May 7, 2021

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 7, 2021

bors merged commit e51522a into rust-lang:master May 7, 2021

ehuss mentioned this pull request May 7, 2021

Update cargo rust-lang/rust#85059

Merged

vojtechkral mentioned this pull request May 8, 2021

Provide access to project paths for tests #2841

Closed

rfcbot added finished-final-comment-period FCP complete to-announce and removed final-comment-period FCP — a period for last comments before action is taken labels May 17, 2021

xd009642 mentioned this pull request Jul 29, 2021

CARGO_TARGET_TMPDIR potential support xd009642/tarpaulin#796

Closed

ehuss mentioned this pull request Aug 12, 2021

failed to remove file target/debug/tmp #9783

Closed

ehuss mentioned this pull request Aug 20, 2021

Move tmp test directory. #9814

Merged

ehuss added this to the 1.54.0 milestone Feb 6, 2022

brainstorm mentioned this pull request Feb 2, 2023

Remove s3-server and dependencies umccr/htsget-rs#150

Merged

epage mentioned this pull request May 30, 2023

Add CARGO_WORKSPACE_DIR env var for integration tests and benchmarks #12158

Closed

dimo414 mentioned this pull request Sep 23, 2023

Use CARGO_TARGET_TMPDIR if set flub/testdir#2

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CARGO_TARGET_TMPDIR env var for integration tests & benches #9375

Add CARGO_TARGET_TMPDIR env var for integration tests & benches #9375

vojtechkral commented Apr 18, 2021 •

edited

Loading

rust-highfive commented Apr 18, 2021

alexcrichton commented Apr 20, 2021

rfcbot commented Apr 20, 2021 •

edited by ehuss

Loading

ehuss commented Apr 21, 2021

vojtechkral commented Apr 26, 2021 •

edited

Loading

ehuss commented Apr 27, 2021

vojtechkral commented Apr 29, 2021 •

edited

Loading

ehuss commented May 1, 2021

vojtechkral commented May 1, 2021

vojtechkral commented May 1, 2021

ehuss commented May 4, 2021

vojtechkral commented May 4, 2021

vojtechkral commented May 6, 2021

ehuss commented May 7, 2021

ehuss commented May 7, 2021

rfcbot commented May 7, 2021

ehuss commented May 7, 2021

bors commented May 7, 2021

bors commented May 7, 2021

bors commented May 7, 2021

ehuss commented Aug 12, 2021

vojtechkral commented Aug 23, 2021

Add CARGO_TARGET_TMPDIR env var for integration tests & benches #9375

Add CARGO_TARGET_TMPDIR env var for integration tests & benches #9375

Conversation

vojtechkral commented Apr 18, 2021 • edited Loading

rust-highfive commented Apr 18, 2021

alexcrichton commented Apr 20, 2021

rfcbot commented Apr 20, 2021 • edited by ehuss Loading

ehuss commented Apr 21, 2021

vojtechkral commented Apr 26, 2021 • edited Loading

ehuss commented Apr 27, 2021

vojtechkral commented Apr 29, 2021 • edited Loading

ehuss commented May 1, 2021

vojtechkral commented May 1, 2021

vojtechkral commented May 1, 2021

ehuss commented May 4, 2021

vojtechkral commented May 4, 2021

vojtechkral commented May 6, 2021

ehuss commented May 7, 2021

ehuss commented May 7, 2021

rfcbot commented May 7, 2021

ehuss commented May 7, 2021

bors commented May 7, 2021

bors commented May 7, 2021

bors commented May 7, 2021

ehuss commented Aug 12, 2021

vojtechkral commented Aug 23, 2021

vojtechkral commented Apr 18, 2021 •

edited

Loading

rfcbot commented Apr 20, 2021 •

edited by ehuss

Loading

vojtechkral commented Apr 26, 2021 •

edited

Loading

vojtechkral commented Apr 29, 2021 •

edited

Loading