-
Notifications
You must be signed in to change notification settings - Fork 12.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix incorrect path resolution in tidy #122209
Conversation
rustbot has assigned @Mark-Simulacrum. Use r? to explicitly pick a reviewer |
src/tools/tidy/src/ui_tests.rs
Outdated
@@ -184,7 +185,7 @@ pub fn check(path: &Path, bless: bool, bad: &mut bool) { | |||
*/ | |||
[ | |||
"#; | |||
let tidy_src = std::env::current_dir().unwrap().join("src/tools/tidy/src"); | |||
let tidy_src = PathBuf::from(env::args_os().nth(1).unwrap()).join("src/tools/tidy/src"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This solution is really unnecessarily cryptic, imo. A better solution would be to pass in the root_path
here rather than the tests_path
:
rust/src/tools/tidy/src/main.rs
Line 105 in a655e64
check!(ui_tests, &tests_path, bless); |
And then adjust the rest of this file to operate from the root, rather than the test dir.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be better now.
src/tools/tidy/src/issues.txt
Outdated
"ui/async-await/issue-68523.rs", | ||
"ui/async-await/issue-68523-start.rs", | ||
"ui/async-await/issue-68523.rs", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR shouldn't affect ordering here, why it changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
preexisting: these weren't sorted, see #122161 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@onur-ozkan Can you rebase this? It should remove these diffs since master should contain the relevant fix.
3c67ab6
to
b51627c
Compare
Previously, reading the current path from the environment led to failure when invoking x from outside the source root. This change fixes this issue by passing the already resolved root path into `ui_tests::check`. Signed-off-by: onur-ozkan <work@onurozkan.dev>
b51627c
to
7c13421
Compare
@bors r+ rollup |
…iaskrgr Rollup of 12 pull requests Successful merges: - rust-lang#121358 (Reduce alignment of TypeId to u64 alignment) - rust-lang#121813 (Misc improvements to non local defs lint implementation) - rust-lang#122160 (Eagerly translate `HelpUseLatestEdition` in parser diagnostics) - rust-lang#122178 (ci: add a runner for vanilla LLVM 18) - rust-lang#122187 (Move metadata header and version checks together) - rust-lang#122209 (fix incorrect path resolution in tidy) - rust-lang#122215 (Some tweaks to the parallel query cycle handler) - rust-lang#122223 (Fix typo in `VisitorResult`) - rust-lang#122224 (Add missing regression tests) - rust-lang#122232 (library/core: fix a comment, and a cfg(miri) warning) - rust-lang#122233 (miri: do not apply aliasing restrictions to Box with custom allocator) - rust-lang#122237 (Remove `Ord` from `ClosureKind`) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#122209 - onur-ozkan:fix-tidy-path-resolution, r=compiler-errors fix incorrect path resolution in tidy Previously, reading the current path from the environment led to failure when invoking x from outside the source root. This change fixes this issue by passing the already resolved root path into `ui_tests::check`. Fixes rust-lang#122202
Previously, reading the current path from the environment led to failure when invoking x from outside the source root. This change fixes this issue by passing the already resolved root path into
ui_tests::check
.Fixes #122202