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

Add a new clippy mode to crater to test lint regressions #391

Merged
merged 4 commits into from
Jan 26, 2019

Conversation

ecstatic-morse
Copy link
Contributor

@ecstatic-morse ecstatic-morse commented Jan 19, 2019

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.

@ecstatic-morse ecstatic-morse changed the title [WIP] Add a new clippy mode to crater to test crater regressions [WIP] Add a new clippy mode to crater to test lint regressions Jan 19, 2019
Copy link
Member

@pietroalbini pietroalbini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything except the mount flag change looks great! Thank you!

I'd prefer not to pass --security-opt=label:disable to Docker though (we're basically running untrusted code and every bit of extra security helps). An alternative is to store the toolchain name on the filesystem urlencoded with a custom encode set (like we do during report generation) that includes : in it.

This commit also adds a method, `Toolchain::install_rustup_component` to
install a rustup componenet on the given toolchain.
The test runs crater in `clippy` mode and compares `stable` with
`stable+rustflags=-Dclippy::all` so that a regression will appear when
clippy would emit a warning on a crate.

Also adds a new local crate which compiles succesfully but causes clippy
to emit a warning.
@ecstatic-morse ecstatic-morse force-pushed the clippy-mode branch 2 times, most recently from e859fb3 to 1d89103 Compare January 26, 2019 06:25
@ecstatic-morse
Copy link
Contributor Author

ecstatic-morse commented Jan 26, 2019

I reverted the changes to docker.rs and started encoding paths using the url::percent_encode. Unicode code points (e.g. U+3D) are also an option.

There's some crossover here with Windows support, so I chose to escape all characters which are invalid as part of a Windows path, not just ':'.

Copy link
Member

@pietroalbini pietroalbini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, thanks!

@pietroalbini pietroalbini changed the title [WIP] Add a new clippy mode to crater to test lint regressions Add a new clippy mode to crater to test lint regressions Jan 26, 2019
@pietroalbini
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jan 26, 2019

📌 Commit 3985d0c has been approved by pietroalbini

@bors
Copy link
Contributor

bors commented Jan 26, 2019

⌛ Testing commit 3985d0c with merge a786b2a...

bors added a commit that referenced this pull request 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
Copy link
Contributor

bors commented Jan 26, 2019

💔 Test failed - checks-travis

@ecstatic-morse
Copy link
Contributor Author

💔

@bors
Copy link
Contributor

bors commented Jan 26, 2019

⌛ Testing commit 3985d0c with merge b77d9f4...

bors added a commit that referenced this pull request 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.
@pietroalbini
Copy link
Member

WTF bors.

@bors r- retry

@ecstatic-morse
Copy link
Contributor Author

Whoops, I forgot to add the results of the clippy minicrater test to the expected results for full. When they finish running locally I'll push again. Sorry!

@ecstatic-morse
Copy link
Contributor Author

Okay, should be good to go now.

@pietroalbini
Copy link
Member

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Jan 26, 2019

📌 Commit 42174eb has been approved by pietroalbini

@bors
Copy link
Contributor

bors commented Jan 26, 2019

⌛ Testing commit 42174eb with merge e39d131...

bors added a commit that referenced this pull request 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
Copy link
Contributor

bors commented Jan 26, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: pietroalbini
Pushing e39d131 to master...

@bors bors merged commit 42174eb into rust-lang:master Jan 26, 2019
@ecstatic-morse ecstatic-morse deleted the clippy-mode branch January 26, 2019 22:22
bors added a commit that referenced this pull request Jan 27, 2019
Add documentation for new clippy mode

Instructions for populating `rustflags` are included elsewhere in the docs, so this single line should suffice.

This should have been included in #391 (whoops).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants