From c26ed6357fb6e2437e8535f9661fea85dbb70b1e Mon Sep 17 00:00:00 2001 From: Will Crichton Date: Tue, 22 Nov 2022 15:19:18 -0600 Subject: [PATCH] Add doc comments to explain scrape-examples feature --- src/cargo/core/compiler/context/mod.rs | 3 + src/cargo/core/compiler/job_queue.rs | 20 ++++-- src/cargo/core/compiler/mod.rs | 3 +- src/cargo/core/compiler/rustdoc.rs | 2 + src/cargo/core/manifest.rs | 3 +- src/cargo/ops/cargo_compile/compile_filter.rs | 67 ++++++++++++++++++- src/cargo/ops/cargo_compile/mod.rs | 53 +-------------- tests/testsuite/docscrape.rs | 20 +++--- 8 files changed, 97 insertions(+), 74 deletions(-) diff --git a/src/cargo/core/compiler/context/mod.rs b/src/cargo/core/compiler/context/mod.rs index 1c3f3caea..08c86a5b6 100644 --- a/src/cargo/core/compiler/context/mod.rs +++ b/src/cargo/core/compiler/context/mod.rs @@ -78,6 +78,9 @@ pub struct Context<'a, 'cfg> { /// See Context::find_metadata_units for more details. pub metadata_for_doc_units: HashMap, + /// Set of metadata of Docscrape units that fail before completion, e.g. + /// because the target has a type error. This is in an Arc> + /// because it is continuously updated as the job progresses. pub failed_scrape_units: Arc>>, } diff --git a/src/cargo/core/compiler/job_queue.rs b/src/cargo/core/compiler/job_queue.rs index d85755a51..b9ca2249a 100644 --- a/src/cargo/core/compiler/job_queue.rs +++ b/src/cargo/core/compiler/job_queue.rs @@ -348,23 +348,29 @@ enum Message { BuildPlanMsg(String, ProcessBuilder, Arc>), Stdout(String), Stderr(String), + + // This is for general stderr output from subprocesses Diagnostic { id: JobId, level: String, diag: String, fixable: bool, }, - // This is distinct from Diagnostic because it gets emitted through - // a different Shell pathway - Warning { - id: JobId, - warning: String, - }, + // This handles duplicate output that is suppressed, for showing + // only a count of duplicate messages instead WarningCount { id: JobId, emitted: bool, fixable: bool, }, + // This is for warnings generated by Cargo's interpretation of the + // subprocess output, e.g. scrape-examples prints a warning if a + // unit fails to be scraped + Warning { + id: JobId, + warning: String, + }, + FixDiagnostic(diagnostic_server::Message), Token(io::Result), Finish(JobId, Artifact, CargoResult<()>), @@ -412,6 +418,7 @@ impl<'a, 'cfg> JobState<'a, 'cfg> { Ok(()) } + /// See [`Message::Diagnostic`] and [`Message::WarningCount`]. pub fn emit_diag(&self, level: String, diag: String, fixable: bool) -> CargoResult<()> { if let Some(dedupe) = self.output { let emitted = dedupe.emit_diag(&diag)?; @@ -433,6 +440,7 @@ impl<'a, 'cfg> JobState<'a, 'cfg> { Ok(()) } + /// See [`Message::Warning`]. pub fn warning(&self, warning: String) -> CargoResult<()> { self.messages.push_bounded(Message::Warning { id: self.id, diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index 809165ef0..d10701510 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/src/cargo/core/compiler/mod.rs @@ -654,8 +654,7 @@ fn rustdoc(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult { rustdoc.arg("-C").arg(format!("metadata={}", metadata)); let scrape_output_path = |unit: &Unit| -> CargoResult { - cx.outputs(unit) - .map(|outputs| outputs[0].path.to_path_buf()) + cx.outputs(unit).map(|outputs| outputs[0].path.clone()) }; if unit.mode.is_doc_scrape() { diff --git a/src/cargo/core/compiler/rustdoc.rs b/src/cargo/core/compiler/rustdoc.rs index 9899e40ac..70e15c62c 100644 --- a/src/cargo/core/compiler/rustdoc.rs +++ b/src/cargo/core/compiler/rustdoc.rs @@ -191,6 +191,8 @@ pub fn add_root_urls( Ok(()) } +/// Indicates whether a target should have examples scraped from it +/// by rustdoc. Configured within Cargo.toml. #[derive(Clone, Hash, PartialEq, Eq, PartialOrd, Ord, Debug, Copy)] pub enum RustdocScrapeExamples { Enabled, diff --git a/src/cargo/core/manifest.rs b/src/cargo/core/manifest.rs index ee7ef1839..c37dd1cb4 100644 --- a/src/cargo/core/manifest.rs +++ b/src/cargo/core/manifest.rs @@ -12,6 +12,7 @@ use serde::Serialize; use toml_edit::easy as toml; use url::Url; +use crate::core::compiler::rustdoc::RustdocScrapeExamples; use crate::core::compiler::{CompileKind, CrateType}; use crate::core::resolver::ResolveBehavior; use crate::core::{Dependency, PackageId, PackageIdSpec, SourceId, Summary}; @@ -21,8 +22,6 @@ use crate::util::interning::InternedString; use crate::util::toml::{TomlManifest, TomlProfiles}; use crate::util::{short_hash, Config, Filesystem}; -use super::compiler::rustdoc::RustdocScrapeExamples; - pub enum EitherManifest { Real(Manifest), Virtual(VirtualManifest), diff --git a/src/cargo/ops/cargo_compile/compile_filter.rs b/src/cargo/ops/cargo_compile/compile_filter.rs index 45bc87916..a537a9a8a 100644 --- a/src/cargo/ops/cargo_compile/compile_filter.rs +++ b/src/cargo/ops/cargo_compile/compile_filter.rs @@ -1,7 +1,9 @@ //! Filters and their rules to select which Cargo targets will be built. use crate::core::compiler::CompileMode; -use crate::core::{Target, TargetKind}; +use crate::core::dependency::DepKind; +use crate::core::resolver::HasDevUnits; +use crate::core::{Package, Target, TargetKind}; use crate::util::restricted_names::is_glob_pattern; #[derive(Debug, PartialEq, Eq, Clone)] @@ -299,4 +301,67 @@ impl CompileFilter { } } } + + /// Generate a CompileFilter that represents the maximal set of targets + /// that should be considered for scraped examples. + pub(super) fn refine_for_docscrape( + &self, + to_builds: &[&Package], + has_dev_units: HasDevUnits, + ) -> CompileFilter { + // In general, the goal is to scrape examples from (a) whatever targets + // the user is documenting, and (b) Example targets. However, if the user + // is documenting a library with dev-dependencies, those dev-deps are not + // needed for the library, while dev-deps are needed for the examples. + // + // If scrape-examples caused `cargo doc` to start requiring dev-deps, this + // would be a breaking change to crates whose dev-deps don't compile. + // Therefore we ONLY want to scrape Example targets if either: + // (1) No package has dev-dependencies, so this is a moot issue, OR + // (2) The provided CompileFilter requires dev-dependencies anyway. + // + // The next two variables represent these two conditions. + + let no_pkg_has_dev_deps = to_builds.iter().all(|pkg| { + pkg.summary() + .dependencies() + .iter() + .all(|dep| !matches!(dep.kind(), DepKind::Development)) + }); + + let reqs_dev_deps = matches!(has_dev_units, HasDevUnits::Yes); + + let example_filter = if no_pkg_has_dev_deps || reqs_dev_deps { + FilterRule::All + } else { + FilterRule::none() + }; + + match self { + CompileFilter::Only { + all_targets, + lib, + bins, + tests, + benches, + .. + } => CompileFilter::Only { + all_targets: *all_targets, + lib: lib.clone(), + bins: bins.clone(), + examples: example_filter, + tests: tests.clone(), + benches: benches.clone(), + }, + + CompileFilter::Default { .. } => CompileFilter::Only { + all_targets: false, + lib: LibRule::Default, + bins: FilterRule::none(), + examples: example_filter, + tests: FilterRule::none(), + benches: FilterRule::none(), + }, + } + } } diff --git a/src/cargo/ops/cargo_compile/mod.rs b/src/cargo/ops/cargo_compile/mod.rs index 92a25f47c..622255f89 100644 --- a/src/cargo/ops/cargo_compile/mod.rs +++ b/src/cargo/ops/cargo_compile/mod.rs @@ -41,7 +41,6 @@ use crate::core::compiler::{standard_lib, CrateType, TargetInfo}; use crate::core::compiler::{BuildConfig, BuildContext, Compilation, Context}; use crate::core::compiler::{CompileKind, CompileMode, CompileTarget, RustcTargetData, Unit}; use crate::core::compiler::{DefaultExecutor, Executor, UnitInterner}; -use crate::core::dependency::DepKind; use crate::core::profiles::{Profiles, UnitFor}; use crate::core::resolver::features::{self, CliFeatures, FeaturesFor}; use crate::core::resolver::{HasDevUnits, Resolve}; @@ -370,7 +369,7 @@ pub fn create_bcx<'a, 'cfg>( let should_scrape = build_config.mode.is_doc() && config.cli_unstable().rustdoc_scrape_examples; let mut scrape_units = if should_scrape { - let scrape_filter = filter_for_scrape_units(&to_builds, has_dev_units, filter); + let scrape_filter = filter.refine_for_docscrape(&to_builds, has_dev_units); let all_units = generate_targets( ws, &to_builds, @@ -567,56 +566,6 @@ pub fn create_bcx<'a, 'cfg>( Ok(bcx) } -/// Generate a CompileFilter that represents the maximal set of targets that should be -/// considered for scraping. Should be a subset of the CLI-provided CompileFilter. -fn filter_for_scrape_units( - to_builds: &[&Package], - has_dev_units: HasDevUnits, - filter: &CompileFilter, -) -> CompileFilter { - let no_pkg_has_dev_deps = to_builds.iter().all(|pkg| { - pkg.summary() - .dependencies() - .iter() - .all(|dep| !matches!(dep.kind(), DepKind::Development)) - }); - - // We are allowed to include examples ONLY if they cannot possibly require dev dependencies, - // or if dev dependencies are already required anyway due to the user's configuration. - let example_filter = if matches!(has_dev_units, HasDevUnits::Yes) || no_pkg_has_dev_deps { - FilterRule::All - } else { - FilterRule::none() - }; - - match filter { - CompileFilter::Only { - all_targets, - lib, - bins, - tests, - benches, - .. - } => CompileFilter::Only { - all_targets: *all_targets, - lib: lib.clone(), - bins: bins.clone(), - examples: example_filter, - tests: tests.clone(), - benches: benches.clone(), - }, - - CompileFilter::Default { .. } => CompileFilter::Only { - all_targets: false, - lib: LibRule::Default, - bins: FilterRule::none(), - examples: example_filter, - tests: FilterRule::none(), - benches: FilterRule::none(), - }, - } -} - /// A proposed target. /// /// Proposed targets are later filtered into actual `Unit`s based on whether or diff --git a/tests/testsuite/docscrape.rs b/tests/testsuite/docscrape.rs index d12b8f7ec..0a045d047 100644 --- a/tests/testsuite/docscrape.rs +++ b/tests/testsuite/docscrape.rs @@ -1,6 +1,5 @@ -//! Tests for the `cargo doc` command. +//! Tests for the `cargo doc` command with `-Zrustdoc-scrape-examples`. -use cargo_test_support::is_nightly; use cargo_test_support::project; #[cargo_test(nightly, reason = "rustdoc scrape examples flags are unstable")] @@ -41,11 +40,6 @@ fn basic() { #[cargo_test(nightly, reason = "rustdoc scrape examples flags are unstable")] fn avoid_build_script_cycle() { - if !is_nightly() { - // -Z rustdoc-scrape-examples is unstable - return; - } - let p = project() // package with build dependency .file( @@ -81,7 +75,7 @@ fn avoid_build_script_cycle() { .file("bar/build.rs", "fn main(){}") .build(); - p.cargo("doc --all -Zunstable-options -Zrustdoc-scrape-examples") + p.cargo("doc --workspace -Zunstable-options -Zrustdoc-scrape-examples") .masquerade_as_nightly_cargo(&["rustdoc-scrape-examples"]) .run(); } @@ -138,7 +132,7 @@ fn complex_reverse_dependencies() { .file("b/src/lib.rs", "") .build(); - p.cargo("doc --all --examples -Zunstable-options -Zrustdoc-scrape-examples") + p.cargo("doc --workspace --examples -Zunstable-options -Zrustdoc-scrape-examples") .masquerade_as_nightly_cargo(&["rustdoc-scrape-examples"]) .run(); } @@ -208,7 +202,7 @@ fn configure_target() { } #[cargo_test(nightly, reason = "rustdoc scrape examples flags are unstable")] -fn configure_profile() { +fn configure_profile_issue_10500() { let p = project() .file( "Cargo.toml", @@ -277,7 +271,7 @@ fn issue_10545() { .file("b/src/lib.rs", "") .build(); - p.cargo("doc --all -Zunstable-options -Zrustdoc-scrape-examples") + p.cargo("doc --workspace -Zunstable-options -Zrustdoc-scrape-examples") .masquerade_as_nightly_cargo(&["rustdoc-scrape-examples"]) .run(); } @@ -412,6 +406,10 @@ error: expected one of `!` or `::`, found `NOT` #[cargo_test(nightly, reason = "rustdoc scrape examples flags are unstable")] fn no_scrape_with_dev_deps() { + // Tests that a crate with dev-dependencies does not have its examples + // scraped unless explicitly prompted to check them. See + // `CompileFilter::refine_for_docscrape` for details on why. + let p = project() .file( "Cargo.toml",