-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat(report): add cargo report rebuilds #16456
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
Conversation
| if len == 1 { "" } else { "s" } | ||
| } | ||
|
|
||
| fn format_dirty_reason( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should also add these as unresolved question in tracking issue
- Determine whether we want to expose the entire DirtyReason.
- If yes, we might want to unify dirty reason representation
- Check whether each variant is still useful.
- For example, I guess the recent
-Cmetadatachange has made rustflags' fingeprints separate, soRustflagsChangedmay be rare or even not shown anymore.
- For example, I guess the recent
- Make the dirty reason more end-user friendly.
85b0f5f to
e29b582
Compare
| /// A dependency was stale. | ||
| StaleDependency { | ||
| name: InternedString, | ||
| unit: u64, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Starting to wonder if we should have a newtype
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I was also wondering if JobId should be replaced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will file a follow-up PR for this, if you don't mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fully understand this being a follow up
| let mut target = if unit.target.is_lib() | ||
| && matches!(unit.mode, CompileMode::Build | CompileMode::Check { .. }) | ||
| { | ||
| // Special case for brevity, since most dependencies hit this path. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There a reason we aren't sharing this with the report command?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am waiting for #16420 and then completely move to log-messages.
| ), | ||
| ) | ||
| .subcommand( | ||
| subcommand("rebuilds") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not thrilled with this command name though we could defer it to the tracking issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have any specific one in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like ehuss made a one off comment for cargo report rebuild (#2904 (comment))
I feel like what is missing is that we are showing the "reason" or "cause" of the build. Not thrilled with any of the names though (build-cause, build-reason, what-changed).
Depending on how we render it, this could instead be a buck what-ran or build though that would likely move too far away from the intended purpose here.
|
|
||
| struct RootRebuild { | ||
| unit_index: u64, | ||
| reason: DirtyReason, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing that will likely disappoint users is this only helps with fingerprint differences and not unit hash differences.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that is an issue I pointed out in #16456 (comment). Now Cargo doesn't even know the "new" build was triggered by rustflags changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was also the reason that previously I tried to track the "previous session id" in unit-fingerprint, so we can compare rustflags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had feature selection in mind, particularly in the presence of unification. I suspect users are more likely to run into that than RUSTFLAGS and be less obvious than something they explicitly set. For RUSTFLAGS, we could get a lot of mileage out of including it in the metadata for the whole operation.
For cascading rebuild reasons we log unit index so that we know why the exact unit of the fingerprint is dirty. Previously we log package name but had no way to reference back to the actual unit.
e29b582 to
183d904
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
Since check are also hit by most dependencies.
Adds a new command to analyze rebuild reasons from previous sessions.
The report includes:
* overview of rebuild/cached/new unit counts
* root rebuilds sorted by number of cascading rebuilds
* `-v` to show all root rebuilds (default showing 5)
* `-vv` to show affected unit lists (default collapsed)
This command doesn't have filtering by package or reason yet.
Can be added when we have use cases.
Example output:
```console
Session: 20251231T204416809Z-a5db680cc3bc96e4
Status: 3 units rebuilt, 0 cached, 0 new
Rebuild impact:
root rebuilds: 1 unit
cascading: 2 units
Root rebuilds:
0. common@0.0.0 (check): file modified: common/src/lib.rs
impact: 2 dependent units rebuilt
[NOTE] pass `-vv` to show all affected rebuilt unit lists
```
```console
$ cargo report rebuilds --verbose`
Session: 20251231T204416809Z-a5db680cc3bc96e4
Status: 6 units rebuilt, 0 cached, 0 new
Rebuild impact:
root rebuilds: 6 units
cascading: 0 units
Root rebuilds:
0. pkg1@0.0.0 (check): declared features changed: [] -> ["feat"]
impact: no cascading rebuilds
1. pkg2@0.0.0 (check): file modified: pkg2/src/lib.rs
impact: no cascading rebuilds
2. pkg3@0.0.0 (check): target configuration changed
impact: no cascading rebuilds
3. pkg4@0.0.0 (check): environment variable changed (__CARGO_TEST_MY_FOO): <unset> -> 1
impact: no cascading rebuilds
4. pkg5@0.0.0 (check): file modified: pkg5/src/lib.rs
impact: no cascading rebuilds
5. pkg6@0.0.0 (check): file modified: pkg6/src/lib.rs
impact: no cascading rebuilds
```
183d904 to
872f02c
Compare
Update cargo submodule 19 commits in 94c368ad2b9db0f0da5bdd8421cea13786ce4412..623444427f04292f3a3c7eac560cfa65ba0bb88e 2025-12-26 19:39:15 +0000 to 2026-01-06 21:05:05 +0000 - docs(unstable): expand docs for `-Zbuild-analysis` (rust-lang/cargo#16476) - test: add `-Zunstable-options` with custom targets (rust-lang/cargo#16467) - feat(report): add cargo report rebuilds (rust-lang/cargo#16456) - feat(test-support): Use test name for dir when running tests (rust-lang/cargo#16121) - refactor: Migrate some cases to expect/reason (rust-lang/cargo#16461) - docs(build-script): clarify OUT_DIR is not cleaned between builds (rust-lang/cargo#16437) - chore: Update dependencies (rust-lang/cargo#16460) - Update handlebars to 6.4.0 (rust-lang/cargo#16457) - chore(deps): update alpine docker tag to v3.23 (rust-lang/cargo#16454) - Any build scripts can now use cargo::metadata=KEY=VALUE (rust-lang/cargo#16436) - fix(log): add `dependencies` field to `UnitRegistered` (rust-lang/cargo#16448) - Implement fine grain locking for `build-dir` (rust-lang/cargo#16155) - feat(resolver): List features when no close match (rust-lang/cargo#16445) - feat(report): new command `cargo report sessions` (rust-lang/cargo#16428) - feat (patch): Display where the patch was defined in patch-related error messages (rust-lang/cargo#16407) - test(build-rs): Reduce from 'build' to 'check' where possible (rust-lang/cargo#16444) - feat(toml): TOML 1.1 parse support (rust-lang/cargo#16415) - feat(report): support --manifest-path in `cargo report timings` (rust-lang/cargo#16441) - fix(vendor): recursively filter git files in subdirectories (rust-lang/cargo#16439) r? ghost
Update cargo submodule 24 commits in 94c368ad2b9db0f0da5bdd8421cea13786ce4412..fc4c92b64d1e0046b66cbdc747cc1c17af8b35a0 2025-12-26 19:39:15 +0000 to 2026-01-08 06:54:56 +0000 - Fixed incorrect version comparision during build script dependency selection (rust-lang/cargo#16486) - refactor: new type for unit index (rust-lang/cargo#16485) - feat(test): Make CARGO_BIN_EXE_ available at runtime (rust-lang/cargo#16421) - fix(package): detect dirty files when run from workspace member (rust-lang/cargo#16479) - fix(timing)!: remove `--timings=<FMT>` optional format values (rust-lang/cargo#16420) - docs(unstable): expand docs for `-Zbuild-analysis` (rust-lang/cargo#16476) - test: add `-Zunstable-options` with custom targets (rust-lang/cargo#16467) - feat(report): add cargo report rebuilds (rust-lang/cargo#16456) - feat(test-support): Use test name for dir when running tests (rust-lang/cargo#16121) - refactor: Migrate some cases to expect/reason (rust-lang/cargo#16461) - docs(build-script): clarify OUT_DIR is not cleaned between builds (rust-lang/cargo#16437) - chore: Update dependencies (rust-lang/cargo#16460) - Update handlebars to 6.4.0 (rust-lang/cargo#16457) - chore(deps): update alpine docker tag to v3.23 (rust-lang/cargo#16454) - Any build scripts can now use cargo::metadata=KEY=VALUE (rust-lang/cargo#16436) - fix(log): add `dependencies` field to `UnitRegistered` (rust-lang/cargo#16448) - Implement fine grain locking for `build-dir` (rust-lang/cargo#16155) - feat(resolver): List features when no close match (rust-lang/cargo#16445) - feat(report): new command `cargo report sessions` (rust-lang/cargo#16428) - feat (patch): Display where the patch was defined in patch-related error messages (rust-lang/cargo#16407) - test(build-rs): Reduce from 'build' to 'check' where possible (rust-lang/cargo#16444) - feat(toml): TOML 1.1 parse support (rust-lang/cargo#16415) - feat(report): support --manifest-path in `cargo report timings` (rust-lang/cargo#16441) - fix(vendor): recursively filter git files in subdirectories (rust-lang/cargo#16439)
What does this PR try to resolve?
This will improve #2904.
Part of #15844.
Adds a new command to analyze rebuild reasons from previous sessions.
The report includes:
-vto show all root rebuilds (default showing 5)-vvto show affected unit lists (default collapsed)This command doesn't have filtering by package or reason yet.
Can be added when we have use cases.
Example output:
How to test and review this PR?
I found it awkward to logging fingerprint,
at least haven't found a reason way to implement that.
This came out first as a basic version of rebuild reporting.
Open questions
cargo tree?cargo report buildsrignt how should have only a basic version to show the usefulness of log messages, and we can expand later if needed.Screenshots
Built rustfix after
cargo update syn@2