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.

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::builder::{
 };
 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 @@ impl Step for Std {
     }
 
     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 @@ impl Step for Std {
     }
 
     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 @@ impl Step for Std {
             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 @@ impl Step for Src {
     /// 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 @@ impl Step for PlainSourceTarball {
             // 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::build_steps::tool::{self, prepare_tool_cargo, SourceType, Tool}
 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 @@ macro_rules! book {
 
             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 @@ impl Step for TheBook {
     /// * 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 @@ macro_rules! tool_doc {
                     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 @@ impl Step for CrtBeginEnd {
 
     /// 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 @@ impl Step for Libunwind {
 
     /// 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 @@ impl Step for RustcGuide {
     }
 
     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 @@ impl Step for Bootstrap {
         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 @@ macro_rules! bootstrap_tool {
             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 @@ impl Step for OptimizedDist {
     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 @@ impl Step for RustcPerf {
 
     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 @@ impl Step for Cargo {
     }
 
     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 @@ impl Step for Vendor {
         }
 
         // 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 @@ impl 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 @@ impl Build {
         }
     }
 
+    /// 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) {