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)
reports generated from cargo report like future-incompat-report

Intermediate build artifact

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
"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)

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

weihanglo · 2025-01-26T17:14:46Z

tests/testsuite/build_dir.rs

We usually add some tests verifying a nightly flag is really gated.

weihanglo · 2025-01-26T17:31:14Z

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

@@ -290,7 +290,9 @@ impl<'a, 'gctx> BuildRunner<'a, 'gctx> {
                });
            }

-            super::output_depinfo(&mut self, unit)?;
+            if !self.bcx.gctx.cli_unstable().build_dir {
+                super::output_depinfo(&mut self, unit)?;


Why do we stop generating this when build-dir is enabled?

Sorry, actually I believe this is a mistake on my part 😅

My original assumption was that the /target/debug/{project-name}.d file was the same file as /target/debug/deps/{project-name}-{hash}.d, so I was trying to avoid copying it into target.
But now that I compared the files they appear to be different so it looks like that was a bad assumption.

I believe this should probably be considered an intermediate build artifact so it should probably be moved to the build-dir location.

Modifying this file in particular was a bit tricky as it was not just replacing .target_dir() with .build_dir().
I think I will probably need to go to where the OutputFile is created and modify the hardlink to be the new build-dir path.

Given this comment I am going to revert this change

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)

epage · 2025-01-27T16:47:53Z

src/doc/src/reference/unstable.md

+## build-dir
+* Original Issue: [#14125](https://github.com/rust-lang/cargo/issues/14125)
+* Tracking Issue: [#14125](https://github.com/rust-lang/cargo/issues/14125)
+
+This feature allows you to specify the directory where intermediate build artifacts will be stored.
+The final artifacts will also be contained in this directory but will be hardlinked into the artifact directory (usually `target`)
+
+This can be specified in `.cargo/config.toml` files using the `build.build-dir`.
+
+Example:
+
+```toml
+[build]
+build-dir = "out"
+```


Could you include documentation in this that mirrors what would be in https://doc.rust-lang.org/cargo/reference/config.html

epage · 2025-01-27T16:49:29Z

src/cargo/core/compiler/layout.rs

+    /// Same as `_lock` but for the build directory.
+    ///
+    /// NOTE: `_lock` and `_build_lock` can eventually be merged once #14125 is stablized.
+    /// Will be `None` when the build-directory and target-directory are the same path as we cannot
+    /// lock the same path twice.


How can we merge these locks?

In the short term, we'll have

target-dir

build-dir

which both need locking

In the long term, we'll have

artifact-dir

build-dir

which will both need locking

Sorry, you are right. Not quiet sure what I was thinking when I wrote this comment.

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.

ranger-ross · 2025-02-02T10:10:08Z

I spent some more time this weekend making some progress on this PR.

Here are my updates

Fixed some issues to make cargo clean & cargo package work with build.build-dir
Added templating support for build.build-dir
Cleanup the commits a bit and tried to follow the request in this comment regarding tests

I updated the PR description with more details as well. notably:

Added another question I would like input form the Cargo team on.
- Are bins generated from benches considered final artifacts? (see PR desc for more details)
Updated the TODO list with a more detailed list of tasks before marking this PR as ready for review.
Cleaned up the "What is a final artifact vs an intermediate build artifact" section
- Also added the following file types to the list of intermediate build artifacts
  - "pre-uplifted" binary executables. (ie. bins for examples that contain the hash in the name)
  - CARGO_TARGET_TMPDIR files (see rational for this here)

epage · 2025-02-04T20:02:14Z

Added templating support for build.build-dir

I haven't looked but would it work to split that out into a follow up PR? Keeping PRs focused can help with ensuring a smooth review and that we are less likely to overlook something.

Are bins generated from benches considered final artifacts? (see PR desc for more details)

My take is that if it isn't staged/uplifted, then its not a final artifact.

I assume this also applies to test binaries, build scripts, and proc-macros.

ranger-ross · 2025-02-05T11:50:38Z

I haven't looked but would it work to split that out into a follow up PR? Keeping PRs focused can help with ensuring a smooth review and that we are less likely to overlook something.

Sure. Its not a ton of code, but I have no problems breaking that out into anther PR to reduce the scope of this one 👍

ranger-ross · 2025-02-05T11:52:42Z

Are bins generated from benches considered final artifacts? (see PR desc for more details)

My take is that if it isn't staged/uplifted, then its not a final artifact.

I assume this also applies to test binaries, build scripts, and proc-macros.

Thanks, I think that sounds reasonable.
I will add that to the classification list in PR description with this as well.

tests/testsuite/build_dir.rs

epage · 2025-02-05T17:38:50Z

tests/testsuite/build_dir.rs

+        name.to_string()
+    };
+
+    assert_eq!(path.join(executable).exists(), exists);


Can you emit the path in the assert message?

epage · 2025-02-05T17:41:04Z

tests/testsuite/build_dir.rs

+}
+
+#[cargo_test]
+fn verify_build_dir_is_disabled_by_feature_flag() {


Can you add this test to the first commit?

epage · 2025-02-05T17:43:33Z

tests/testsuite/build_dir.rs

+#[cargo_test]
+fn verify_build_dir_is_disabled_by_feature_flag() {
+    let p = project()
+        .file("src/main.rs", r#"fn main() { println!("Hello, World!") }"#)
+        .file(
+            ".cargo/config.toml",
+            r#"
+            [build]
+            build-dir = "build"
+            "#,
+        )
+        .build();
+
+    p.cargo("build")
        .masquerade_as_nightly_cargo(&[])
        .enable_mac_dsym()
        .run();

+    assert_build_dir(p.root().join("target"), "debug", true);
+    assert_executeable_exists(p.root().join("target/debug"), "foo", true);
    assert!(!p.root().join("build").exists());
 }


Looks like you are taking the approach "Ignore the feature that is used without a gate " (see https://doc.rust-lang.org/nightly/nightly-rustc/cargo/core/features/index.html)

Could you speak to that (and update the PR description with that)

Sure, I updated the PR description with a feature gating section

epage · 2025-02-05T17:48:03Z

src/cargo/util/context/mod.rs

+        if let Some(val) = &self.build_config()?.build_dir {
+            let path = val.resolve_path(self);


Compared to pub fn target_dir(),

We don't have self.build_dir which is fine as we don't have a CLI flag

We don't check CARGO_BUILD_DIR env variable as a shortcut for CARGO_BUIILD_BUILD_DIR. Should we have this shortcut?

We don't check CARGO_BUILD_DIR env variable as a shortcut for CARGO_BUILD_BUILD_DIR. Should we have this shortcut?

I am in favor of adding this as CARGO_BUILD_BUILD_DIR is a bit rough on the eyes.
I can see this being useful when modifying the .cargo/config.toml is not possible or inconvenient.
ie. Its much easier to set an env var than update a toml file in CI bash script.

epage · 2025-02-05T17:49:39Z

src/cargo/util/context/mod.rs

+        } else {
+            // For now, fallback to the previous implementation.
+            // This will change in the future.
+            return self.target_dir();
+        }


Should we handle this fallback here or in Workspace::build_dir?

(I can see both ways)

Looks like it is in the follow up commit, duplicating the fallback

yes, I added the fallback in both places "just in case"

epage · 2025-02-05T17:50:33Z

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

 fn target_root(build_runner: &BuildRunner<'_, '_>) -> PathBuf {
-    build_runner.bcx.ws.target_dir().into_path_unlocked()
+    build_runner.bcx.ws.build_dir().into_path_unlocked()


Should target_root be renamed?

epage · 2025-02-05T17:52:01Z

src/cargo/core/compiler/future_incompat.rs

+            .build_dir()
            .open_rw_exclusive_create(
                FUTURE_INCOMPAT_FILE,


I'm not seeing this discussed in the PR as intermediate vs final.

Thanks for pointing this out. I reverted the change and added the following to the "final artifacts" category

reports generated from cargo report like future-incompat-report

epage · 2025-02-05T17:53:57Z

src/cargo/core/compiler/mod.rs

@@ -297,7 +297,7 @@ fn rustc(
    let exec = exec.clone();

    let root_output = build_runner.files().host_dest().to_path_buf();
-    let target_dir = build_runner.bcx.ws.target_dir().into_path_unlocked();
+    let target_dir = build_runner.bcx.ws.build_dir().into_path_unlocked();


Should we rename this variable?

epage · 2025-02-05T17:58:51Z

tests/testsuite/build_dir.rs

    assert_build_dir(p.root().join("build"), "debug", true);
-    assert_executeable_exists(p.root().join("build/debug"), "foo", true);
-    assert!(!p.root().join("target").exists());
+    assert_build_dir(p.root().join("target"), "debug", false);


In reading these tests, the intent of the assertion isn't too clear (asserting that the target dir is a build dir? what does false mean?). I think it'd be better to have two separate functions. If they truly must share an implementation, then they can just be wrappers on top of that.

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)

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

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

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 dd06427 to f53007f Compare February 2, 2025 08:40

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

epage reviewed Feb 5, 2025

View reviewed changes

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

epage reviewed Feb 5, 2025

View reviewed changes

ranger-ross force-pushed the target-build-dir branch 2 times, most recently from ec7d7ba to 2af0c91 Compare February 9, 2025 07:27

ranger-ross added 5 commits February 9, 2025 16:46

Added target-dir tests

19044aa

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

Added build-dir unstable feature flag

521f8b4

Derive PartialEq and Eq for Filesystem

68bda82

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

Added build.build_dir configuration option

20ed8cb

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

Seperated build directory from target directory

1a326c2

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

ranger-ross force-pushed the target-build-dir branch from 2af0c91 to 1a326c2 Compare February 9, 2025 07:47

rustbot assigned epage and unassigned ehuss Feb 9, 2025

ranger-ross marked this pull request as ready for review February 9, 2025 08:35

		if let Some(val) = &self.build_config()?.build_dir {
		let path = val.resolve_path(self);

Implemented build.build-dir config option #15104

Are you sure you want to change the base?

Implemented build.build-dir config option #15104

Conversation

ranger-ross commented Jan 26, 2025 • edited Loading

What does this PR try to resolve?

What is a final artifact vs an intermediate build artifact

Final artifacts

Intermediate build artifact

Feature Gating Strategy

Testing Strategy

How should we test and review this PR?

Questions

TODO

rustbot commented Jan 26, 2025

weihanglo left a 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

ranger-ross commented Jan 27, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epage commented Jan 27, 2025

epage commented Jan 27, 2025

epage commented Jan 27, 2025

ranger-ross commented Jan 28, 2025

ranger-ross commented Jan 28, 2025

epage commented Jan 28, 2025

epage commented Jan 28, 2025

epage commented Jan 28, 2025

ranger-ross commented Feb 2, 2025

epage commented Feb 4, 2025

ranger-ross commented Feb 5, 2025

ranger-ross commented Feb 5, 2025

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

ranger-ross commented Feb 9, 2025 • edited Loading

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

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

ranger-ross commented Jan 26, 2025 •

edited

Loading

ranger-ross commented Feb 9, 2025 •

edited

Loading