Deprecate plugin-clippy

[oli-obk]

I don't think anybody gains much from our crates.io releases, as it's a mess to figure out and breaks often.

Fixing the master branch is usually much better, since contributors can do so and we just have to punch the merge button instead of having to do the release dance on a local machine.

The plugin method is also something that the rustc devs are trying to phase out, so lets stay ahead of the curve

[oli-obk]

cc @Manishearth @llogiq @mcarton @phansch opinions?

[killercup]

Just FYI I use pinned (clippy, rustc) versions on CI to get a consistent cargo clippy experience. This change means I'd need to use cargo install --git https://github.com/rust-lang-nursery/rust-clippy --rev … instead, right?

Any way you could easily auto-publish crates.io releases or git tags that map to nightlies so I can easily determine which rev to install? E.g., push a 2018-05-01 tag when a PR lands on master that builds with todays nightly?

You should also adjust cargo clippy -V output.

[Manishearth]

I think we should continue publishing numbered versions (for use cases like killercup's, but also just since git installs are weird) but stop providing the plugin interface. But I don't have a strong opinion.

[oli-obk]

It would be easier to auto-tag the every merge with the rustc version that it works with than to auto-publish versions to crates.io

[sgrif]

As long as it's easy for me to get the right clippy version for a given Rust version, and that works with travis caching, I don't think it matters much. That said, I think auto publishing to crates.io makes much more sense than publishing differently than the rest of the ecosystem.

[phansch]

I totally agree that we have to find ways to fix clippy faster and to have less failures overall. But I don't think that not publishing on crates.io would make things better. Users would have to come here to figure out the last working commit and master could still fail when a new nightly has been released.
As a user I would also find it weird to have to navigate through the commit history to pin my clippy to a specific commit. Versions can carry much more meaning than git revisions.

We can advertise using the master branch as an emergency workaround maybe?

[dtolnay]

I'm happy to type cargo publish every now and then if that would help. 😉

[matthiaskrgr]

Perhaps we could have clippy-beta and clippy-stable releases on crates.io at some point? :)

[oli-obk]

Ok. This PR is now just about deprecating the #![plugin(clippy)] api. Users will get an unsilenceable warning after this PR telling them to move to rls+clippy or cargo clippy. In a month or so we can nuke the API entirely if noone complains too loudly ;)

[sgrif]

Is this the right place to complain loudly? 😄

[oli-obk]

sure. But I haven't seen a complaint yet ;)

[oli-obk]

@sgrif did you have a complaint? I really really want to get rid of the plugin API. And even though for a week there's been an unconditional warning that should be fairly annoying, everything has been very quiet around that.

[sgrif]

I've been on vacation, sorry. Haven't updated our minimum clippy version in a while either since there's a bug in recent nightlies that breaks Diesel.

I don't like getting rid of the plugin API personally. We have it set so that when Diesel's test suite is run locally, it's linted as well. cargo clippy is a separate command that requires recompiling all the crates again (adding minutes to the feedback cycle), and that I have to remember to run in the first place. Running it as a separate step makes sense for CI, but hurts the work flow for local development.

[oli-obk]

I don't like getting rid of the plugin API personally.

rustc will get rid of it at some point. At that point there's no recovery solution. Right now you can run clippy both as a plugin and as a cargo clippy command, so you can experiment with moving without having much pressure.

Let's continue the discussion for diesel in diesel-rs/diesel#1709

[U007D]

Can you explain how to get clippy to work the way it did as a plugin? Right now, this is a regression for me, unless I accept the plugins-are-deprecated warning spew. I use Clippy in the first place because I take warnings seriously.

In the plug-in world, I run cargo test --features clippy in CLion as I develop to do a test run with the benefit of a strict build.

After cargo install clippy and removing Clippy plugin attributes from my source, cargo clippy just builds, and cargo test clippy and the original cargo test --features clippy warn about clippy lints and runs no tests.

Am I missing something?

--

Another vote for continuing crates.io. One of the strengths of the Rust/Cargo ecosystem is its consistency. The idea of handling some crates differently from others feels like a step back, to me. Instead, is it possible to invest in automation for publishing to crates.io so that publishing is not painful?

[oli-obk]

Another vote for continuing crates.io. One of the strengths of the Rust/Cargo ecosystem is its consistency. The idea of handling some crates differently from others feels like a step back, to me. Instead, is it possible to invest in automation for publishing to crates.io so that publishing is not painful?

We will continue publishing normally via crates.io as long as you cannot obtain clippy via rustup.

After cargo install clippy and removing Clippy plugin attributes from my source, cargo clippy just builds, and cargo test clippy and the original cargo test --features clippy warn about clippy lints and runs no tests.

cargo clippy is essentially a wrapper around RUSTC_WRAPPER=clippy-driver cargo check. So you can use cargo clippy --tests just like cargo check --tests. If anything doesn't work, please do open issues!

[U007D]

@oli-obk perhaps the problem is I'm not using cargo check (at least not explicitly). I use cargo test with a number of switches to get both a strict build and a test run in one pass.

Given Cargo.toml:

...
[features]
default = ["clippy"]
test = ["di_containers/test"]
...

cargo test --all --tests --features test yields

    Finished dev [unoptimized + debuginfo] target(s) in 6.71s
     Running target/debug/deps/ada-62d08023e6eb711b

...
tests:

Given "a FrictBrakeAmt::from_unorm() constructor"
  When "called with the minimum valid unorm value"
    Then "it should equal that value" ... ok
    Then "and it should equal FrictBrakeAmt::MIN" ... ok
  When "called with the maximum valid unorm value"
    Then "and it should equal that value" ... ok
    Then "it should equal FrictBrakeAmt:MAX" ... ok
  When "called with a valid, intermediate unorm value"
    Then "it should equal that value" ... ok
...

which is the expected behavior--code is built strictly, with clippy lints enforced, and tests are run.

cargo clippy --all --tests --features test (and every other variation I've tried) gives just a clippy linted build:

    Checking shal v0.1.1 (file:///home/brad/Development/rust/ada/shal)
    Checking rpi3_chal v0.1.1 (file:///home/brad/Development/rust/ada/rpi3_chal)
    Checking ada_shal v0.1.1 (file:///home/brad/Development/rust/ada/ada_shal)
    Checking ada_utils v0.1.0 (file:///home/brad/Development/rust/ada/ada_utils)
    Checking di_containers v0.1.0 (file:///home/brad/Development/rust/ada/di_containers)
    Checking ada v0.1.2 (file:///home/brad/Development/rust/ada)
    Finished dev [unoptimized + debuginfo] target(s) in 56.33s

Process finished with exit code 0

which now makes some sense since you mention that cargo clippy is a fancy cargo check.

How do I build strictly and test in one step now? Hopefully I'm missing something simple here, because compiling twice now for every edit/build/test cycle doesn't seem right?

[oli-obk]

This is still hacky, but should work:

CLIPPY_TESTS=true RUSTC_WRAPPER=clippy-driver cargo test

We will work on improving the experience around this. cargo clippy test sounds totally reasonable.

[U007D]

We will work on improving the experience around this.

Happy to hear this. In my case, the usage pattern is exactly g++ -Wall or clang -Weverything. The Clippy warnings serve to enstricten (not a real word ;)) the build, but are completely orthogonal to the context of the build (build/check/test/--release/--target etc.)

CLIPPY_TESTS=true RUSTC_WRAPPER=clippy-driver cargo test

That did the trick. Thank you.

[mulkieran]

You merged this PR w/ the warning only after clippy had become so flaky that we had been forced to make it optional on Travis. So, we haven't really been looking at the clippy results so much, and only saw the deprecation warning belatedly. Maybe that is what happened to others, too.

[oli-obk]

Yea I'm keeping this open for a while. Maybe even moving to reporting an error with "you need to enable a magical environment varable to keep using the plugin API", that'll break everybody's workflow, but they have a trivial fix.

Sorry to hear about the flakiness. All these changes are moving towards simplifying clippy and finally getting it into rustup.

Note to anyone reading this: Currently there are only plans to move the clippy-driver (so the cargo clippy variant) to rustup, there is no plan for the plugin.

[mulkieran]

Ack. Thanks for the hard work.