-
Notifications
You must be signed in to change notification settings - Fork 2.8k
fix(report): get the same feature parity of --timings
#16414
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
...argo_report_timings/html_snapshot/out/cargo-timing-20060724T012128000Z-b0fd440798ab3cfb.html
Outdated
Show resolved
Hide resolved
src/cargo/util/log_message.rs
Outdated
| /// Build profile name (e.g., "dev", "release"). | ||
| profile: String, | ||
| /// The requested target platforms of compilation for this build. | ||
| requested_platforms: Vec<String>, |
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 feel like this belongs on the UnitRegistered messages. Once there, can we derive this?
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.
Naming question: if in unit-registered, what should it call it:
platformplatform_targettarget_platform:target_triple(though for target-spec.json it won't be triple/tuple)target
Also, we might want to rename the Cargo target to something
artifact(though it is weird that this is more close to the input than output)build_target
This reminds me the thread: https://internals.rust-lang.org/t/renaming-cargo-target/21585/62. We definitely need to decide when stabilizing this.
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.
Pick something and note it for stabilization?
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.
The latest revision I chose to move forward with
target: build targetplatform: platform target
src/cargo/ops/cargo_compile/mod.rs
Outdated
| let elapsed = ws.gctx().creation_time().elapsed().as_secs_f64(); | ||
| logger.log(LogMessage::UnitGraphFinished { elapsed }); | ||
| logger.log(LogMessage::UnitGraphFinished { | ||
| requested_units, |
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 like the name requested. I've been using selected but this works better.
This caused a headache that * You need to remember to generate timings report, otherwise the HTML get stale. * The source diff of both the log file and the HTML report are basically unreviewable, and the timestamp and elapsed time change every time. We'd like to find a better way to help review timing HTML instead of this.
This shows what `--timings` showing.
we additionally log this info in each `unit-registered` event
This is done by tracking unit-fingerprint events.
Done by having `requested: true` field in user-requested units Instead of calling it "root units", here we use "requested units", which represent the actual build targets that users requested. This is for reserving future extension that requested units may not always be the "root units" of the unit graph. We might have some synthetic units like rustdoc-merge, lipo universal binary for macOS, post-processing, or any user-defined super root units. The requested units sounds more future-proof than root_units,
Update cargo submodule 13 commits in e91b2baa632c0c7e84216c91ecfe107c37d887c1..3861f60f6b58f57524c0e7aab1f5c1ad83e35409 2025-12-13 16:29:21 +0000 to 2025-12-19 21:53:33 +0000 - fix(sbom): Don't set CARGO_SBOM_PATH when empty (like stable) (rust-lang/cargo#16419) - chore(ci): report-timings to upload HTML report (rust-lang/cargo#16416) - fix(report): get the same feature parity of `--timings` (rust-lang/cargo#16414) - fix(docs): Document the only possible values for DEBUG in build scripts (rust-lang/cargo#16413) - fix(log): emit fingeprint log also for no-rebuild unit (rust-lang/cargo#16408) - Fix link to `doc_cfg` docs in cargo book (rust-lang/cargo#16404) - Add best pratice for how to check in generated files (rust-lang/cargo#16405) - test: Use a larger default term width (rust-lang/cargo#16403) - Document more services and permissions of the cargo team (rust-lang/cargo#16402) - fix(new): Improve quality of package name error messages (rust-lang/cargo#16398) - feat(log): add more log events (rust-lang/cargo#16390) - chore: bump to 0.95.0; update changelog (rust-lang/cargo#16395) - Cache submodule into git db (rust-lang/cargo#16246) r? ghost
Update cargo submodule 13 commits in e91b2baa632c0c7e84216c91ecfe107c37d887c1..3861f60f6b58f57524c0e7aab1f5c1ad83e35409 2025-12-13 16:29:21 +0000 to 2025-12-19 21:53:33 +0000 - fix(sbom): Don't set CARGO_SBOM_PATH when empty (like stable) (rust-lang/cargo#16419) - chore(ci): report-timings to upload HTML report (rust-lang/cargo#16416) - fix(report): get the same feature parity of `--timings` (rust-lang/cargo#16414) - fix(docs): Document the only possible values for DEBUG in build scripts (rust-lang/cargo#16413) - fix(log): emit fingeprint log also for no-rebuild unit (rust-lang/cargo#16408) - Fix link to `doc_cfg` docs in cargo book (rust-lang/cargo#16404) - Add best pratice for how to check in generated files (rust-lang/cargo#16405) - test: Use a larger default term width (rust-lang/cargo#16403) - Document more services and permissions of the cargo team (rust-lang/cargo#16402) - fix(new): Improve quality of package name error messages (rust-lang/cargo#16398) - feat(log): add more log events (rust-lang/cargo#16390) - chore: bump to 0.95.0; update changelog (rust-lang/cargo#16395) - Cache submodule into git db (rust-lang/cargo#16246) r? ghost
What does this PR try to resolve?
After this
cargo report timingshas 99.9% of the feature parity of what--timingsrenders.This adds some new fields to some events:
features: a list of enabled features for the unitrequested_units: a list of user-requested units. Not calliing this root units because we may have synthetic "super-root" units in the future (like doc-merge, post-processing, or user-defined units), which people may care less and want to focus on what they are actually building.requested_platforms: alist of target platforms the build is forThe only thing left is CPU usage, but I don't consider it is a necessary.
Part of #15844
How to test and review this PR?
Rendered HTML snapshot