Skip to content

Commit

Permalink
Rollup merge of #127958 - jieyouxu:compiletest-rmake-cleanup, r=Kobzol
Browse files Browse the repository at this point in the history 
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.

Fixes #127920.

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
  • Loading branch information
@matthiaskrgr
matthiaskrgr committed Jul 20, 2024
2 parents dd8113e + a8463be commit ae28d5c
Show file tree
Hide file tree
Showing 2 changed files with 186 additions and 92 deletions.
10 changes: 8 additions & 2 deletions src/tools/compiletest/src/common.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -760,8 +760,14 @@ pub fn output_testname_unique(
/// test/revision should reside. Example:
/// /path/to/build/host-triple/test/ui/relative/testname.revision.mode/
pub fn output_base_dir(config: &Config, testpaths: &TestPaths, revision: Option<&str>) -> PathBuf {
output_relative_path(config, &testpaths.relative_dir)
.join(output_testname_unique(config, testpaths, revision))
// In run-make tests, constructing a relative path + unique testname causes a double layering
// since revisions are not supported, causing unnecessary nesting.
if config.mode == Mode::RunMake {
output_relative_path(config, &testpaths.relative_dir)
} else {
output_relative_path(config, &testpaths.relative_dir)
.join(output_testname_unique(config, testpaths, revision))
}
}

/// Absolute path to the base filename used as output for the given
Expand Down
268 changes: 178 additions & 90 deletions src/tools/compiletest/src/runtest.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3433,29 +3433,51 @@ impl<'test> TestCx<'test> {

fn run_rmake_v2_test(&self) {
// For `run-make` V2, we need to perform 2 steps to build and run a `run-make` V2 recipe
// (`rmake.rs`) to run the actual tests. The support library is already built as a tool
// dylib and is available under `build/$TARGET/stageN-tools-bin/librun_make_support.rlib`.
// (`rmake.rs`) to run the actual tests. The support library is already built as a tool rust
// library and is available under `build/$TARGET/stageN-tools-bin/librun_make_support.rlib`.
//
// 1. We need to build the recipe `rmake.rs` and link in the support library.
// 2. We need to run the recipe to build and run the tests.
let cwd = env::current_dir().unwrap();
let src_root = self.config.src_base.parent().unwrap().parent().unwrap();
let src_root = cwd.join(&src_root);
let build_root = self.config.build_base.parent().unwrap().parent().unwrap();
let build_root = cwd.join(&build_root);
// 1. We need to build the recipe `rmake.rs` as a binary and link in the `run_make_support`
// library.
// 2. We need to run the recipe binary.

// So we assume the rust-lang/rust project setup looks like the following (our `.` is the
// top-level directory, irrelevant entries to our purposes omitted):
//
// ```
// . // <- `source_root`
// ├── build/ // <- `build_root`
// ├── compiler/
// ├── library/
// ├── src/
// │ └── tools/
// │ └── run_make_support/
// └── tests
// └── run-make/
// ```

// `source_root` is the top-level directory containing the rust-lang/rust checkout.
let source_root =
self.config.find_rust_src_root().expect("could not determine rust source root");
// `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 `source_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();

// We construct the following directory tree for each rmake.rs test:
// ```
// base_dir/
// <base_dir>/
// rmake.exe
// rmake_out/
// ```
// having the executable separate from the output artifacts directory allows the recipes to
// `remove_dir_all($TMPDIR)` without running into permission denied issues because
// the executable is not under the `rmake_out/` directory.
// having the recipe executable separate from the output artifacts directory allows the
// recipes to `remove_dir_all($TMPDIR)` without running into issues related trying to remove
// a currently running executable because the recipe executable is not under the
// `rmake_out/` directory.
//
// This setup intentionally diverges from legacy Makefile run-make tests.
let base_dir = cwd.join(self.output_base_name());
let base_dir = self.output_base_name();
if base_dir.exists() {
self.aggressive_rm_rf(&base_dir).unwrap();
}
Expand All @@ -3477,120 +3499,186 @@ impl<'test> TestCx<'test> {
}
}

// HACK: assume stageN-target, we only want stageN.
// `self.config.stage_id` looks like `stage1-<target_triple>`, but we only want
// the `stage1` part as that is what the output directories of bootstrap are prefixed with.
// Note that this *assumes* build layout from bootstrap is produced as:
//
// ```
// build/<target_triple>/ // <- this is `build_root`
// ├── stage0
// ├── stage0-bootstrap-tools
// ├── stage0-codegen
// ├── stage0-rustc
// ├── stage0-std
// ├── stage0-sysroot
// ├── stage0-tools
// ├── stage0-tools-bin
// ├── stage1
// ├── stage1-std
// ├── stage1-tools
// ├── stage1-tools-bin
// └── test
// ```
// FIXME(jieyouxu): improve the communication between bootstrap and compiletest here so
// we don't have to hack out a `stageN`.
let stage = self.config.stage_id.split('-').next().unwrap();

// First, we construct the path to the built support library.
let mut support_lib_path = PathBuf::new();
support_lib_path.push(&build_root);
support_lib_path.push(format!("{}-tools-bin", stage));
support_lib_path.push("librun_make_support.rlib");
// In order to link in the support library as a rlib when compiling recipes, we need three
// paths:
// 1. Path of the built support library rlib itself.
// 2. Path of the built support library's dependencies directory.
// 3. Path of the built support library's dependencies' dependencies directory.
//
// The paths look like
//
// ```
// build/<target_triple>/
// ├── stageN-tools-bin/
// │ └── librun_make_support.rlib // <- support rlib itself
// ├── stageN-tools/
// │ ├── release/deps/ // <- deps of deps
// │ └── <host_triple>/release/deps/ // <- deps
// ```
//
// FIXME(jieyouxu): there almost certainly is a better way to do this (specifically how the
// support lib and its deps are organized, can't we copy them to the tools-bin dir as
// well?), but this seems to work for now.

let mut stage_std_path = PathBuf::new();
stage_std_path.push(&build_root);
stage_std_path.push(&stage);
stage_std_path.push("lib");
let stage_tools_bin = build_root.join(format!("{stage}-tools-bin"));
let support_lib_path = stage_tools_bin.join("librun_make_support.rlib");

// Then, we need to build the recipe `rmake.rs` and link in the support library.
let recipe_bin = base_dir.join(if self.config.target.contains("windows") {
"rmake.exe"
} else {
"rmake"
});

let mut support_lib_deps = PathBuf::new();
support_lib_deps.push(&build_root);
support_lib_deps.push(format!("{}-tools", stage));
support_lib_deps.push(&self.config.host);
support_lib_deps.push("release");
support_lib_deps.push("deps");

let mut support_lib_deps_deps = PathBuf::new();
support_lib_deps_deps.push(&build_root);
support_lib_deps_deps.push(format!("{}-tools", stage));
support_lib_deps_deps.push("release");
support_lib_deps_deps.push("deps");

debug!(?support_lib_deps);
debug!(?support_lib_deps_deps);

let orig_dylib_env_paths =
let stage_tools = build_root.join(format!("{stage}-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");

// To compile the recipe with rustc, we need to provide suitable dynamic library search
// paths to rustc. This includes both:
// 1. The "base" dylib search paths that was provided to compiletest, e.g. `LD_LIBRARY_PATH`
// on some linux distros.
// 2. Specific library paths in `self.config.compile_lib_path` needed for running rustc.

let base_dylib_search_paths =
Vec::from_iter(env::split_paths(&env::var(dylib_env_var()).unwrap()));

let mut host_dylib_env_paths = Vec::new();
host_dylib_env_paths.push(cwd.join(&self.config.compile_lib_path));
host_dylib_env_paths.extend(orig_dylib_env_paths.iter().cloned());
let host_dylib_env_paths = env::join_paths(host_dylib_env_paths).unwrap();
let host_dylib_search_paths = {
let mut paths = vec![self.config.compile_lib_path.clone()];
paths.extend(base_dylib_search_paths.iter().cloned());
paths
};

// Calculate the paths of the recipe binary. As previously discussed, this is placed at
// `<base_dir>/<bin_name>` with `bin_name` being `rmake` or `rmake.exe` depending on
// platform.
let recipe_bin = {
let mut p = base_dir.join("rmake");
p.set_extension(env::consts::EXE_EXTENSION);
p
};

let mut cmd = Command::new(&self.config.rustc_path);
cmd.arg("-o")
let mut rustc = Command::new(&self.config.rustc_path);
rustc
.arg("-o")
.arg(&recipe_bin)
// Specify library search paths for `run_make_support`.
.arg(format!("-Ldependency={}", &support_lib_path.parent().unwrap().to_string_lossy()))
.arg(format!("-Ldependency={}", &support_lib_deps.to_string_lossy()))
.arg(format!("-Ldependency={}", &support_lib_deps_deps.to_string_lossy()))
// Provide `run_make_support` as extern prelude, so test writers don't need to write
// `extern run_make_support;`.
.arg("--extern")
.arg(format!("run_make_support={}", &support_lib_path.to_string_lossy()))
.arg("--edition=2021")
.arg(&self.testpaths.file.join("rmake.rs"))
.env("TARGET", &self.config.target)
.env("PYTHON", &self.config.python)
.env("RUST_BUILD_STAGE", &self.config.stage_id)
.env("RUSTC", cwd.join(&self.config.rustc_path))
.env("LD_LIB_PATH_ENVVAR", dylib_env_var())
.env(dylib_env_var(), &host_dylib_env_paths)
.env("HOST_RPATH_DIR", cwd.join(&self.config.compile_lib_path))
.env("TARGET_RPATH_DIR", cwd.join(&self.config.run_lib_path))
.env("LLVM_COMPONENTS", &self.config.llvm_components);
// Provide necessary library search paths for rustc.
.env(dylib_env_var(), &env::join_paths(host_dylib_search_paths).unwrap());

// In test code we want to be very pedantic about values being silently discarded that are
// annotated with `#[must_use]`.
cmd.arg("-Dunused_must_use");

rustc.arg("-Dunused_must_use");

// > `cg_clif` uses `COMPILETEST_FORCE_STAGE0=1 ./x.py test --stage 0` for running the rustc
// > test suite. With the introduction of rmake.rs this broke. `librun_make_support.rlib` is
// > compiled using the bootstrap rustc wrapper which sets `--sysroot
// > build/aarch64-unknown-linux-gnu/stage0-sysroot`, but then compiletest will compile
// > `rmake.rs` using the sysroot of the bootstrap compiler causing it to not find the
// > `libstd.rlib` against which `librun_make_support.rlib` is compiled.
//
// The gist here is that we have to pass the proper stage0 sysroot if we want
//
// ```
// $ COMPILETEST_FORCE_STAGE0=1 ./x test run-make --stage 0
// ```
//
// to work correctly.
//
// See <https://github.com/rust-lang/rust/pull/122248> for more background.
if std::env::var_os("COMPILETEST_FORCE_STAGE0").is_some() {
let mut stage0_sysroot = build_root.clone();
stage0_sysroot.push("stage0-sysroot");
debug!(?stage0_sysroot);
debug!(exists = stage0_sysroot.exists());

cmd.arg("--sysroot").arg(&stage0_sysroot);
let stage0_sysroot = build_root.join("stage0-sysroot");
rustc.arg("--sysroot").arg(&stage0_sysroot);
}

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

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

// Compute stage-specific standard library paths.
let stage_std_path = build_root.join(&stage).join("lib");

let mut dylib_env_paths = orig_dylib_env_paths.clone();
dylib_env_paths.push(support_lib_path.parent().unwrap().to_path_buf());
dylib_env_paths.push(stage_std_path.join("rustlib").join(&self.config.host).join("lib"));
let dylib_env_paths = env::join_paths(dylib_env_paths).unwrap();
// Compute dynamic library search paths for recipes.
let recipe_dylib_search_paths = {
let mut paths = base_dylib_search_paths.clone();
paths.push(support_lib_path.parent().unwrap().to_path_buf());
paths.push(stage_std_path.join("rustlib").join(&self.config.host).join("lib"));
paths
};

let mut target_rpath_env_path = Vec::new();
target_rpath_env_path.push(&rmake_out_dir);
target_rpath_env_path.extend(&orig_dylib_env_paths);
let target_rpath_env_path = env::join_paths(target_rpath_env_path).unwrap();
// Compute runtime library search paths for recipes. This is target-specific.
let target_runtime_dylib_search_paths = {
let mut paths = vec![rmake_out_dir.clone()];
paths.extend(base_dylib_search_paths.iter().cloned());
paths
};

// FIXME(jieyouxu): please rename `TARGET_RPATH_ENV`, `HOST_RPATH_DIR` and
// `TARGET_RPATH_DIR`, it is **extremely** confusing!
let mut cmd = Command::new(&recipe_bin);
cmd.current_dir(&rmake_out_dir)
.stdout(Stdio::piped())
.stderr(Stdio::piped())
// Provide the target-specific env var that is used to record dylib search paths. For
// example, this could be `LD_LIBRARY_PATH` on some linux distros but `PATH` on Windows.
.env("LD_LIB_PATH_ENVVAR", dylib_env_var())
.env("TARGET_RPATH_ENV", &target_rpath_env_path)
.env(dylib_env_var(), &dylib_env_paths)
// Provide the dylib search paths.
.env(dylib_env_var(), &env::join_paths(recipe_dylib_search_paths).unwrap())
// Provide runtime dylib search paths.
.env("TARGET_RPATH_ENV", &env::join_paths(target_runtime_dylib_search_paths).unwrap())
// Provide the target.
.env("TARGET", &self.config.target)
// Some tests unfortunately still need Python, so provide path to a Python interpreter.
.env("PYTHON", &self.config.python)
.env("SOURCE_ROOT", &src_root)
.env("RUST_BUILD_STAGE", &self.config.stage_id)
.env("RUSTC", cwd.join(&self.config.rustc_path))
.env("HOST_RPATH_DIR", cwd.join(&self.config.compile_lib_path))
.env("TARGET_RPATH_DIR", cwd.join(&self.config.run_lib_path))
// Provide path to checkout root. This is the top-level directory containing
// rust-lang/rust checkout.
.env("SOURCE_ROOT", &source_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
// to be confused with `TARGET_RPATH_ENV` or `TARGET_RPATH_DIR`. This is needed if the
// recipe wants to invoke rustc.
.env("HOST_RPATH_DIR", &self.config.compile_lib_path)
// Provide the directory to libraries that might be needed to run compiled binaries
// (further compiled by the recipe!).
.env("TARGET_RPATH_DIR", &self.config.run_lib_path)
// Provide which LLVM components are available (e.g. which LLVM components are provided
// through a specific CI runner).
.env("LLVM_COMPONENTS", &self.config.llvm_components);

if let Some(ref rustdoc) = self.config.rustdoc_path {
cmd.env("RUSTDOC", cwd.join(rustdoc));
cmd.env("RUSTDOC", source_root.join(rustdoc));
}

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

0 comments on commit ae28d5c

Please sign in to comment.