Add require_and_update_submodule to ensure submodules exist

ehuss · ehuss · commit bbe9056c2a68 · 2024-07-25T16:40:37.000-07:00
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.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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) {
