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

Expand the scope of clippy checks #21

Open
alexandruag opened this issue Jun 18, 2020 · 7 comments
Open

Expand the scope of clippy checks #21

alexandruag opened this issue Jun 18, 2020 · 7 comments
Labels
enhancement New feature or request

Comments

@alexandruag
Copy link
Contributor

The current way of invoking clippy as part of the CI pipeline (cargo clippy --all -- -D warnings) does not check some parts of the code. It looks like the exhaustive list of relevant arguments is something along the lines of cargo clippy --workspace --bins --examples --tests --benches --all-targets --all-features -- -D warnings, with the caveat that --all-features might not make sense for all crates.

@andreeaflorescu
Copy link
Member

Shouldn't all-features be a nop in case the crate doesn't have any features?

@alexandruag
Copy link
Contributor Author

I was mainly thinking about crates where some features are contradictory and/or not meant to be enabled at the same time. Dunno exactly what could happen (with clippy at least) in that case :-s

@andreeaflorescu
Copy link
Member

Right.

@alxiord
Copy link
Member

alxiord commented Jun 19, 2020

We already cargo test --all-features, so the same concern applies there and I have no idea how to address it without dedicated buildkite pipelines, which I'd rather avoid. For now, we don't have any contradicting features in any crate.
Is --bins necessary? No crate produces a binary yet, and besides --workspace should cover it.

@alexandruag
Copy link
Contributor Author

Sure, just raising the issue, and that we should be careful with crates where features are not orthogonal. For example, kvm-bindings has some additional steps in its pipeline for the different features (or so it seems); in the kvm-bindings case --all-features effectively selects just some of the features, due to our #cfg annotations.

--bins doesn't seem necessary for now; just added it to the list for good measure.

@alexandruag
Copy link
Contributor Author

Ok so after a bit of digging around, it seems we get the most potential complaining out of clippy by running it with something like cargo clippy --bins --examples --tests --benches --release --all-targets --all-features --workspace -- -W clippy::all -W clippy::pedantic -W clippy::restriction -W clippy::nursery -W clippy::cargo. I guess we have to decide how strict we want to be (especially when it comes to the pedantic and restriction things), but either way it seems we've been missing some useful lints by running clippy the way we did before.

lauralt added a commit to lauralt/rust-vmm-ci that referenced this issue Oct 7, 2020
Without this flag, it looks like not all workspace files
are checked. You can test this by adding for example an
unused import (`use std::clone;`) in
rust-vmm/vmm-reference#11,
file src/vmm/tests/integration_tests.rs. cargo check will
fail only if we pass `--workspace` flag too.
For clippy, this flag doesn't seem to have an obvious
effect, this should be further investigated in rust-vmm#21.

Signed-off-by: Laura Loghin <lauralg@amazon.com>
lauralt added a commit to lauralt/rust-vmm-ci that referenced this issue Oct 7, 2020
Without this flag, it looks like not all workspace files
are checked. You can test this by adding for example an
unused import (`use std::clone;`) in
rust-vmm/vmm-reference#11,
file src/vmm/tests/integration_tests.rs. cargo check will
fail only if we pass `--workspace` flag too.
For clippy, this flag doesn't seem to have an obvious
effect, this should be further investigated in rust-vmm#21.

Signed-off-by: Laura Loghin <lauralg@amazon.com>
alxiord pushed a commit that referenced this issue Oct 8, 2020
Without this flag, it looks like not all workspace files
are checked. You can test this by adding for example an
unused import (`use std::clone;`) in
rust-vmm/vmm-reference#11,
file src/vmm/tests/integration_tests.rs. cargo check will
fail only if we pass `--workspace` flag too.
For clippy, this flag doesn't seem to have an obvious
effect, this should be further investigated in #21.

Signed-off-by: Laura Loghin <lauralg@amazon.com>
@andreeaflorescu
Copy link
Member

I ran some experiments with clippy as well. We're now missing the most basic of them all (--all-features) which is already causing problems.

First, I propose we use the ones that are non-debatable:

  • examples
  • bin
  • all-features
  • workspace

The tests and benches can be too restrictive for all rust-vmm repos, as in tests you can do some of the things that clippy "disagrees" with on purpose. I'll submit a PR for the first part, and we can leave this issue open for further debates.

andreeaflorescu added a commit to andreeaflorescu/rust-vmm-ci that referenced this issue Mar 15, 2021
We were not testing clippy for all features, the workspace, bins, and
examples.

Partially addresses: rust-vmm#21

Signed-off-by: Andreea Florescu <fandree@amazon.com>
alxiord pushed a commit that referenced this issue Mar 15, 2021
We were not testing clippy for all features, the workspace, bins, and
examples.

Partially addresses: #21

Signed-off-by: Andreea Florescu <fandree@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants