[compiletest-related cleanups 3/7] Make the distinction between sources root vs test suite sources root in compiletest less confusing
#136474
+72
−98
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
A-compiletest
jieyouxu
changed the title
compiletest-related cleanups3/7] Make the distinction between sources root vs test suite sources root in compiletest less confusingcompiletest-related cleanups 3/7] Make the distinction between sources root vs test suite sources root in compiletest less confusing
jieyouxu
removed
the
S-waiting-on-review
rustbot
added
the
S-blocked
jieyouxu
commented
Feb 3, 2025
Comment on lines
-1037
to
-1041
| 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 | ||
| } | ||
|
|
There was a problem hiding this comment.
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.
|
☔ The latest upstream changes (presumably #136809) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Prerequisite PRs have merged so this is now unblocked. |
rustbot
added
S-waiting-on-review
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.
jieyouxu
commented
Feb 10, 2025
Comment on lines
-936
to
+955
| // | ||
| // 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")); |
There was a problem hiding this comment.
Also helps to cleanup this
Comment on lines
-1029
to
+1046
| // 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(); |
| self.config.find_rust_src_root().unwrap().join("vendor").display(), | ||
| self.config.src_root.join("vendor").to_str().unwrap(), |
Comment on lines
1634
to
+1635
| "--remap-path-prefix={}={}", | ||
| self.config.src_base.display(), | ||
| self.config.src_test_suite_root.to_str().unwrap(), |
There was a problem hiding this comment.
This was very confusing to me, because this is not the source root, but actually the root of the test suite sources
Comment on lines
-411
to
+390
| cmd.env("CARGO", source_root.join(cargo)); | ||
| cmd.env("CARGO", cargo); |
There was a problem hiding this comment.
Redundant, bootstrap already passes abs path to cargo/rustdoc
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reference for overall changes: #136437
Part 3 of 7 of the
compiletest-related cleanups PR series.Summary
--src-basecompiletest in favor of new flags--src-rootand--src-test-suite-rootwhich more accurately conveys the intent.--src-basepreviously actually meant--src-test-suite-rootand has caused multiple confusions.--src-rootto 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 anOption<PathBuf>.Review advice
Best reviewed commit-by-commit.
r? bootstrap