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

Makefile: changed Rust lint clippy to the one included in SDK #2793

Merged
merged 2 commits into from
Mar 2, 2023

Conversation

mjsterckx
Copy link
Contributor

@mjsterckx mjsterckx commented Feb 9, 2023

Issue number:

Closes #2704

Description of changes:

Changed the clippy used in the check-clippy target for Rust linting to the one included in the BUILDSYS_SDK to avoid conflicting lints between local builds and GitHub actions. Blocked until bottlerocket-os/bottlerocket-sdk#89 is merged and included in an SDK update.

Testing done:

Ran cargo make check-clippy with a local cargo install which resulted in a few warnings/errors. Ran it again with the SDK version of clippy which finished with no problems and exit code 0.

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

@jpculp
Copy link
Member

jpculp commented Feb 15, 2023

I'm not sure how often it would actually get used, but do we want to add an override based on an environment variable to use the local clippy? It might be nice for developers that want to ensure this workflow will pass with a newer toolchain than the SDK provides.

@mjsterckx
Copy link
Contributor Author

I personally don't think it would get used enough to warrant an override. Developers who want to check their syntax with a newer toolchain can just run cargo clippy. @bcressey @webern any thoughts?

@webern
Copy link
Member

webern commented Feb 21, 2023

I personally don't think it would get used enough to warrant an override. Developers who want to check their syntax with a newer toolchain can just run cargo clippy. @bcressey @webern any thoughts?

I tend to agree. I think it's easy enough to go into the sources or tools directory and run cargo clippy by hand rather than using an override. (That's what I would probably do...)

@mjsterckx mjsterckx marked this pull request as ready for review February 28, 2023 22:58
@bcressey
Copy link
Contributor

bcressey commented Mar 1, 2023

Doesn't have to happen in this PR, but do we also want to give cargo fmt the same treatment, in case the behavior changes across versions?

@mjsterckx
Copy link
Contributor Author

Doesn't have to happen in this PR, but do we also want to give cargo fmt the same treatment, in case the behavior changes across versions?

I'm not sure how frequently the behavior changes, but I like the idea of staying consistent. It should be an easy addition, so I can create an issue for that and work on it in a different PR.

@mjsterckx mjsterckx merged commit 92aba85 into bottlerocket-os:develop Mar 2, 2023
@mjsterckx mjsterckx deleted the sdk-clippy-lint branch March 2, 2023 14:45
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.

Use the SDK for Linting
4 participants