Skip to content

Implement Clippy Aspect & Build Rule #339

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

Merged
merged 18 commits into from
Jun 17, 2020
Merged

Implement Clippy Aspect & Build Rule #339

merged 18 commits into from
Jun 17, 2020

Conversation

smklein
Copy link
Contributor

@smklein smklein commented Jun 13, 2020

Follow-up from #338

Working on issue: #304

@smklein smklein marked this pull request as ready for review June 13, 2020 23:55
@smklein smklein changed the title Clippy aspect Implement Clippy Aspect & Build Rule Jun 13, 2020
@smklein
Copy link
Contributor Author

smklein commented Jun 16, 2020

As a final validity test, I ran:

cd examples
bazel build --aspects=@io_bazel_rules_rust//rust:rust.bzl%rust_clippy_aspect --output_groups=clippy_checks //...

This uncovered a handful of issues -- which have since been fixed. Of note:

  • Many of the "compilation-command-building" arguments assume they ingest ctx, and operate on ctx.files or ctx.file directly. When building directly on a target, this is fine, but in the context of an aspect, this is non-ideal, since the actual desired file/files exist within ctx.rule.files or ctx.rule.file, respectively. The functions have been refactored to act on a user-supplied file/files in these cases.
  • Rather than trying to "recreate" CrateInfo, I realized I could just access it directly from the Aspect's target, so I'm doing that now.

@damienmg I appreciate the help getting this patch reviewed, but I don't want to slide something in after the "+2" -- would you be willing to take another look?

Also, I'm currently running clippy on the examples/ directory manually - I think it would be cool if we could actually add that to the automated build. I tried adding a top-level rust_clippy targets that includes all rust_(library,binary,test) targets, but this would require significant changes to the visibility within examples/.

Any advice on this? How could I go about changing the buildkite script to run this as a check on future PRs?

@smklein smklein requested a review from damienmg June 16, 2020 16:38
@damienmg
Copy link
Collaborator

So for test, I think this is fine as it is.

You could also modify the preubmit.yml file to add the test you want but I am fine with checking this PR in as-is.

@smklein
Copy link
Contributor Author

smklein commented Jun 17, 2020

So for test, I think this is fine as it is.

You could also modify the preubmit.yml file to add the test you want but I am fine with checking this PR in as-is.

Oh rad, didn't see that file! I added clippy_examples and clippy_failure as tests to the presubmit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants