[`compiletest`-related cleanups 3/7] Make the distinction between sources root vs test suite sources root in compiletest less confusing by jieyouxu · Pull Request #136474 · rust-lang/rust

jieyouxu · 2025-02-03T09:52:29Z

Reference for overall changes: #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

jieyouxu · 2025-02-03T10:00:09Z

@rustbot blocked (on #136472 and #136441)

jieyouxu · 2025-02-03T10:14:40Z

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
-    }
-


This was semi-necessary back in #16322 (Aug 2014, 11 years ago) because bootstrap didn't exist and compiletest was invoked directly, but now we have bootstrap so this isn't needed anymore.

bors · 2025-02-10T15:33:39Z

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

jieyouxu · 2025-02-10T15:46:00Z

Prerequisite PRs have merged so this is now unblocked.
@rustbot ready

jieyouxu · 2025-02-10T15:47:28Z

src/tools/compiletest/src/lib.rs

-    //
-    // FIXME(jieyouxu): untangle these paths, we should provide both a path to root `tests/` or
-    // `tests/auxiliary/` and the test suite in question. `src_base` is also a terrible name.
-    related.push(config.src_base.parent().unwrap().join("auxiliary").join("minicore.rs"));
+    related.push(config.src_root.join("tests").join("auxiliary").join("minicore.rs"));


Also helps to cleanup this

jieyouxu · 2025-02-10T15:49:05Z

src/tools/compiletest/src/lib.rs

-    // Print the name of the file, relative to the repository root.
-    // `src_base` looks like `/path/to/rust/tests/ui`
-    let root_directory = config.src_base.parent().unwrap().parent().unwrap();
-    let path = testpaths.file.strip_prefix(root_directory).unwrap();
+    // Print the name of the file, relative to the sources root.
+    let path = testpaths.file.strip_prefix(&config.src_root).unwrap();


jieyouxu · 2025-02-10T15:49:15Z

src/tools/compiletest/src/runtest.rs

        rustc.arg("-Z").arg(format!(
            "ignore-directory-in-diagnostics-source-blocks={}",
-            self.config.find_rust_src_root().unwrap().join("vendor").display(),
+            self.config.src_root.join("vendor").to_str().unwrap(),


jieyouxu · 2025-02-10T15:49:39Z

src/tools/compiletest/src/runtest.rs

                "--remap-path-prefix={}={}",
-                self.config.src_base.display(),
+                self.config.src_test_suite_root.to_str().unwrap(),


This was very confusing to me, because this is not the source root, but actually the root of the test suite sources

jieyouxu · 2025-02-10T15:50:38Z

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

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


Redundant, bootstrap already passes abs path to cargo/rustdoc

jieyouxu · 2025-02-21T10:45:19Z

r? bootstrap

clubby789

LGTM, r=me with or without nit

src/tools/compiletest/src/lib.rs

Instead of only having `--src-base` and `src_base` which *actually* refers to the directory containing the test suite and not the sources root. More importantly, kill off `find_rust_src_root` when we can simply pass that info from bootstrap.

…--src-base`

jieyouxu · 2025-02-21T17:45:33Z

Fixed nit and rebased.
@bors r=@clubby789 rollup

bors · 2025-02-21T17:45:38Z

📌 Commit 6d0c27e has been approved by clubby789

It is now in the queue for this repository.

[`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

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#136458 (Do not deduplicate list of associated types provided by dyn principal) - rust-lang#136474 ([`compiletest`-related cleanups 3/7] Make the distinction between sources root vs test suite sources root in compiletest less confusing) - rust-lang#136592 (Make sure we don't overrun the stack in canonicalizer) - rust-lang#136787 (Remove `lifetime_capture_rules_2024` feature) - rust-lang#137180 (Give `global_asm` a fake body to store typeck results, represent `sym fn` as a hir expr to fix `sym fn` operands with lifetimes) - rust-lang#137257 (Ignore fake borrows for packed field check) - rust-lang#137348 (More sophisticated span trimming for suggestions) - rust-lang#137399 (fix ICE in layout computation with unnormalizable const) r? `@ghost` `@rustbot` modify labels: rollup

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#136458 (Do not deduplicate list of associated types provided by dyn principal) - rust-lang#136474 ([`compiletest`-related cleanups 3/7] Make the distinction between sources root vs test suite sources root in compiletest less confusing) - rust-lang#136592 (Make sure we don't overrun the stack in canonicalizer) - rust-lang#136787 (Remove `lifetime_capture_rules_2024` feature) - rust-lang#137207 (Add #[track_caller] to Duration Div impl) - rust-lang#137245 (Tweak E0277 when predicate comes indirectly from ?) - rust-lang#137257 (Ignore fake borrows for packed field check) - rust-lang#137399 (fix ICE in layout computation with unnormalizable const) r? `@ghost` `@rustbot` modify labels: rollup

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

rustbot assigned clubby789 Feb 3, 2025

jieyouxu removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 3, 2025

rustbot added the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Feb 3, 2025

jieyouxu commented Feb 3, 2025

View reviewed changes

jieyouxu mentioned this pull request Feb 3, 2025

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

Closed

jieyouxu force-pushed the src-base branch from dcc27f0 to 6208db8 Compare February 4, 2025 09:46

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

jieyouxu force-pushed the src-base branch from 6208db8 to e0f7395 Compare February 10, 2025 15:45

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Feb 10, 2025

jieyouxu force-pushed the src-base branch from e0f7395 to 71817b9 Compare February 10, 2025 15:48

jieyouxu commented Feb 10, 2025

View reviewed changes

This was referenced Feb 10, 2025

Retire the legacy Makefile-based run-make test infra #136581

Merged

Perform deeper compiletest path normalization for $TEST_BUILD_DIR to account for compare-mode/debugger cases, and normalize long type file filename hashes #136865

Merged

jieyouxu mentioned this pull request Feb 19, 2025

Making compiletest more maintainable: reworking directive handling rust-lang/rust-project-goals#259

Closed

5 tasks

rustbot assigned onur-ozkan and unassigned clubby789 Feb 21, 2025

clubby789 approved these changes Feb 21, 2025

View reviewed changes

src/tools/compiletest/src/lib.rs Outdated Show resolved Hide resolved

jieyouxu assigned clubby789 and unassigned onur-ozkan Feb 21, 2025

bootstrap: pass --src-root and --src-test-suite-root instead of `…

6d0c27e

…--src-base`

jieyouxu force-pushed the src-base branch from 71817b9 to 6d0c27e Compare February 21, 2025 17:44

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 21, 2025

matthiaskrgr mentioned this pull request Feb 21, 2025

Rollup of 9 pull requests #137401

Closed

matthiaskrgr mentioned this pull request Feb 21, 2025

Rollup of 8 pull requests #137402

Closed

matthiaskrgr mentioned this pull request Feb 22, 2025

Rollup of 8 pull requests #137406

Merged

bors merged commit 57da7d3 into rust-lang:master Feb 22, 2025
6 checks passed

rustbot added this to the 1.87.0 milestone Feb 22, 2025

jieyouxu deleted the src-base branch February 22, 2025 06:59

lqd mentioned this pull request Feb 27, 2025

Raw comments rust-lang/rust-project-goals#298

Merged

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

Uh oh!

Conversation

jieyouxu commented Feb 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Review advice

Uh oh!

jieyouxu commented Feb 3, 2025

Uh oh!

jieyouxu Feb 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors commented Feb 10, 2025

Uh oh!

jieyouxu commented Feb 10, 2025

Uh oh!

jieyouxu Feb 10, 2025

Choose a reason for hiding this comment

Uh oh!

jieyouxu Feb 10, 2025

Choose a reason for hiding this comment

Uh oh!

jieyouxu Feb 10, 2025

Choose a reason for hiding this comment

Uh oh!

jieyouxu Feb 10, 2025

Choose a reason for hiding this comment

Uh oh!

jieyouxu Feb 10, 2025

Choose a reason for hiding this comment

Uh oh!

jieyouxu commented Feb 21, 2025

Uh oh!

clubby789 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jieyouxu commented Feb 21, 2025

Uh oh!

bors commented Feb 21, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jieyouxu commented Feb 3, 2025 •

edited

Loading

jieyouxu Feb 3, 2025 •

edited

Loading