CI: enforce clippy rules in clippy command

[dianpopa]

Changes

• move clippy rules from source files to clippy integ test

Reason

• does not pollute code
• enfroces the clippy rules on all ulterior source files/crates

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.

PR Checklist

• All commits in this PR are signed (git commit -s).
• If a specific issue led to this PR, this PR closes the issue.
• The description of changes is clear and encompassing.
• Any required documentation changes (code and docs) are included in this PR.
• New unsafe code is documented.
• API changes follow the Runbook for Firecracker API changes.
• User-facing changes are mentioned in CHANGELOG.md.
• All added/changed functionality is tested.

---

• This functionality can be added in rust-vmm.

[roypat]

Very much in favor of this, it will also make reviewing PRs that introduce new clippy lints so much easier to review because the change set wont be polluted by the addition of warn attributes.

[roypat]

nit: make this a single string

also: why do we have to append the "-D warnings" below, can't we include the it here, like it was before?

[dianpopa]

What do you mean I should make a single string?

I got it

[JonathanWoollett-Light]

I think we should include the rules in the code and in the command.

By moving them outside the code, cargo clippy becomes less useful, it would be required to copy and paste the long form command to test clippy.

[dianpopa]

I think we should include the rules in the code and in the command.

By moving them outside the code, cargo clippy becomes less useful, it would be required to copy and paste the long form command to test clippy.

Less useful would mean that with this change, we run the risk to uncover less errors. i do not think this is the case, right?

Are you maybe saying that it is easier to test clippy if we leave the warnings inside the source code?

[JonathanWoollett-Light]

I think we should include the rules in the code and in the command.
By moving them outside the code, cargo clippy becomes less useful, it would be required to copy and paste the long form command to test clippy.

Less useful would mean that with this change, we run the risk to uncover less errors. i do not think this is the case, right?

Are you maybe saying that it is easier to test clippy if we leave the warnings inside the source code?

Yes you are right, I did mean that it is easier to use clippy if the warnings are set inside the source code (it will quickly become like cargo fmt currently where it is practically required to copy and paste the command to run it which is somewhat awkward).

[dianpopa]

I think we should include the rules in the code and in the command.
By moving them outside the code, cargo clippy becomes less useful, it would be required to copy and paste the long form command to test clippy.

Less useful would mean that with this change, we run the risk to uncover less errors. i do not think this is the case, right?
Are you maybe saying that it is easier to test clippy if we leave the warnings inside the source code?

Yes you are right, I did mean that it is easier to use clippy if the warnings are set inside the source code (it will quickly become like cargo fmt currently where it is practically required to copy and paste the command to run it which is somewhat awkward).

I feel that by making it easier for integration tests to be run standalone we might be sending the wrong message or encouragement.
Running these commands outside of devtool should not be smth we recommend (for the obvious reasons -> toochain compatibility etc).
If already inside devtool, I do not see why running the actual integ test (i.e pytest integration_tests/build/test_clippy) is more complicated than using cargo clippy, it is one command afterall.

Another aspect of keeping the clippy warnings inside source code is just something not sustainable. We will be replacing crates in the future or just add new ones; how do we stay consistent? is the effort of staying consistent worth the benefits?

Additionally, we should be striving to have in Firecracker source code that does not tie into specificities of deployment or integration testing (for ease of forking).

[JonathanWoollett-Light]

I feel that by making it easier for integration tests to be run standalone we might be sending the wrong message or encouragement.
Running these commands outside of devtool should not be smth we recommend (for the obvious reasons -> toochain compatibility etc).

Even developing within devtool the integration test is more awkward sudo ./tools/devtool test -- integration_tests/build/test_clippy.py is not very clear. I think it would be better to use the tools directly rather than relying on custom scripts, I think this is the most ergonomic approach.

Another aspect of keeping the clippy warnings inside source code is just something not sustainable. We will be replacing crates in the future or just add new ones; how do we stay consistent? is the effort of staying consistent worth the benefits?

At the moment this is awkward due to the number of crates in the workspace, at the moment we have too many separate crates, in the future we should have less crates (this draft PR #3079 reduces the number of crates to 5) and this should decrease the effort of maintaining consistency here.

I understand your points here, but I still would argue it is more effort to need to copy and paste a command from an integration test on every PR than to maintain <20 lines of lint specifics in each crate, which will likely rarely need to changed.

Additionally, we should be striving to have in Firecracker source code that does not tie into specificities of deployment or integration testing (for ease of forking).

I'm not sure I see your point here.

[ShadowCurse]

Even developing within devtool the integration test is more awkward sudo ./tools/devtool test -- integration_tests/build/test_clippy.py is not very clear. I think it would be better to use the tools directly rather than relying on custom scripts, I think this is the most ergonomic approach.

I agree with @JonathanWoollett-Light here. It should be easy to use tool directly + it would be faster, considering how slow pytest is.
But at the same time having lints in the code is not ideal, considering that they must appear in multiple places.
From what I know, clippy can be configured through RUST_FLAGS:

[build]
rustflags = ["-Dclippy::print-stdout"]

so maybe it would be easier to have all lints in .cargo/config?

[roypat]

Even developing within devtool the integration test is more awkward sudo ./tools/devtool test -- integration_tests/build/test_clippy.py is not very clear. I think it would be better to use the tools directly rather than relying on custom scripts, I think this is the most ergonomic approach.

I agree with @JonathanWoollett-Light here. It should be easy to use tool directly + it would be faster, considering how slow pytest is. But at the same time having lints in the code is not ideal, considering that they must appear in multiple places. From what I know, clippy can be configured through RUST_FLAGS:

[build]
rustflags = ["-Dclippy::print-stdout"]

so maybe it would be easier to have all lints in .cargo/config?

iirc the problem with having the lint in .cargo/config was that RUSTFLAGS is passed to all rustc invocations, even those that compile our dependencies. So if one of our dependencies causes a lint, it might cause compilation to fail in specific instances.

[JonathanWoollett-Light]

We can have a clippy.rs at the root then each library can have include!("./clippy.rs") (https://doc.rust-lang.org/std/macro.include.html).

[dianpopa]

We internally decided to go ahead with having a clippy.rs at the root then each library can have include!("./clippy.rs") (https://doc.rust-lang.org/std/macro.include.html).