Rollup merge of #136542 - jieyouxu:build-base, r=onur-ozkan

[`compiletest`-related cleanups 4/7] Make the distinction between root build directory vs test suite specific build directory in compiletest less confusing

Reference for overall changes: #136437
Part **4** of **7** of the *`compiletest`-related cleanups* PR series.

### Summary

- Remove `--build-base` compiletest flag, and introduce `--build-{root,test-suite-root}` flags instead. `--build-base` previously actually is test suite specific build directory, not the root `build/` directory.
- Feed the root build directory directly from bootstrap to compiletest via `--build-root` instead of doing multiple layers of parent unwrapping[^parent] based on the test suite specific build directory.
- Remove a redundant `to_path_buf()`.

[^parent]: Please do not unwrap the parents.

r? bootstrap

Author: matthiaskrgr

src/bootstrap/src/core/build_steps/test.rs

@@ -1793,14 +1793,18 @@ NOTE: if you're sure you want to do this, please open an issue as to why. In the
         cmd.arg("--src-root").arg(&builder.src);
         cmd.arg("--src-test-suite-root").arg(builder.src.join("tests").join(suite));
 
-        cmd.arg("--build-base").arg(testdir(builder, compiler.host).join(suite));
+        // N.B. it's important to distinguish between the *root* build directory, the *host* build
+        // directory immediately under the root build directory, and the test-suite-specific build
+        // directory.
+        cmd.arg("--build-root").arg(&builder.out);
+        cmd.arg("--build-test-suite-root").arg(testdir(builder, compiler.host).join(suite));
 
         // When top stage is 0, that means that we're testing an externally provided compiler.
         // In that case we need to use its specific sysroot for tests to pass.
         let sysroot = if builder.top_stage == 0 {
             builder.initial_sysroot.clone()
         } else {
-            builder.sysroot(compiler).to_path_buf()
+            builder.sysroot(compiler)
         };
 
         cmd.arg("--sysroot-base").arg(sysroot);

src/tools/compiletest/src/common.rs

@@ -220,8 +220,10 @@ pub struct Config {
     /// The directory containing the test suite sources. Must be a subdirectory of `src_root`.
     pub src_test_suite_root: PathBuf,
 
-    /// The directory where programs should be built
-    pub build_base: PathBuf,
+    /// Root build directory (e.g. `build/`).
+    pub build_root: PathBuf,
+    /// Test suite specific build directory (e.g. `build/host/test/ui/`).
+    pub build_test_suite_root: PathBuf,
 
     /// The directory containing the compiler sysroot
     pub sysroot_base: PathBuf,
@@ -347,7 +349,7 @@ pub struct Config {
 
     /// If true, this will generate a coverage file with UI test files that run `MachineApplicable`
     /// diagnostics but are missing `run-rustfix` annotations. The generated coverage file is
-    /// created in `/<build_base>/rustfix_missing_coverage.txt`
+    /// created in `<test_suite_build_root>/rustfix_missing_coverage.txt`
     pub rustfix_coverage: bool,
 
     /// whether to run `tidy` (html-tidy) when a rustdoc test fails
@@ -812,12 +814,16 @@ pub const UI_STDERR_16: &str = "16bit.stderr";
 pub const UI_COVERAGE: &str = "coverage";
 pub const UI_COVERAGE_MAP: &str = "cov-map";
 
-/// Absolute path to the directory where all output for all tests in the given
-/// `relative_dir` group should reside. Example:
-///   /path/to/build/host-tuple/test/ui/relative/
+/// Absolute path to the directory where all output for all tests in the given `relative_dir` group
+/// should reside. Example:
+///
+/// ```text
+/// /path/to/build/host-tuple/test/ui/relative/
+/// ```
+///
 /// This is created early when tests are collected to avoid race conditions.
 pub fn output_relative_path(config: &Config, relative_dir: &Path) -> PathBuf {
-    config.build_base.join(relative_dir)
+    config.build_test_suite_root.join(relative_dir)
 }
 
 /// Generates a unique name for the test, such as `testname.revision.mode`.

src/tools/compiletest/src/header.rs

@@ -1070,10 +1070,11 @@ impl Config {
     }
 }
 
+// FIXME(jieyouxu): fix some of these variable names to more accurately reflect what they do.
 fn expand_variables(mut value: String, config: &Config) -> String {
     const CWD: &str = "{{cwd}}";
     const SRC_BASE: &str = "{{src-base}}";
-    const BUILD_BASE: &str = "{{build-base}}";
+    const TEST_SUITE_BUILD_BASE: &str = "{{build-base}}";
     const RUST_SRC_BASE: &str = "{{rust-src-base}}";
     const SYSROOT_BASE: &str = "{{sysroot-base}}";
     const TARGET_LINKER: &str = "{{target-linker}}";
@@ -1088,12 +1089,13 @@ fn expand_variables(mut value: String, config: &Config) -> String {
         value = value.replace(SRC_BASE, &config.src_test_suite_root.to_str().unwrap());
     }
 
-    if value.contains(BUILD_BASE) {
-        value = value.replace(BUILD_BASE, &config.build_base.to_string_lossy());
+    if value.contains(TEST_SUITE_BUILD_BASE) {
+        value =
+            value.replace(TEST_SUITE_BUILD_BASE, &config.build_test_suite_root.to_str().unwrap());
     }
 
     if value.contains(SYSROOT_BASE) {
-        value = value.replace(SYSROOT_BASE, &config.sysroot_base.to_string_lossy());
+        value = value.replace(SYSROOT_BASE, &config.sysroot_base.to_str().unwrap());
     }
 
     if value.contains(TARGET_LINKER) {

src/tools/compiletest/src/header/tests.rs

@@ -155,7 +155,8 @@ impl ConfigBuilder {
             "--jsondocck-path=",
             "--src-root=",
             "--src-test-suite-root=",
-            "--build-base=",
+            "--build-root=",
+            "--build-test-suite-root=",
             "--sysroot-base=",
             "--cc=c",
             "--cxx=c++",

src/tools/compiletest/src/lib.rs

@@ -63,7 +63,8 @@ pub fn parse_config(args: Vec<String>) -> Config {
         .optopt("", "llvm-filecheck", "path to LLVM's FileCheck binary", "DIR")
         .reqopt("", "src-root", "directory containing sources", "PATH")
         .reqopt("", "src-test-suite-root", "directory containing test suite sources", "PATH")
-        .reqopt("", "build-base", "directory to deposit test outputs", "PATH")
+        .reqopt("", "build-root", "path to root build directory", "PATH")
+        .reqopt("", "build-test-suite-root", "path to test suite specific build directory", "PATH")
         .reqopt("", "sysroot-base", "directory containing the compiler sysroot", "PATH")
         .reqopt("", "stage", "stage number under test", "N")
         .reqopt("", "stage-id", "the target-stage identifier", "stageN-TARGET")
@@ -157,7 +158,7 @@ pub fn parse_config(args: Vec<String>) -> Config {
             "",
             "rustfix-coverage",
             "enable this to generate a Rustfix coverage file, which is saved in \
-            `./<build_base>/rustfix_missing_coverage.txt`",
+            `./<build_test_suite_root>/rustfix_missing_coverage.txt`",
         )
         .optflag("", "force-rerun", "rerun tests even if the inputs are unchanged")
         .optflag("", "only-modified", "only run tests that result been modified")
@@ -309,6 +310,10 @@ pub fn parse_config(args: Vec<String>) -> Config {
         src_test_suite_root.display()
     );
 
+    let build_root = opt_path(matches, "build-root");
+    let build_test_suite_root = opt_path(matches, "build-test-suite-root");
+    assert!(build_test_suite_root.starts_with(&build_root));
+
     Config {
         bless: matches.opt_present("bless"),
         compile_lib_path: make_absolute(opt_path(matches, "compile-lib-path")),
@@ -327,7 +332,9 @@ pub fn parse_config(args: Vec<String>) -> Config {
         src_root,
         src_test_suite_root,
 
-        build_base: opt_path(matches, "build-base"),
+        build_root,
+        build_test_suite_root,
+
         sysroot_base: opt_path(matches, "sysroot-base"),
 
         stage,
@@ -438,7 +445,11 @@ pub fn log_config(config: &Config) {
     logv(c, format!("src_root: {}", config.src_root.display()));
     logv(c, format!("src_test_suite_root: {}", config.src_test_suite_root.display()));
 
-    logv(c, format!("build_base: {:?}", config.build_base.display()));
+    logv(c, format!("build_root: {}", config.build_root.display()));
+    logv(c, format!("build_test_suite_root: {}", config.build_test_suite_root.display()));
+
+    logv(c, format!("sysroot_base: {}", config.sysroot_base.display()));
+
     logv(c, format!("stage: {}", config.stage));
     logv(c, format!("stage_id: {}", config.stage_id));
     logv(c, format!("mode: {}", config.mode));
@@ -488,7 +499,7 @@ pub fn run_tests(config: Arc<Config>) {
     // we first make sure that the coverage file does not exist.
     // It will be created later on.
     if config.rustfix_coverage {
-        let mut coverage_file_path = config.build_base.clone();
+        let mut coverage_file_path = config.build_test_suite_root.clone();
         coverage_file_path.push("rustfix_missing_coverage.txt");
         if coverage_file_path.exists() {
             if let Err(e) = fs::remove_file(&coverage_file_path) {

src/tools/compiletest/src/runtest.rs

@@ -535,7 +535,9 @@ impl<'test> TestCx<'test> {
             .arg(&out_dir)
             .arg(&format!("--target={}", target))
             .arg("-L")
-            .arg(&self.config.build_base)
+            // FIXME(jieyouxu): this search path seems questionable. Is this intended for
+            // `rust_test_helpers` in ui tests?
+            .arg(&self.config.build_test_suite_root)
             .arg("-L")
             .arg(aux_dir)
             .arg("-A")
@@ -1366,7 +1368,7 @@ impl<'test> TestCx<'test> {
         // Note: avoid adding a subdirectory of an already filtered directory here, otherwise the
         // same slice of text will be double counted and the truncation might not happen.
         add_path(&self.config.src_test_suite_root);
-        add_path(&self.config.build_base);
+        add_path(&self.config.build_test_suite_root);
 
         read2_abbreviated(child, &filter_paths_from_len).expect("failed to read output")
     }
@@ -1421,7 +1423,7 @@ impl<'test> TestCx<'test> {
     }
 
     fn get_mir_dump_dir(&self) -> PathBuf {
-        let mut mir_dump_dir = PathBuf::from(self.config.build_base.as_path());
+        let mut mir_dump_dir = self.config.build_test_suite_root.clone();
         debug!("input_file: {:?}", self.testpaths.file);
         mir_dump_dir.push(&self.testpaths.relative_dir);
         mir_dump_dir.push(self.testpaths.file.file_stem().unwrap());
@@ -2410,14 +2412,10 @@ impl<'test> TestCx<'test> {
         let rust_src_dir = rust_src_dir.read_link().unwrap_or(rust_src_dir.to_path_buf());
         normalize_path(&rust_src_dir.join("library"), "$SRC_DIR_REAL");
 
-        // Paths into the build directory
-        let test_build_dir = &self.config.build_base;
-        let parent_build_dir = test_build_dir.parent().unwrap().parent().unwrap().parent().unwrap();
-
         // eg. /home/user/rust/build/x86_64-unknown-linux-gnu/test/ui
-        normalize_path(test_build_dir, "$TEST_BUILD_DIR");
+        normalize_path(&self.config.build_test_suite_root, "$TEST_BUILD_DIR");
         // eg. /home/user/rust/build
-        normalize_path(parent_build_dir, "$BUILD_DIR");
+        normalize_path(&self.config.build_root, "$BUILD_DIR");
 
         if json {
             // escaped newlines in json strings should be readable

src/tools/compiletest/src/runtest/run_make.rs

@@ -179,12 +179,7 @@ impl TestCx<'_> {
         //    library.
         // 2. We need to run the recipe binary.
 
-        // `self.config.build_base` is actually the build base folder + "test" + test suite name, it
-        // looks like `build/<host_triple>/test/run-make`. But we want `build/<host_triple>/`. Note
-        // that the `build` directory does not need to be called `build`, nor does it need to be
-        // under `src_root`, so we must compute it based off of `self.config.build_base`.
-        let build_root =
-            self.config.build_base.parent().and_then(Path::parent).unwrap().to_path_buf();
+        let host_build_root = self.config.build_root.join(&self.config.host);
 
         // We construct the following directory tree for each rmake.rs test:
         // ```
@@ -242,10 +237,10 @@ impl TestCx<'_> {
 
         let stage_number = self.config.stage;
 
-        let stage_tools_bin = build_root.join(format!("stage{stage_number}-tools-bin"));
+        let stage_tools_bin = host_build_root.join(format!("stage{stage_number}-tools-bin"));
         let support_lib_path = stage_tools_bin.join("librun_make_support.rlib");
 
-        let stage_tools = build_root.join(format!("stage{stage_number}-tools"));
+        let stage_tools = host_build_root.join(format!("stage{stage_number}-tools"));
         let support_lib_deps = stage_tools.join(&self.config.host).join("release").join("deps");
         let support_lib_deps_deps = stage_tools.join("release").join("deps");
 
@@ -311,7 +306,7 @@ impl TestCx<'_> {
         // to work correctly.
         //
         // See <https://github.com/rust-lang/rust/pull/122248> for more background.
-        let stage0_sysroot = build_root.join("stage0-sysroot");
+        let stage0_sysroot = host_build_root.join("stage0-sysroot");
         if std::env::var_os("COMPILETEST_FORCE_STAGE0").is_some() {
             rustc.arg("--sysroot").arg(&stage0_sysroot);
         }
@@ -326,7 +321,7 @@ impl TestCx<'_> {
         // provided through env vars.
 
         // Compute stage-specific standard library paths.
-        let stage_std_path = build_root.join(format!("stage{stage_number}")).join("lib");
+        let stage_std_path = host_build_root.join(format!("stage{stage_number}")).join("lib");
 
         // Compute dynamic library search paths for recipes.
         let recipe_dylib_search_paths = {
@@ -372,7 +367,7 @@ impl TestCx<'_> {
             // Provide path to sources root.
             .env("SOURCE_ROOT", &self.config.src_root)
             // Path to the host build directory.
-            .env("BUILD_ROOT", &build_root)
+            .env("BUILD_ROOT", &host_build_root)
             // Provide path to stage-corresponding rustc.
             .env("RUSTC", &self.config.rustc_path)
             // Provide the directory to libraries that are needed to run the *compiler*. This is not

src/tools/compiletest/src/runtest/ui.rs

@@ -66,7 +66,7 @@ impl TestCx<'_> {
                 && !self.props.run_rustfix
                 && !self.props.rustfix_only_machine_applicable
             {
-                let mut coverage_file_path = self.config.build_base.clone();
+                let mut coverage_file_path = self.config.build_test_suite_root.clone();
                 coverage_file_path.push("rustfix_missing_coverage.txt");
                 debug!("coverage_file_path: {}", coverage_file_path.display());