Skip to content

Commit a40b73e

Browse files
committed
Auto merge of rust-lang#127958 - jieyouxu:compiletest-rmake-cleanup, r=<try>
Cleanup rmake.rs setup in compiletest While debugging rmake.rs tests I realized that the rmake.rs setup itself in compiletest is very messy and confused. Now that I know some of the bootstrap steps and the rmake.rs tests themselves better, I realized there are cleanups that are possible: - Rework how `source_root` and `build_root` are calculated. They should now be less fragile then before. - Shuffle around path calculations to make them more logically grouped and closer to eventual use site(s). - Cleanup executable extension calculation with `std::env::consts::EXE_EXTENSION`. - Cleanup various dylib search path handling: renamed variables to better reflect their purpose, minimized mutability scope of said variables. - Prune useless env vars passed to both `rustc` and recipe binary commands. - Vastly improve the documentation for the setup of rmake.rs tests, including assumed bootstrap-provided build layouts, rmake.rs test layout, dylib search paths, intended purpose of passed env vars and the `COMPILETEST_FORCE_STAGE0=1 ./x test run-make --stage 0` stage0 sysroot special handling. This PR is best reviewed commit-by-commit. r? bootstrap (or Kobzol, or Mark, or T-compiler) try-job: aarch64-apple try-job: armhf-gnu try-job: dist-x86_64-linux try-job: test-various try-job: x86_64-mingw try-job: x86_64-msvc try-job: x86_64-gnu-llvm-18
2 parents 8c3a94a + 24e6599 commit a40b73e

File tree

1 file changed

+215
-86
lines changed

1 file changed

+215
-86
lines changed

src/tools/compiletest/src/runtest.rs

+215-86
Original file line numberDiff line numberDiff line change
@@ -3431,29 +3431,53 @@ impl<'test> TestCx<'test> {
34313431

34323432
fn run_rmake_v2_test(&self) {
34333433
// For `run-make` V2, we need to perform 2 steps to build and run a `run-make` V2 recipe
3434-
// (`rmake.rs`) to run the actual tests. The support library is already built as a tool
3435-
// dylib and is available under `build/$TARGET/stageN-tools-bin/librun_make_support.rlib`.
3434+
// (`rmake.rs`) to run the actual tests. The support library is already built as a tool rust
3435+
// library and is available under `build/$TARGET/stageN-tools-bin/librun_make_support.rlib`.
34363436
//
3437-
// 1. We need to build the recipe `rmake.rs` and link in the support library.
3438-
// 2. We need to run the recipe to build and run the tests.
3439-
let cwd = env::current_dir().unwrap();
3440-
let src_root = self.config.src_base.parent().unwrap().parent().unwrap();
3441-
let src_root = cwd.join(&src_root);
3442-
let build_root = self.config.build_base.parent().unwrap().parent().unwrap();
3443-
let build_root = cwd.join(&build_root);
3437+
// 1. We need to build the recipe `rmake.rs` as a binary and link in the `run_make_support`
3438+
// library.
3439+
// 2. We need to run the recipe binary.
3440+
3441+
// So we assume the rust-lang/rust project setup looks like (our `.` is the top-level
3442+
// directory, irrelevant entries to our purposes omitted):
3443+
//
3444+
// ```
3445+
// . // <- `source_root`
3446+
// ├── build/ // <- `build_root`
3447+
// ├── compiler/
3448+
// ├── library/
3449+
// ├── src/
3450+
// │ └── tools/
3451+
// │ └── run_make_support/
3452+
// └── tests
3453+
// └── run-make/
3454+
// ```
3455+
3456+
// `source_root` is the top-level directory containing the rust-lang/rust checkout.
3457+
let source_root =
3458+
self.config.find_rust_src_root().expect("could not determine rust source root");
3459+
debug!(?source_root);
3460+
// `self.config.build_base` looks like `build/<host_tuplet>/test/run-make`, but we want
3461+
// `build/<host_tuplet>/`.
3462+
debug!(?self.config.host);
3463+
debug!(?self.config.target);
3464+
debug!(?self.config.build_base);
3465+
let build_root = source_root.join("build").join(&self.config.host);
3466+
debug!(?build_root);
34443467

34453468
// We construct the following directory tree for each rmake.rs test:
34463469
// ```
3447-
// base_dir/
3470+
// <base_dir>/
34483471
// rmake.exe
34493472
// rmake_out/
34503473
// ```
3451-
// having the executable separate from the output artifacts directory allows the recipes to
3452-
// `remove_dir_all($TMPDIR)` without running into permission denied issues because
3453-
// the executable is not under the `rmake_out/` directory.
3474+
// having the recipe executable separate from the output artifacts directory allows the
3475+
// recipes to `remove_dir_all($TMPDIR)` without running into issues related trying to remove
3476+
// a currently running executable because the recipe executable is not under the
3477+
// `rmake_out/` directory.
34543478
//
34553479
// This setup intentionally diverges from legacy Makefile run-make tests.
3456-
let base_dir = cwd.join(self.output_base_name());
3480+
let base_dir = self.output_base_name();
34573481
if base_dir.exists() {
34583482
self.aggressive_rm_rf(&base_dir).unwrap();
34593483
}
@@ -3475,120 +3499,225 @@ impl<'test> TestCx<'test> {
34753499
}
34763500
}
34773501

3478-
// HACK: assume stageN-target, we only want stageN.
3502+
// `self.config.stage_id` looks like `stage1-<target_tuplet>`, but we only want
3503+
// the `stage1` part as that is what the output directories of bootstrap are prefixed with.
3504+
// Note that this *assumes* build layout from bootstrap is produced as:
3505+
//
3506+
// ```
3507+
// build/<target_tuplet>/ // <- this is `build_root`
3508+
// ├── stage0
3509+
// ├── stage0-bootstrap-tools
3510+
// ├── stage0-codegen
3511+
// ├── stage0-rustc
3512+
// ├── stage0-std
3513+
// ├── stage0-sysroot
3514+
// ├── stage0-tools
3515+
// ├── stage0-tools-bin
3516+
// ├── stage1
3517+
// ├── stage1-std
3518+
// ├── stage1-tools
3519+
// ├── stage1-tools-bin
3520+
// └── test
3521+
// ```
3522+
// FIXME(jieyouxu): improve the communication between bootstrap and compiletest here so
3523+
// we don't have to hack out a `stageN`.
3524+
debug!(?self.config.stage_id);
34793525
let stage = self.config.stage_id.split('-').next().unwrap();
34803526

3481-
// First, we construct the path to the built support library.
3482-
let mut support_lib_path = PathBuf::new();
3483-
support_lib_path.push(&build_root);
3484-
support_lib_path.push(format!("{}-tools-bin", stage));
3485-
support_lib_path.push("librun_make_support.rlib");
3486-
3487-
let mut stage_std_path = PathBuf::new();
3488-
stage_std_path.push(&build_root);
3489-
stage_std_path.push(&stage);
3490-
stage_std_path.push("lib");
3491-
3492-
// Then, we need to build the recipe `rmake.rs` and link in the support library.
3493-
let recipe_bin = base_dir.join(if self.config.target.contains("windows") {
3494-
"rmake.exe"
3495-
} else {
3496-
"rmake"
3497-
});
3498-
3499-
let mut support_lib_deps = PathBuf::new();
3500-
support_lib_deps.push(&build_root);
3501-
support_lib_deps.push(format!("{}-tools", stage));
3502-
support_lib_deps.push(&self.config.host);
3503-
support_lib_deps.push("release");
3504-
support_lib_deps.push("deps");
3505-
3506-
let mut support_lib_deps_deps = PathBuf::new();
3507-
support_lib_deps_deps.push(&build_root);
3508-
support_lib_deps_deps.push(format!("{}-tools", stage));
3509-
support_lib_deps_deps.push("release");
3510-
support_lib_deps_deps.push("deps");
3527+
// In order to link in the support library as a rlib when compiling recipes, we need three
3528+
// paths:
3529+
// 1. Path of the built support library rlib itself.
3530+
// 2. Path of the built support library's dependencies directory.
3531+
// 3. Path of the built support library's dependencies' dependencies directory.
3532+
//
3533+
// The paths look like
3534+
//
3535+
// ```
3536+
// build/<target_tuplet>/
3537+
// ├── stageN-tools-bin/
3538+
// │ └── librun_make_support.rlib // <- support rlib itself
3539+
// ├── stageN-tools/
3540+
// │ ├── release/deps/ // <- deps of deps
3541+
// │ └── <host_tuplet>/release/deps/ // <- deps
3542+
// ```
3543+
//
3544+
// There almost certainly is a better way to do this, but this seems to work for now.
35113545

3546+
let support_lib_path = {
3547+
let mut p = build_root.clone();
3548+
p.push(format!("{}-tools-bin", stage));
3549+
p.push("librun_make_support.rlib");
3550+
p
3551+
};
3552+
debug!(?support_lib_path);
3553+
3554+
let support_lib_deps = {
3555+
let mut p = build_root.clone();
3556+
p.push(format!("{}-tools", stage));
3557+
p.push(&self.config.host);
3558+
p.push("release");
3559+
p.push("deps");
3560+
p
3561+
};
35123562
debug!(?support_lib_deps);
3563+
3564+
let support_lib_deps_deps = {
3565+
let mut p = build_root.clone();
3566+
p.push(format!("{}-tools", stage));
3567+
p.push("release");
3568+
p.push("deps");
3569+
p
3570+
};
35133571
debug!(?support_lib_deps_deps);
35143572

3515-
let orig_dylib_env_paths =
3573+
// To compile the recipe with rustc, we need to provide suitable dynamic library search
3574+
// paths to rustc. This includes both:
3575+
// 1. The "base" dylib search paths that was provided to compiletest, e.g. `LD_LIBRARY_PATH`
3576+
// on some linux distros.
3577+
// 2. Specific library paths in `self.config.compile_lib_path` needed for running rustc.
3578+
3579+
let base_dylib_search_paths =
35163580
Vec::from_iter(env::split_paths(&env::var(dylib_env_var()).unwrap()));
35173581

3518-
let mut host_dylib_env_paths = Vec::new();
3519-
host_dylib_env_paths.push(cwd.join(&self.config.compile_lib_path));
3520-
host_dylib_env_paths.extend(orig_dylib_env_paths.iter().cloned());
3521-
let host_dylib_env_paths = env::join_paths(host_dylib_env_paths).unwrap();
3582+
let host_dylib_search_paths = {
3583+
let mut paths = vec![self.config.compile_lib_path.clone()];
3584+
paths.extend(base_dylib_search_paths.iter().cloned());
3585+
paths
3586+
};
3587+
3588+
// Calculate the paths of the recipe binary. As previously discussed, this is placed at
3589+
// `<base_dir>/<bin_name>` with `bin_name` being `rmake` or `rmake.exe` dependending on
3590+
// platform.
3591+
let recipe_bin = {
3592+
let mut p = base_dir.join("rmake");
3593+
p.set_extension(env::consts::EXE_EXTENSION);
3594+
p
3595+
};
3596+
debug!(?recipe_bin);
35223597

3523-
let mut cmd = Command::new(&self.config.rustc_path);
3524-
cmd.arg("-o")
3598+
let mut rustc = Command::new(&self.config.rustc_path);
3599+
rustc
3600+
// Specify output path
3601+
.arg("-o")
35253602
.arg(&recipe_bin)
3603+
// Specify library search paths for `run_make_support`.
35263604
.arg(format!("-Ldependency={}", &support_lib_path.parent().unwrap().to_string_lossy()))
35273605
.arg(format!("-Ldependency={}", &support_lib_deps.to_string_lossy()))
35283606
.arg(format!("-Ldependency={}", &support_lib_deps_deps.to_string_lossy()))
3607+
// Provide `run_make_support` as extern prelude, so test writers don't need to write
3608+
// `extern run_make_support;`.
35293609
.arg("--extern")
35303610
.arg(format!("run_make_support={}", &support_lib_path.to_string_lossy()))
3611+
// Default to Edition 2021.
35313612
.arg("--edition=2021")
3613+
// Specify compilation target.
3614+
.arg("--target")
3615+
.arg(&self.config.target)
3616+
// The recipe file itself.
35323617
.arg(&self.testpaths.file.join("rmake.rs"))
3533-
.env("TARGET", &self.config.target)
3534-
.env("PYTHON", &self.config.python)
3535-
.env("RUST_BUILD_STAGE", &self.config.stage_id)
3536-
.env("RUSTC", cwd.join(&self.config.rustc_path))
3537-
.env("LD_LIB_PATH_ENVVAR", dylib_env_var())
3538-
.env(dylib_env_var(), &host_dylib_env_paths)
3539-
.env("HOST_RPATH_DIR", cwd.join(&self.config.compile_lib_path))
3540-
.env("TARGET_RPATH_DIR", cwd.join(&self.config.run_lib_path))
3541-
.env("LLVM_COMPONENTS", &self.config.llvm_components);
3618+
// Provide necessary library search paths for rustc.
3619+
.env(dylib_env_var(), &env::join_paths(host_dylib_search_paths).unwrap());
35423620

35433621
// In test code we want to be very pedantic about values being silently discarded that are
35443622
// annotated with `#[must_use]`.
3545-
cmd.arg("-Dunused_must_use");
3546-
3623+
rustc.arg("-Dunused_must_use");
3624+
3625+
// > `cg_clif` uses `COMPILETEST_FORCE_STAGE0=1 ./x.py test --stage 0` for running the rustc
3626+
// > test suite. With the introduction of rmake.rs this broke. `librun_make_support.rlib` is
3627+
// > compiled using the bootstrap rustc wrapper which sets `--sysroot
3628+
// > build/aarch64-unknown-linux-gnu/stage0-sysroot`, but then compiletest will compile
3629+
// > `rmake.rs` using the sysroot of the bootstrap compiler causing it to not find the
3630+
// > `libstd.rlib` against which `librun_make_support.rlib` is compiled.
3631+
//
3632+
// The gist here is that we have to pass the proper stage0 sysroot if we want
3633+
//
3634+
// ```
3635+
// $ COMPILETEST_FORCE_STAGE0=1 ./x test run-make --stage 0
3636+
// ```
3637+
//
3638+
// to work correctly.
3639+
//
3640+
// See <https://github.com/rust-lang/rust/pull/122248> for more background.
35473641
if std::env::var_os("COMPILETEST_FORCE_STAGE0").is_some() {
3548-
let mut stage0_sysroot = build_root.clone();
3549-
stage0_sysroot.push("stage0-sysroot");
3642+
let stage0_sysroot = {
3643+
let mut p = build_root.clone();
3644+
p.push("stage0-sysroot");
3645+
p
3646+
};
35503647
debug!(?stage0_sysroot);
3551-
debug!(exists = stage0_sysroot.exists());
35523648

3553-
cmd.arg("--sysroot").arg(&stage0_sysroot);
3649+
rustc.arg("--sysroot").arg(&stage0_sysroot);
35543650
}
35553651

3556-
let res = self.run_command_to_procres(&mut cmd);
3652+
// Now run rustc to build the recipe.
3653+
let res = self.run_command_to_procres(&mut rustc);
35573654
if !res.status.success() {
35583655
self.fatal_proc_rec("run-make test failed: could not build `rmake.rs` recipe", &res);
35593656
}
35603657

3561-
// Finally, we need to run the recipe binary to build and run the actual tests.
3562-
debug!(?recipe_bin);
3658+
// To actually run the recipe, we have to provide the recipe with a bunch of information
3659+
// provided through env vars.
35633660

3564-
let mut dylib_env_paths = orig_dylib_env_paths.clone();
3565-
dylib_env_paths.push(support_lib_path.parent().unwrap().to_path_buf());
3566-
dylib_env_paths.push(stage_std_path.join("rustlib").join(&self.config.host).join("lib"));
3567-
let dylib_env_paths = env::join_paths(dylib_env_paths).unwrap();
3661+
// Compute stage-specific standard library paths.
3662+
let stage_std_path = {
3663+
let mut p = build_root.clone();
3664+
p.push(&stage);
3665+
p.push("lib");
3666+
p
3667+
};
3668+
debug!(?stage_std_path);
3669+
3670+
// Compute dynamic library search paths for recipes.
3671+
let recipe_dylib_search_paths = {
3672+
let mut paths = base_dylib_search_paths.clone();
3673+
paths.push(support_lib_path.parent().unwrap().to_path_buf());
3674+
paths.push(stage_std_path.join("rustlib").join(&self.config.host).join("lib"));
3675+
paths
3676+
};
3677+
debug!(?recipe_dylib_search_paths);
35683678

3569-
let mut target_rpath_env_path = Vec::new();
3570-
target_rpath_env_path.push(&rmake_out_dir);
3571-
target_rpath_env_path.extend(&orig_dylib_env_paths);
3572-
let target_rpath_env_path = env::join_paths(target_rpath_env_path).unwrap();
3679+
// Compute runtime library search paths for recipes. This is target-specific.
3680+
let target_runtime_dylib_search_paths = {
3681+
let mut paths = vec![rmake_out_dir.clone()];
3682+
paths.extend(base_dylib_search_paths.iter().cloned());
3683+
paths
3684+
};
35733685

3686+
// FIXME(jieyouxu): please rename `TARGET_RPATH_ENV`, `HOST_RPATH_DIR` and
3687+
// `TARGET_RPATH_DIR`, it is **extremely** confusing!
35743688
let mut cmd = Command::new(&recipe_bin);
35753689
cmd.current_dir(&rmake_out_dir)
35763690
.stdout(Stdio::piped())
35773691
.stderr(Stdio::piped())
3692+
// Provide the target-specific env var that is used to record dylib search paths. For
3693+
// example, this could be `LD_LIBRARY_PATH` on some linux distros but `PATH` on Windows.
35783694
.env("LD_LIB_PATH_ENVVAR", dylib_env_var())
3579-
.env("TARGET_RPATH_ENV", &target_rpath_env_path)
3580-
.env(dylib_env_var(), &dylib_env_paths)
3695+
// Provide the dylib search paths.
3696+
.env(dylib_env_var(), &env::join_paths(recipe_dylib_search_paths).unwrap())
3697+
// Provide runtime dylib search paths.
3698+
.env("TARGET_RPATH_ENV", &env::join_paths(target_runtime_dylib_search_paths).unwrap())
3699+
// Provide the target.
35813700
.env("TARGET", &self.config.target)
3701+
// Some tests unfortunately still need Python, so provide path to a Python interpreter.
35823702
.env("PYTHON", &self.config.python)
3583-
.env("SOURCE_ROOT", &src_root)
3584-
.env("RUST_BUILD_STAGE", &self.config.stage_id)
3585-
.env("RUSTC", cwd.join(&self.config.rustc_path))
3586-
.env("HOST_RPATH_DIR", cwd.join(&self.config.compile_lib_path))
3587-
.env("TARGET_RPATH_DIR", cwd.join(&self.config.run_lib_path))
3703+
// Provide path to checkout root. This is the top-level directory containing
3704+
// rust-lang/rust checkout.
3705+
.env("SOURCE_ROOT", &source_root)
3706+
// Provide path to stage-corresponding rustc.
3707+
.env("RUSTC", &self.config.rustc_path)
3708+
// Provide the directory to libraries that are needed to run the *compiler*. This is not
3709+
// to be confused with `TARGET_RPATH_ENV` or `TARGET_RPATH_DIR`. This is needed if the
3710+
// recipe wants to invoke rustc.
3711+
.env("HOST_RPATH_DIR", &self.config.compile_lib_path)
3712+
// Provide the directory to libraries that might be needed to run compiled binaries
3713+
// (further compiled by the recipe!).
3714+
.env("TARGET_RPATH_DIR", &self.config.run_lib_path)
3715+
// Provide which LLVM components are available (e.g. which LLVM components are provided
3716+
// through a specific CI runner).
35883717
.env("LLVM_COMPONENTS", &self.config.llvm_components);
35893718

35903719
if let Some(ref rustdoc) = self.config.rustdoc_path {
3591-
cmd.env("RUSTDOC", cwd.join(rustdoc));
3720+
cmd.env("RUSTDOC", source_root.join(rustdoc));
35923721
}
35933722

35943723
if let Some(ref node) = self.config.nodejs {

0 commit comments

Comments
 (0)