-
Notifications
You must be signed in to change notification settings - Fork 2.9k
feat: rebuild(-ish) rustdoc json for different version of same crates #16773
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -7,11 +7,12 @@ use std::sync::{Arc, Mutex}; | |||
| use crate::core::PackageId; | ||||
| use crate::core::compiler::compilation::{self, UnitOutput}; | ||||
| use crate::core::compiler::locking::LockManager; | ||||
| use crate::core::compiler::rustdoc::is_rustdoc_json_output; | ||||
| use crate::core::compiler::{self, Unit, UserIntent, artifact}; | ||||
| use crate::util::cache_lock::CacheLockMode; | ||||
| use crate::util::errors::CargoResult; | ||||
| use anyhow::{Context as _, bail}; | ||||
| use cargo_util::paths; | ||||
| use cargo_util::paths::{self, copy}; | ||||
| use cargo_util_terminal::report::{Level, Message}; | ||||
| use filetime::FileTime; | ||||
| use itertools::Itertools; | ||||
|
|
@@ -235,6 +236,11 @@ impl<'a, 'gctx> BuildRunner<'a, 'gctx> { | |||
| for unit in &self.bcx.roots { | ||||
| self.collect_tests_and_executables(unit)?; | ||||
|
|
||||
| // Uplift rustdoc | ||||
| if is_rustdoc_json_output(&self) { | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
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 |
||||
| self.uplift_rustdoc(unit)?; | ||||
| } | ||||
|
|
||||
| // Collect information for `rustdoc --test`. | ||||
| if unit.mode.is_doc_test() { | ||||
| let mut unstable_opts = false; | ||||
|
|
@@ -787,4 +793,21 @@ impl<'a, 'gctx> BuildRunner<'a, 'gctx> { | |||
| .insert(unit.clone(), self.files().metadata(metadata_unit)); | ||||
| } | ||||
| } | ||||
|
|
||||
| /// Uplifts final json to <doc_dir>/<crate_name>.json (for backward compatibility) | ||||
| /// The final output is produced only after root unit is complete. | ||||
| // TODO: It would be better if we don't do uplifting when new build layout is specified, | ||||
| // but there is no way we can collect new out dir paths from ops. | ||||
| // Thus we here always do uplifting. | ||||
| fn uplift_rustdoc(&self, unit: &Unit) -> CargoResult<()> { | ||||
| let doc_dir = self.files().output_dir(unit); | ||||
| let doc_json_dir = self.files().out_dir_new_layout(unit); | ||||
| let crate_name = unit.target.crate_name(); | ||||
| let filename = format!("{crate_name}.json"); | ||||
|
|
||||
| let src_path = doc_json_dir.join(&filename); | ||||
| copy(src_path, doc_dir.join(&filename))?; | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have |
||||
|
|
||||
| Ok(()) | ||||
| } | ||||
| } | ||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -242,6 +242,12 @@ pub fn add_root_urls( | |||||||||||||||||||||||||||||||||||
| Ok(()) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| /// Checks whether JSON output should be enabled for rusdoc invocation | ||||||||||||||||||||||||||||||||||||
| pub fn is_rustdoc_json_output(build_runner: &BuildRunner<'_, '_>) -> bool { | ||||||||||||||||||||||||||||||||||||
| build_runner.bcx.build_config.intent.wants_doc_json_output() | ||||||||||||||||||||||||||||||||||||
| && build_runner.bcx.gctx.cli_unstable().unstable_options | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| /// Adds unstable flag [`--output-format`][1] to the given `rustdoc` | ||||||||||||||||||||||||||||||||||||
| /// invocation. This is for unstable feature `-Zunstable-features`. | ||||||||||||||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||||||||||||||
|
|
@@ -250,15 +256,13 @@ pub fn add_output_format( | |||||||||||||||||||||||||||||||||||
| build_runner: &BuildRunner<'_, '_>, | ||||||||||||||||||||||||||||||||||||
| rustdoc: &mut ProcessBuilder, | ||||||||||||||||||||||||||||||||||||
| ) -> CargoResult<()> { | ||||||||||||||||||||||||||||||||||||
| let gctx = build_runner.bcx.gctx; | ||||||||||||||||||||||||||||||||||||
| if !gctx.cli_unstable().unstable_options { | ||||||||||||||||||||||||||||||||||||
| tracing::debug!("`unstable-options` is ignored, required -Zunstable-options flag"); | ||||||||||||||||||||||||||||||||||||
| return Ok(()); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if we actually don't need this guard The place we set cargo/src/bin/cargo/commands/rustdoc.rs Lines 56 to 72 in 35f8202
So that may imply we also don't need the |
||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| if build_runner.bcx.build_config.intent.wants_doc_json_output() { | ||||||||||||||||||||||||||||||||||||
| if is_rustdoc_json_output(build_runner) { | ||||||||||||||||||||||||||||||||||||
| rustdoc.arg("-Zunstable-options"); | ||||||||||||||||||||||||||||||||||||
| rustdoc.arg("--output-format=json"); | ||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||
| if !build_runner.bcx.gctx.cli_unstable().unstable_options { | ||||||||||||||||||||||||||||||||||||
| tracing::debug!("`unstable-options` is ignored, required -Zunstable-options flag"); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| Ok(()) | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -501,6 +501,46 @@ fn cargo_doc_should_output_to_target_dir() { | |
| assert_exists(&docs_dir.join("foo/index.html")); | ||
| } | ||
|
|
||
| #[cargo_test(nightly, reason = "--output-format is unstable")] | ||
| fn cargo_rustdoc_json_should_output_to_target_dir() { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ugh, sorry about my stupidness of forgetting atomic commits again... thank you for pointing out!
This comment was marked as spam.
Sorry, something went wrong. |
||
| let p = project() | ||
| .file("src/lib.rs", "") | ||
| .file( | ||
| ".cargo/config.toml", | ||
| r#" | ||
| [build] | ||
| target-dir = "target-dir" | ||
| build-dir = "build-dir" | ||
| "#, | ||
| ) | ||
| .build(); | ||
|
|
||
| p.cargo("-Zbuild-dir-new-layout rustdoc -Zfine-grain-locking -Zunstable-options --output-format json") | ||
| .masquerade_as_nightly_cargo(&["new build-dir layout", "rustdoc-output-format"]) | ||
| .enable_mac_dsym() | ||
| .run(); | ||
|
|
||
| let docs_dir = p.root().join("target-dir/doc"); | ||
|
|
||
| assert_exists(&docs_dir); | ||
| assert_exists(&docs_dir.join("foo.json")); | ||
|
|
||
| p.root().join("build-dir").assert_build_dir_layout(str![ | ||
| r#" | ||
| [ROOT]/foo/build-dir/.rustc_info.json | ||
| [ROOT]/foo/build-dir/.rustdoc_fingerprint.json | ||
| [ROOT]/foo/build-dir/CACHEDIR.TAG | ||
| [ROOT]/foo/build-dir/debug/.cargo-build-lock | ||
| [ROOT]/foo/build-dir/debug/build/foo/[HASH]/.lock | ||
| [ROOT]/foo/build-dir/debug/build/foo/[HASH]/fingerprint/doc-lib-foo | ||
| [ROOT]/foo/build-dir/debug/build/foo/[HASH]/fingerprint/doc-lib-foo.json | ||
| [ROOT]/foo/build-dir/debug/build/foo/[HASH]/fingerprint/invoked.timestamp | ||
| [ROOT]/foo/build-dir/debug/build/foo/[HASH]/out/foo.json | ||
|
|
||
| "# | ||
| ]); | ||
| } | ||
|
|
||
| #[cargo_test] | ||
| fn cargo_package_should_build_in_build_dir_and_output_to_target_dir() { | ||
| let p = project() | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.