Implemented `build.build-dir` config option #15104

ranger-ross · 2025-01-26T08:21:37Z

What does this PR try to resolve?

This PR adds a new build.build-dir configuration option that was proposed in #14125 (comment)

This new config option allows the user to specify a directory where intermediate build artifacts should be stored.
I have shortened it to just build-dir from target-build-dir, although naming is still subject to change.

What is a final artifact vs an intermediate build artifact

Final artifacts

These are the files that end users will typically want to access directly or indirectly via a third party tool.

binary executable for bin crates
binary executable for examples
.crate files output from cargo package
depinfo files (.d files) for third party build-system integrations (see https://github.com/rust-lang/cargo/blob/master/src/cargo/core/compiler/fingerprint/mod.rs#L194)
cargo doc output (html/css/js/etc)
Cargo's --timings HTML report

Intermediate build artifact, caches, and state

These are files that are used internally by Cargo/Rustc during the build process

other depinfo files (generated by rustc, fingerprint, etc. See https://github.com/rust-lang/cargo/blob/master/src/cargo/core/compiler/fingerprint/mod.rs#L164)
rlibs and debug info from dependencies
build script OUT_DIR
output from proc macros (previously stored in target/build)
incremental build output from rustc
fingerprint files used by Cargo for rebuild detection
scratchpad used for cargo package verify step
Cache of rustc invocations (.rustc_info.json)
"pre and non uplifted" binary executables. (ie. bins for examples that contain the hash in the name, bins for benches, proc macros, build scripts)
CARGO_TARGET_TMPDIR files (see rational for this here)
future-incompat-report's .future-incompat-report.json file

Feature Gating Strategy

We are following the "Ignore the feature that is used without a gate" approach as described here.

The rational for this is:
The build.build-dir is likely going to be set by by users "globally" (ie. $CARGO_HOME/config.toml) to set a shared build directory to reduce rebuilding dependencies. For users that multiple cargo versions having having an error would be disrupted.
The fallback behavior is to revert to the behavior of the current stable release (building in $CARGO_TARGET_DIR)

Testing Strategy

We have the existing Cargo testsuite to be sure we do not introduce regressions.
- I have also run the testsuite locally with the cli flag remove to verify all tests pass with the default build dir (which falls back to the target dir)
For testing thus far, I have been using small hello world project with a few dependencies like rand to verify files are being output to the correct directory.
When this PR is closer to merging, I plan to test with some larger projects with more dependencies, build scripts, ect.
Other testing recommendations are welcome 🙇

How should we test and review this PR?

This is probably best reviewed commit by commit. I documented each commit.
I tied to follow the atomic commits recommendation in the Cargo contributors guide, but I split out some commits for ease of review. (Otherwise I think this would have ended up being 1 or 2 large commits 😅)

Questions

What is the expected behavior of cargo clean?
- At the time of writing it only cleans target and does not impact the build-dir but this is easily changable.
- Answer: See Implemented build.build-dir config option #15104 (comment)
When using cargo package are was expecting just the .crate file to be in target while all other output be stored in build.build-dir? Not sure if we consider things like Cargo.toml, Cargo.toml.orig, .cargo_vcs_info.json part of the user facing interface.
- Current consensus is that only .crate is considered a final artifact
Where should cargo doc output go? HTML/JS for many crates can be pretty large. Moving to the build-dir would help reduce duplication if we find the that acceptable. For cargo doc --open this is not a problem but may be problematic for other use cases?
- Answer: Implemented build.build-dir config option #15104 (comment)
Are bins generated from benches considered final artifacts?
- Since bins from examples are considered final artifacts, it seems natural that benches should also be considered final artifacts. However, unlike examples the benches bins are stored in target/{profile}/deps instead of a dedicated directory (like target/{profile}/examples). We could move them into a dedicated directory (target/{profile}/benches) but that mean would also be changing the structure of the target directory which feels out of scope for this change. If we decide that benches are final artifacts, it would probably be better to consider that changes as part of --artifact-dir (nee --out-dir) Tracking Issue #6790
- Answer: Implemented build.build-dir config option #15104 (comment)
Do we want to include a CARGO_BUILD_DIR shortcut env var?
- The current commit (2af0c91) has included the CARGO_BUILD_DIR shortcut. This can be removed before merging if there a good reason to.

TODO

rustbot · 2025-01-26T08:21:42Z

r? @ehuss

rustbot has assigned @ehuss.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

weihanglo

Just want to make sure I didn't miss something. From what I can tell these directories/files have been removed right?

target/<profile>/.metabuild
target/<profile>/.fingerprint
target/<profile>/deps
target/<profile>/incremental
target/<profile>/build
target/.cargo-lock
target/tmp
target/.rustc_info.json

tests/testsuite/build_dir.rs

src/cargo/core/compiler/build_runner/mod.rs

ranger-ross · 2025-01-27T12:07:57Z

Just want to make sure I didn't miss something. From what I can tell these directories/files have been removed right?

target/<profile>/.metabuild

target/<profile>/.fingerprint

target/<profile>/deps

target/<profile>/incremental

target/<profile>/build

target/.cargo-lock

target/tmp

target/.rustc_info.json

Yes, that with the exception of target/.cargo-lock.
I think we will still want this cargo lock for backwards compatibility with previous versions of cargo.

Ideally in the longer term it can be removed in favor of fine grain locking like #4282

So a typical target directory will be something like

target
├── CACHEDIR.TAG
└── debug
    ├── .cargo-lock
    ├── examples
    └── hello_world // (the binary)

src/doc/src/reference/unstable.md

src/cargo/core/compiler/layout.rs

tests/testsuite/build_dir.rs

epage · 2025-01-27T16:59:24Z

What is the expected behavior of cargo clean?

It should clean the build dir

epage · 2025-01-27T17:00:06Z

When using cargo package are was expecting just the .crate file to be in target while all other output be stored in build.build-dir? Not sure if we consider things like Cargo.toml, Cargo.toml.orig, .cargo_vcs_info.json part of the user facing interface.

imo The artifact for cargo package is the .crate. Everything else is part of the "build" process.

epage · 2025-01-27T17:01:18Z

Can we call out explicitly what our testing strategy is?

We likely should also explicitly document in the PR what is considered an artifact and what is a build output and make sure we have tests for these.

ranger-ross · 2025-01-28T14:50:22Z

One other question that came to my mind was the output of cargo doc. HTML/JS for many crates can be pretty large. Moving to the build-dir would help reduce duplication if we find the that acceptable. For cargo doc --open this is not a problem but may be problematic for other use cases?

Perhaps symlinking the index.html from the build dir into target could be an option if we care about keeping an entry point.

ranger-ross · 2025-01-28T14:53:27Z

Can we call out explicitly what our testing strategy is?

We likely should also explicitly document in the PR what is considered an artifact and what is a build output and make sure we have tests for these.

@epage sure, I updated the PR description but let me know if I missed anything.

epage · 2025-01-28T18:27:34Z

One other question that came to my mind was the output of cargo doc. HTML/JS for many crates can be pretty large. Moving to the build-dir would help reduce duplication if we find the that acceptable. For cargo doc --open this is not a problem but may be problematic for other use cases?

imo cargo docs output is an artifact that people will want access to. I suspect it'd be a breaking change to move it out of target-dir.

epage · 2025-01-28T18:31:26Z

depinfo files (.d files)

There are multiple types of depinfo files. I suspect the ones next to final artifacts are also considered final artifacts, see https://doc.rust-lang.org/cargo/reference/build-cache.html#dep-info-files

epage · 2025-01-28T18:35:51Z

FYI I added to the PR description a couple more intermediate artifacts

rlibs and debug info from dependencies

build script OUT_DIR

When are workspace member rlibs considered final artifacts? We're putting them in target/<profile> at times, so I take it that has already been answered.

tests/testsuite/build_dir.rs

src/cargo/util/context/mod.rs

src/cargo/core/compiler/fingerprint/mod.rs

src/cargo/core/compiler/future_incompat.rs

src/cargo/core/compiler/mod.rs

tests/testsuite/build_dir.rs

ranger-ross · 2025-02-09T08:35:05Z

I think this PR is now complete to the point that I will mark this PR as ready to review.

Since the last review I have:

Added a shortcut for CARGO_BUILD_DIR
Updated the docs in unstable.md
Cleaned up/fixed the tests and added a few more different crate types
Updated the PR description with a feature gating strategy

r? @epage

src/cargo/ops/cargo_package/verify.rs

src/doc/src/reference/unstable.md

epage · 2025-02-10T16:29:38Z

tests/testsuite/build_dir.rs

From #15104 (comment)

Can we call out explicitly what our testing strategy is?

We likely should also explicitly document in the PR what is considered an artifact and what is a build output and make sure we have tests for these.

From #15104 (comment)

@epage sure, I updated the PR description but let me know if I missed anything.

From #15104 (comment)

Sorry if I wasn't clear but my discussion of testing strategy was in the context of tracking the categorization of artifacts. I'm not saying we have to exhaustively test it but we should think about it and document the choice made and why.

Sorry, I think I misunderstood your request. (perhaps multiple times)

I added a doc comment at the top of build_dir.rs with a summary of the test strategy for that file and the rational why.

Let me know if that is what you were asking for.

In the ideal scenario, we verify

each file listed under artifact-dir vs build-dir is in the right place

ensure new files get categorized correctly

I'm assuming we can't meet that ideal. I would like to understand why we can't meet that ideal and understand how close we should get to it along with a call out of what gaps are left.

These ares are in preparation to split target-dir into artifact-dir and build-dir

This is in preparation for splitting the intermediate build artifacts from the `target` directory.

This commit adds a `build_dir` option to the `build` table in `config.toml` and adds the equivalent field to `Workspace` and `GlobalContext`.

This commits implements the seperation of the intermidate artifact directory (called "build directory") from the target directory. (see rust-lang#14125)

rustbot · 2025-02-11T14:03:22Z

☔ The latest upstream changes (possibly 321f14e) made this pull request unmergeable. Please resolve the merge conflicts.

weihanglo · 2025-02-11T19:48:48Z

src/cargo/util/context/mod.rs

+        if !self.cli_unstable().build_dir {
+            return self.target_dir();
+        }
+        if let Some(dir) = self.get_env_os("CARGO_BUILD_DIR") {


I understand CARGO_BUILD_DIR is proposed here because we already have CARGO_TARGET_DIR. However I feel like CARGO_TARGET_DIR was a mistake.

The build.build-dir config already comes with a CARGO_BUILD_BUILD_DIR environment variable for free. We may want to stick with it and stop stacking up extra environment variables.

This is open for discussions btw.

However I feel like CARGO_TARGET_DIR was a mistake.

Can I ask why you feel it was a mistake?

As a user of cargo I think it might be a bit confusing to see BUILD show up twice in the same variable name.
It make sense when strictly following the Cargo configuration, but newcomers are often not aware of this.

I think there is value in providing a shortcut that is easily understandable.

Can I ask why you feel it was a mistake?

It's a hindsight honestly. It is ideal to me that each environment variable maps to one configuration field, and vice versa. We have cargo --config available already if people want to pass complex value.

CARGO_TARGET_DIR is a standalone environment variable which cargo and other community tools need to handle specially and understand the precedence of these options. If it were only one CARGO_BUILD_TARGET_DIR then people don't need to deal with two environment variables. And the --target-dir flag can just be a alias equivalent to --config 'build.target-dir=foo', and could eliminate the special handling.

It make sense when strictly following the Cargo configuration, but newcomers are often not aware of this.

Could you elaborate a bit more on this? In the book there is a list of variables for each configuration field, which I assume no discovery issue here. (Could expand the doc of CARGO_BUILD_TARGET_DIR, if the other one is removed, yet we won't remove it for sure.)

it might be a bit confusing to see BUILD show up twice in the same variable name.

Feel like it is a naming issue, regardless whether we have CARGO_BUILD_DIR variable or not.

And the --target-dir flag can just be a alias equivalent to --config 'build.target-dir=foo', and could eliminate the special handling.

The plan was to not offer a --build-dir CLI option and build-dir would eventually be decoupled from --target-dir

I think there is value in providing a shortcut that is easily understandable.

Not sure about “shortcut”. If you meant 6-character shorter yeah it is, though usablility depends on whether people type it everyday in interactive sessions. I would assume the majority of users set it once in shell startup script, or in ~/.cargo/config.toml. Then the length doesn't really matter.

This is unstable, so we don't have to have a final decision. The question is mostly if there is enough reason for or against it for the initial implementation.

While I lean towards CARGO_BUILD_DIR, starting with fewer features makes it easier to identify when a feature is needed compared to when you offer everything, its hard to tell what is used and could be removed.

Could you elaborate a bit more on this? In the book there is a list of variables for each configuration field, which I assume no discovery issue here. (Could expand the doc of CARGO_BUILD_TARGET_DIR, if the other one is removed, yet we won't remove it for sure.)

I think that anyone remotely familiar with Rust can fairly quickly understand CARGO_TARGET_DIR does without going to the documentation page. Seeing CARGO_BUILD_BUILD_DIR is much less obvious and will probably require some google searching to figure out what it does.

But perhaps to your later point, if people just set it once in their shared cargo config, perhaps it does not matter that much.

While I lean towards CARGO_BUILD_DIR, starting with fewer features makes it easier to identify when a feature is needed compared to when you offer everything, its hard to tell what is used and could be removed.

I think this is fair. I can remove it from this PR and we can revisit if it's really needed later

weihanglo · 2025-02-11T19:53:54Z

src/cargo/core/compiler/layout.rs

        let dest = root.join(dest);
        // If the root directory doesn't already exist go ahead and create it
        // here. Use this opportunity to exclude it from backups as well if the
        // system supports it since this is a freshly created folder.
        //
        paths::create_dir_all_excluded_from_backups_atomic(root.as_path_unlocked())?;
+        if root != build_root {
+            paths::create_dir_all_excluded_from_backups_atomic(build_root.as_path_unlocked())?;


Not really on topic, but this reminds me #11548 and #15061.

For the new build-dir, we don't have backward compatibility issue, and files inside are really intermediate caches. We might want to reconsider the self-ignoring directory approach.

rustbot assigned ehuss Jan 26, 2025

ranger-ross changed the title ~~Added build-directory unstable feature flag~~ Implemented build.build-dir config option Jan 26, 2025

weihanglo reviewed Jan 26, 2025

View reviewed changes

tests/testsuite/build_dir.rs Show resolved Hide resolved

src/cargo/core/compiler/build_runner/mod.rs Outdated Show resolved Hide resolved

epage reviewed Jan 27, 2025

View reviewed changes

src/doc/src/reference/unstable.md Outdated Show resolved Hide resolved

epage reviewed Jan 27, 2025

View reviewed changes

src/cargo/core/compiler/layout.rs Outdated Show resolved Hide resolved

epage reviewed Jan 27, 2025

View reviewed changes

tests/testsuite/build_dir.rs Show resolved Hide resolved

epage reviewed Jan 27, 2025

View reviewed changes

tests/testsuite/build_dir.rs Outdated Show resolved Hide resolved

epage mentioned this pull request Jan 27, 2025

Redefine CARGO_TARGET_DIR to be only an artifacts directory #14125

Open

4 tasks

ranger-ross force-pushed the target-build-dir branch from a68a5eb to e49bf62 Compare February 5, 2025 14:52