[DO NOT LOOK] a bunch of compiletest-related changes #136437

jieyouxu · 2025-02-02T13:01:52Z

Some exploratory changes while trying to figure out why cg_clif's tests were failing.

Not intended for review, this PR needs to be broken up into reviewable PRs with logical commits. Only posted for reference purposes.

-        cmd.arg("--src-base").arg(builder.src.join("tests").join(suite));
-        cmd.arg("--build-base").arg(testdir(builder, compiler.host).join(suite));
+        cmd.arg("--src-root").arg(&builder.src);
+        cmd.arg("--src-test-suite-root").arg(builder.src.join("tests").join(suite));
+        cmd.arg("--build-root").arg(&builder.out);
+        cmd.arg("--build-test-suite-root").arg(testdir(builder, compiler.host).join(suite));

$ find tests -type f -exec sed -i 's/{{src-base}}/{{test-suite-src-base}}/g' {} \;
$ find tests -type f -exec sed -i 's/{{rust-src-base}}/{{sysroot-rust-src-base}}/g' {} \;
$ find tests -type f -exec sed -i 's/{{build-base}}/{{test-suite-build-base}}/g' {} \;

Planned PR series

Cleanup is_rustdoc matching logic and remove a useless rustdoc-json Path::join. [compiletest-related cleanups 1/7] Cleanup is_rustdoc logic and remove a useless path join in rustdoc-json runtest logic #136441
Pass --stage as a compiletest flag to not have to do stage-id gymnastics in run-make runtest logic. [compiletest-related cleanups 2/7] Feed stage number to compiletest directly #136472
Rectify src_base handling: split --src-base into two flags, --src-root and --src-test-suite-root, and tidy up logic that exercises src_base previously. Remove find_rust_src_root. [compiletest-related cleanups 3/7] Make the distinction between sources root vs test suite sources root in compiletest less confusing #136474
Rectify build_base handling: split --build-base into two flags, --build-root and --build-test-suite-root, and tidy up logic that exercises build_base previously. [compiletest-related cleanups 4/7] Make the distinction between root build directory vs test suite specific build directory in compiletest less confusing #136542
Rename {{src-base}} -> {{test-suite-src-base}}. Rebless tests. Update rustc-dev-guide.
Rename {{rust-src-base}} -> {{sysroot-rust-src-base}}. Rebless tests. Update rustc-dev-guide.
Rename {{build-base}} -> {{test-suite-build-base}}. Rebless tests. Update rustc-dev-guide.

jieyouxu · 2025-02-02T13:02:36Z

Sorry, forgor to write ghost.

jieyouxu · 2025-02-02T13:09:45Z

src/tools/compiletest/src/runtest/rustdoc_json.rs

-        let root = self.config.find_rust_src_root().unwrap();
        let mut json_out = out_dir.join(self.testpaths.file.file_stem().unwrap());
        json_out.set_extension("json");
        let res = self.run_command_to_procres(
            Command::new(self.config.jsondocck_path.as_ref().unwrap())
                .arg("--doc-dir")
-                .arg(root.join(&out_dir))
+                .arg(&out_dir)


This was simply wrong, and only happens to work because PathBuf::join when the arg is an absolute path out_dir will simply became a replacement operation.

jieyouxu · 2025-02-02T13:11:03Z

tests/ui/errors/remap-path-prefix-reverse.rs

-// [local-self] no-remap-src-base: The hack should work regardless of remapping.
+// The hack should work regardless of remapping.


This is kinda-wrong, in the sense that no-remap-src-base is not a directive yet this line looks like a revisioned directive, but also, because this isn't recognized as a revisioned compiletest directive (not //@), it ends up functioning as intended.

jieyouxu · 2025-02-02T13:12:01Z

src/tools/compiletest/src/runtest/debuginfo.rs

-            let rust_src_root =
-                self.config.find_rust_src_root().expect("Could not find Rust source root");
-            let rust_pp_module_rel_path = Path::new("./src/etc");
-            let rust_pp_module_abs_path =
-                rust_src_root.join(rust_pp_module_rel_path).to_str().unwrap().to_owned();


Beautiful incantation :ferrisClueless:

jieyouxu · 2025-02-02T13:12:39Z

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

-        // 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();


Yeah, for example, by actually passing the stage number through a flag like --stage

jieyouxu · 2025-02-02T13:13:08Z

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

+            // Path to `$build_dir/$host_triple/`.
+            .env("BUILD_ROOT", &host_build_root)


Cheeky, not build/, but build/$host/.

jieyouxu · 2025-02-02T13:13:25Z

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

-            cmd.env("CARGO", source_root.join(cargo));
+            cmd.env("CARGO", cargo);
        }

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


jieyouxu · 2025-02-02T13:13:54Z

src/tools/compiletest/src/runtest.rs

-        self.config.src_base.ends_with("rustdoc-ui")
-            || self.config.src_base.ends_with("rustdoc-js")
-            || self.config.src_base.ends_with("rustdoc-json")
+        matches!(self.config.suite.as_str(), "rustdoc-ui" | "rustdoc-js" | "rustdoc-json")


Not sure what's happening here lol

jieyouxu · 2025-02-02T13:14:37Z

src/tools/compiletest/src/header.rs

-    pub fn find_rust_src_root(&self) -> Option<PathBuf> {
-        let mut path = self.src_base.clone();
-        let path_postfix = Path::new("src/etc/lldb_batchmode.py");
-
-        while path.pop() {
-            if path.join(&path_postfix).is_file() {
-                return Some(path);
-            }
-        }
-
-        None
-    }


No need to use a heuristic to find source root, when we can just provide it through bootstrap.

jieyouxu · 2025-02-02T13:16:38Z

src/tools/compiletest/src/common.rs

-    /// The directory containing the tests to run
-    pub src_base: PathBuf,
-
-    /// The directory where programs should be built
-    pub build_base: PathBuf,
-
-    /// The directory containing the compiler sysroot
-    pub sysroot_base: PathBuf,
-
+    /// Path to directory containing rust-lang/rust sources.
+    pub src_root: PathBuf,
+    /// Path to the directory containg the test suite sources. Expected to be a subdirectory of
+    /// `src_root`.
+    pub src_test_suite_root: PathBuf,
+
+    /// Bootstrap build directory.
+    pub build_root: PathBuf,
+    /// Bootstrap test suite build directory. Expected to be a subdirectory of `build_root`.
+    pub build_test_suite_root: PathBuf,


These are terribly named, and looking at the runtest impls, it has confused multiple people. src_base is not sources root, it's the directory $SRC_ROOT/tests/<test-suite>/. build_base is not build directory root, it's $BUILD/<host-triple>/test/<test-suite>.

jieyouxu · 2025-02-02T13:49:43Z

src/tools/compiletest/src/runtest.rs

        // 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");
+        eprintln!("build_test_suite_root = {:?}", self.config.build_test_suite_root.display());


Reminder: delete this

…piler-errors [`compiletest`-related cleanups 1/7] Cleanup `is_rustdoc` logic and remove a useless path join in rustdoc-json runtest logic Reference for overall changes: rust-lang#136437 Part **1** of **7** of the *`compiletest`-related cleanups* PR series. ### Summary - Don't match on path when we already have test suite names. - Remove a useless path join. r? bootstrap (or compiler)

Rollup merge of rust-lang#136441 - jieyouxu:cleanup-is-rustdoc, r=compiler-errors [`compiletest`-related cleanups 1/7] Cleanup `is_rustdoc` logic and remove a useless path join in rustdoc-json runtest logic Reference for overall changes: rust-lang#136437 Part **1** of **7** of the *`compiletest`-related cleanups* PR series. ### Summary - Don't match on path when we already have test suite names. - Remove a useless path join. r? bootstrap (or compiler)

[`compiletest`-related cleanups 1/7] Cleanup `is_rustdoc` logic and remove a useless path join in rustdoc-json runtest logic Reference for overall changes: rust-lang/rust#136437 Part **1** of **7** of the *`compiletest`-related cleanups* PR series. ### Summary - Don't match on path when we already have test suite names. - Remove a useless path join. r? bootstrap (or compiler)

…crum [`compiletest`-related cleanups 2/7] Feed stage number to compiletest directly Reference for overall changes: rust-lang#136437 Part **2** of **7** of the *`compiletest`-related cleanups* PR series. ### Summary - Pass stage number via new `--stage` compiletest flag directly from bootstrap, instead of deriving that info in compiletest by doing gymnastics on `--stage-id`. - Just a cleanup, should have no functional changes. r? bootstrap

Rollup merge of rust-lang#136472 - jieyouxu:pass-stage, r=Mark-Simulacrum [`compiletest`-related cleanups 2/7] Feed stage number to compiletest directly Reference for overall changes: rust-lang#136437 Part **2** of **7** of the *`compiletest`-related cleanups* PR series. ### Summary - Pass stage number via new `--stage` compiletest flag directly from bootstrap, instead of deriving that info in compiletest by doing gymnastics on `--stage-id`. - Just a cleanup, should have no functional changes. r? bootstrap

bors · 2025-02-10T15:32:51Z

☔ The latest upstream changes (presumably #136809) made this pull request unmergeable. Please resolve the merge conflicts.

[`compiletest`-related cleanups 3/7] Make the distinction between sources root vs test suite sources root in compiletest less confusing Reference for overall changes: rust-lang#136437 Part **3** of **7** of the *`compiletest`-related cleanups* PR series. ### Summary - Remove `--src-base` compiletest in favor of new flags `--src-root` and `--src-test-suite-root` which more accurately conveys the intent. `--src-base` previously actually meant `--src-test-suite-root` and has caused multiple confusions. - Use `--src-root` to have bootstrap directly feed source root path to compiletest, instead of doing a hacky directory parent search heuristic (`find_rust_src_root`) that somehow returns an `Option<PathBuf>`. ### Review advice Best reviewed commit-by-commit. r? bootstrap

Rollup merge of rust-lang#136474 - jieyouxu:src-base, r=clubby789 [`compiletest`-related cleanups 3/7] Make the distinction between sources root vs test suite sources root in compiletest less confusing Reference for overall changes: rust-lang#136437 Part **3** of **7** of the *`compiletest`-related cleanups* PR series. ### Summary - Remove `--src-base` compiletest in favor of new flags `--src-root` and `--src-test-suite-root` which more accurately conveys the intent. `--src-base` previously actually meant `--src-test-suite-root` and has caused multiple confusions. - Use `--src-root` to have bootstrap directly feed source root path to compiletest, instead of doing a hacky directory parent search heuristic (`find_rust_src_root`) that somehow returns an `Option<PathBuf>`. ### Review advice Best reviewed commit-by-commit. r? bootstrap

[`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: rust-lang#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

Rollup merge of rust-lang#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: rust-lang#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

jieyouxu · 2025-03-20T08:14:45Z

Closing because I'm not sure if the substitution renaming is worth the churn.

a bunch of changes

ab3a013

rustbot assigned oli-obk Feb 2, 2025

This comment was marked as off-topic.

Sign in to view

jieyouxu unassigned oli-obk Feb 2, 2025

jieyouxu commented Feb 2, 2025

View reviewed changes

jieyouxu mentioned this pull request Feb 4, 2025

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

Merged

bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 10, 2025

jieyouxu closed this Mar 20, 2025

jieyouxu deleted the compiletest-rust-src-root branch March 20, 2025 08:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DO NOT LOOK] a bunch of compiletest-related changes #136437

[DO NOT LOOK] a bunch of compiletest-related changes #136437

jieyouxu commented Feb 2, 2025 •

edited

Loading

This comment was marked as off-topic.

jieyouxu commented Feb 2, 2025

jieyouxu Feb 2, 2025

jieyouxu Feb 2, 2025 •

edited

Loading

jieyouxu Feb 2, 2025

jieyouxu Feb 2, 2025

jieyouxu Feb 2, 2025

jieyouxu Feb 2, 2025

jieyouxu Feb 2, 2025

jieyouxu Feb 2, 2025

jieyouxu Feb 2, 2025

jieyouxu Feb 2, 2025

bors commented Feb 10, 2025

jieyouxu commented Mar 20, 2025

		// [local-self] no-remap-src-base: The hack should work regardless of remapping.
		// The hack should work regardless of remapping.

		// Path to `$build_dir/$host_triple/`.
		.env("BUILD_ROOT", &host_build_root)

[DO NOT LOOK] a bunch of compiletest-related changes #136437

[DO NOT LOOK] a bunch of compiletest-related changes #136437

Conversation

jieyouxu commented Feb 2, 2025 • edited Loading

Planned PR series

This comment was marked as off-topic.

jieyouxu commented Feb 2, 2025

Choose a reason for hiding this comment

jieyouxu Feb 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Feb 10, 2025

jieyouxu commented Mar 20, 2025

jieyouxu commented Feb 2, 2025 •

edited

Loading

jieyouxu Feb 2, 2025 •

edited

Loading