Use binary-dep-depinfo to resolve UI test dependencies #7631
Conversation
rust-highfive
added
the
S-waiting-on-review
|
This looks like the better approach. I'll close the other one. |
| [build] | ||
| rustflags = ["-Zunstable-options"] | ||
| # -Zbinary-dep-depinfo allows us to track which rlib files to use for compiling UI tests | ||
| rustflags = ["-Zunstable-options", "-Zbinary-dep-depinfo"] |
There was a problem hiding this comment.
the new "nightly-only" flag can't cause problems when running clippy tests on the stable channel inside the rustc repo in a couple of months, can it? 🤔
There was a problem hiding this comment.
I mean, we have the nightly-only flag -Zunstable-options in there forever, so I assume it won't. (?)
tests/compile-test.rs
Outdated
| let path = line.strip_suffix(':')?; | ||
| let part = path | ||
| .strip_suffix(".rlib") | ||
| .or_else(|| path.strip_suffix(".so")) | ||
| .or_else(|| path.strip_suffix(".dylib")) | ||
| .or_else(|| path.strip_suffix(".dll"))?; | ||
| let part = part.rsplit_once('-')?.0; | ||
| let name = part.rsplit_once(|c| matches!(c, '/' | '\\'))?.1.strip_prefix("lib")?; | ||
| CRATES.contains(&name).then(|| (name, path))? |
There was a problem hiding this comment.
This seems to make really strong assumptions about the format of the depinfo file. Since this is still unstable this may just change without a warning. Also reading the tracking issue rust-lang/rust#63012, I wonder if this also works on Windows?
There was a problem hiding this comment.
Well let's spell out those assumptions (I should put this in a comment too). The depinfo file is a Makefile. For every rlib, there is a "rule" where the "target" is the rlib file and there are no "prerequisites" after the :. There is no extra whitespace around the :. On the plus side there is no assumption about where the path is in the filesystem.
I agree these are pretty strong assumptions. But I also think this buys us much better stability overall than the current solution. The file is machine-generated and seems to be unchanged for 2 years, which is quite good in this ecosystem. The flag is also used in rustc builds so that's a plus.
I wonder if this also works on Windows?
I think it will since I allow for either slash and don't look at the beginning of the path. But someone should test this before merging for sure.
There was a problem hiding this comment.
(I should put this in a comment too)
Yep :D
The file is machine-generated and seems to be unchanged for 2 years
Sounds good!
I think it will since I allow for either slash and don't look at the beginning of the path. But someone should test this before merging for sure.
@rust-lang/clippy anyone has access/uses a windows PC where they can test this? (I'll also provide a rust repo branch to test this in the Rust repo)
There was a problem hiding this comment.
You'll need to do:
let part = part.rsplit_once('-')?.0.rsplit_once(|c| matches!(c, '/' | '\\'))?.1;
let name = part.strip_prefix("lib").unwrap_or(part);dll names on windows don't have a lib prefix. Works fine with that change.
|
flip1995
added
the
I-sync-blocker
|
Current version is working on Windows. |
|
Rebased onto master |
|
This seemed to fix the problem in the Clippy update rust-lang/rust#88615. I had to add |
|
It's |
|
@bors r+ Thanks! |
|
📌 Commit 5d3fc6f has been approved by |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
I believe this is possible as of rust-lang#7631
Closes #7343
Closes #6809
Closes #3643
changelog: none
r? @flip1995
cc @Jarcho