Don't pass --sysroot twice if SYSROOT is set#10149
Conversation
|
r? @dswij (rustbot has picked a reviewer for you, use r? to override) |
|
r? @flip1995 |
f19004e to
3c61189
Compare
| LazyLock::force(&ICE_HOOK); | ||
| exit(rustc_driver::catch_with_exit_code(move || { | ||
| let mut orig_args: Vec<String> = env::args().collect(); | ||
| let has_sysroot_arg = arg_value(&orig_args, "--sysroot", |_| true).is_some(); |
There was a problem hiding this comment.
Note that if we do want to support passing --sysroot to clippy-driver (with -- to cargo-clippy, not rustflags), this will need to be changed to look at clippy_args as well, not just orig_args.
There was a problem hiding this comment.
Could I send a PR to support passing --sysroot to clippy-driver?
c4926f7 to
2e40e36
Compare
flip1995
left a comment
There was a problem hiding this comment.
The integration test is not the right place for this. The integration test is really just for running Clippy on a repo and check that it doesn't produce an ICE.
IIUC what you want to test is that when running SYSROOT=/path/to/smth RUSTFLAGS=--sysroot=/path/tp/lib cargo clippy will only use the sysroot given by RUSTFLAGS?
I'll try to move this to .github/driver.sh tomorrow before merging.
Yes, exactly.
❤️ |
This is useful for rust-lang/rust to allow setting a sysroot that's *only* for build scripts, different from the regular sysroot passed in RUSTFLAGS (since cargo doesn't apply RUSTFLAGS to build scripts or proc-macros). That said, the exact motivation is not particularly important: this fixes a regression from rust-lang@5907e91#r1060215684. Note that only RUSTFLAGS is tested in the new integration test; passing --sysroot through `clippy-driver` never worked as far as I can tell, and no one is using it, so I didn't fix it here.
Whne SYSROOT is defined, clippy-driver will insert a --sysroot argument when calling rustc. However, when a sysroot argument is already defined, e.g. through RUSTFLAGS=--sysroot=... the `cargo clippy` call would error. This tests that the sysroot argument is only passed once and that SYSROOT is ignored in this case.
2e40e36 to
fe00717
Compare
|
@bors r+ Thanks for addressing this and sorry for taking so long to review. I'll sync this to rustc right after this PR is merged. |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
|
Thank you! No worries at all, I just wanted to make sure it landed before the beta bump :) |
This is useful for rust-lang/rust to allow setting a sysroot that's only for build scripts, different from the regular sysroot passed in RUSTFLAGS (since cargo doesn't apply RUSTFLAGS to build scripts or proc-macros).
That said, the exact motivation is not particularly important: this fixes a regression from
5907e91#r1060215684.
Note that only RUSTFLAGS is tested in the new integration test; passing --sysroot through
clippy-drivernever worked as far as I can tell, and no one is using it, so I didn't fix it here.Helps with rust-lang/rust#106394.
changelog: other:
SYSROOTand--sysrootcan now be set at the same time#10149