From c571558fd948f22bda9b91e541e72ec1cb189c80 Mon Sep 17 00:00:00 2001 From: jyn Date: Thu, 25 May 2023 13:31:54 -0500 Subject: [PATCH 1/8] Give a more helpful error when calling `cargo_crates_in_set` for an alias Before: ``` thread 'main' panicked at 'no entry found for key', builder.rs:110:30 ``` After: ``` thread 'main' panicked at 'missing crate for path library', check.rs:89:26 ``` --- src/bootstrap/builder.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/bootstrap/builder.rs b/src/bootstrap/builder.rs index 2fa445506bc4a..caab40fcd9dd0 100644 --- a/src/bootstrap/builder.rs +++ b/src/bootstrap/builder.rs @@ -103,11 +103,14 @@ impl RunConfig<'_> { } /// Return a list of crate names selected by `run.paths`. + #[track_caller] pub fn cargo_crates_in_set(&self) -> Interned> { let mut crates = Vec::new(); for krate in &self.paths { let path = krate.assert_single_path(); - let crate_name = self.builder.crate_paths[&path.path]; + let Some(crate_name) = self.builder.crate_paths.get(&path.path) else { + panic!("missing crate for path {}", path.path.display()) + }; crates.push(crate_name.to_string()); } INTERNER.intern_list(crates) From 564e3adfdfdd761a8fa94e5e33abd04ae5f14675 Mon Sep 17 00:00:00 2001 From: jyn Date: Wed, 24 May 2023 19:39:16 -0500 Subject: [PATCH 2/8] Allow checking individual crates This is useful for profiling metadata generation. This comes very close to removing all_krates, but doesn't quite - there's one last usage left in `doc`. --- src/bootstrap/check.rs | 77 ++++++++++++++++++++++++++++++++---------- 1 file changed, 60 insertions(+), 17 deletions(-) diff --git a/src/bootstrap/check.rs b/src/bootstrap/check.rs index b11be96cefe62..a4fcaa5e196ea 100644 --- a/src/bootstrap/check.rs +++ b/src/bootstrap/check.rs @@ -1,6 +1,6 @@ //! Implementation of compiling the compiler and standard library, in "check"-based modes. -use crate::builder::{Builder, Kind, RunConfig, ShouldRun, Step}; +use crate::builder::{crate_description, Builder, Kind, RunConfig, ShouldRun, Step}; use crate::cache::Interned; use crate::compile::{add_to_sysroot, run_cargo, rustc_cargo, rustc_cargo_env, std_cargo}; use crate::config::TargetSelection; @@ -12,6 +12,12 @@ use std::path::{Path, PathBuf}; #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)] pub struct Std { pub target: TargetSelection, + /// Whether to build only a subset of crates. + /// + /// This shouldn't be used from other steps; see the comment on [`compile::Rustc`]. + /// + /// [`compile::Rustc`]: crate::compile::Rustc + crates: Interned>, } /// Returns args for the subcommand itself (not for cargo) @@ -66,16 +72,24 @@ fn cargo_subcommand(kind: Kind) -> &'static str { } } +impl Std { + pub fn new(target: TargetSelection) -> Self { + Self { target, crates: INTERNER.intern_list(vec![]) } + } +} + impl Step for Std { type Output = (); const DEFAULT: bool = true; fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> { - run.all_krates("sysroot").path("library") + let crates = run.builder.in_tree_crates("sysroot", None); + run.crates(crates).path("library") } fn make_run(run: RunConfig<'_>) { - run.builder.ensure(Std { target: run.target }); + let crates = run.cargo_crates_in_set(); + run.builder.ensure(Std { target: run.target, crates }); } fn run(self, builder: &Builder<'_>) { @@ -97,7 +111,14 @@ impl Step for Std { cargo.arg("--lib"); } - let _guard = builder.msg_check("library artifacts", target); + for krate in &*self.crates { + cargo.arg("-p").arg(krate); + } + + let _guard = builder.msg_check( + format_args!("library artifacts{}", crate_description(&self.crates)), + target, + ); run_cargo( builder, cargo, @@ -117,7 +138,8 @@ impl Step for Std { } // don't run on std twice with x.py clippy - if builder.kind == Kind::Clippy { + // don't check test dependencies if we haven't built libtest + if builder.kind == Kind::Clippy || !self.crates.is_empty() { return; } @@ -147,8 +169,8 @@ impl Step for Std { // Explicitly pass -p for all dependencies krates -- this will force cargo // to also check the tests/benches/examples for these crates, rather // than just the leaf crate. - for krate in builder.in_tree_crates("test", Some(target)) { - cargo.arg("-p").arg(krate.name); + for krate in &*self.crates { + cargo.arg("-p").arg(krate); } let _guard = builder.msg_check("library test/bench/example targets", target); @@ -167,6 +189,22 @@ impl Step for Std { #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)] pub struct Rustc { pub target: TargetSelection, + /// Whether to build only a subset of crates. + /// + /// This shouldn't be used from other steps; see the comment on [`compile::Rustc`]. + /// + /// [`compile::Rustc`]: crate::compile::Rustc + crates: Interned>, +} + +impl Rustc { + pub fn new(target: TargetSelection, builder: &Builder<'_>) -> Self { + let mut crates = vec![]; + for krate in builder.in_tree_crates("rustc-main", None) { + crates.push(krate.name.to_string()); + } + Self { target, crates: INTERNER.intern_list(crates) } + } } impl Step for Rustc { @@ -175,11 +213,13 @@ impl Step for Rustc { const DEFAULT: bool = true; fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> { - run.all_krates("rustc-main").path("compiler") + let crates = run.builder.in_tree_crates("rustc-main", None); + run.crates(crates).path("compiler") } fn make_run(run: RunConfig<'_>) { - run.builder.ensure(Rustc { target: run.target }); + let crates = run.cargo_crates_in_set(); + run.builder.ensure(Rustc { target: run.target, crates }); } /// Builds the compiler. @@ -200,7 +240,7 @@ impl Step for Rustc { builder.ensure(crate::compile::Std::new(compiler, compiler.host)); builder.ensure(crate::compile::Std::new(compiler, target)); } else { - builder.ensure(Std { target }); + builder.ensure(Std::new(target)); } let mut cargo = builder.cargo( @@ -218,14 +258,17 @@ impl Step for Rustc { cargo.arg("--all-targets"); } - // Explicitly pass -p for all compiler krates -- this will force cargo + // Explicitly pass -p for all compiler crates -- this will force cargo // to also check the tests/benches/examples for these crates, rather // than just the leaf crate. - for krate in builder.in_tree_crates("rustc-main", Some(target)) { - cargo.arg("-p").arg(krate.name); + for krate in &*self.crates { + cargo.arg("-p").arg(krate); } - let _guard = builder.msg_check("compiler artifacts", target); + let _guard = builder.msg_check( + format_args!("compiler artifacts{}", crate_description(&self.crates)), + target, + ); run_cargo( builder, cargo, @@ -268,7 +311,7 @@ impl Step for CodegenBackend { let target = self.target; let backend = self.backend; - builder.ensure(Rustc { target }); + builder.ensure(Rustc::new(target, builder)); let mut cargo = builder.cargo( compiler, @@ -318,7 +361,7 @@ impl Step for RustAnalyzer { let compiler = builder.compiler(builder.top_stage, builder.config.build); let target = self.target; - builder.ensure(Std { target }); + builder.ensure(Std::new(target)); let mut cargo = prepare_tool_cargo( builder, @@ -386,7 +429,7 @@ macro_rules! tool_check_step { let compiler = builder.compiler(builder.top_stage, builder.config.build); let target = self.target; - builder.ensure(Rustc { target }); + builder.ensure(Rustc::new(target, builder)); let mut cargo = prepare_tool_cargo( builder, From 20372f1817e9498f00100c149b4f47fc8ba00329 Mon Sep 17 00:00:00 2001 From: jyn Date: Thu, 25 May 2023 13:37:24 -0500 Subject: [PATCH 3/8] Add a `make_run_crates` function and use it Rustc and Std This fixes the panic from the previous commit. --- src/bootstrap/check.rs | 8 +++++--- src/bootstrap/compile.rs | 19 ++++++++++++------- 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/src/bootstrap/check.rs b/src/bootstrap/check.rs index a4fcaa5e196ea..09835516f7bd3 100644 --- a/src/bootstrap/check.rs +++ b/src/bootstrap/check.rs @@ -2,7 +2,9 @@ use crate::builder::{crate_description, Builder, Kind, RunConfig, ShouldRun, Step}; use crate::cache::Interned; -use crate::compile::{add_to_sysroot, run_cargo, rustc_cargo, rustc_cargo_env, std_cargo}; +use crate::compile::{ + add_to_sysroot, make_run_crates, run_cargo, rustc_cargo, rustc_cargo_env, std_cargo, +}; use crate::config::TargetSelection; use crate::tool::{prepare_tool_cargo, SourceType}; use crate::INTERNER; @@ -88,7 +90,7 @@ impl Step for Std { } fn make_run(run: RunConfig<'_>) { - let crates = run.cargo_crates_in_set(); + let crates = make_run_crates(&run, "library"); run.builder.ensure(Std { target: run.target, crates }); } @@ -218,7 +220,7 @@ impl Step for Rustc { } fn make_run(run: RunConfig<'_>) { - let crates = run.cargo_crates_in_set(); + let crates = make_run_crates(&run, "compiler"); run.builder.ensure(Rustc { target: run.target, crates }); } diff --git a/src/bootstrap/compile.rs b/src/bootstrap/compile.rs index 33addb90da372..7b9c4c0f3b3a8 100644 --- a/src/bootstrap/compile.rs +++ b/src/bootstrap/compile.rs @@ -48,6 +48,17 @@ impl Std { } } +/// Given an `alias` selected by the `Step` and the paths passed on the command line, +/// return a list of the crates that should be built. +/// +/// Normally, people will pass *just* `library` if they pass it. +/// But it's possible (although strange) to pass something like `library std core`. +/// Build all crates anyway, as if they hadn't passed the other args. +pub(crate) fn make_run_crates(run: &RunConfig<'_>, alias: &str) -> Interned> { + let has_alias = run.paths.iter().any(|set| set.assert_single_path().path.ends_with(alias)); + if has_alias { Default::default() } else { run.cargo_crates_in_set() } +} + impl Step for Std { type Output = (); const DEFAULT: bool = true; @@ -62,16 +73,10 @@ impl Step for Std { } fn make_run(run: RunConfig<'_>) { - // Normally, people will pass *just* library if they pass it. - // But it's possible (although strange) to pass something like `library std core`. - // Build all crates anyway, as if they hadn't passed the other args. - let has_library = - run.paths.iter().any(|set| set.assert_single_path().path.ends_with("library")); - let crates = if has_library { Default::default() } else { run.cargo_crates_in_set() }; run.builder.ensure(Std { compiler: run.builder.compiler(run.builder.top_stage, run.build_triple()), target: run.target, - crates, + crates: make_run_crates(&run, "library"), }); } From cb4b7f631980e6f78a486be46ec653ad322ce12e Mon Sep 17 00:00:00 2001 From: jyn Date: Thu, 25 May 2023 13:33:43 -0500 Subject: [PATCH 4/8] Extend `msg` and `description` to work with any subcommand Previously `description` only supported `Testing` and `Benchmarking`, and `msg` gave weird results for `doc` (it would say `Docing`). --- src/bootstrap/builder.rs | 8 ++++++-- src/bootstrap/lib.rs | 4 ++-- src/bootstrap/test.rs | 2 +- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/bootstrap/builder.rs b/src/bootstrap/builder.rs index caab40fcd9dd0..b2e9ba144d6bd 100644 --- a/src/bootstrap/builder.rs +++ b/src/bootstrap/builder.rs @@ -644,12 +644,16 @@ impl Kind { } } - pub fn test_description(&self) -> &'static str { + pub fn description(&self) -> String { match self { Kind::Test => "Testing", Kind::Bench => "Benchmarking", - _ => panic!("not a test command: {}!", self.as_str()), + Kind::Doc => "Documenting", + Kind::Run => "Running", + Kind::Suggest => "Suggesting", + _ => return format!("{self:?}"), } + .to_owned() } } diff --git a/src/bootstrap/lib.rs b/src/bootstrap/lib.rs index fb76dffd07156..f7d30de67ebab 100644 --- a/src/bootstrap/lib.rs +++ b/src/bootstrap/lib.rs @@ -1020,8 +1020,8 @@ impl Build { host: impl Into>, target: impl Into>, ) -> Option { - let action = action.into(); - let msg = |fmt| format!("{action:?}ing stage{stage} {what}{fmt}"); + let action = action.into().description(); + let msg = |fmt| format!("{action} stage{stage} {what}{fmt}"); let msg = if let Some(target) = target.into() { let host = host.into().unwrap(); if host == target { diff --git a/src/bootstrap/test.rs b/src/bootstrap/test.rs index 44cd84be705ab..25941fc64c507 100644 --- a/src/bootstrap/test.rs +++ b/src/bootstrap/test.rs @@ -101,7 +101,7 @@ impl Step for CrateBootstrap { ); builder.info(&format!( "{} {} stage0 ({})", - builder.kind.test_description(), + builder.kind.description(), path, bootstrap_host, )); From 58e18ddf86e67a1210a60ab4a0ad5e1adfbbc819 Mon Sep 17 00:00:00 2001 From: jyn Date: Thu, 25 May 2023 13:39:10 -0500 Subject: [PATCH 5/8] Switch doc::{Std, Rustc} to `crate_or_deps` Previously they were using `all_krates` and various hacks to determine which crates to document. Switch them to `crate_or_deps` so `ShouldRun` tells them which crate to document instead of having to guess. This also makes a few other refactors: - Remove the now unused `all_krates`; new code should only use `crate_or_deps`. - Add tests for documenting Std - Remove the unnecessary `run_cargo_rustdoc_for` closure so that we only run cargo once - Give a more helpful error message when documenting a no_std target - Use `builder.msg` in the Steps instead of `builder.info` --- src/bootstrap/builder.rs | 19 ---- src/bootstrap/builder/tests.rs | 14 +++ src/bootstrap/dist.rs | 6 +- src/bootstrap/doc.rs | 173 +++++++++++++++------------------ src/bootstrap/test.rs | 12 +-- 5 files changed, 100 insertions(+), 124 deletions(-) diff --git a/src/bootstrap/builder.rs b/src/bootstrap/builder.rs index b2e9ba144d6bd..b16e34c0b935c 100644 --- a/src/bootstrap/builder.rs +++ b/src/bootstrap/builder.rs @@ -430,25 +430,6 @@ impl<'a> ShouldRun<'a> { } } - /// Indicates it should run if the command-line selects the given crate or - /// any of its (local) dependencies. - /// - /// Compared to `krate`, this treats the dependencies as aliases for the - /// same job. Generally it is preferred to use `krate`, and treat each - /// individual path separately. For example `./x.py test src/liballoc` - /// (which uses `krate`) will test just `liballoc`. However, `./x.py check - /// src/liballoc` (which uses `all_krates`) will check all of `libtest`. - /// `all_krates` should probably be removed at some point. - pub fn all_krates(mut self, name: &str) -> Self { - let mut set = BTreeSet::new(); - for krate in self.builder.in_tree_crates(name, None) { - let path = krate.local_path(self.builder); - set.insert(TaskPath { path, kind: Some(self.kind) }); - } - self.paths.insert(PathSet::Set(set)); - self - } - /// Indicates it should run if the command-line selects the given crate or /// any of its (local) dependencies. /// diff --git a/src/bootstrap/builder/tests.rs b/src/bootstrap/builder/tests.rs index edca8fe9b13af..d76b830b0e530 100644 --- a/src/bootstrap/builder/tests.rs +++ b/src/bootstrap/builder/tests.rs @@ -1,5 +1,6 @@ use super::*; use crate::config::{Config, DryRun, TargetSelection}; +use crate::doc::DocumentationFormat; use std::thread; fn configure(cmd: &str, host: &[&str], target: &[&str]) -> Config { @@ -66,6 +67,16 @@ macro_rules! std { }; } +macro_rules! doc_std { + ($host:ident => $target:ident, stage = $stage:literal) => { + doc::Std::new( + $stage, + TargetSelection::from_user(stringify!($target)), + DocumentationFormat::HTML, + ) + }; +} + macro_rules! rustc { ($host:ident => $target:ident, stage = $stage:literal) => { compile::Rustc::new( @@ -144,6 +155,9 @@ fn alias_and_path_for_library() { first(cache.all::()), &[std!(A => A, stage = 0), std!(A => A, stage = 1)] ); + + let mut cache = run_build(&["library".into(), "core".into()], configure("doc", &["A"], &["A"])); + assert_eq!(first(cache.all::()), &[doc_std!(A => A, stage = 0)]); } #[test] diff --git a/src/bootstrap/dist.rs b/src/bootstrap/dist.rs index b49845386da17..46fc5b80e99d4 100644 --- a/src/bootstrap/dist.rs +++ b/src/bootstrap/dist.rs @@ -106,11 +106,7 @@ impl Step for JsonDocs { /// Builds the `rust-docs-json` installer component. fn run(self, builder: &Builder<'_>) -> Option { let host = self.host; - builder.ensure(crate::doc::Std { - stage: builder.top_stage, - target: host, - format: DocumentationFormat::JSON, - }); + builder.ensure(crate::doc::Std::new(builder.top_stage, host, DocumentationFormat::JSON)); let dest = "share/doc/rust/json"; diff --git a/src/bootstrap/doc.rs b/src/bootstrap/doc.rs index b52c3b68cc4ff..357cd778b6c39 100644 --- a/src/bootstrap/doc.rs +++ b/src/bootstrap/doc.rs @@ -16,6 +16,7 @@ use crate::builder::crate_description; use crate::builder::{Builder, Compiler, Kind, RunConfig, ShouldRun, Step}; use crate::cache::{Interned, INTERNER}; use crate::compile; +use crate::compile::make_run_crates; use crate::config::{Config, TargetSelection}; use crate::tool::{self, prepare_tool_cargo, SourceType, Tool}; use crate::util::{symlink_dir, t, up_to_date}; @@ -87,15 +88,6 @@ book!( StyleGuide, "src/doc/style-guide", "style-guide"; ); -// "library/std" -> ["library", "std"] -// -// Used for deciding whether a particular step is one requested by the user on -// the `x.py doc` command line, which determines whether `--open` will open that -// page. -pub(crate) fn components_simplified(path: &PathBuf) -> Vec<&str> { - path.iter().map(|component| component.to_str().unwrap_or("???")).collect() -} - #[derive(Debug, Copy, Clone, Hash, PartialEq, Eq)] pub struct UnstableBook { target: TargetSelection, @@ -425,11 +417,18 @@ impl Step for SharedAssets { } } -#[derive(Debug, Copy, Clone, Hash, PartialEq, Eq)] +#[derive(Debug, Copy, Clone, Hash, PartialEq, Eq, PartialOrd, Ord)] pub struct Std { pub stage: u32, pub target: TargetSelection, pub format: DocumentationFormat, + crates: Interned>, +} + +impl Std { + pub(crate) fn new(stage: u32, target: TargetSelection, format: DocumentationFormat) -> Self { + Std { stage, target, format, crates: INTERNER.intern_list(vec![]) } + } } impl Step for Std { @@ -438,7 +437,7 @@ impl Step for Std { fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> { let builder = run.builder; - run.all_krates("sysroot").path("library").default_condition(builder.config.docs) + run.crate_or_deps("sysroot").path("library").default_condition(builder.config.docs) } fn make_run(run: RunConfig<'_>) { @@ -450,6 +449,7 @@ impl Step for Std { } else { DocumentationFormat::HTML }, + crates: make_run_crates(&run, "library"), }); } @@ -457,7 +457,7 @@ impl Step for Std { /// /// This will generate all documentation for the standard library and its /// dependencies. This is largely just a wrapper around `cargo doc`. - fn run(self, builder: &Builder<'_>) { + fn run(mut self, builder: &Builder<'_>) { let stage = self.stage; let target = self.target; let out = match self.format { @@ -487,25 +487,7 @@ impl Step for Std { extra_args.push(OsStr::new("--disable-minification")); } - let requested_crates = builder - .paths - .iter() - .map(components_simplified) - .filter_map(|path| { - if path.len() >= 2 && path.get(0) == Some(&"library") { - // single crate - Some(path[1].to_owned()) - } else if !path.is_empty() { - // ?? - Some(path[0].to_owned()) - } else { - // all library crates - None - } - }) - .collect::>(); - - doc_std(builder, self.format, stage, target, &out, &extra_args, &requested_crates); + doc_std(builder, self.format, stage, target, &out, &extra_args, &self.crates); // Don't open if the format is json if let DocumentationFormat::JSON = self.format { @@ -514,7 +496,11 @@ impl Step for Std { // Look for library/std, library/core etc in the `x.py doc` arguments and // open the corresponding rendered docs. - for requested_crate in requested_crates { + if self.crates.is_empty() { + self.crates = INTERNER.intern_list(vec!["library".to_owned()]); + }; + + for requested_crate in &*self.crates { if requested_crate == "library" { // For `x.py doc library --open`, open `std` by default. let index = out.join("std").join("index.html"); @@ -538,7 +524,7 @@ impl Step for Std { /// or remote link. const STD_PUBLIC_CRATES: [&str; 5] = ["core", "alloc", "std", "proc_macro", "test"]; -#[derive(Debug, Copy, Clone, Hash, PartialEq, Eq)] +#[derive(Debug, Copy, Clone, Hash, PartialEq, Eq, PartialOrd, Ord)] pub enum DocumentationFormat { HTML, JSON, @@ -566,21 +552,19 @@ fn doc_std( extra_args: &[&OsStr], requested_crates: &[String], ) { - builder.info(&format!( - "Documenting{} stage{} library ({}) in {} format", - crate_description(requested_crates), - stage, - target, - format.as_str() - )); if builder.no_std(target) == Some(true) { panic!( "building std documentation for no_std target {target} is not supported\n\ - Set `docs = false` in the config to disable documentation." + Set `docs = false` in the config to disable documentation, or pass `--exclude doc::library`." ); } + let compiler = builder.compiler(stage, builder.config.build); + let description = + format!("library{} in {} format", crate_description(&requested_crates), format.as_str()); + let _guard = builder.msg(Kind::Doc, stage, &description, compiler.host, target); + let target_doc_dir_name = if format == DocumentationFormat::JSON { "json-doc" } else { "doc" }; let target_dir = builder.stage_out(compiler, Mode::Std).join(target.triple).join(target_doc_dir_name); @@ -590,35 +574,27 @@ fn doc_std( // as a function parameter. let out_dir = target_dir.join(target.triple).join("doc"); - let run_cargo_rustdoc_for = |package: &str| { - let mut cargo = builder.cargo(compiler, Mode::Std, SourceType::InTree, target, "rustdoc"); - compile::std_cargo(builder, target, compiler.stage, &mut cargo); - cargo - .arg("--target-dir") - .arg(&*target_dir.to_string_lossy()) - .arg("-p") - .arg(package) - .arg("-Zskip-rustdoc-fingerprint") - .arg("--") - .arg("-Z") - .arg("unstable-options") - .arg("--resource-suffix") - .arg(&builder.version) - .args(extra_args); - if builder.config.library_docs_private_items { - cargo.arg("--document-private-items").arg("--document-hidden-items"); - } - builder.run(&mut cargo.into()); - }; + let mut cargo = builder.cargo(compiler, Mode::Std, SourceType::InTree, target, "rustdoc"); + compile::std_cargo(builder, target, compiler.stage, &mut cargo); + cargo.arg("--target-dir").arg(&*target_dir.to_string_lossy()).arg("-Zskip-rustdoc-fingerprint"); - for krate in STD_PUBLIC_CRATES { - run_cargo_rustdoc_for(krate); - if requested_crates.iter().any(|p| p == krate) { - // No need to document more of the libraries if we have the one we want. - break; - } + for krate in requested_crates { + cargo.arg("-p").arg(krate); } + cargo + .arg("--") + .arg("-Z") + .arg("unstable-options") + .arg("--resource-suffix") + .arg(&builder.version) + .args(extra_args); + + if builder.config.library_docs_private_items { + cargo.arg("--document-private-items").arg("--document-hidden-items"); + } + + builder.run(&mut cargo.into()); builder.cp_r(&out_dir, &out); } @@ -626,6 +602,28 @@ fn doc_std( pub struct Rustc { pub stage: u32, pub target: TargetSelection, + crates: Interned>, +} + +impl Rustc { + pub(crate) fn new(stage: u32, target: TargetSelection, builder: &Builder<'_>) -> Self { + // Find dependencies for top level crates. + let root_crates = vec![ + INTERNER.intern_str("rustc_driver"), + INTERNER.intern_str("rustc_codegen_llvm"), + INTERNER.intern_str("rustc_codegen_ssa"), + ]; + let crates: Vec<_> = root_crates + .iter() + .flat_map(|krate| { + builder + .in_tree_crates(krate, Some(target)) + .into_iter() + .map(|krate| krate.name.to_string()) + }) + .collect(); + Self { stage, target, crates: INTERNER.intern_list(crates) } + } } impl Step for Rustc { @@ -641,7 +639,11 @@ impl Step for Rustc { } fn make_run(run: RunConfig<'_>) { - run.builder.ensure(Rustc { stage: run.builder.top_stage, target: run.target }); + run.builder.ensure(Rustc { + stage: run.builder.top_stage, + target: run.target, + crates: make_run_crates(&run, "compiler"), + }); } /// Generates compiler documentation. @@ -654,15 +656,6 @@ impl Step for Rustc { let stage = self.stage; let target = self.target; - let paths = builder - .paths - .iter() - .filter(|path| { - let components = components_simplified(path); - components.len() >= 2 && components[0] == "compiler" - }) - .collect::>(); - // This is the intended out directory for compiler documentation. let out = builder.compiler_doc_out(target); t!(fs::create_dir_all(&out)); @@ -672,7 +665,13 @@ impl Step for Rustc { let compiler = builder.compiler(stage, builder.config.build); builder.ensure(compile::Std::new(compiler, builder.config.build)); - builder.info(&format!("Documenting stage{} compiler ({})", stage, target)); + let _guard = builder.msg( + Kind::Doc, + stage, + &format!("compiler{}", crate_description(&self.crates)), + compiler.host, + target, + ); // This uses a shared directory so that librustdoc documentation gets // correctly built and merged with the rustc documentation. This is @@ -710,22 +709,8 @@ impl Step for Rustc { cargo.rustdocflag("--extern-html-root-url"); cargo.rustdocflag("ena=https://docs.rs/ena/latest/"); - let root_crates = if paths.is_empty() { - vec![ - INTERNER.intern_str("rustc_driver"), - INTERNER.intern_str("rustc_codegen_llvm"), - INTERNER.intern_str("rustc_codegen_ssa"), - ] - } else { - paths.into_iter().map(|p| builder.crate_paths[p]).collect() - }; - // Find dependencies for top level crates. - let compiler_crates = root_crates.iter().flat_map(|krate| { - builder.in_tree_crates(krate, Some(target)).into_iter().map(|krate| krate.name) - }); - let mut to_open = None; - for krate in compiler_crates { + for krate in &*self.crates { // Create all crate output directories first to make sure rustdoc uses // relative links. // FIXME: Cargo should probably do this itself. @@ -785,7 +770,7 @@ macro_rules! tool_doc { if true $(&& $rustc_tool)? { // Build rustc docs so that we generate relative links. - builder.ensure(Rustc { stage, target }); + builder.ensure(Rustc::new(stage, target, builder)); // Rustdoc needs the rustc sysroot available to build. // FIXME: is there a way to only ensure `check::Rustc` here? Last time I tried it failed diff --git a/src/bootstrap/test.rs b/src/bootstrap/test.rs index 25941fc64c507..960abb31b2016 100644 --- a/src/bootstrap/test.rs +++ b/src/bootstrap/test.rs @@ -220,7 +220,7 @@ impl Step for HtmlCheck { } // Ensure that a few different kinds of documentation are available. builder.default_doc(&[]); - builder.ensure(crate::doc::Rustc { target: self.target, stage: builder.top_stage }); + builder.ensure(crate::doc::Rustc::new(builder.top_stage, self.target, builder)); try_run(builder, builder.tool_cmd(Tool::HtmlChecker).arg(builder.doc_out(self.target))); } @@ -886,11 +886,11 @@ impl Step for RustdocJSStd { command.arg("--test-file").arg(path); } } - builder.ensure(crate::doc::Std { - target: self.target, - stage: builder.top_stage, - format: DocumentationFormat::HTML, - }); + builder.ensure(crate::doc::Std::new( + builder.top_stage, + self.target, + DocumentationFormat::HTML, + )); builder.run(&mut command); } else { builder.info("No nodejs found, skipping \"tests/rustdoc-js-std\" tests"); From 3e765a7f719951a9c1b69a13487e9f817e5a03b1 Mon Sep 17 00:00:00 2001 From: jyn Date: Thu, 25 May 2023 13:52:30 -0500 Subject: [PATCH 6/8] Switch Steps from crates to crate_or_deps where possible and document why the single remaining place can't switch --- src/bootstrap/builder.rs | 2 ++ src/bootstrap/check.rs | 6 ++---- src/bootstrap/compile.rs | 2 ++ 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/bootstrap/builder.rs b/src/bootstrap/builder.rs index b16e34c0b935c..cf9ae4f08181c 100644 --- a/src/bootstrap/builder.rs +++ b/src/bootstrap/builder.rs @@ -442,6 +442,8 @@ impl<'a> ShouldRun<'a> { /// Indicates it should run if the command-line selects any of the given crates. /// /// `make_run` will be called a single time with all matching command-line paths. + /// + /// Prefer [`ShouldRun::crate_or_deps`] to this function where possible. pub(crate) fn crates(mut self, crates: Vec<&Crate>) -> Self { for krate in crates { let path = krate.local_path(self.builder); diff --git a/src/bootstrap/check.rs b/src/bootstrap/check.rs index 09835516f7bd3..f5a93854bf2c4 100644 --- a/src/bootstrap/check.rs +++ b/src/bootstrap/check.rs @@ -85,8 +85,7 @@ impl Step for Std { const DEFAULT: bool = true; fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> { - let crates = run.builder.in_tree_crates("sysroot", None); - run.crates(crates).path("library") + run.crate_or_deps("sysroot").path("library") } fn make_run(run: RunConfig<'_>) { @@ -215,8 +214,7 @@ impl Step for Rustc { const DEFAULT: bool = true; fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> { - let crates = run.builder.in_tree_crates("rustc-main", None); - run.crates(crates).path("compiler") + run.crate_or_deps("rustc-main").path("compiler") } fn make_run(run: RunConfig<'_>) { diff --git a/src/bootstrap/compile.rs b/src/bootstrap/compile.rs index 7b9c4c0f3b3a8..a7ebd018a8791 100644 --- a/src/bootstrap/compile.rs +++ b/src/bootstrap/compile.rs @@ -620,6 +620,8 @@ impl Step for Rustc { fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> { let mut crates = run.builder.in_tree_crates("rustc-main", None); for (i, krate) in crates.iter().enumerate() { + // We can't allow `build rustc` as an alias for this Step, because that's reserved by `Assemble`. + // Ideally Assemble would use `build compiler` instead, but that seems too confusing to be worth the breaking change. if krate.name == "rustc-main" { crates.swap_remove(i); break; From 71770d5e6e5c6d77777e7f9c86f7c9533882c449 Mon Sep 17 00:00:00 2001 From: jyn Date: Thu, 25 May 2023 14:00:55 -0500 Subject: [PATCH 7/8] Document `ShouldRun::paths` --- src/bootstrap/builder.rs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/bootstrap/builder.rs b/src/bootstrap/builder.rs index cf9ae4f08181c..30359e47e73be 100644 --- a/src/bootstrap/builder.rs +++ b/src/bootstrap/builder.rs @@ -473,7 +473,15 @@ impl<'a> ShouldRun<'a> { self.paths(&[path]) } - // multiple aliases for the same job + /// Multiple aliases for the same job. + /// + /// This differs from [`path`] in that multiple calls to path will end up calling `make_run` + /// multiple times, whereas a single call to `paths` will only ever generate a single call to + /// `paths`. + /// + /// This is analogous to `all_krates`, although `all_krates` is gone now. Prefer [`path`] where possible. + /// + /// [`path`]: ShouldRun::path pub fn paths(mut self, paths: &[&str]) -> Self { static SUBMODULES_PATHS: OnceCell> = OnceCell::new(); From c28ee603c8ddc2f171bee4ba02336fb6a3479010 Mon Sep 17 00:00:00 2001 From: jyn Date: Thu, 25 May 2023 15:14:56 -0500 Subject: [PATCH 8/8] Fix bugs in `doc` refactor - Switch from `cargo rustdoc` to `cargo doc` This allows passing `-p` to multiple packages. - Remove `OsStr` support It doesn't work with RUSTDOCFLAGS, and we don't support non-utf8 paths anyway. - Pass `-p std` for each crate in the standard library By default cargo only documents the top-level crate, which is `sysroot` and has no docs. --- src/bootstrap/doc.rs | 65 +++++++++++++++++++++++++++----------------- 1 file changed, 40 insertions(+), 25 deletions(-) diff --git a/src/bootstrap/doc.rs b/src/bootstrap/doc.rs index 357cd778b6c39..3de85c91516c4 100644 --- a/src/bootstrap/doc.rs +++ b/src/bootstrap/doc.rs @@ -7,7 +7,6 @@ //! Everything here is basically just a shim around calling either `rustbook` or //! `rustdoc`. -use std::ffi::OsStr; use std::fs; use std::io; use std::path::{Path, PathBuf}; @@ -471,20 +470,21 @@ impl Step for Std { builder.ensure(SharedAssets { target: self.target }); } - let index_page = builder.src.join("src/doc/index.md").into_os_string(); + let index_page = builder + .src + .join("src/doc/index.md") + .into_os_string() + .into_string() + .expect("non-utf8 paths are unsupported"); let mut extra_args = match self.format { - DocumentationFormat::HTML => vec![ - OsStr::new("--markdown-css"), - OsStr::new("rust.css"), - OsStr::new("--markdown-no-toc"), - OsStr::new("--index-page"), - &index_page, - ], - DocumentationFormat::JSON => vec![OsStr::new("--output-format"), OsStr::new("json")], + DocumentationFormat::HTML => { + vec!["--markdown-css", "rust.css", "--markdown-no-toc", "--index-page", &index_page] + } + DocumentationFormat::JSON => vec!["--output-format", "json"], }; if !builder.config.docs_minification { - extra_args.push(OsStr::new("--disable-minification")); + extra_args.push("--disable-minification"); } doc_std(builder, self.format, stage, target, &out, &extra_args, &self.crates); @@ -549,7 +549,7 @@ fn doc_std( stage: u32, target: TargetSelection, out: &Path, - extra_args: &[&OsStr], + extra_args: &[&str], requested_crates: &[String], ) { if builder.no_std(target) == Some(true) { @@ -574,24 +574,39 @@ fn doc_std( // as a function parameter. let out_dir = target_dir.join(target.triple).join("doc"); - let mut cargo = builder.cargo(compiler, Mode::Std, SourceType::InTree, target, "rustdoc"); + let mut cargo = builder.cargo(compiler, Mode::Std, SourceType::InTree, target, "doc"); compile::std_cargo(builder, target, compiler.stage, &mut cargo); - cargo.arg("--target-dir").arg(&*target_dir.to_string_lossy()).arg("-Zskip-rustdoc-fingerprint"); + cargo + .arg("--no-deps") + .arg("--target-dir") + .arg(&*target_dir.to_string_lossy()) + .arg("-Zskip-rustdoc-fingerprint") + .rustdocflag("-Z") + .rustdocflag("unstable-options") + .rustdocflag("--resource-suffix") + .rustdocflag(&builder.version); + for arg in extra_args { + cargo.rustdocflag(arg); + } - for krate in requested_crates { - cargo.arg("-p").arg(krate); + if builder.config.library_docs_private_items { + cargo.rustdocflag("--document-private-items").rustdocflag("--document-hidden-items"); } - cargo - .arg("--") - .arg("-Z") - .arg("unstable-options") - .arg("--resource-suffix") - .arg(&builder.version) - .args(extra_args); + // HACK: because we use `--manifest-path library/sysroot/Cargo.toml`, cargo thinks we only want to document that specific crate, not its dependencies. + // Override its default. + let built_crates = if requested_crates.is_empty() { + builder + .in_tree_crates("sysroot", None) + .into_iter() + .map(|krate| krate.name.to_string()) + .collect() + } else { + requested_crates.to_vec() + }; - if builder.config.library_docs_private_items { - cargo.arg("--document-private-items").arg("--document-hidden-items"); + for krate in built_crates { + cargo.arg("-p").arg(krate); } builder.run(&mut cargo.into());