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

Comments

Projects
None yet
3 participants
@cramertj
Copy link

cramertj commented Jan 11, 2019

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

This comment has been minimized.

Copy link
Author

cramertj commented Jan 11, 2019

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

@pietroalbini

This comment has been minimized.

Copy link
Member

pietroalbini commented Jan 11, 2019

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!

@ecstatic-morse

This comment has been minimized.

Copy link
Contributor

ecstatic-morse commented Jan 18, 2019

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

bors added a commit that referenced this issue Jan 26, 2019

Auto merge of #391 - ecstatic-morse:clippy-mode, r=pietroalbini
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

Auto merge of #391 - ecstatic-morse:clippy-mode, r=pietroalbini
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

Auto merge of #391 - ecstatic-morse:clippy-mode, r=pietroalbini
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 in #391 Jan 26, 2019

@ecstatic-morse

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

pietroalbini commented Jan 26, 2019

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

@ecstatic-morse

This comment has been minimized.

Copy link
Contributor

ecstatic-morse commented Jan 26, 2019

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

@pietroalbini

This comment has been minimized.

Copy link
Member

pietroalbini commented Jan 26, 2019

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

@pietroalbini

This comment has been minimized.

Copy link
Member

pietroalbini commented Jan 27, 2019

Now it's deployed!

@ecstatic-morse

This comment has been minimized.

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