Skip to content
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

Nuke the clippy plugin interface #2783

Closed
wants to merge 1 commit into from
Closed

Nuke the clippy plugin interface #2783

wants to merge 1 commit into from

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented May 20, 2018

It's been a month or so 10 days since the plugin interface emits unsilencable warnings. I have not seen any complaints. I guess noone was using the plugin interface anymore.

@oli-obk oli-obk added the S-needs-discussion Status: Needs further discussion before merging or work can be started label May 20, 2018
@phansch
Copy link
Member

phansch commented May 20, 2018

Hmm, this GitHub search reveals quite a few results where the plugin interface is used.

Some popular users include:

Perhaps we should communicate this better (through the dev-tools news, maybe) before removing it?

@oli-obk
Copy link
Contributor Author

oli-obk commented May 20, 2018

@oli-obk
Copy link
Contributor Author

oli-obk commented May 20, 2018

Getting the word out via twir: https://twitter.com/elegmit/status/998178638234341377

@ssokolow
Copy link

ssokolow commented May 30, 2018

Drat. I was looking forward to when clippy was available on stable channel so I could switch to the plugin API.

I suppose I could do the same sort of thing I was hoping to avoid with my contributions to the request for a post-build script and set up a system where cargo build and cargo run error out if called directly, rather than via a wrapper.

(The wrapper would first call cargo clippy and I'd then tie essentially all compiler output to #![cfg_attr(feature="cargo-clippy", ...)] to avoid duplication.)

Of course, then I'd also have to fork setuptools-rust to keep things working in some of my projects, which would be annoying.

@oli-obk
Copy link
Contributor Author

oli-obk commented May 30, 2018

Well... even without this change we would not be stabilizing the plugin interface.

What you can always do is just set the RUSTC_WRAPPER env var to clippy-driver and CLIPPY_TESTS to true to unconditionally replace rustc with the driver.

@ssokolow
Copy link

ssokolow commented May 30, 2018

Ahh. That'd help.

I haven't needed to use build scripts before so I'm uncertain how powerful they are.

Are they capable of forcing the value of RUSTC_WRAPPER and CLIPPY_TESTS within the appropriate execution context when cargo build or cargo run are run, or do I have to resort to using env! or something in the vein of #[cfg(not(cargo-clippy))] to abort the build with a "Use the wrapper" message?

@oli-obk
Copy link
Contributor Author

oli-obk commented May 30, 2018

You want to force users of your library to use clippy?

@ssokolow
Copy link

ssokolow commented May 30, 2018

Only in the case of certain projects and, in all such cases, it being a library is an implementation detail of a single-repository, multi-language binary.

For example, I have a project which is a Python-Rust hybrid creation where I'd do it purely in Rust as a binary, but there are no acceptably mature QWidget bindings for Rust, so I'm forced to resort to putting as much as possible in a Rust backend via rust-cpython, which makes no attempt to be reusable outside the Python code which glues it to Qt's Python bindings.

(Naturally, I will eventually split the reusable bits out into their own non-clippy-forcing crates when I find time to work on my "GUI for divvying up a git repository's contents while verifying correct assignment of hunks in previous revisions" project.)

@oli-obk
Copy link
Contributor Author

oli-obk commented May 30, 2018

Yea in that case I'd just resolve to the #[cfg(not(cargo-clippy))] trick. Seems like an edge case

@flip1995 flip1995 mentioned this pull request Oct 22, 2018
@flip1995
Copy link
Member

flip1995 commented Nov 1, 2018

Do we want to do this before releasing Clippy 1.0?

@oli-obk
Copy link
Contributor Author

oli-obk commented Nov 1, 2018

It seems all the linked issues have been resolved.

The only thing left is to figure out how to make cargo not rebuild everything when going from cargo build to cargo clippy. I believe we need cargo support for this.

@flip1995
Copy link
Member

flip1995 commented Jun 6, 2019

Ping @oli-obk. Should we S-inactive-closed this and revisit it (in a new PR) once #3837 is resolved?

@oli-obk oli-obk added the S-inactive-closed Status: Closed due to inactivity label Jun 6, 2019
@oli-obk oli-obk closed this Jun 6, 2019
@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 6, 2019

yea, that makes sense

@tylerhawkes
Copy link

Well... even without this change we would not be stabilizing the plugin interface.

What you can always do is just set the RUSTC_WRAPPER env var to clippy-driver and CLIPPY_TESTS to true to unconditionally replace rustc with the driver.

I just tried this on a non-trivial project and it doubled my build time from 20 minutes to 40 minutes. I've been wanting a way to use clippy without having to also separately build but nothing seems to be working. It's nice to have clippy automatically run whenever I do a build so I don't have to remember it and fix clippy issues later. Any ideas on how we can keep build times reasonable without having to do multiple clippy steps?

@flip1995 flip1995 mentioned this pull request Oct 5, 2019
3 tasks
@mati865
Copy link
Contributor

mati865 commented Oct 9, 2019

@tylerhawkes on nightly you can try cargo clippy-preview -Z unstable-options.

@flip1995 flip1995 mentioned this pull request Oct 22, 2019
@flip1995 flip1995 deleted the plugincalypse branch October 28, 2019 08:26
@jonboh
Copy link
Contributor

jonboh commented Oct 10, 2023

@rustbot label -S-inactive-closed

@rustbot rustbot removed the S-inactive-closed Status: Closed due to inactivity label Oct 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-needs-discussion Status: Needs further discussion before merging or work can be started
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants