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

Implemented build.build-dir config option #15104

Merged
merged 5 commits into from
Feb 25, 2025

Conversation

ranger-ross
Copy link
Contributor

@ranger-ross ranger-ross commented Jan 26, 2025

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.

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?
  • 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?
  • 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

  • Implementation
    • Add support in cargo clean
    • Implement templating for build.build-dir
    • Fix issue with target/examples still containing "pre-uplifted" binaries
    • Verify build-dir with non-bin crate types
  • Prepare for review
    • Clean up/improve docs
    • Review tests and add more as needed
    • Fix tests in CI (Windows is currently failing)
    • Clean up commits
    • Resolve remaining questions
  • Request review

Sorry, something went wrong.

@rustbot
Copy link
Collaborator

rustbot commented Jan 26, 2025

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

@rustbot rustbot added A-build-execution Area: anything dealing with executing the compiler A-build-scripts Area: build.rs scripts A-configuration Area: cargo config files and env vars A-documenting-cargo-itself Area: Cargo's documentation A-filesystem Area: issues with filesystems A-future-incompat Area: future incompatible reporting A-layout Area: target output directory layout, naming, and organization A-rebuild-detection Area: rebuild detection and fingerprinting A-unstable Area: nightly unstable support A-workspaces Area: workspaces Command-package S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 26, 2025
@ranger-ross ranger-ross changed the title Added build-directory unstable feature flag Implemented build.build-dir config option Jan 26, 2025
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ranger-ross
Copy link
Contributor Author

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
Copy link
Contributor

epage commented Jan 27, 2025

What is the expected behavior of cargo clean?

It should clean the build dir

@epage
Copy link
Contributor

epage commented Jan 27, 2025

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
Copy link
Contributor

epage commented Jan 27, 2025

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor

epage commented Jan 28, 2025

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
Copy link
Contributor

epage commented Jan 28, 2025

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
Copy link
Contributor

epage commented Jan 28, 2025

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.

@rustbot

This comment has been minimized.

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())?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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`.
@ranger-ross
Copy link
Contributor Author

Rebased to pick up the CI fixes in #15222 to hopefully make CI green

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

epage commented Feb 25, 2025

Thanks for putting in all this work on this!

@epage epage added this pull request to the merge queue Feb 25, 2025
Merged via the queue into rust-lang:master with commit ef12f10 Feb 25, 2025
21 checks passed
@ranger-ross ranger-ross deleted the target-build-dir branch February 26, 2025 14:35
@epage epage mentioned this pull request Feb 26, 2025
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 28, 2025
Update cargo

11 commits in 1d1d646c06a84c1aa53967b394b7f1218f85db82..2622e844bc1e2e6123e54e94e4706f7b6195ce3d
2025-02-21 21:38:53 +0000 to 2025-02-28 12:33:57 +0000
- Bump `cc` to 1.2.16 to fix `x86` windows jobs in rust-lang/rust CI (rust-lang/cargo#15245)
- refactor(tree): Abstract the concept of a NodeId (rust-lang/cargo#15237)
- feat: implement RFC 3553 to add SBOM support (rust-lang/cargo#13709)
- refactor(tree): Abstract the concept of an edge (rust-lang/cargo#15233)
- chore: bump openssl to v3 (rust-lang/cargo#15232)
- fix(package): Register workspace member renames in overlay  (rust-lang/cargo#15228)
- Implemented `build.build-dir` config option (rust-lang/cargo#15104)
- feat: add completions for `--manifest-path` (rust-lang/cargo#15225)
- chore: semver-check build-rs against beta channel (rust-lang/cargo#15223)
- chore: depend on openssl-sys to correctly pin its version (rust-lang/cargo#15224)
- chore: dont check cargo-util semver until 1.86 is released (rust-lang/cargo#15222)
@rustbot rustbot added this to the 1.87.0 milestone Feb 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-build-execution Area: anything dealing with executing the compiler A-build-scripts Area: build.rs scripts A-configuration Area: cargo config files and env vars A-documenting-cargo-itself Area: Cargo's documentation A-filesystem Area: issues with filesystems A-future-incompat Area: future incompatible reporting A-layout Area: target output directory layout, naming, and organization A-rebuild-detection Area: rebuild detection and fingerprinting A-unstable Area: nightly unstable support A-workspaces Area: workspaces Command-clean Command-package S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants