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

cargo clippy #681

Merged
merged 1 commit into from May 10, 2016

Conversation

Projects
None yet
7 participants
@oli-obk
Collaborator

oli-obk commented Feb 17, 2016

cc arcnmx/cargo-clippy#16

@arcnmx are you ok with moving this here?

@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth Feb 17, 2016

Collaborator

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.

Collaborator

Manishearth commented Feb 17, 2016

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

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth Feb 17, 2016

Collaborator

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.

Collaborator

Manishearth commented Feb 17, 2016

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

This comment has been minimized.

Show comment
Hide comment
@oli-obk

oli-obk Feb 17, 2016

Collaborator

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

Collaborator

oli-obk commented Feb 17, 2016

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

@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth Feb 17, 2016

Collaborator

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

Collaborator

Manishearth commented Feb 17, 2016

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

@oli-obk

This comment has been minimized.

Show comment
Hide comment
@oli-obk

oli-obk Feb 17, 2016

Collaborator

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

Collaborator

oli-obk commented Feb 17, 2016

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

This comment has been minimized.

Show comment
Hide comment
@oli-obk

oli-obk Feb 17, 2016

Collaborator

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

Collaborator

oli-obk commented Feb 17, 2016

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

Show outdated Hide outdated 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 comment has been minimized.

@Manishearth

Manishearth Feb 17, 2016

Collaborator

This won't always be true

@Manishearth

Manishearth Feb 17, 2016

Collaborator

This won't always be true

Show outdated Hide outdated src/lib.rs
let arg = arg.as_ref().to_owned();
args.push(arg);
}
args.push("-L".to_owned());

This comment has been minimized.

@Manishearth

Manishearth Feb 17, 2016

Collaborator

What happens if it's a cargo package that needs further link args?

@Manishearth

Manishearth Feb 17, 2016

Collaborator

What happens if it's a cargo package that needs further link args?

This comment has been minimized.

@oli-obk

oli-obk Feb 17, 2016

Collaborator

will we ever get far enough in the compilation process for it to matter? I'm more worried about features and such

@oli-obk

oli-obk Feb 17, 2016

Collaborator

will we ever get far enough in the compilation process for it to matter? I'm more worried about features and such

This comment has been minimized.

@Manishearth

Manishearth Feb 17, 2016

Collaborator

Um, yes, the -L args are necessary since we need to pick up type data. Clippy needs this information too.

@Manishearth

Manishearth Feb 17, 2016

Collaborator

Um, yes, the -L args are necessary since we need to pick up type data. Clippy needs this information too.

This comment has been minimized.

@oli-obk

oli-obk Feb 18, 2016

Collaborator

yes, but aren't they all in target/$debug_or_release/deps

@oli-obk

oli-obk Feb 18, 2016

Collaborator

yes, but aren't they all in target/$debug_or_release/deps

This comment has been minimized.

@Manishearth

Manishearth Feb 18, 2016

Collaborator

There can be duplicates. Cargo passes a slew of --extern flags.

@Manishearth

Manishearth Feb 18, 2016

Collaborator

There can be duplicates. Cargo passes a slew of --extern flags.

@oli-obk

This comment has been minimized.

Show comment
Hide comment
@oli-obk

oli-obk Feb 18, 2016

Collaborator

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.

Collaborator

oli-obk commented Feb 18, 2016

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 oli-obk changed the title from split clippy into a plugin and a staticlib and add cargo-clippy to cargo clippy Mar 2, 2016

@oli-obk

This comment has been minimized.

Show comment
Hide comment
@oli-obk

oli-obk May 4, 2016

Collaborator

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

  1. 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
  2. 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
  3. 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
Collaborator

oli-obk commented May 4, 2016

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

  1. 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
  2. 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
  3. 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

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth May 4, 2016

Collaborator

Why not read $MULTIRUST_TOOLCHAIN or something?

Collaborator

Manishearth commented May 4, 2016

Why not read $MULTIRUST_TOOLCHAIN or something?

@oli-obk

This comment has been minimized.

Show comment
Hide comment
@oli-obk

oli-obk May 4, 2016

Collaborator

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

Collaborator

oli-obk commented May 4, 2016

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

@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth May 4, 2016

Collaborator

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.

Collaborator

Manishearth commented May 4, 2016

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

This comment has been minimized.

Show comment
Hide comment
@oli-obk

oli-obk May 4, 2016

Collaborator

done

Collaborator

oli-obk commented May 4, 2016

done

@oli-obk

This comment has been minimized.

Show comment
Hide comment
@oli-obk

oli-obk May 4, 2016

Collaborator

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.

Collaborator

oli-obk commented May 4, 2016

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

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth May 4, 2016

Collaborator

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

Collaborator

Manishearth commented May 4, 2016

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

Show outdated Hide outdated src/lib.rs
args.push(String::from("--sysroot"));
args.push(sysroot);
args.push("-Zno-trans".to_owned());
args.push("-Dclippy".to_owned());

This comment has been minimized.

@Manishearth

Manishearth May 5, 2016

Collaborator

Do we want -Dclippy by default? cc @llogiq

@Manishearth

Manishearth May 5, 2016

Collaborator

Do we want -Dclippy by default? cc @llogiq

This comment has been minimized.

@llogiq

llogiq May 5, 2016

Collaborator

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

@llogiq

llogiq May 5, 2016

Collaborator

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

This comment has been minimized.

@Manishearth

Manishearth May 5, 2016

Collaborator

should cargo clippy -Dclippy by default?

(I lean towards no)

@Manishearth

Manishearth May 5, 2016

Collaborator

should cargo clippy -Dclippy by default?

(I lean towards no)

This comment has been minimized.

@llogiq

llogiq May 5, 2016

Collaborator

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

@llogiq

llogiq May 5, 2016

Collaborator

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

This comment has been minimized.

@oli-obk

oli-obk May 6, 2016

Collaborator

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.

@oli-obk

oli-obk May 6, 2016

Collaborator

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.

Show outdated Hide outdated src/lib.rs
_ => option_env!("SYSROOT").expect("need to specify SYSROOT env var during clippy compilation").to_owned(),
};
if env::var("AUTOBOTS").map(|s| s == "ROLLOUT").unwrap_or(false) {

This comment has been minimized.

@Manishearth

Manishearth May 5, 2016

Collaborator

what does autobots do?

@Manishearth

Manishearth May 5, 2016

Collaborator

what does autobots do?

This comment has been minimized.

@oli-obk

oli-obk May 6, 2016

Collaborator

this is the magic behind it all. The cargo-clippy executable that is installed forwards to cargo rustc with the RUSTC env flag set to the executable itself. We'd get into an infinite loop now if we'd not also set some special flag that ensures we know we should run clippy instead of cargo rustc. So I added an arbitrary flag. We could do other things like check for the RUSTC env flag, but that might interact badly with other code.

name can be arbitrarily bikeshedded, I just needed something that definitely won't collide with other env vars

@oli-obk

oli-obk May 6, 2016

Collaborator

this is the magic behind it all. The cargo-clippy executable that is installed forwards to cargo rustc with the RUSTC env flag set to the executable itself. We'd get into an infinite loop now if we'd not also set some special flag that ensures we know we should run clippy instead of cargo rustc. So I added an arbitrary flag. We could do other things like check for the RUSTC env flag, but that might interact badly with other code.

name can be arbitrarily bikeshedded, I just needed something that definitely won't collide with other env vars

This comment has been minimized.

@Manishearth

Manishearth May 6, 2016

Collaborator

All env vars should be namespaced, CLIPPY_AUTOBOTS wfm

@Manishearth

Manishearth May 6, 2016

Collaborator

All env vars should be namespaced, CLIPPY_AUTOBOTS wfm

Show outdated Hide outdated src/lib.rs
pub fn main() {
use std::env;
if env::var("DOGFOOD").map(|_| true).unwrap_or(false) {

This comment has been minimized.

@Manishearth

Manishearth May 5, 2016

Collaborator

CLIPPY_DOGFOOD

@Manishearth

Manishearth May 5, 2016

Collaborator

CLIPPY_DOGFOOD

Show outdated Hide outdated src/lib.rs
let mut args = vec!["rustc".to_owned()];
let mut found_dashes = false;
for arg in old_args.skip(2) {

This comment has been minimized.

@Manishearth

Manishearth May 5, 2016

Collaborator

it can be invoked as cargo clippy and cargo-clippy, so perhaps we should skip conditionally

@Manishearth

Manishearth May 5, 2016

Collaborator

it can be invoked as cargo clippy and cargo-clippy, so perhaps we should skip conditionally

This comment has been minimized.

@oli-obk

oli-obk May 6, 2016

Collaborator

Wait.... What behaviour do you expect from calling cargo-clippy directly?

@oli-obk

oli-obk May 6, 2016

Collaborator

Wait.... What behaviour do you expect from calling cargo-clippy directly?

@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth May 5, 2016

Collaborator

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)

Collaborator

Manishearth commented May 5, 2016

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

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth May 5, 2016

Collaborator

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

Collaborator

Manishearth commented May 5, 2016

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

Show outdated Hide outdated src/lib.rs
args.push(sysroot);
args.push("-Zno-trans".to_owned());
args.push("-Dclippy".to_owned());
args
}

This comment has been minimized.

@mcarton

mcarton May 5, 2016

Collaborator

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! 🎉

@mcarton

mcarton May 5, 2016

Collaborator

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! 🎉

This comment has been minimized.

@oli-obk

oli-obk May 6, 2016

Collaborator

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

@oli-obk

oli-obk May 6, 2016

Collaborator

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

This comment has been minimized.

@oli-obk

oli-obk May 6, 2016

Collaborator

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

@oli-obk

oli-obk May 6, 2016

Collaborator

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

This comment has been minimized.

@Manishearth

Manishearth May 6, 2016

Collaborator

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

@Manishearth

Manishearth May 6, 2016

Collaborator

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

This comment has been minimized.

@oli-obk

oli-obk May 6, 2016

Collaborator

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:

  1. clippy-lints: the current code except for the #[plugin_registrar] attribute
  2. clippy: a plugin with #[plugin_registrar] that simply calls the plugin_registrar function in clippy-lints
  3. clippy-cargo: an executable that calls plugin_registrar of clippy-lints like it does now
@oli-obk

oli-obk May 6, 2016

Collaborator

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:

  1. clippy-lints: the current code except for the #[plugin_registrar] attribute
  2. clippy: a plugin with #[plugin_registrar] that simply calls the plugin_registrar function in clippy-lints
  3. clippy-cargo: an executable that calls plugin_registrar of clippy-lints like it does now

This comment has been minimized.

@oli-obk

oli-obk May 6, 2016

Collaborator

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

@oli-obk

oli-obk May 6, 2016

Collaborator

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

This comment has been minimized.

@oli-obk

oli-obk May 6, 2016

Collaborator

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

@oli-obk

oli-obk May 6, 2016

Collaborator

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

@oli-obk

This comment has been minimized.

Show comment
Hide comment
@oli-obk

oli-obk May 6, 2016

Collaborator

addressed comments

Collaborator

oli-obk commented May 6, 2016

addressed comments

@oli-obk

This comment has been minimized.

Show comment
Hide comment
@oli-obk

oli-obk May 6, 2016

Collaborator

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

Collaborator

oli-obk commented May 6, 2016

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

This comment has been minimized.

Show comment
Hide comment
@oli-obk

oli-obk May 6, 2016

Collaborator

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!

Collaborator

oli-obk commented May 6, 2016

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

This comment has been minimized.

Show comment
Hide comment
@oli-obk

oli-obk May 6, 2016

Collaborator

rebased and added docs

Collaborator

oli-obk commented May 6, 2016

rebased and added docs

@llogiq

This comment has been minimized.

Show comment
Hide comment
@llogiq

llogiq May 6, 2016

Collaborator

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

Collaborator

llogiq commented May 6, 2016

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

Show outdated Hide outdated 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(),

This comment has been minimized.

@Manishearth

Manishearth May 8, 2016

Collaborator

... or use multirust

@Manishearth

Manishearth May 8, 2016

Collaborator

... or use multirust

@oli-obk

This comment has been minimized.

Show comment
Hide comment
@oli-obk

oli-obk May 8, 2016

Collaborator

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?

Collaborator

oli-obk commented May 8, 2016

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

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth May 8, 2016

Collaborator

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

Collaborator

Manishearth commented May 8, 2016

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

Show outdated Hide outdated 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

This comment has been minimized.

@Manishearth

Manishearth May 8, 2016

Collaborator

capitalize

@Manishearth

Manishearth May 8, 2016

Collaborator

capitalize

@oli-obk

This comment has been minimized.

Show comment
Hide comment

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth May 10, 2016

Collaborator

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)

Collaborator

Manishearth commented May 10, 2016

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

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth May 10, 2016

Collaborator

works great!

Collaborator

Manishearth commented May 10, 2016

works great!

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

@oli-obk

This comment has been minimized.

Show comment
Hide comment
@oli-obk

oli-obk May 10, 2016

Collaborator

yay! :) I'll add it to TWIR

Collaborator

oli-obk commented May 10, 2016

yay! :) I'll add it to TWIR

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

This comment has been minimized.

@mcarton

mcarton May 10, 2016

Collaborator

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).

@mcarton

mcarton May 10, 2016

Collaborator

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).

This comment has been minimized.

@oli-obk

oli-obk May 10, 2016

Collaborator

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.

@oli-obk

oli-obk May 10, 2016

Collaborator

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.

This comment has been minimized.

@mcarton

mcarton May 10, 2016

Collaborator

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

@mcarton

mcarton May 10, 2016

Collaborator

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

This comment has been minimized.

@Manishearth

Manishearth May 10, 2016

Collaborator

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

@Manishearth

Manishearth May 10, 2016

Collaborator

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

@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth May 10, 2016

Collaborator

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.

Collaborator

Manishearth commented May 10, 2016

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

This comment has been minimized.

Show comment
Hide comment
@cuviper

cuviper May 10, 2016

Contributor

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.

Contributor

cuviper commented May 10, 2016

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

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth May 11, 2016

Collaborator

@arcnmx thoughts?

Collaborator

Manishearth commented May 11, 2016

@arcnmx thoughts?

@arcnmx

This comment has been minimized.

Show comment
Hide comment
@arcnmx

arcnmx May 11, 2016

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

arcnmx commented May 11, 2016

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

@White-Oak

This comment has been minimized.

Show comment
Hide comment
@White-Oak

White-Oak May 12, 2016

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

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

@llogiq

This comment has been minimized.

Show comment
Hide comment
@llogiq

llogiq May 12, 2016

Collaborator

Yes, that's what it does.

Collaborator

llogiq commented May 12, 2016

Yes, that's what it does.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment