Skip to content

Conversation

@blorente
Copy link
Contributor

@blorente blorente commented Oct 7, 2025

Extract the action to run clippy into an importable struct, so that other rulesets can use it.

This is an alternative fix for #3607. Since rules_lint does allow transitive semantics for linting, we can use the clippy action there, and users who want clippy to be transitive can use rules_lint to run it.

The tests pass, and I've also tested it manually against a branch of rules_lint.

Reviewing instructions:

  • Start from rust/defs.bzl. We expose two new functions: One to get the crate info, and one to create the clippy action itself.
  • Then, read the docstring for run_clippy. I've tried to select a reasonable boundary for the arguments, but I'm definitely open to opinions. For instance, we don't ever read ctx.attrs in run_clippy, because we don't want to force users to implement the same arguments and flags, but that may be wrong.
  • Then, go on to the call site of run_clippy, to see if it preserves the semantics you'd expect, particularly around capturing output.
  • Finally, go on to the implementation of run_clippy

@blorente blorente force-pushed the blorente/extract_action_for_rules_lint branch from 7a9fb72 to b1b6e9e Compare October 7, 2025 08:09
success_marker (File): A file that will be written if the clippy succeeds.
cap_at_warnings (bool): If set, it will cap all reports as warnings, allowing the build to continue even with clippy failures
extra_clippy_flags (List[str]): A list of extra options to pass to clippy
error_format (str): Which error format to use. Must be acceptable by rustc: https://doc.rust-lang.org/beta/rustc/command-line-arguments.html#--error-format-control-how-errors-are-produced
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Inside rules_rust, we can use the rustc.bzl file's utility functions to get this. I tried exposing them, but it felt like a leaky enough abstraction that this solution seemed cleaner.

@blorente blorente force-pushed the blorente/extract_action_for_rules_lint branch 2 times, most recently from 593f792 to f0d657a Compare October 7, 2025 08:16
@blorente blorente force-pushed the blorente/extract_action_for_rules_lint branch from f0d657a to b76a1bb Compare October 7, 2025 08:19
@blorente blorente marked this pull request as ready for review October 7, 2025 08:48
@blorente blorente force-pushed the blorente/extract_action_for_rules_lint branch from b76a1bb to c253b1a Compare October 8, 2025 06:46
@blorente
Copy link
Contributor Author

blorente commented Oct 8, 2025

Could I get another approval for the formatting workflow?

Copy link
Collaborator

@UebelAndre UebelAndre left a comment

Choose a reason for hiding this comment

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

One small request but otherwise seems fine!

@blorente blorente requested a review from UebelAndre October 8, 2025 17:32
Copy link
Collaborator

@UebelAndre UebelAndre left a comment

Choose a reason for hiding this comment

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

Thank you!

@UebelAndre UebelAndre enabled auto-merge October 8, 2025 17:49
@UebelAndre UebelAndre added this pull request to the merge queue Oct 8, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 8, 2025
@blorente
Copy link
Contributor Author

blorente commented Oct 8, 2025

The merge queue failure seems to be unrelated: https://buildkite.com/bazel/rules-rust-rustlang/builds/15743/steps/canvas?sid=0199c539-cfda-4661-9b9c-ebe0adfcc847#0199c54a-fed2-41f9-b51e-22dafb24d35f/10-34
@UebelAndre could you confirm if this is unrelated? Happy to investigate if you think it may be related, but better to ask first just in case :D

@UebelAndre UebelAndre added this pull request to the merge queue Oct 8, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 8, 2025
@UebelAndre UebelAndre added this pull request to the merge queue Oct 8, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 8, 2025
@UebelAndre UebelAndre added this pull request to the merge queue Oct 9, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 9, 2025
@UebelAndre UebelAndre added this pull request to the merge queue Oct 9, 2025
Merged via the queue into bazelbuild:main with commit 84ef26a Oct 9, 2025
3 checks passed
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.

2 participants