Skip to content

perf(package): Always reuse the workspace's target-dir #15783

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

Merged
merged 2 commits into from
Aug 12, 2025

Conversation

ranger-ross
Copy link
Contributor

@ranger-ross ranger-ross commented Jul 29, 2025

What does this PR try to resolve?

While preparing to stabilize build-dir, it was discovered that cargo package will sometimes use the build cache for dependency crates if CARGO_TARGET_DIR is explicitly set.

If target-dir is not set, the build will be preformed in an "inner target dir" at target/package/{name}-{version}/target and rebuild everything.

This PR changes the default behavior to always use the build cache,
matching the behavior of having target-dir set.

How to test and review this PR?

Added a dedicated test to verify the previous behavior (cargo recompiled non-workspace crates) in the first commit.
Then updated it to verify the new behavior.

Related links/notes

…verify

When running `cargo package` the verify the build cache
(target-dir/build-dir) will not be used and all dependencies will be
recompiled.

This is inconsistent as setting target dir (via `CARGO_TARGET_DIR` for
example) will cause `cargo package` to reuse the build cache.

This commit changes the default behavior to always use the build cache,
matching the behavior of having target-dir set.
@rustbot
Copy link
Collaborator

rustbot commented Jul 29, 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 Command-package S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 29, 2025
@ranger-ross ranger-ross changed the title test(package): Add test to verify package build cache behavior Fixed inconsistent build cache behavior during package verify Jul 29, 2025
@ranger-ross
Copy link
Contributor Author

Linking to the Cargo meeting discussion about this PR.

It appears no consensus was made as many members were out that week.

Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

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

I think I'm fine with making this change. I don't recall any reasoning for keeping it separate, but I suspect if it was intentional that the intent was to avoid bad fingerprinting from properly validating the package. However, I'm comfortable now with proper fingerprinting from catching anything that needs to be rebuilt.

@epage or @weihanglo Do you have any other context on this? Otherwise I think we should merge.

@epage
Copy link
Contributor

epage commented Aug 10, 2025

Build scripts and proc macros can mess with things.

However, if the packaged packages (which is what we care about verifying) are getting a unique package id (which it should) it should then have a unique fingerprint, avoiding those problems.

Unless people are digging around outside of whats fingerprinted. I assume the current-dir is the package root and I'm assuming that is in the packaging directory, so that interferes with mucking around. I don't think we officially expose target-dir but people try to approximate it through what cargo metadata returns (dir for config lookup will be different). Unsure what problems people will have, if any, and if its worth holding this up.

@epage epage changed the title Fixed inconsistent build cache behavior during package verify perf(package): Always reuse the workspace's target-dir Aug 12, 2025
@epage
Copy link
Contributor

epage commented Aug 12, 2025

We discussed this in today's meeting. There were no objections and we can always revert this later.

@epage epage added this pull request to the merge queue Aug 12, 2025
Merged via the queue into rust-lang:master with commit 2dc9c20 Aug 12, 2025
24 checks passed
@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 12, 2025
@ranger-ross ranger-ross deleted the verify-with-cache branch August 13, 2025 01:22
bors added a commit to rust-lang/rust that referenced this pull request Aug 16, 2025
Update cargo

23 commits in 840b83a10fb0e039a83f4d70ad032892c287570a..9a0712751ca9444775c7352d3c942a155b656437
2025-07-30 13:59:19 +0000 to 2025-08-15 20:30:47 +0000
- Fixes error while running the cargo clippy --all-targets -- -D warning (rust-lang/cargo#15843)
- Clarify that `cargo doc --no-deps` is cumulative and won’t delete prev (rust-lang/cargo#15800)
- docs: Formatting and cross-linking to build-dir/target-dir docs (rust-lang/cargo#15840)
- Stabilize `build.build-dir` (rust-lang/cargo#15833)
- make resolve features public for cargo-as-a-library (rust-lang/cargo#15835)
- chore(deps): bump slab from 0.4.10 to 0.4.11 (rust-lang/cargo#15832)
- chore: remove x86_64-apple-darwin from CI and tests (rust-lang/cargo#15831)
- chore(deps): update msrv (3 versions) to v1.87 (rust-lang/cargo#15819)
- perf(package): Always reuse the workspace's target-dir (rust-lang/cargo#15783)
- More helpful error for invalid cargo-features = [] (rust-lang/cargo#15781)
- Add initial integration for `--json=timings` behing `-Zsection-timings` (rust-lang/cargo#15780)
- add is_inherited methods to InheritableDependency and InheritableField (rust-lang/cargo#15828)
- chore(deps): update compatible (rust-lang/cargo#15804)
- docs(unstable): Link out to the Plumbing commands effort (rust-lang/cargo#15821)
- chore(deps): update cargo-semver-checks to v0.43.0 (rust-lang/cargo#15825)
- test(build-std): relax the thread name assertion (rust-lang/cargo#15822)
- chore(deps): update msrv (1 version) to v1.89 (rust-lang/cargo#15815)
- Update semver tests for 1.89 (rust-lang/cargo#15816)
- Accessing each build script's `OUT_DIR` and in the correct order (rust-lang/cargo#15776)
- chore: bump to 0.92.0; update changelog (rust-lang/cargo#15807)
- docs: `-Zpackage-workspace` has been stabilized (rust-lang/cargo#15808)
- chore(deps): update rust crate cargo_metadata to 0.21.0 (rust-lang/cargo#15795)
- docs(build-rs): Fix broken intra-doc links (rust-lang/cargo#15810)

r? ghost
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants