cargo clippy #681

oli-obk · 2016-02-17T16:49:06Z

@arcnmx are you ok with moving this here?

Manishearth · 2016-02-17T16:50:58Z

Oh, no, this isn't what I meant. I was suggesting we create src/main.rs or something (perhaps a different binary), and use the CompilerCalls API to directly run plugin_registrar.

Manishearth · 2016-02-17T16:52:04Z

That way we don't need to have clippy as a staticlib at all. It can be run as a plugin and run as a compiler drop-in.

oli-obk · 2016-02-17T16:54:51Z

hmm... that works? I'll see what cargo says

Manishearth · 2016-02-17T16:55:42Z

Basically, this way folks can continue using it as a plugin, but cargo install clippy works silky smooth.

oli-obk · 2016-02-17T17:18:12Z

i so didn't think this would work, but behold: I'm using lib.rs for both clippy the plugin and cargo-clippy the executable

oli-obk · 2016-02-17T17:21:02Z

travis failure is due to my inability to figure out the location of the standard libs in a portable way

Manishearth · 2016-02-17T17:28:12Z

src/lib.rs

+    args.push("-L".to_owned());
+    args.push(dep_path.as_ref().to_string_lossy().into_owned());
+    args.push(String::from("--sysroot"));
+    args.push(format!("{}/.multirust/toolchains/nightly", std::env::var("HOME").unwrap()));


This won't always be true

oli-obk · 2016-02-18T15:28:17Z

so this isn't quite so trivial... Simply replacing rustc by using the RUSTC env variable (that cargo uses) will rebuild all dependencies and fail at that b/c I can't reproduce a working rustc. Just calling it directly on src/lib.rs or src/main.rs is not very portable and will be missing all the compiler-flags.

oli-obk · 2016-05-04T15:21:15Z

so... I got it to work. Here's the current status quo:

you need to set the environment variable SYSROOT during the compilation of the clippy executable
- SYSROOT=/home/blarg/.multirust/toolchains/nightly cargo install
- I haven't found a way to detect it automatically during compilation, a buildscript could help, or we appeal to rustc/cargo to set some environment variable
cargo install installs only for the current toolchain, so you need to call multirust run nightly cargo clippy if you are working on stable or another nightly
There's no way to get it to work across toolchains, since cargo doesn't know about toolchains, and we need cargo to build the dependencies of the crate we want to test

Manishearth · 2016-05-04T15:23:42Z

Why not read $MULTIRUST_TOOLCHAIN or something?

oli-obk · 2016-05-04T15:26:14Z

because we don't necessarily have multirust, but I can do it if it's there

Manishearth · 2016-05-04T15:27:39Z

Please do. You'll need to use this in conjunction with MULTIRUST_HOME and some other variables which I forget (read through the rustc shell script).

They are only set within an invocation of rustc or cargo, though. Which works for us.

oli-obk · 2016-05-04T16:09:00Z

done

oli-obk · 2016-05-04T16:12:08Z

I left the fallback in, so anyone not using multirust can set the SYSROOT environment variable.

cargo clippy works just like cargo rustc so one has to specify --lib or --bin foo to get it to run on crates supplying both a lib and binaries.

To run the pedantic lints, just run cargo clippy -- -W clippy_pedantic, all arguments that cargo rustc can take are legal for cargo clippy

It would also be easy to add a flag to run clippy while compiling dependencies, not sure if that is of any use.

Manishearth · 2016-05-04T18:22:52Z

(will review soon, bit busy with packing and unpacking and repacking et cetera)

Manishearth · 2016-05-05T06:43:07Z

src/lib.rs

+    args.push(String::from("--sysroot"));
+    args.push(sysroot);
+    args.push("-Zno-trans".to_owned());
+    args.push("-Dclippy".to_owned());


Do we want -Dclippy by default? cc @llogiq

In what context is this (sorry, am on mobile)?

should cargo clippy -Dclippy by default?

(I lean towards no)

Agreed. The default behavior of clippy should also be cargo-cclippy's default.

yea, it makes no difference, since there's -Z-no-trans so compilation stops after the lints anyway, I just had this in there for testing some stuff.

Manishearth · 2016-05-05T07:51:39Z

Overall LGTM, small fixes.

We should also update the readme to mention cargo install clippy and cargo clippy, perhaps removing the reference to the old cargo-clippy, as well as mentioning how it is to be used with multirust/rustup (directly) and with a regular rust install (sysroot)

Manishearth · 2016-05-05T07:52:33Z

works awesomely btw, really excited to get this up! 😄

mcarton · 2016-05-05T10:13:35Z

src/lib.rs

+    args.push(sysroot);
+    args.push("-Zno-trans".to_owned());
+    args.push("-Dclippy".to_owned());
+    args
 }


Also can all of this be moved to src/bin or at least src/main.rs? Especially since src/lib.rs is mostly generated from the Python scripts.
Besides that great! 🎉

I'll check, not sure how cargo install interacts with this

ugh, I remember why I chose not to do that. Clippy is built as a dylib, so it needs to be distributed (cargo install only ever installs a single file). I'll check if I can enforce clippy to be built both as a dylib and a staticlib

cargo install installs the binary which is different from the lib anyway?

So... I haven't figured it out. There's crate-type = ["staticlib", "dylib"], but then I get lots of errors about dependency 'foo' not found in rlib format. We can split clippy threeways:

clippy-lints: the current code except for the #[plugin_registrar] attribute

clippy: a plugin with #[plugin_registrar] that simply calls the plugin_registrar function in clippy-lints

clippy-cargo: an executable that calls plugin_registrar of clippy-lints like it does now

cargo install installs the binary which is different from the lib anyway?

yea, but if the lib is built as a dylib, then it's not statically linked into the binary, so executing the binary fails with libclippy.so not found

@Manishearth and I chatted on irc. The decision was to do this later.

oli-obk · 2016-05-06T09:27:29Z

addressed comments

oli-obk · 2016-05-06T09:39:02Z

as a next step we should probably let travis run cargo install and then run cargo clippy --lib to verify that everything works

oli-obk · 2016-05-06T10:48:58Z

travis likes it now... Takes around 6 minutes (previously it was 4 minutes), since cargo install compiles all deps and clippy all over again :(

But cargo clippy takes just 5 seconds on clippy itself!

oli-obk · 2016-05-06T14:58:25Z

rebased and added docs

llogiq · 2016-05-06T15:25:54Z

I'm OK with Travis taking a bit longer if we also check cargo-clippy in the process.

Manishearth · 2016-05-08T13:04:34Z

src/lib.rs

+    let dep_path = env::current_dir().expect("current dir is not readable").join("target").join("debug").join("deps");
+    let sys_root = match (option_env!("MULTIRUST_HOME"), option_env!("MULTIRUST_TOOLCHAIN")) {
+        (Some(home), Some(toolchain)) => format!("{}/toolchains/{}", home, toolchain),
+        _ => option_env!("SYSROOT").expect("need to specify SYSROOT env var during clippy compilation").to_owned(),


... or use multirust

oli-obk · 2016-05-08T13:07:33Z

This got lost in a hidden github comment:

What is the behaviour you expect from calling cargo-clippy directly. Do you want to call it on a .rs file directly? or in a cargo repository?

Manishearth · 2016-05-08T13:14:46Z

In a cargo repo. I'm okay with having cargo clippy foo.rs as well where we check if we have an rs argument.

Alternatively, provide a different binary "clippy" that doesn't do the cargo stuff. But you mentioned that splitting lib.rs doesn't work well, which would be necessary here. Hmm. (postpone this to the future?)

rustfmt has cargo fmt and rustfmt for this reason

Manishearth · 2016-05-08T13:15:21Z

README.md

@@ -243,6 +267,8 @@ cargo rustc -- -L /path/to/clippy_so -Z extra-plugins=clippy
 *[Note](https://github.com/Manishearth/rust-clippy/wiki#a-word-of-warning):*
 Be sure that clippy was compiled with the same version of rustc that cargo invokes here!

+###optional dependency


oli-obk · 2016-05-09T11:16:47Z

addressed all comments.

r=@Manishearth (https://botbot.me/mozilla/rust/2016-05-08/?msg=65674824&page=8)

Manishearth · 2016-05-10T07:42:00Z

I shall make a publish and test this through the publish tomorrow (cargo install in the folder works already, so there should be no change)

Manishearth · 2016-05-10T14:37:53Z

works great!

oli-obk · 2016-05-10T15:08:38Z

yay! :) I'll add it to TWIR

mcarton · 2016-05-10T19:32:18Z

src/lib.rs

+    args.push(String::from("--sysroot"));
+    args.push(sysroot);
+    args.push("-Zno-trans".to_owned());
+    args
 }


By the way, GitHub hide that but it’s really ugly IMO to have

extern crate rustc_driver; extern crate getopts; fn main() { … } extern crate some; extern crate other; extern crate krates; pub fn plugin_registrar(reg: &mut Registry) { … }

And I really would have preferred to split that file in src/lib.rs + src/bin.rs (but GitHub also censored that comment at some point).

I noticed the comment, there was some discussion (that disappeared) and some discussion on irc, it's really hard to do this without splitting the main clippy stuff into its own crate, because I couldn't figure out how to let src/main.rs link against clippy statically, because clippy sets plugin = true and then that's that, no way to also compile it as a staticlib. So we'd need to split clippy into a crate with all the code, and a plugin crate + binary crate (the last two can be together again)... or, I just noticed, the plugin crate can link against the binary crate, but that's probably even messier than what we have now.

Ah ok then, you’ve done enough wizardry already here 😄, I can live with it.

Yeah, its a follow-up bug I may look into

Manishearth · 2016-05-10T20:17:20Z

Could you ensure this works with rustup.rs? Someone on IRC had trouble with it but didn't elaborate.

Otherwise I'll look into it tomorrow.

cuviper · 2016-05-10T20:18:32Z

Might be worth asking if cargo-clippy will now yank theirs, as I accidentally got that one at first. That one does a brute git-clone into $CARGO_HOME/rust-clippy which is not nearly as nice.

And in fact rustup doesn't work -- I just filed #910.

Manishearth · 2016-05-11T06:07:56Z

@arcnmx thoughts?

arcnmx · 2016-05-11T16:44:03Z

@Manishearth that's not mine, it's @White-Oak's.

White-Oak · 2016-05-12T15:24:28Z

Ah, so cargo install clippy now also install cargo clippy subcommand? If yes, I'll happily yank that crate.

llogiq · 2016-05-12T15:56:05Z

Yes, that's what it does.

Manishearth reviewed Feb 17, 2016
View reviewed changes

oli-obk changed the title ~~split clippy into a plugin and a staticlib and add cargo-clippy~~ cargo clippy Mar 2, 2016

Manishearth reviewed May 5, 2016
View reviewed changes

mcarton reviewed May 5, 2016
View reviewed changes

Manishearth reviewed May 8, 2016
View reviewed changes

cargo clippy subcommand

654154d

Manishearth merged commit 855b292 into rust-lang:master May 10, 2016

oli-obk deleted the split branch May 10, 2016 15:08

mcarton reviewed May 10, 2016
View reviewed changes

flip1995 mentioned this pull request Jan 22, 2020

[WIP][DNM] Switch to GitHub Actions #5071

Closed

5 tasks

cargo clippy #681

cargo clippy #681

Conversation

oli-obk commented Feb 17, 2016

Manishearth commented Feb 17, 2016

Manishearth commented Feb 17, 2016

oli-obk commented Feb 17, 2016

Manishearth commented Feb 17, 2016

oli-obk commented Feb 17, 2016

oli-obk commented Feb 17, 2016

Choose a reason for hiding this comment

oli-obk commented Feb 18, 2016

oli-obk commented May 4, 2016

Manishearth commented May 4, 2016

oli-obk commented May 4, 2016

Manishearth commented May 4, 2016

oli-obk commented May 4, 2016

oli-obk commented May 4, 2016 • edited Loading

Manishearth commented May 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Manishearth May 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk May 6, 2016 • edited Loading

Choose a reason for hiding this comment

Manishearth commented May 5, 2016

Manishearth commented May 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk May 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk May 6, 2016 • edited Loading

Choose a reason for hiding this comment

oli-obk May 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk commented May 6, 2016

oli-obk commented May 6, 2016

oli-obk commented May 6, 2016 • edited Loading

oli-obk commented May 6, 2016

llogiq commented May 6, 2016

Choose a reason for hiding this comment

oli-obk commented May 8, 2016

Manishearth commented May 8, 2016

Choose a reason for hiding this comment

oli-obk commented May 9, 2016

Manishearth commented May 10, 2016

Manishearth commented May 10, 2016

oli-obk commented May 10, 2016

mcarton May 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Manishearth commented May 10, 2016

cuviper commented May 10, 2016

Manishearth commented May 11, 2016

arcnmx commented May 11, 2016

White-Oak commented May 12, 2016

llogiq commented May 12, 2016

oli-obk commented May 4, 2016 •

edited

Loading

Manishearth May 5, 2016 •

edited

Loading

oli-obk May 6, 2016 •

edited

Loading

oli-obk May 6, 2016 •

edited

Loading

oli-obk May 6, 2016 •

edited

Loading

oli-obk May 6, 2016 •

edited

Loading

oli-obk commented May 6, 2016 •

edited

Loading

mcarton May 10, 2016 •

edited

Loading