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

Test Clippy Lint #388

Closed
cramertj opened this issue Jan 11, 2019 · 9 comments
Closed

Test Clippy Lint #388

cramertj opened this issue Jan 11, 2019 · 9 comments
Labels
C-new-feature Category: a new feature to implement E-easy Call for participation: this issue is easy to fix E-mentor Call for participation: this issue has instructions how to fix it

Comments

@cramertj
Copy link
Member

It'd be really helpful for rust-lang/rust#49000 (comment) to be able to find places in the ecosystem that trigger the clippy lint added in rust-lang/rust-clippy#3344. Would it be possible to add a feature to crater to allow running with a clippy warning set to deny-by-default?

@cramertj
Copy link
Member Author

cc @pietroalbini who mentioned they didn't think this would be too hard to add when I spoke with them on discord.

@pietroalbini pietroalbini added the C-new-feature Category: a new feature to implement label Jan 11, 2019
@pietroalbini
Copy link
Member

Crater already supports denying lints (with rustflags=-Dlint_name), so the only thing that needs to be added is executing cargo clippy.

That should be implemented as a new experiment mode called clippy with a corresponding task step in the runner. Once you add the variants to the two enums fixing the errors emitted by the compiler should be enough to implement the mode.

Since clippy is not installed by default you should also install it at the start of the runner (only when the experiment mode is clippy). Adding a method like toolchain.install_rustup_component("clippy") to do that would be awesome.

Also, adding a test to minicrater would be awesome!

If you want to work on this issue please comment on it so other contributors know it's taken. And if you need some help or you get stuck please reply here or ask on the #crater channel on Discord!

@pietroalbini pietroalbini added E-easy Call for participation: this issue is easy to fix E-mentor Call for participation: this issue has instructions how to fix it labels Jan 11, 2019
@ecstatic-morse
Copy link
Contributor

I just started working on this. PR to come shortly.

bors added a commit that referenced this issue Jan 26, 2019
Add a new `clippy` mode to crater to test lint regressions

Resolves #388.

This adds a new mode, `clippy`, to crater which runs clippy on crates (duh :).

I had to add some new options to minicrater, as well as change the invocation of `docker create`. See the commit messages for more info.

This is still a work in progress as I'd like to add some documentation for the feature, but should be enough for a meaningful review.
bors added a commit that referenced this issue Jan 26, 2019
Add a new `clippy` mode to crater to test lint regressions

Resolves #388.

This adds a new mode, `clippy`, to crater which runs clippy on crates (duh :).

I had to add some new options to minicrater, as well as change the invocation of `docker create`. See the commit messages for more info.

This is still a work in progress as I'd like to add some documentation for the feature, but should be enough for a meaningful review.
bors added a commit that referenced this issue Jan 26, 2019
Add a new `clippy` mode to crater to test lint regressions

Resolves #388.

This adds a new mode, `clippy`, to crater which runs clippy on crates (duh :).

I had to add some new options to minicrater, as well as change the invocation of `docker create`. See the commit messages for more info.

This is still a work in progress as I'd like to add some documentation for the feature, but should be enough for a meaningful review.
@bors bors closed this as completed in #391 Jan 26, 2019
@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented Jan 26, 2019

@cramertj, This should now will soon be possible using something like:

@craterbot run name=clippy-test-run mode=clippy start=stable end=stable+rustflags=-Dclippy::your_lint_here

@pietroalbini
Copy link
Member

I didn't have time to deploy the changes yet.

@ecstatic-morse
Copy link
Contributor

Sorry! I don't understand how bors works I guess.

@pietroalbini
Copy link
Member

The changes are merged by bors on the master branch, but unfortunately deploying them on the servers is still a manual process :(

@pietroalbini
Copy link
Member

Now it's deployed!

@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented Jan 31, 2019

@pietroalbini

Unfortunately, denying clippy lints with cargo via RUSTFLAGS=-Dclippy::lint causes a build failure for crates with a build.rs, so this isn't terribly useful at the moment 😣. Invoking cargo clippy -- -Dclippy::lint succeeds as expected.

I'm not sure how to proceed here. Perhaps this should be fixed in cargo or clippy? Otherwise we could pass flags to rustc without using the environment variable, or strip anything that looks like a clippy lint out before setting RUSTFLAGS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-new-feature Category: a new feature to implement E-easy Call for participation: this issue is easy to fix E-mentor Call for participation: this issue has instructions how to fix it
Projects
None yet
Development

No branches or pull requests

3 participants