-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Add cargo lintcheck --recursive
to check dependencies of crates
#9510
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
Conversation
Can we avoid dependencies change between runs somehow? |
The dependencies wouldn't change between runs on the same machine at least, as the sources that don't ship a You can get clippy to run on dependencies by doing |
lintcheck/src/main.rs
Outdated
.arg("check") | ||
.arg("--quiet") | ||
.current_dir(&self.path) | ||
.env("CLIPPY_ARGS", rustc_args.join("__CLIPPY_HACKERY__")) |
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.
aren't these clippy args technically? Perhaps rename to clippy_args
? 🤔
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.
Yeah, that's true
lintcheck/src/main.rs
Outdated
let status = Command::new("cargo") | ||
.arg("check") | ||
.arg("--quiet") | ||
.current_dir(&self.path) | ||
.env("CLIPPY_ARGS", rustc_args.join("__CLIPPY_HACKERY__")) | ||
.env("CARGO_TARGET_DIR", target) | ||
.env("RUSTC_WRAPPER", env::current_exe().unwrap()) | ||
.env("CLIPPY_DRIVER", clippy_driver_path) | ||
.env("LINTCHECK_SERVER", server.local_addr.to_string()) | ||
.status() | ||
.expect("failed to run cargo"); | ||
|
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.
What does this status do?
We just check that cargo check/clippy would run successful (no invalid args passed etc..)?
Would you mind adding a short comment at the top? 🙃
let mut times = [CLIPPY_DRIVER_PATH, CARGO_CLIPPY_PATH].iter().map(|p| { | ||
let [cargo, driver] = paths.map(|p| { | ||
std::fs::metadata(p) | ||
.expect("failed to get metadata of file") | ||
.modified() | ||
.expect("failed to get modification date") | ||
}); | ||
// the oldest modification of either of the binaries | ||
std::cmp::max(times.next().unwrap(), times.next().unwrap()) | ||
std::cmp::max(cargo, driver) |
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.
neat! 👍
let cargo_clippy_path = fs::canonicalize(format!("target/debug/cargo-clippy{EXE_SUFFIX}")).unwrap(); | ||
let clippy_driver_path = fs::canonicalize(format!("target/debug/clippy-driver{EXE_SUFFIX}")).unwrap(); | ||
|
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 makes me wonder if we should actually offer a --release mode somehow 😅
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.
That could be cool, I wonder how much you need to lint before it makes sense to apply 😄
b304dca
to
fc77d91
Compare
|
||
// `cargo clippy` is a wrapper around `cargo check` that mainly sets `RUSTC_WORKSPACE_WRAPPER` to | ||
// `clippy-driver`. We do the same thing here with a couple changes: | ||
// | ||
// `RUSTC_WRAPPER` is used instead of `RUSTC_WORKSPACE_WRAPPER` so that we can lint all crate | ||
// dependencies rather than only workspace members | ||
// | ||
// The wrapper is set to the `lintcheck` so we can force enable linting and ignore certain crates | ||
// (see `crate::driver`) | ||
let status = Command::new("cargo") | ||
.arg("check") | ||
.arg("--quiet") | ||
.current_dir(&self.path) | ||
.env("CLIPPY_ARGS", clippy_args.join("__CLIPPY_HACKERY__")) | ||
.env("CARGO_TARGET_DIR", target) | ||
.env("RUSTC_WRAPPER", env::current_exe().unwrap()) | ||
// Pass the absolute path so `crate::driver` can find `clippy-driver`, as it's executed in various | ||
// different working directories | ||
.env("CLIPPY_DRIVER", clippy_driver_path) | ||
.env("LINTCHECK_SERVER", server.local_addr.to_string()) | ||
.status() | ||
.expect("failed to run cargo"); | ||
|
||
assert_eq!(status.code(), Some(0)); | ||
|
||
return Vec::new(); |
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.
I'm trying to wrap my head around this 😅
the lintcheck binary also acts as some kind of wrapper that pretends to be a rustc when called onto a dependency and runs lintcheck/clippy on the dependency it is called on?
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.
So the same binary acts as "worker" running clippy on a single crate, intercepting its output and sending it to a server, as well as server, (receiving the output from N workers at a time and summarizing them)". Do I understand this correctly? This seems a bit funky, but I've also never implemented something like this myself.
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.
Yeah that's right. It is pretty funky, I took the idea from cargo - https://github.com/rust-lang/cargo/blob/0.65.0/src/cargo/ops/fix.rs
The RUSTC_WRAPPER
part could be made a separate binary that lintcheck
cargo build
s first if that would make it clearer
} | ||
|
||
if let Some(server) = server { | ||
let target = shared_target_dir.join("recursive"); |
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 does set a different target dir for --recursive, right? did you run into some problems when not doing this?
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.
It shouldn't cause a problem, IIRC I was trying to avoid the regular lintcheck
's target dir getting removed unnecessarily. But as it may get deleted anyway on account of clippy
's timestamp maybe it's not really making a difference doing this
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.
Thanks!
@bors r+ |
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
r? @matthiaskrgr
This just adds the mode without changing how a regular run works
Takes a fair bit longer to run than a
-j4
or regular runBut finds more results:
Stats
changelog: none