feat: rebuild(-ish) rustdoc json for different version of same crates#16773
feat: rebuild(-ish) rustdoc json for different version of same crates#16773motorailgun wants to merge 2 commits intorust-lang:masterfrom
Conversation
|
r? @epage rustbot has assigned @epage. Use Why was this reviewer chosen?The reviewer was selected based on:
|
a1ef71b to
04024f4
Compare
be69519 to
2f32d53
Compare
|
Updated to use new directory layout. |
This comment has been minimized.
This comment has been minimized.
2f32d53 to
184bf76
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. |
| } | ||
|
|
||
| #[cargo_test(nightly, reason = "--output-format is unstable")] | ||
| fn cargo_rustdoc_json_should_output_to_target_dir() { |
There was a problem hiding this comment.
Can we add new tests first, and the fix and snapshots in the second commit? So that we can see the behavior change in the snapshot updates. See https://epage.github.io/dev/pr-style/#c-test.
There was a problem hiding this comment.
Ugh, sorry about my stupidness of forgetting atomic commits again... thank you for pointing out!
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
184bf76 to
939c055
Compare
939c055 to
79399ac
Compare
|
Note: updated PR message reflecting changes. |
| self.collect_tests_and_executables(unit)?; | ||
|
|
||
| // Uplift rustdoc | ||
| if is_rustdoc_json_output(&self) { |
There was a problem hiding this comment.
Sorry I didn't really review this carefully until this round.
I feel like this is the not the correct place to uplift. In Cargo we track whether a OutputFile will be uplifted through its hard_link. If we make rustdoc JSON as part of that, we'll get uplifting logic for free. We would probably get uplifting logic for free if we set it here:
Also, this seems to unconditionally uplift rustdoc JSON for every root unit. Not wrong but a bit error-prone as it assumes every root unit is rustdoc (which is true actually as output-format is available only in cargo rustdoc.
| let filename = format!("{crate_name}.json"); | ||
|
|
||
| let src_path = doc_json_dir.join(&filename); | ||
| copy(src_path, doc_dir.join(&filename))?; |
There was a problem hiding this comment.
We have cargo_util::paths::link_or_copy though we should leverage OutputFile::hardlink
| if !gctx.cli_unstable().unstable_options { | ||
| tracing::debug!("`unstable-options` is ignored, required -Zunstable-options flag"); | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
I wonder if we actually don't need this guard unstable_optionts here?
The place we set UserIntent::Doc { json: true } has already checked -Zunstable-options presence:
cargo/src/bin/cargo/commands/rustdoc.rs
Lines 56 to 72 in 35f8202
So that may imply we also don't need the is_rustdoc_json_output at all
Fix #16291.
What does this PR try to resolve?
As discussed in the linked issue,
cargo rustdoc --output-format=jsondoesn't rebuild for same crate name but with different versions. This is because current cargo implementation around rustdoc only specifies output paths/names by only its crate names, but not with version(hash) info.This PR forces new layout for json output, and uplift the file into backward-compatible place.
How to test and review this PR?
--output-format=json? (and stop uplifting)