From 5fa6ede9d4d42c9b656fdcb67fb853958fcc4244 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Thu, 25 Jul 2024 15:39:27 -0700 Subject: [PATCH 01/17] Remove comment about bootstrap.py handling submodules bootstrap.py handling of submodules was removed in https://github.com/rust-lang/rust/pull/97513. --- src/bootstrap/src/core/build_steps/tool.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/tool.rs b/src/bootstrap/src/core/build_steps/tool.rs index 087df2f8a88..66fcafb1c52 100644 --- a/src/bootstrap/src/core/build_steps/tool.rs +++ b/src/bootstrap/src/core/build_steps/tool.rs @@ -1087,8 +1087,6 @@ fn run(mut $sel, $builder: &Builder<'_>) -> PathBuf { // NOTE: tools need to be also added to `Builder::get_step_descriptions` in `builder.rs` // to make `./x.py build ` work. -// NOTE: Most submodule updates for tools are handled by bootstrap.py, since they're needed just to -// invoke Cargo to build bootstrap. See the comment there for more details. tool_extended!((self, builder), Cargofmt, "src/tools/rustfmt", "cargo-fmt", stable=true; CargoClippy, "src/tools/clippy", "cargo-clippy", stable=true; From 6642f8dcfcd44d7d4723c002b204e22b3255d70e Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Thu, 25 Jul 2024 16:10:54 -0700 Subject: [PATCH 02/17] Remove `pub` from update_existing_submodules This is not used anywhere outside this module. --- src/bootstrap/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bootstrap/src/lib.rs b/src/bootstrap/src/lib.rs index 79bea50c626..e461105aba2 100644 --- a/src/bootstrap/src/lib.rs +++ b/src/bootstrap/src/lib.rs @@ -582,7 +582,7 @@ pub(crate) fn update_submodule(&self, relative_path: &Path) { /// If any submodule has been initialized already, sync it unconditionally. /// This avoids contributors checking in a submodule change by accident. - pub fn update_existing_submodules(&self) { + fn update_existing_submodules(&self) { // Avoid running git when there isn't a git checkout. if !self.config.submodules(self.rust_info()) { return; From 2c6222b3f290ebb9b818253e8f0dc6050348a859 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Thu, 25 Jul 2024 16:13:01 -0700 Subject: [PATCH 03/17] Clarify comment on update_existing_submodules This felt like an important point to me. --- src/bootstrap/src/lib.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/bootstrap/src/lib.rs b/src/bootstrap/src/lib.rs index e461105aba2..ab640bf39aa 100644 --- a/src/bootstrap/src/lib.rs +++ b/src/bootstrap/src/lib.rs @@ -583,7 +583,8 @@ pub(crate) fn update_submodule(&self, relative_path: &Path) { /// If any submodule has been initialized already, sync it unconditionally. /// This avoids contributors checking in a submodule change by accident. fn update_existing_submodules(&self) { - // Avoid running git when there isn't a git checkout. + // Avoid running git when there isn't a git checkout, or the user has + // explicitly disabled submodules in `config.toml`. if !self.config.submodules(self.rust_info()) { return; } From 922fdd84627aa40237ca7adab07719fd98d4378b Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Thu, 25 Jul 2024 16:15:59 -0700 Subject: [PATCH 04/17] Remove argument from the submodules method The argument was not necessary, since it was only ever passed one value that exists in the config itself. --- src/bootstrap/src/core/config/config.rs | 7 +++++-- src/bootstrap/src/lib.rs | 6 +++--- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/bootstrap/src/core/config/config.rs b/src/bootstrap/src/core/config/config.rs index e32288e2caf..3435225af7b 100644 --- a/src/bootstrap/src/core/config/config.rs +++ b/src/bootstrap/src/core/config/config.rs @@ -2404,8 +2404,11 @@ pub fn split_debuginfo(&self, target: TargetSelection) -> SplitDebuginfo { .unwrap_or_else(|| SplitDebuginfo::default_for_platform(target)) } - pub fn submodules(&self, rust_info: &GitInfo) -> bool { - self.submodules.unwrap_or(rust_info.is_managed_git_subrepository()) + /// Returns whether or not submodules should be managed by bootstrap. + pub fn submodules(&self) -> bool { + // If not specified in config, the default is to only manage + // submodules if we're currently inside a git repository. + self.submodules.unwrap_or(self.rust_info.is_managed_git_subrepository()) } pub fn codegen_backends(&self, target: TargetSelection) -> &[String] { diff --git a/src/bootstrap/src/lib.rs b/src/bootstrap/src/lib.rs index ab640bf39aa..e8f412d785d 100644 --- a/src/bootstrap/src/lib.rs +++ b/src/bootstrap/src/lib.rs @@ -475,7 +475,7 @@ pub fn new(mut config: Config) -> Build { /// /// `relative_path` should be relative to the root of the git repository, not an absolute path. pub(crate) fn update_submodule(&self, relative_path: &Path) { - if !self.config.submodules(self.rust_info()) { + if !self.config.submodules() { return; } @@ -585,7 +585,7 @@ pub(crate) fn update_submodule(&self, relative_path: &Path) { fn update_existing_submodules(&self) { // Avoid running git when there isn't a git checkout, or the user has // explicitly disabled submodules in `config.toml`. - if !self.config.submodules(self.rust_info()) { + if !self.config.submodules() { return; } let output = helpers::git(Some(&self.src)) @@ -609,7 +609,7 @@ fn update_existing_submodules(&self) { /// Updates the given submodule only if it's initialized already; nothing happens otherwise. pub fn update_existing_submodule(&self, submodule: &Path) { // Avoid running git when there isn't a git checkout. - if !self.config.submodules(self.rust_info()) { + if !self.config.submodules() { return; } From 6a449d97fe01a5c34f606b082d75d553b7cb3f80 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Thu, 25 Jul 2024 16:18:46 -0700 Subject: [PATCH 05/17] Remove outdated comment about update_submodule Although its origins were in bootstrap.py, that code in bootstrap.py no longer exists since it was removed. --- src/bootstrap/src/lib.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/bootstrap/src/lib.rs b/src/bootstrap/src/lib.rs index e8f412d785d..0ce50ccaf3f 100644 --- a/src/bootstrap/src/lib.rs +++ b/src/bootstrap/src/lib.rs @@ -470,7 +470,6 @@ pub fn new(mut config: Config) -> Build { build } - // modified from `check_submodule` and `update_submodule` in bootstrap.py /// Given a path to the directory of a submodule, update it. /// /// `relative_path` should be relative to the root of the git repository, not an absolute path. From ee75f24945a4e7d614b25c89d70b3eb88dc20ccd Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Thu, 25 Jul 2024 16:26:21 -0700 Subject: [PATCH 06/17] Fix rustbook submodule update location I put this submodule update in the entirely wrong location. I put it in the `RustcBook` step (for generating src/doc/rustc), when it really should exist for all steps that use the `Rustbook` tool. --- src/bootstrap/src/core/build_steps/doc.rs | 6 ------ src/bootstrap/src/core/build_steps/tool.rs | 12 +++++++++++- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/doc.rs b/src/bootstrap/src/core/build_steps/doc.rs index d8204ea00f7..633e66afe59 100644 --- a/src/bootstrap/src/core/build_steps/doc.rs +++ b/src/bootstrap/src/core/build_steps/doc.rs @@ -1172,12 +1172,6 @@ fn make_run(run: RunConfig<'_>) { /// in the "md-doc" directory in the build output directory. Then /// "rustbook" is used to convert it to HTML. fn run(self, builder: &Builder<'_>) { - // These submodules are required to be checked out to build rustbook - // because they have Cargo dependencies that are needed. - #[allow(clippy::single_element_loop)] // This will change soon. - for path in ["src/doc/book"] { - builder.update_submodule(Path::new(path)); - } let out_base = builder.md_doc_out(self.target).join("rustc"); t!(fs::create_dir_all(&out_base)); let out_listing = out_base.join("src/lints"); diff --git a/src/bootstrap/src/core/build_steps/tool.rs b/src/bootstrap/src/core/build_steps/tool.rs index 66fcafb1c52..d5fd3301b92 100644 --- a/src/bootstrap/src/core/build_steps/tool.rs +++ b/src/bootstrap/src/core/build_steps/tool.rs @@ -241,6 +241,7 @@ macro_rules! bootstrap_tool { $(,is_external_tool = $external:expr)* $(,is_unstable_tool = $unstable:expr)* $(,allow_features = $allow_features:expr)? + $(,submodules = $submodules:expr)? ; )+) => { #[derive(PartialEq, Eq, Clone)] @@ -287,6 +288,11 @@ fn make_run(run: RunConfig<'_>) { } fn run(self, builder: &Builder<'_>) -> PathBuf { + $( + for submodule in $submodules { + builder.update_submodule(Path::new(submodule)); + } + )* builder.ensure(ToolBuild { compiler: self.compiler, target: self.target, @@ -314,7 +320,7 @@ fn run(self, builder: &Builder<'_>) -> PathBuf { } bootstrap_tool!( - Rustbook, "src/tools/rustbook", "rustbook"; + Rustbook, "src/tools/rustbook", "rustbook", submodules = SUBMODULES_FOR_RUSTBOOK; UnstableBookGen, "src/tools/unstable-book-gen", "unstable-book-gen"; Tidy, "src/tools/tidy", "tidy"; Linkchecker, "src/tools/linkchecker", "linkchecker"; @@ -340,6 +346,10 @@ fn run(self, builder: &Builder<'_>) -> PathBuf { WasmComponentLd, "src/tools/wasm-component-ld", "wasm-component-ld", is_unstable_tool = true, allow_features = "min_specialization"; ); +/// These are the submodules that are required for rustbook to work due to +/// depending on mdbook plugins. +pub static SUBMODULES_FOR_RUSTBOOK: &[&str] = &["src/doc/book"]; + #[derive(Debug, Clone, Hash, PartialEq, Eq)] pub struct OptimizedDist { pub compiler: Compiler, From bbe9056c2a68495bf4139a358745d8ca4c398576 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Thu, 25 Jul 2024 16:36:09 -0700 Subject: [PATCH 07/17] Add require_and_update_submodule to ensure submodules exist This adds a new method `require_and_update_submodule` to replace `update_submodule`. This new method will generate an error if the submodule doesn't actually exist. This replaces some ad-hoc checks that were performing this function. This helps ensure that a good error message is always displayed. This also adds require_and_update_all_submodules which does this for all submodules. Ideally this should not have any change other than better error messages when submodules are missing. --- src/bootstrap/src/core/build_steps/check.rs | 4 +- src/bootstrap/src/core/build_steps/clippy.rs | 4 +- src/bootstrap/src/core/build_steps/compile.rs | 21 +++++---- src/bootstrap/src/core/build_steps/dist.rs | 7 +-- src/bootstrap/src/core/build_steps/doc.rs | 24 ++++------ src/bootstrap/src/core/build_steps/llvm.rs | 6 +-- src/bootstrap/src/core/build_steps/test.rs | 6 +-- src/bootstrap/src/core/build_steps/tool.rs | 10 ++-- src/bootstrap/src/core/build_steps/vendor.rs | 6 +-- src/bootstrap/src/core/builder/tests.rs | 2 +- src/bootstrap/src/lib.rs | 47 ++++++++++++++++++- 11 files changed, 87 insertions(+), 50 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/check.rs b/src/bootstrap/src/core/build_steps/check.rs index ed5b9edc86d..5b720ffbc76 100644 --- a/src/bootstrap/src/core/build_steps/check.rs +++ b/src/bootstrap/src/core/build_steps/check.rs @@ -9,7 +9,7 @@ }; use crate::core::config::TargetSelection; use crate::{Compiler, Mode, Subcommand}; -use std::path::{Path, PathBuf}; +use std::path::PathBuf; pub fn cargo_subcommand(kind: Kind) -> &'static str { match kind { @@ -52,7 +52,7 @@ fn make_run(run: RunConfig<'_>) { } fn run(self, builder: &Builder<'_>) { - builder.update_submodule(&Path::new("library").join("stdarch")); + builder.require_and_update_submodule("library/stdarch", None); let target = self.target; let compiler = builder.compiler(builder.top_stage, builder.config.build); diff --git a/src/bootstrap/src/core/build_steps/clippy.rs b/src/bootstrap/src/core/build_steps/clippy.rs index ee7fb368a8c..add28f858ef 100644 --- a/src/bootstrap/src/core/build_steps/clippy.rs +++ b/src/bootstrap/src/core/build_steps/clippy.rs @@ -1,7 +1,5 @@ //! Implementation of running clippy on the compiler, standard library and various tools. -use std::path::Path; - use crate::builder::Builder; use crate::builder::ShouldRun; use crate::core::builder; @@ -127,7 +125,7 @@ fn make_run(run: RunConfig<'_>) { } fn run(self, builder: &Builder<'_>) { - builder.update_submodule(&Path::new("library").join("stdarch")); + builder.require_and_update_submodule("library/stdarch", None); let target = self.target; let compiler = builder.compiler(builder.top_stage, builder.config.build); diff --git a/src/bootstrap/src/core/build_steps/compile.rs b/src/bootstrap/src/core/build_steps/compile.rs index 9bbebf9b870..1efa3c8ada3 100644 --- a/src/bootstrap/src/core/build_steps/compile.rs +++ b/src/bootstrap/src/core/build_steps/compile.rs @@ -182,11 +182,14 @@ fn run(self, builder: &Builder<'_>) { return; } - builder.update_submodule(&Path::new("library").join("stdarch")); + builder.require_and_update_submodule("library/stdarch", None); // Profiler information requires LLVM's compiler-rt if builder.config.profiler { - builder.update_submodule(Path::new("src/llvm-project")); + builder.require_and_update_submodule( + "src/llvm-project", + Some("The `build.profiler` config option requires compiler-rt sources."), + ); } let mut target_deps = builder.ensure(StartupObjects { compiler, target }); @@ -456,13 +459,15 @@ pub fn std_cargo(builder: &Builder<'_>, target: TargetSelection, stage: u32, car // That's probably ok? At least, the difference wasn't enforced before. There's a comment in // the compiler_builtins build script that makes me nervous, though: // https://github.com/rust-lang/compiler-builtins/blob/31ee4544dbe47903ce771270d6e3bea8654e9e50/build.rs#L575-L579 - builder.update_submodule(&Path::new("src").join("llvm-project")); + builder.require_and_update_submodule( + "src/llvm-project", + Some( + "need LLVM sources available to build `compiler-rt`, but they weren't present; \ + consider disabling `optimized-compiler-builtins`", + ), + ); let compiler_builtins_root = builder.src.join("src/llvm-project/compiler-rt"); - if !compiler_builtins_root.exists() { - panic!( - "need LLVM sources available to build `compiler-rt`, but they weren't present; consider enabling `build.submodules = true` or disabling `optimized-compiler-builtins`" - ); - } + assert!(compiler_builtins_root.exists()); // Note that `libprofiler_builtins/build.rs` also computes this so if // you're changing something here please also change that. cargo.env("RUST_COMPILER_RT_ROOT", &compiler_builtins_root); diff --git a/src/bootstrap/src/core/build_steps/dist.rs b/src/bootstrap/src/core/build_steps/dist.rs index 649bcdd8733..998f083b676 100644 --- a/src/bootstrap/src/core/build_steps/dist.rs +++ b/src/bootstrap/src/core/build_steps/dist.rs @@ -907,7 +907,7 @@ fn make_run(run: RunConfig<'_>) { /// Creates the `rust-src` installer component fn run(self, builder: &Builder<'_>) -> GeneratedTarball { if !builder.config.dry_run() { - builder.update_submodule(Path::new("src/llvm-project")); + builder.require_and_update_submodule("src/llvm-project", None); } let tarball = Tarball::new_targetless(builder, "rust-src"); @@ -1022,10 +1022,7 @@ fn run(self, builder: &Builder<'_>) -> GeneratedTarball { // FIXME: This code looks _very_ similar to what we have in `src/core/build_steps/vendor.rs` // perhaps it should be removed in favor of making `dist` perform the `vendor` step? - // Ensure we have all submodules from src and other directories checked out. - for submodule in build_helper::util::parse_gitmodules(&builder.src) { - builder.update_submodule(Path::new(submodule)); - } + builder.require_and_update_all_submodules(); // Vendor all Cargo dependencies let mut cmd = command(&builder.initial_cargo); diff --git a/src/bootstrap/src/core/build_steps/doc.rs b/src/bootstrap/src/core/build_steps/doc.rs index 633e66afe59..079539388bd 100644 --- a/src/bootstrap/src/core/build_steps/doc.rs +++ b/src/bootstrap/src/core/build_steps/doc.rs @@ -16,7 +16,7 @@ use crate::core::builder::{self, crate_description}; use crate::core::builder::{Alias, Builder, Compiler, Kind, RunConfig, ShouldRun, Step}; use crate::core::config::{Config, TargetSelection}; -use crate::utils::helpers::{dir_is_empty, symlink_dir, t, up_to_date}; +use crate::utils::helpers::{symlink_dir, t, up_to_date}; use crate::Mode; macro_rules! submodule_helper { @@ -53,8 +53,8 @@ fn make_run(run: RunConfig<'_>) { fn run(self, builder: &Builder<'_>) { $( - let path = Path::new(submodule_helper!( $path, submodule $( = $submodule )? )); - builder.update_submodule(&path); + let path = submodule_helper!( $path, submodule $( = $submodule )? ); + builder.require_and_update_submodule(path, None); )? builder.ensure(RustbookSrc { target: self.target, @@ -217,22 +217,14 @@ fn make_run(run: RunConfig<'_>) { /// * Index page /// * Redirect pages fn run(self, builder: &Builder<'_>) { - let relative_path = Path::new("src").join("doc").join("book"); - builder.update_submodule(&relative_path); + builder.require_and_update_submodule("src/doc/book", None); let compiler = self.compiler; let target = self.target; - let absolute_path = builder.src.join(&relative_path); + let absolute_path = builder.src.join("src/doc/book"); let redirect_path = absolute_path.join("redirects"); - if !absolute_path.exists() - || !redirect_path.exists() - || dir_is_empty(&absolute_path) - || dir_is_empty(&redirect_path) - { - eprintln!("Please checkout submodule: {}", relative_path.display()); - crate::exit!(1); - } + // build book builder.ensure(RustbookSrc { target, @@ -932,8 +924,8 @@ fn run(self, builder: &Builder<'_>) { let _ = source_type; // silence the "unused variable" warning let source_type = SourceType::Submodule; - let path = Path::new(submodule_helper!( $path, submodule $( = $submodule )? )); - builder.update_submodule(&path); + let path = submodule_helper!( $path, submodule $( = $submodule )? ); + builder.require_and_update_submodule(path, None); )? let stage = builder.top_stage; diff --git a/src/bootstrap/src/core/build_steps/llvm.rs b/src/bootstrap/src/core/build_steps/llvm.rs index f234b08f5e2..5d892275ad3 100644 --- a/src/bootstrap/src/core/build_steps/llvm.rs +++ b/src/bootstrap/src/core/build_steps/llvm.rs @@ -110,7 +110,7 @@ pub fn prebuilt_llvm_config(builder: &Builder<'_>, target: TargetSelection) -> L } // Initialize the llvm submodule if not initialized already. - builder.update_submodule(&Path::new("src").join("llvm-project")); + builder.require_and_update_submodule("src/llvm-project", None); let root = "src/llvm-project/llvm"; let out_dir = builder.llvm_out(target); @@ -1197,7 +1197,7 @@ fn make_run(run: RunConfig<'_>) { /// Build crtbegin.o/crtend.o for musl target. fn run(self, builder: &Builder<'_>) -> Self::Output { - builder.update_submodule(Path::new("src/llvm-project")); + builder.require_and_update_submodule("src/llvm-project", None); let out_dir = builder.native_dir(self.target).join("crt"); @@ -1270,7 +1270,7 @@ fn make_run(run: RunConfig<'_>) { /// Build libunwind.a fn run(self, builder: &Builder<'_>) -> Self::Output { - builder.update_submodule(Path::new("src/llvm-project")); + builder.require_and_update_submodule("src/llvm-project", None); if builder.config.dry_run() { return PathBuf::new(); diff --git a/src/bootstrap/src/core/build_steps/test.rs b/src/bootstrap/src/core/build_steps/test.rs index cc5931c68db..108b01de561 100644 --- a/src/bootstrap/src/core/build_steps/test.rs +++ b/src/bootstrap/src/core/build_steps/test.rs @@ -2398,8 +2398,8 @@ fn make_run(run: RunConfig<'_>) { } fn run(self, builder: &Builder<'_>) { - let relative_path = Path::new("src").join("doc").join("rustc-dev-guide"); - builder.update_submodule(&relative_path); + let relative_path = "src/doc/rustc-dev-guide"; + builder.require_and_update_submodule(relative_path, None); let src = builder.src.join(relative_path); let mut rustbook_cmd = builder.tool_cmd(Tool::Rustbook).delay_failure(); @@ -3009,7 +3009,7 @@ fn run(self, builder: &Builder<'_>) { let _guard = builder.msg(Kind::Test, 0, "bootstrap", host, host); // Some tests require cargo submodule to be present. - builder.build.update_submodule(Path::new("src/tools/cargo")); + builder.build.require_and_update_submodule("src/tools/cargo", None); let mut check_bootstrap = command(builder.python()); check_bootstrap diff --git a/src/bootstrap/src/core/build_steps/tool.rs b/src/bootstrap/src/core/build_steps/tool.rs index d5fd3301b92..bd0caef3963 100644 --- a/src/bootstrap/src/core/build_steps/tool.rs +++ b/src/bootstrap/src/core/build_steps/tool.rs @@ -1,6 +1,6 @@ use std::env; use std::fs; -use std::path::{Path, PathBuf}; +use std::path::PathBuf; use crate::core::build_steps::compile; use crate::core::build_steps::toolstate::ToolState; @@ -290,7 +290,7 @@ fn make_run(run: RunConfig<'_>) { fn run(self, builder: &Builder<'_>) -> PathBuf { $( for submodule in $submodules { - builder.update_submodule(Path::new(submodule)); + builder.require_and_update_submodule(submodule, None); } )* builder.ensure(ToolBuild { @@ -373,7 +373,7 @@ fn make_run(run: RunConfig<'_>) { fn run(self, builder: &Builder<'_>) -> PathBuf { // We need to ensure the rustc-perf submodule is initialized when building opt-dist since // the tool requires it to be in place to run. - builder.update_submodule(Path::new("src/tools/rustc-perf")); + builder.require_and_update_submodule("src/tools/rustc-perf", None); builder.ensure(ToolBuild { compiler: self.compiler, @@ -414,7 +414,7 @@ fn make_run(run: RunConfig<'_>) { fn run(self, builder: &Builder<'_>) -> PathBuf { // We need to ensure the rustc-perf submodule is initialized. - builder.update_submodule(Path::new("src/tools/rustc-perf")); + builder.require_and_update_submodule("src/tools/rustc-perf", None); let tool = ToolBuild { compiler: self.compiler, @@ -715,7 +715,7 @@ fn make_run(run: RunConfig<'_>) { } fn run(self, builder: &Builder<'_>) -> PathBuf { - builder.build.update_submodule(Path::new("src/tools/cargo")); + builder.build.require_and_update_submodule("src/tools/cargo", None); builder.ensure(ToolBuild { compiler: self.compiler, diff --git a/src/bootstrap/src/core/build_steps/vendor.rs b/src/bootstrap/src/core/build_steps/vendor.rs index e6b3cb320cf..d6b0a9a44fc 100644 --- a/src/bootstrap/src/core/build_steps/vendor.rs +++ b/src/bootstrap/src/core/build_steps/vendor.rs @@ -1,6 +1,6 @@ use crate::core::builder::{Builder, RunConfig, ShouldRun, Step}; use crate::utils::exec::command; -use std::path::{Path, PathBuf}; +use std::path::PathBuf; #[derive(Debug, Clone, Hash, PartialEq, Eq)] pub(crate) struct Vendor { @@ -35,8 +35,8 @@ fn run(self, builder: &Builder<'_>) -> Self::Output { } // These submodules must be present for `x vendor` to work. - for path in ["src/tools/cargo", "src/doc/book"] { - builder.build.update_submodule(Path::new(path)); + for submodule in ["src/tools/cargo", "src/doc/book"] { + builder.build.require_and_update_submodule(submodule, None); } // Sync these paths by default. diff --git a/src/bootstrap/src/core/builder/tests.rs b/src/bootstrap/src/core/builder/tests.rs index 97c9ece0036..922d02f2d8b 100644 --- a/src/bootstrap/src/core/builder/tests.rs +++ b/src/bootstrap/src/core/builder/tests.rs @@ -21,7 +21,7 @@ fn configure_with_args(cmd: &[String], host: &[&str], target: &[&str]) -> Config rust_codegen_backends: vec![], ..Config::parse(&["check".to_owned()]) }); - submodule_build.update_submodule(Path::new("src/doc/book")); + submodule_build.require_and_update_submodule("src/doc/book", None); config.submodules = Some(false); config.ninja_in_file = false; diff --git a/src/bootstrap/src/lib.rs b/src/bootstrap/src/lib.rs index 0ce50ccaf3f..6f736b6052b 100644 --- a/src/bootstrap/src/lib.rs +++ b/src/bootstrap/src/lib.rs @@ -473,7 +473,13 @@ pub fn new(mut config: Config) -> Build { /// Given a path to the directory of a submodule, update it. /// /// `relative_path` should be relative to the root of the git repository, not an absolute path. - pub(crate) fn update_submodule(&self, relative_path: &Path) { + /// + /// This *does not* update the submodule if `config.toml` explicitly says + /// not to, or if we're not in a git repository (like a plain source + /// tarball). Typically [`Build::require_and_update_submodule`] should be + /// used instead to provide a nice error to the user if the submodule is + /// missing. + fn update_submodule(&self, relative_path: &Path) { if !self.config.submodules() { return; } @@ -579,6 +585,45 @@ pub(crate) fn update_submodule(&self, relative_path: &Path) { } } + /// Updates a submodule, and exits with a failure if submodule management + /// is disabled and the submodule does not exist. + /// + /// The given `err_hint` will be shown to the user if the submodule is not + /// checked out. + pub fn require_and_update_submodule(&self, submodule: &str, err_hint: Option<&str>) { + // When testing bootstrap itself, it is much faster to ignore + // submodules. Almost all Steps work fine without their submodules. + if cfg!(test) && !self.config.submodules() { + return; + } + let relative_path = Path::new(submodule); + self.update_submodule(relative_path); + let absolute_path = self.config.src.join(relative_path); + if dir_is_empty(&absolute_path) { + let maybe_enable = if !self.config.submodules() + && self.config.rust_info.is_managed_git_subrepository() + { + "\nConsider setting `build.submodules = true` or manually initializing the submodules." + } else { + "" + }; + let err_hint = err_hint.map_or_else(String::new, |e| format!("\n{e}")); + eprintln!( + "submodule {submodule} does not appear to be checked out, \ + but it is required for this step{maybe_enable}{err_hint}" + ); + exit!(1); + } + } + + /// Updates all submodules, and exits with an error if submodule + /// management is disabled and the submodule does not exist. + pub fn require_and_update_all_submodules(&self) { + for submodule in build_helper::util::parse_gitmodules(&self.src) { + self.require_and_update_submodule(submodule, None); + } + } + /// If any submodule has been initialized already, sync it unconditionally. /// This avoids contributors checking in a submodule change by accident. fn update_existing_submodules(&self) { From 18aa41958324ff1ba9e1dcb0c5d9c8f14bc0ae69 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Thu, 25 Jul 2024 16:36:57 -0700 Subject: [PATCH 08/17] Clarify comment about why bootstrap tests need src/doc/book --- src/bootstrap/src/core/builder/tests.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/bootstrap/src/core/builder/tests.rs b/src/bootstrap/src/core/builder/tests.rs index 922d02f2d8b..f9d6d551370 100644 --- a/src/bootstrap/src/core/builder/tests.rs +++ b/src/bootstrap/src/core/builder/tests.rs @@ -13,9 +13,12 @@ fn configure_with_args(cmd: &[String], host: &[&str], target: &[&str]) -> Config config.save_toolstates = None; config.dry_run = DryRun::SelfCheck; - // Ignore most submodules, since we don't need them for a dry run. - // But make sure to check out the `doc` and `rust-analyzer` submodules, since some steps need them - // just to know which commands to run. + // Ignore most submodules, since we don't need them for a dry run, and the + // tests run much faster without them. + // + // The src/doc/book submodule is needed because TheBook step tries to + // access files even during a dry-run (may want to consider just skipping + // that in a dry run). let submodule_build = Build::new(Config { // don't include LLVM, so CI doesn't require ninja/cmake to be installed rust_codegen_backends: vec![], From a20db06d5be876fbdaf1d4542d2974f2f76abba0 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Thu, 25 Jul 2024 17:10:08 -0700 Subject: [PATCH 09/17] Make sure submodules are checked out with `x test` If the submodule is not checked out, then these tests would fail. --- src/bootstrap/src/core/build_steps/test.rs | 24 +++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/test.rs b/src/bootstrap/src/core/build_steps/test.rs index 108b01de561..e2eef1baaf5 100644 --- a/src/bootstrap/src/core/build_steps/test.rs +++ b/src/bootstrap/src/core/build_steps/test.rs @@ -2257,7 +2257,12 @@ fn run_local_doc(self, builder: &Builder<'_>) { } macro_rules! test_book { - ($($name:ident, $path:expr, $book_name:expr, default=$default:expr;)+) => { + ($( + $name:ident, $path:expr, $book_name:expr, + default=$default:expr + $(,submodules = $submodules:expr)? + ; + )+) => { $( #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub struct $name { @@ -2280,6 +2285,11 @@ fn make_run(run: RunConfig<'_>) { } fn run(self, builder: &Builder<'_>) { + $( + for submodule in $submodules { + builder.require_and_update_submodule(submodule, None); + } + )* builder.ensure(BookTest { compiler: self.compiler, path: PathBuf::from($path), @@ -2293,15 +2303,15 @@ fn run(self, builder: &Builder<'_>) { } test_book!( - Nomicon, "src/doc/nomicon", "nomicon", default=false; - Reference, "src/doc/reference", "reference", default=false; + Nomicon, "src/doc/nomicon", "nomicon", default=false, submodules=["src/doc/nomicon"]; + Reference, "src/doc/reference", "reference", default=false, submodules=["src/doc/reference"]; RustdocBook, "src/doc/rustdoc", "rustdoc", default=true; RustcBook, "src/doc/rustc", "rustc", default=true; - RustByExample, "src/doc/rust-by-example", "rust-by-example", default=false; - EmbeddedBook, "src/doc/embedded-book", "embedded-book", default=false; - TheBook, "src/doc/book", "book", default=false; + RustByExample, "src/doc/rust-by-example", "rust-by-example", default=false, submodules=["src/doc/rust-by-example"]; + EmbeddedBook, "src/doc/embedded-book", "embedded-book", default=false, submodules=["src/doc/embedded-book"]; + TheBook, "src/doc/book", "book", default=false, submodules=["src/doc/book"]; UnstableBook, "src/doc/unstable-book", "unstable-book", default=true; - EditionGuide, "src/doc/edition-guide", "edition-guide", default=false; + EditionGuide, "src/doc/edition-guide", "edition-guide", default=false, submodules=["src/doc/edition-guide"]; ); #[derive(Debug, Clone, PartialEq, Eq, Hash)] From 53ef052d455ba8bd9a4823c59696efb257aef2fa Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Thu, 25 Jul 2024 17:38:22 -0700 Subject: [PATCH 10/17] Integrate mdbook-spec for the reference. This updates the reference which is now using a new mdbook plugin. This requires a little extra work than a normal book because the plugin uses `rustdoc` to generate links to the standard library. It also ensures that the submodule is available for *any* command that uses rustbook, since it is now part of the rustbook workspace. --- src/bootstrap/src/core/build_steps/doc.rs | 72 +++++++++++++++++--- src/bootstrap/src/core/build_steps/tool.rs | 2 +- src/bootstrap/src/core/build_steps/vendor.rs | 3 +- src/doc/reference | 2 +- src/tools/rustbook/Cargo.lock | 21 ++++++ src/tools/rustbook/Cargo.toml | 1 + src/tools/rustbook/src/main.rs | 12 ++++ src/tools/tidy/src/deps.rs | 2 +- 8 files changed, 102 insertions(+), 13 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/doc.rs b/src/bootstrap/src/core/build_steps/doc.rs index 079539388bd..78e6e7af687 100644 --- a/src/bootstrap/src/core/build_steps/doc.rs +++ b/src/bootstrap/src/core/build_steps/doc.rs @@ -9,7 +9,7 @@ use std::io::{self, Write}; use std::path::{Path, PathBuf}; -use std::{fs, mem}; +use std::{env, fs, mem}; use crate::core::build_steps::compile; use crate::core::build_steps::tool::{self, prepare_tool_cargo, SourceType, Tool}; @@ -62,6 +62,7 @@ fn run(self, builder: &Builder<'_>) { src: builder.src.join($path), parent: Some(self), languages: $lang.into(), + rustdoc: None, }) } } @@ -80,7 +81,6 @@ fn run(self, builder: &Builder<'_>) { EditionGuide, "src/doc/edition-guide", "edition-guide", &[], submodule; EmbeddedBook, "src/doc/embedded-book", "embedded-book", &[], submodule; Nomicon, "src/doc/nomicon", "nomicon", &[], submodule; - Reference, "src/doc/reference", "reference", &[], submodule; RustByExample, "src/doc/rust-by-example", "rust-by-example", &["ja"], submodule; RustdocBook, "src/doc/rustdoc", "rustdoc", &[]; StyleGuide, "src/doc/style-guide", "style-guide", &[]; @@ -112,6 +112,7 @@ fn run(self, builder: &Builder<'_>) { src: builder.md_doc_out(self.target).join("unstable-book"), parent: Some(self), languages: vec![], + rustdoc: None, }) } } @@ -123,6 +124,7 @@ struct RustbookSrc { src: PathBuf, parent: Option

, languages: Vec<&'static str>, + rustdoc: Option, } impl Step for RustbookSrc

{ @@ -153,13 +155,18 @@ fn run(self, builder: &Builder<'_>) { builder.info(&format!("Rustbook ({target}) - {name}")); let _ = fs::remove_dir_all(&out); - builder - .tool_cmd(Tool::Rustbook) - .arg("build") - .arg(&src) - .arg("-d") - .arg(&out) - .run(builder); + let mut rustbook_cmd = builder.tool_cmd(Tool::Rustbook); + if let Some(mut rustdoc) = self.rustdoc { + rustdoc.pop(); + let old_path = env::var_os("PATH").unwrap_or_default(); + let new_path = + env::join_paths(std::iter::once(rustdoc).chain(env::split_paths(&old_path))) + .expect("could not add rustdoc to PATH"); + + rustbook_cmd.env("PATH", new_path); + } + + rustbook_cmd.arg("build").arg(&src).arg("-d").arg(&out).run(builder); for lang in &self.languages { let out = out.join(lang); @@ -232,6 +239,7 @@ fn run(self, builder: &Builder<'_>) { src: absolute_path.clone(), parent: Some(self), languages: vec![], + rustdoc: None, }); // building older edition redirects @@ -244,6 +252,7 @@ fn run(self, builder: &Builder<'_>) { // treat the other editions as not having a parent. parent: Option::::None, languages: vec![], + rustdoc: None, }); } @@ -1214,6 +1223,51 @@ fn run(self, builder: &Builder<'_>) { src: out_base, parent: Some(self), languages: vec![], + rustdoc: None, + }); + } +} + +#[derive(Ord, PartialOrd, Debug, Clone, Hash, PartialEq, Eq)] +pub struct Reference { + pub compiler: Compiler, + pub target: TargetSelection, +} + +impl Step for Reference { + type Output = (); + const DEFAULT: bool = true; + const ONLY_HOSTS: bool = true; + + fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> { + let builder = run.builder; + run.path("src/doc/reference").default_condition(builder.config.docs) + } + + fn make_run(run: RunConfig<'_>) { + run.builder.ensure(Reference { + compiler: run.builder.compiler(run.builder.top_stage, run.builder.config.build), + target: run.target, + }); + } + + /// Builds the reference book. + fn run(self, builder: &Builder<'_>) { + builder.require_and_update_submodule("src/doc/reference", None); + + // This is needed for generating links to the standard library using + // the mdbook-spec plugin. + builder.ensure(compile::Std::new(self.compiler, builder.config.build)); + let rustdoc = builder.rustdoc(self.compiler); + + // Run rustbook/mdbook to generate the HTML pages. + builder.ensure(RustbookSrc { + target: self.target, + name: "reference".to_owned(), + src: builder.src.join("src/doc/reference"), + parent: Some(self), + languages: vec![], + rustdoc: Some(rustdoc), }); } } diff --git a/src/bootstrap/src/core/build_steps/tool.rs b/src/bootstrap/src/core/build_steps/tool.rs index bd0caef3963..57447117b29 100644 --- a/src/bootstrap/src/core/build_steps/tool.rs +++ b/src/bootstrap/src/core/build_steps/tool.rs @@ -348,7 +348,7 @@ fn run(self, builder: &Builder<'_>) -> PathBuf { /// These are the submodules that are required for rustbook to work due to /// depending on mdbook plugins. -pub static SUBMODULES_FOR_RUSTBOOK: &[&str] = &["src/doc/book"]; +pub static SUBMODULES_FOR_RUSTBOOK: &[&str] = &["src/doc/book", "src/doc/reference"]; #[derive(Debug, Clone, Hash, PartialEq, Eq)] pub struct OptimizedDist { diff --git a/src/bootstrap/src/core/build_steps/vendor.rs b/src/bootstrap/src/core/build_steps/vendor.rs index d6b0a9a44fc..d5cd3c0832a 100644 --- a/src/bootstrap/src/core/build_steps/vendor.rs +++ b/src/bootstrap/src/core/build_steps/vendor.rs @@ -1,3 +1,4 @@ +use crate::core::build_steps::tool::SUBMODULES_FOR_RUSTBOOK; use crate::core::builder::{Builder, RunConfig, ShouldRun, Step}; use crate::utils::exec::command; use std::path::PathBuf; @@ -35,7 +36,7 @@ fn run(self, builder: &Builder<'_>) -> Self::Output { } // These submodules must be present for `x vendor` to work. - for submodule in ["src/tools/cargo", "src/doc/book"] { + for submodule in SUBMODULES_FOR_RUSTBOOK.iter().chain(["src/tools/cargo"].iter()) { builder.build.require_and_update_submodule(submodule, None); } diff --git a/src/doc/reference b/src/doc/reference index e2f0bdc4031..2e191814f16 160000 --- a/src/doc/reference +++ b/src/doc/reference @@ -1 +1 @@ -Subproject commit e2f0bdc4031866734661dcdb548184bde1450baf +Subproject commit 2e191814f163ee1e77e2d6094eee4dd78a289c5b diff --git a/src/tools/rustbook/Cargo.lock b/src/tools/rustbook/Cargo.lock index 75b89a162e9..df051ed447e 100644 --- a/src/tools/rustbook/Cargo.lock +++ b/src/tools/rustbook/Cargo.lock @@ -661,6 +661,20 @@ dependencies = [ "textwrap", ] +[[package]] +name = "mdbook-spec" +version = "0.1.2" +dependencies = [ + "anyhow", + "mdbook", + "once_cell", + "pathdiff", + "regex", + "semver", + "serde_json", + "tempfile", +] + [[package]] name = "mdbook-trpl-listing" version = "0.1.0" @@ -794,6 +808,12 @@ dependencies = [ "windows-targets 0.52.6", ] +[[package]] +name = "pathdiff" +version = "0.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8835116a5c179084a830efb3adc117ab007512b535bc1a21c991d3b32a6b44dd" + [[package]] name = "percent-encoding" version = "2.3.1" @@ -1079,6 +1099,7 @@ dependencies = [ "env_logger", "mdbook", "mdbook-i18n-helpers", + "mdbook-spec", "mdbook-trpl-listing", "mdbook-trpl-note", ] diff --git a/src/tools/rustbook/Cargo.toml b/src/tools/rustbook/Cargo.toml index 51ba58483c5..2c29a2848b7 100644 --- a/src/tools/rustbook/Cargo.toml +++ b/src/tools/rustbook/Cargo.toml @@ -12,6 +12,7 @@ env_logger = "0.11" mdbook-trpl-listing = { path = "../../doc/book/packages/mdbook-trpl-listing" } mdbook-trpl-note = { path = "../../doc/book/packages/mdbook-trpl-note" } mdbook-i18n-helpers = "0.3.3" +mdbook-spec = { path = "../../doc/reference/mdbook-spec"} [dependencies.mdbook] version = "0.4.37" diff --git a/src/tools/rustbook/src/main.rs b/src/tools/rustbook/src/main.rs index 31bba56adde..e94c2f5958e 100644 --- a/src/tools/rustbook/src/main.rs +++ b/src/tools/rustbook/src/main.rs @@ -9,6 +9,7 @@ use mdbook::MDBook; use mdbook_i18n_helpers::preprocessors::Gettext; +use mdbook_spec::Spec; use mdbook_trpl_listing::TrplListing; use mdbook_trpl_note::TrplNote; @@ -83,6 +84,13 @@ pub fn build(args: &ArgMatches) -> Result3<()> { book.config.build.build_dir = dest_dir.into(); } + // NOTE: Replacing preprocessors using this technique causes error + // messages to be displayed when the original preprocessor doesn't work + // (but it otherwise succeeds). + // + // This should probably be fixed in mdbook to remove the existing + // preprocessor, or this should modify the config and use + // MDBook::load_with_config. if book.config.get_preprocessor("trpl-note").is_some() { book.with_preprocessor(TrplNote); } @@ -91,6 +99,10 @@ pub fn build(args: &ArgMatches) -> Result3<()> { book.with_preprocessor(TrplListing); } + if book.config.get_preprocessor("spec").is_some() { + book.with_preprocessor(Spec::new()); + } + book.build()?; Ok(()) diff --git a/src/tools/tidy/src/deps.rs b/src/tools/tidy/src/deps.rs index 6d61e0dd3ef..d53e535a750 100644 --- a/src/tools/tidy/src/deps.rs +++ b/src/tools/tidy/src/deps.rs @@ -72,7 +72,7 @@ //("src/tools/miri/test-cargo-miri", &[], None), // FIXME uncomment once all deps are vendored //("src/tools/miri/test_dependencies", &[], None), // FIXME uncomment once all deps are vendored ("src/tools/rust-analyzer", EXCEPTIONS_RUST_ANALYZER, None, &[]), - ("src/tools/rustbook", EXCEPTIONS_RUSTBOOK, None, &["src/doc/book"]), + ("src/tools/rustbook", EXCEPTIONS_RUSTBOOK, None, &["src/doc/book", "src/doc/reference"]), ("src/tools/rustc-perf", EXCEPTIONS_RUSTC_PERF, None, &["src/tools/rustc-perf"]), ("src/tools/x", &[], None, &[]), // tidy-alphabetical-end From 5ebb821fa9f845e8edd854cbf15ace5dc36a61a4 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Sat, 27 Jul 2024 07:57:50 -0700 Subject: [PATCH 11/17] Fix mistake setting ONLY_HOSTS for Reference. This was a copy/paste mistake. --- src/bootstrap/src/core/build_steps/doc.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/bootstrap/src/core/build_steps/doc.rs b/src/bootstrap/src/core/build_steps/doc.rs index 78e6e7af687..1fbc78a94df 100644 --- a/src/bootstrap/src/core/build_steps/doc.rs +++ b/src/bootstrap/src/core/build_steps/doc.rs @@ -1237,7 +1237,6 @@ pub struct Reference { impl Step for Reference { type Output = (); const DEFAULT: bool = true; - const ONLY_HOSTS: bool = true; fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> { let builder = run.builder; From f76ab647d3b39581c3d29e5107c8d460f736989f Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Sat, 27 Jul 2024 07:59:37 -0700 Subject: [PATCH 12/17] Add more descriptions to why submodules are required. --- src/bootstrap/src/core/build_steps/compile.rs | 8 +++++--- src/bootstrap/src/core/build_steps/llvm.rs | 10 ++++++++-- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/compile.rs b/src/bootstrap/src/core/build_steps/compile.rs index 1efa3c8ada3..6bea163cdfe 100644 --- a/src/bootstrap/src/core/build_steps/compile.rs +++ b/src/bootstrap/src/core/build_steps/compile.rs @@ -188,7 +188,9 @@ fn run(self, builder: &Builder<'_>) { if builder.config.profiler { builder.require_and_update_submodule( "src/llvm-project", - Some("The `build.profiler` config option requires compiler-rt sources."), + Some( + "The `build.profiler` config option requires `compiler-rt` sources from LLVM.", + ), ); } @@ -462,8 +464,8 @@ pub fn std_cargo(builder: &Builder<'_>, target: TargetSelection, stage: u32, car builder.require_and_update_submodule( "src/llvm-project", Some( - "need LLVM sources available to build `compiler-rt`, but they weren't present; \ - consider disabling `optimized-compiler-builtins`", + "The `build.optimized-compiler-builtins` config option \ + requires `compiler-rt` sources from LLVM.", ), ); let compiler_builtins_root = builder.src.join("src/llvm-project/compiler-rt"); diff --git a/src/bootstrap/src/core/build_steps/llvm.rs b/src/bootstrap/src/core/build_steps/llvm.rs index 5d892275ad3..f8a22b97488 100644 --- a/src/bootstrap/src/core/build_steps/llvm.rs +++ b/src/bootstrap/src/core/build_steps/llvm.rs @@ -1197,7 +1197,10 @@ fn make_run(run: RunConfig<'_>) { /// Build crtbegin.o/crtend.o for musl target. fn run(self, builder: &Builder<'_>) -> Self::Output { - builder.require_and_update_submodule("src/llvm-project", None); + builder.require_and_update_submodule( + "src/llvm-project", + Some("The LLVM sources are required for the CRT from `compiler-rt`."), + ); let out_dir = builder.native_dir(self.target).join("crt"); @@ -1270,7 +1273,10 @@ fn make_run(run: RunConfig<'_>) { /// Build libunwind.a fn run(self, builder: &Builder<'_>) -> Self::Output { - builder.require_and_update_submodule("src/llvm-project", None); + builder.require_and_update_submodule( + "src/llvm-project", + Some("The LLVM sources are required for libunwind."), + ); if builder.config.dry_run() { return PathBuf::new(); From 9b0115c7436e6367f11a2b93894a2345c90a60e0 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Sat, 27 Jul 2024 08:02:06 -0700 Subject: [PATCH 13/17] Consistently use a string to represent a submodule. This makes it easier to call these functions without needing to form a Path. --- src/bootstrap/src/core/build_steps/llvm.rs | 2 +- src/bootstrap/src/lib.rs | 20 ++++++++++---------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/llvm.rs b/src/bootstrap/src/core/build_steps/llvm.rs index f8a22b97488..cd64492164f 100644 --- a/src/bootstrap/src/core/build_steps/llvm.rs +++ b/src/bootstrap/src/core/build_steps/llvm.rs @@ -89,7 +89,7 @@ fn push_all(&mut self, s: impl AsRef) { /// if not). pub fn prebuilt_llvm_config(builder: &Builder<'_>, target: TargetSelection) -> LlvmBuildStatus { // If we have llvm submodule initialized already, sync it. - builder.update_existing_submodule(&Path::new("src").join("llvm-project")); + builder.update_existing_submodule("src/llvm-project"); builder.config.maybe_download_ci_llvm(); diff --git a/src/bootstrap/src/lib.rs b/src/bootstrap/src/lib.rs index 6f736b6052b..431b04ae774 100644 --- a/src/bootstrap/src/lib.rs +++ b/src/bootstrap/src/lib.rs @@ -441,7 +441,7 @@ pub fn new(mut config: Config) -> Build { // Cargo.toml files. let rust_submodules = ["library/backtrace", "library/stdarch"]; for s in rust_submodules { - build.update_submodule(Path::new(s)); + build.update_submodule(s); } // Now, update all existing submodules. build.update_existing_submodules(); @@ -479,7 +479,7 @@ pub fn new(mut config: Config) -> Build { /// tarball). Typically [`Build::require_and_update_submodule`] should be /// used instead to provide a nice error to the user if the submodule is /// missing. - fn update_submodule(&self, relative_path: &Path) { + fn update_submodule(&self, relative_path: &str) { if !self.config.submodules() { return; } @@ -527,7 +527,7 @@ fn update_submodule(&self, relative_path: &Path) { return; } - println!("Updating submodule {}", relative_path.display()); + println!("Updating submodule {relative_path}"); helpers::git(Some(&self.src)) .run_always() .args(["submodule", "-q", "sync"]) @@ -596,9 +596,8 @@ pub fn require_and_update_submodule(&self, submodule: &str, err_hint: Option<&st if cfg!(test) && !self.config.submodules() { return; } - let relative_path = Path::new(submodule); - self.update_submodule(relative_path); - let absolute_path = self.config.src.join(relative_path); + self.update_submodule(submodule); + let absolute_path = self.config.src.join(submodule); if dir_is_empty(&absolute_path) { let maybe_enable = if !self.config.submodules() && self.config.rust_info.is_managed_git_subrepository() @@ -642,22 +641,23 @@ fn update_existing_submodules(&self) { for line in output.lines() { // Look for `submodule.$name.path = $path` // Sample output: `submodule.src/rust-installer.path src/tools/rust-installer` - let submodule = Path::new(line.split_once(' ').unwrap().1); + let submodule = line.split_once(' ').unwrap().1; + let path = Path::new(submodule); // Don't update the submodule unless it's already been cloned. - if GitInfo::new(false, submodule).is_managed_git_subrepository() { + if GitInfo::new(false, path).is_managed_git_subrepository() { self.update_submodule(submodule); } } } /// Updates the given submodule only if it's initialized already; nothing happens otherwise. - pub fn update_existing_submodule(&self, submodule: &Path) { + pub fn update_existing_submodule(&self, submodule: &str) { // Avoid running git when there isn't a git checkout. if !self.config.submodules() { return; } - if GitInfo::new(false, submodule).is_managed_git_subrepository() { + if GitInfo::new(false, Path::new(submodule)).is_managed_git_subrepository() { self.update_submodule(submodule); } } From 686e27ef49ab0d0d84e451311529283269d456e8 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Sat, 27 Jul 2024 08:04:46 -0700 Subject: [PATCH 14/17] Change the blanket submodule update for library submodules to be required These are required 100% of the time, but they are almost always required for any command that runs Cargo in the main workspace. Ideally, initializing these two standard library submodules would be lazy and only initialized when required (see https://github.com/rust-lang/rust/pull/82653). However, it would require updating these in almost every Step (anything that runs `cargo` in the main workspace). --- src/bootstrap/src/lib.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/bootstrap/src/lib.rs b/src/bootstrap/src/lib.rs index 431b04ae774..68dfc89c5a1 100644 --- a/src/bootstrap/src/lib.rs +++ b/src/bootstrap/src/lib.rs @@ -441,7 +441,13 @@ pub fn new(mut config: Config) -> Build { // Cargo.toml files. let rust_submodules = ["library/backtrace", "library/stdarch"]; for s in rust_submodules { - build.update_submodule(s); + build.require_and_update_submodule( + s, + Some( + "The submodule is required for the standard library \ + and the main Cargo workspace.", + ), + ); } // Now, update all existing submodules. build.update_existing_submodules(); From 78ee5d057baaa3a08e81c57828317126474c3ccf Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Sat, 27 Jul 2024 08:06:04 -0700 Subject: [PATCH 15/17] Change prebuilt_llvm_config to not be required. I misread this one. It is only checking if LLVM needs to be rebuilt. There is code below that handles the case where it is unable to compute the stamp if the source is missing. --- src/bootstrap/src/core/build_steps/llvm.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/bootstrap/src/core/build_steps/llvm.rs b/src/bootstrap/src/core/build_steps/llvm.rs index cd64492164f..b302e6eb632 100644 --- a/src/bootstrap/src/core/build_steps/llvm.rs +++ b/src/bootstrap/src/core/build_steps/llvm.rs @@ -110,7 +110,8 @@ pub fn prebuilt_llvm_config(builder: &Builder<'_>, target: TargetSelection) -> L } // Initialize the llvm submodule if not initialized already. - builder.require_and_update_submodule("src/llvm-project", None); + // If submodules are disabled, this does nothing. + builder.update_submodule("src/llvm-project"); let root = "src/llvm-project/llvm"; let out_dir = builder.llvm_out(target); From 0f387eb39b33d018eac138bdfc4f16d26e4207f1 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Sat, 27 Jul 2024 08:06:20 -0700 Subject: [PATCH 16/17] Add clarifying documentation to require_and_update_submodule. --- src/bootstrap/src/lib.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/bootstrap/src/lib.rs b/src/bootstrap/src/lib.rs index 68dfc89c5a1..49df30022e3 100644 --- a/src/bootstrap/src/lib.rs +++ b/src/bootstrap/src/lib.rs @@ -594,8 +594,11 @@ fn update_submodule(&self, relative_path: &str) { /// Updates a submodule, and exits with a failure if submodule management /// is disabled and the submodule does not exist. /// + /// The given submodule name should be its path relative to the root of + /// the main repository. + /// /// The given `err_hint` will be shown to the user if the submodule is not - /// checked out. + /// checked out and submodule management is disabled. pub fn require_and_update_submodule(&self, submodule: &str, err_hint: Option<&str>) { // When testing bootstrap itself, it is much faster to ignore // submodules. Almost all Steps work fine without their submodules. From 1c98b8f9226d0e7a4e0ef42dd558de2857812f7a Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Sat, 27 Jul 2024 08:07:53 -0700 Subject: [PATCH 17/17] Rename require_and_update_submodule to require_submodule Just trying to be a little less verbose here. --- src/bootstrap/src/core/build_steps/check.rs | 2 +- src/bootstrap/src/core/build_steps/clippy.rs | 2 +- src/bootstrap/src/core/build_steps/compile.rs | 6 +++--- src/bootstrap/src/core/build_steps/dist.rs | 2 +- src/bootstrap/src/core/build_steps/doc.rs | 8 ++++---- src/bootstrap/src/core/build_steps/llvm.rs | 4 ++-- src/bootstrap/src/core/build_steps/test.rs | 6 +++--- src/bootstrap/src/core/build_steps/tool.rs | 8 ++++---- src/bootstrap/src/core/build_steps/vendor.rs | 2 +- src/bootstrap/src/core/builder/tests.rs | 2 +- src/bootstrap/src/lib.rs | 8 ++++---- 11 files changed, 25 insertions(+), 25 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/check.rs b/src/bootstrap/src/core/build_steps/check.rs index 5b720ffbc76..3e0bb2884cd 100644 --- a/src/bootstrap/src/core/build_steps/check.rs +++ b/src/bootstrap/src/core/build_steps/check.rs @@ -52,7 +52,7 @@ fn make_run(run: RunConfig<'_>) { } fn run(self, builder: &Builder<'_>) { - builder.require_and_update_submodule("library/stdarch", None); + builder.require_submodule("library/stdarch", None); let target = self.target; let compiler = builder.compiler(builder.top_stage, builder.config.build); diff --git a/src/bootstrap/src/core/build_steps/clippy.rs b/src/bootstrap/src/core/build_steps/clippy.rs index add28f858ef..68abf1e464a 100644 --- a/src/bootstrap/src/core/build_steps/clippy.rs +++ b/src/bootstrap/src/core/build_steps/clippy.rs @@ -125,7 +125,7 @@ fn make_run(run: RunConfig<'_>) { } fn run(self, builder: &Builder<'_>) { - builder.require_and_update_submodule("library/stdarch", None); + builder.require_submodule("library/stdarch", None); let target = self.target; let compiler = builder.compiler(builder.top_stage, builder.config.build); diff --git a/src/bootstrap/src/core/build_steps/compile.rs b/src/bootstrap/src/core/build_steps/compile.rs index 6bea163cdfe..59649058046 100644 --- a/src/bootstrap/src/core/build_steps/compile.rs +++ b/src/bootstrap/src/core/build_steps/compile.rs @@ -182,11 +182,11 @@ fn run(self, builder: &Builder<'_>) { return; } - builder.require_and_update_submodule("library/stdarch", None); + builder.require_submodule("library/stdarch", None); // Profiler information requires LLVM's compiler-rt if builder.config.profiler { - builder.require_and_update_submodule( + builder.require_submodule( "src/llvm-project", Some( "The `build.profiler` config option requires `compiler-rt` sources from LLVM.", @@ -461,7 +461,7 @@ pub fn std_cargo(builder: &Builder<'_>, target: TargetSelection, stage: u32, car // That's probably ok? At least, the difference wasn't enforced before. There's a comment in // the compiler_builtins build script that makes me nervous, though: // https://github.com/rust-lang/compiler-builtins/blob/31ee4544dbe47903ce771270d6e3bea8654e9e50/build.rs#L575-L579 - builder.require_and_update_submodule( + builder.require_submodule( "src/llvm-project", Some( "The `build.optimized-compiler-builtins` config option \ diff --git a/src/bootstrap/src/core/build_steps/dist.rs b/src/bootstrap/src/core/build_steps/dist.rs index 998f083b676..4f4ef29c1d7 100644 --- a/src/bootstrap/src/core/build_steps/dist.rs +++ b/src/bootstrap/src/core/build_steps/dist.rs @@ -907,7 +907,7 @@ fn make_run(run: RunConfig<'_>) { /// Creates the `rust-src` installer component fn run(self, builder: &Builder<'_>) -> GeneratedTarball { if !builder.config.dry_run() { - builder.require_and_update_submodule("src/llvm-project", None); + builder.require_submodule("src/llvm-project", None); } let tarball = Tarball::new_targetless(builder, "rust-src"); diff --git a/src/bootstrap/src/core/build_steps/doc.rs b/src/bootstrap/src/core/build_steps/doc.rs index 1fbc78a94df..bdd48fa644a 100644 --- a/src/bootstrap/src/core/build_steps/doc.rs +++ b/src/bootstrap/src/core/build_steps/doc.rs @@ -54,7 +54,7 @@ fn make_run(run: RunConfig<'_>) { fn run(self, builder: &Builder<'_>) { $( let path = submodule_helper!( $path, submodule $( = $submodule )? ); - builder.require_and_update_submodule(path, None); + builder.require_submodule(path, None); )? builder.ensure(RustbookSrc { target: self.target, @@ -224,7 +224,7 @@ fn make_run(run: RunConfig<'_>) { /// * Index page /// * Redirect pages fn run(self, builder: &Builder<'_>) { - builder.require_and_update_submodule("src/doc/book", None); + builder.require_submodule("src/doc/book", None); let compiler = self.compiler; let target = self.target; @@ -934,7 +934,7 @@ fn run(self, builder: &Builder<'_>) { let source_type = SourceType::Submodule; let path = submodule_helper!( $path, submodule $( = $submodule )? ); - builder.require_and_update_submodule(path, None); + builder.require_submodule(path, None); )? let stage = builder.top_stage; @@ -1252,7 +1252,7 @@ fn make_run(run: RunConfig<'_>) { /// Builds the reference book. fn run(self, builder: &Builder<'_>) { - builder.require_and_update_submodule("src/doc/reference", None); + builder.require_submodule("src/doc/reference", None); // This is needed for generating links to the standard library using // the mdbook-spec plugin. diff --git a/src/bootstrap/src/core/build_steps/llvm.rs b/src/bootstrap/src/core/build_steps/llvm.rs index b302e6eb632..369e14df14b 100644 --- a/src/bootstrap/src/core/build_steps/llvm.rs +++ b/src/bootstrap/src/core/build_steps/llvm.rs @@ -1198,7 +1198,7 @@ fn make_run(run: RunConfig<'_>) { /// Build crtbegin.o/crtend.o for musl target. fn run(self, builder: &Builder<'_>) -> Self::Output { - builder.require_and_update_submodule( + builder.require_submodule( "src/llvm-project", Some("The LLVM sources are required for the CRT from `compiler-rt`."), ); @@ -1274,7 +1274,7 @@ fn make_run(run: RunConfig<'_>) { /// Build libunwind.a fn run(self, builder: &Builder<'_>) -> Self::Output { - builder.require_and_update_submodule( + builder.require_submodule( "src/llvm-project", Some("The LLVM sources are required for libunwind."), ); diff --git a/src/bootstrap/src/core/build_steps/test.rs b/src/bootstrap/src/core/build_steps/test.rs index e2eef1baaf5..daafb41e9d8 100644 --- a/src/bootstrap/src/core/build_steps/test.rs +++ b/src/bootstrap/src/core/build_steps/test.rs @@ -2287,7 +2287,7 @@ fn make_run(run: RunConfig<'_>) { fn run(self, builder: &Builder<'_>) { $( for submodule in $submodules { - builder.require_and_update_submodule(submodule, None); + builder.require_submodule(submodule, None); } )* builder.ensure(BookTest { @@ -2409,7 +2409,7 @@ fn make_run(run: RunConfig<'_>) { fn run(self, builder: &Builder<'_>) { let relative_path = "src/doc/rustc-dev-guide"; - builder.require_and_update_submodule(relative_path, None); + builder.require_submodule(relative_path, None); let src = builder.src.join(relative_path); let mut rustbook_cmd = builder.tool_cmd(Tool::Rustbook).delay_failure(); @@ -3019,7 +3019,7 @@ fn run(self, builder: &Builder<'_>) { let _guard = builder.msg(Kind::Test, 0, "bootstrap", host, host); // Some tests require cargo submodule to be present. - builder.build.require_and_update_submodule("src/tools/cargo", None); + builder.build.require_submodule("src/tools/cargo", None); let mut check_bootstrap = command(builder.python()); check_bootstrap diff --git a/src/bootstrap/src/core/build_steps/tool.rs b/src/bootstrap/src/core/build_steps/tool.rs index 57447117b29..a0cb0b0dbb9 100644 --- a/src/bootstrap/src/core/build_steps/tool.rs +++ b/src/bootstrap/src/core/build_steps/tool.rs @@ -290,7 +290,7 @@ fn make_run(run: RunConfig<'_>) { fn run(self, builder: &Builder<'_>) -> PathBuf { $( for submodule in $submodules { - builder.require_and_update_submodule(submodule, None); + builder.require_submodule(submodule, None); } )* builder.ensure(ToolBuild { @@ -373,7 +373,7 @@ fn make_run(run: RunConfig<'_>) { fn run(self, builder: &Builder<'_>) -> PathBuf { // We need to ensure the rustc-perf submodule is initialized when building opt-dist since // the tool requires it to be in place to run. - builder.require_and_update_submodule("src/tools/rustc-perf", None); + builder.require_submodule("src/tools/rustc-perf", None); builder.ensure(ToolBuild { compiler: self.compiler, @@ -414,7 +414,7 @@ fn make_run(run: RunConfig<'_>) { fn run(self, builder: &Builder<'_>) -> PathBuf { // We need to ensure the rustc-perf submodule is initialized. - builder.require_and_update_submodule("src/tools/rustc-perf", None); + builder.require_submodule("src/tools/rustc-perf", None); let tool = ToolBuild { compiler: self.compiler, @@ -715,7 +715,7 @@ fn make_run(run: RunConfig<'_>) { } fn run(self, builder: &Builder<'_>) -> PathBuf { - builder.build.require_and_update_submodule("src/tools/cargo", None); + builder.build.require_submodule("src/tools/cargo", None); builder.ensure(ToolBuild { compiler: self.compiler, diff --git a/src/bootstrap/src/core/build_steps/vendor.rs b/src/bootstrap/src/core/build_steps/vendor.rs index d5cd3c0832a..8732b30e940 100644 --- a/src/bootstrap/src/core/build_steps/vendor.rs +++ b/src/bootstrap/src/core/build_steps/vendor.rs @@ -37,7 +37,7 @@ fn run(self, builder: &Builder<'_>) -> Self::Output { // These submodules must be present for `x vendor` to work. for submodule in SUBMODULES_FOR_RUSTBOOK.iter().chain(["src/tools/cargo"].iter()) { - builder.build.require_and_update_submodule(submodule, None); + builder.build.require_submodule(submodule, None); } // Sync these paths by default. diff --git a/src/bootstrap/src/core/builder/tests.rs b/src/bootstrap/src/core/builder/tests.rs index f9d6d551370..ccabcad243f 100644 --- a/src/bootstrap/src/core/builder/tests.rs +++ b/src/bootstrap/src/core/builder/tests.rs @@ -24,7 +24,7 @@ fn configure_with_args(cmd: &[String], host: &[&str], target: &[&str]) -> Config rust_codegen_backends: vec![], ..Config::parse(&["check".to_owned()]) }); - submodule_build.require_and_update_submodule("src/doc/book", None); + submodule_build.require_submodule("src/doc/book", None); config.submodules = Some(false); config.ninja_in_file = false; diff --git a/src/bootstrap/src/lib.rs b/src/bootstrap/src/lib.rs index 49df30022e3..d4303aab6d6 100644 --- a/src/bootstrap/src/lib.rs +++ b/src/bootstrap/src/lib.rs @@ -441,7 +441,7 @@ pub fn new(mut config: Config) -> Build { // Cargo.toml files. let rust_submodules = ["library/backtrace", "library/stdarch"]; for s in rust_submodules { - build.require_and_update_submodule( + build.require_submodule( s, Some( "The submodule is required for the standard library \ @@ -482,7 +482,7 @@ pub fn new(mut config: Config) -> Build { /// /// This *does not* update the submodule if `config.toml` explicitly says /// not to, or if we're not in a git repository (like a plain source - /// tarball). Typically [`Build::require_and_update_submodule`] should be + /// tarball). Typically [`Build::require_submodule`] should be /// used instead to provide a nice error to the user if the submodule is /// missing. fn update_submodule(&self, relative_path: &str) { @@ -599,7 +599,7 @@ fn update_submodule(&self, relative_path: &str) { /// /// The given `err_hint` will be shown to the user if the submodule is not /// checked out and submodule management is disabled. - pub fn require_and_update_submodule(&self, submodule: &str, err_hint: Option<&str>) { + pub fn require_submodule(&self, submodule: &str, err_hint: Option<&str>) { // When testing bootstrap itself, it is much faster to ignore // submodules. Almost all Steps work fine without their submodules. if cfg!(test) && !self.config.submodules() { @@ -628,7 +628,7 @@ pub fn require_and_update_submodule(&self, submodule: &str, err_hint: Option<&st /// management is disabled and the submodule does not exist. pub fn require_and_update_all_submodules(&self) { for submodule in build_helper::util::parse_gitmodules(&self.src) { - self.require_and_update_submodule(submodule, None); + self.require_submodule(submodule, None); } }