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.

Sign up for GitHub

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

Jump to bottom

clippy: use --cap-lints=warn; apply clippy_flags when capture_output=True #2451

Merged
merged 6 commits into from
Apr 10, 2024
Merged

clippy: use --cap-lints=warn; apply clippy_flags when capture_output=True #2451

merged 6 commits into from
Apr 10, 2024

Conversation

goffrie
Copy link
Contributor

@goffrie goffrie commented Feb 1, 2024

Instead of using -Wclippy::all to override deny-by-default clippy lints into warnings, use --cap-lints=warn.
This allows users to provide whatever lint flags they like and the compiler will still succeed.

@krasimirgg krasimirgg self-assigned this Feb 12, 2024
krasimirgg
krasimirgg reviewed Feb 12, 2024
View reviewed changes
Copy link
Collaborator

@krasimirgg krasimirgg left a comment

Choose a reason for hiding this comment

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

This is cool! Just want to make sure folks on the old discussion thread are aware of this PR.

rust/private/clippy.bzl
@@ -141,12 +141,11 @@ def _clippy_aspect_impl(target, ctx):
args.process_wrapper_flags.add("--stderr-file", clippy_out)

if clippy_flags:
fail("""Combining @rules_rust//:clippy_flags with @rules_rust//:capture_clippy_output=true is currently not supported.
See https://github.com/bazelbuild/rules_rust/pull/1264#discussion_r853241339 for more detail.""")
args.rustc_flags.add_all(clippy_flags)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you ping that discussion thread to mention that this PR lifts this restriction?

@goffrie goffrie mentioned this pull request Feb 12, 2024
Merged
@buchgr
Copy link
Contributor

buchgr commented Feb 13, 2024 •
edited

Thanks @goffrie. IIUC this allows users to set additional lints as warnings and anything more strict will be ignored. The need to specify additional lints as warnings makes sense to me. However, I'd expect the rules to error if a an unsupported deny lint is specified, rather than silently ignore it. What do you think? Would it be sensible to add this failure behavior?

@goffrie
Copy link
Contributor Author

goffrie commented Feb 15, 2024

I don't see this as a problem. --deny flags will be treated as though they were --warn, but specifying those flags as deny can still be useful. For example, a workspace may have clippy targets that are sometimes built directly by the developer without capture_clippy_output, in which case the --deny would do the right thing; and sometimes depended upon by other rules with a capture_clippy_output transition, in which case --deny would be downgraded.

@krasimirgg krasimirgg mentioned this pull request Mar 13, 2024
Open
@krasimirgg
Copy link
Collaborator

Thank you! Chatted with @buchgr this looks good to merge, with a follow-up issue #2548 about the potential user confusion around user-defined --deny flags.

@scentini scentini enabled auto-merge April 3, 2024 08:37
@scentini scentini self-requested a review April 10, 2024 08:15
@scentini scentini added this pull request to the merge queue Apr 10, 2024
Merged via the queue into bazelbuild:main with commit 8802f4d Apr 10, 2024
3 checks passed
rrbutani pushed a commit to bazel-contrib/toolchains_llvm that referenced this pull request Apr 10, 2024
@renovate
Update dependency rules_rust to v0.42.0 (#313)
3d9f02b 
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [rules_rust](https://togithub.com/bazelbuild/rules_rust) |
http_archive | minor | `0.41.1` -> `0.42.0` |

---

### Release Notes

<details>
<summary>bazelbuild/rules_rust (rules_rust)</summary>

###
[`v0.42.0`](https://togithub.com/bazelbuild/rules_rust/releases/tag/0.42.0)

[Compare
Source](https://togithub.com/bazelbuild/rules_rust/compare/0.41.1...0.42.0)

### 0.42.0

```python
load("@&#8203;bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
http_archive(
    name = "rules_rust",
    integrity = "sha256-XT1YVJ6FHJTXBr1v3px2fV37/OCS3dQk3ul+XvfIIf8=",
    urls = ["https://github.com/bazelbuild/rules_rust/releases/download/0.42.0/rules_rust-v0.42.0.tar.gz"],
)
```

Additional documentation can be found at:
https://bazelbuild.github.io/rules_rust/#setup

#### What's Changed

- Fix crates.io URL by
[@&#8203;ericmcbride](https://togithub.com/ericmcbride) in
[bazelbuild/rules_rust#2597
- Add support for `--compile_one_dependency` by
[@&#8203;william-smith-skydio](https://togithub.com/william-smith-skydio)
in
[bazelbuild/rules_rust#2598
- Update rules_apple by
[@&#8203;sgowroji](https://togithub.com/sgowroji) in
[bazelbuild/rules_rust#2602
- Support building more things with bzlmod by
[@&#8203;matts1](https://togithub.com/matts1) in
[bazelbuild/rules_rust#2601
- Make bazel lock file cross-platform by
[@&#8203;cameron-martin](https://togithub.com/cameron-martin) in
[bazelbuild/rules_rust#2453
- Added Rust 1.77.1 by
[@&#8203;UebelAndre](https://togithub.com/UebelAndre) in
[bazelbuild/rules_rust#2591
- Fix (+) In vendored crates semver metadata by
[@&#8203;ericmcbride](https://togithub.com/ericmcbride) in
[bazelbuild/rules_rust#2579
- Keep default_features parity from bzlmod to workspace by
[@&#8203;Lev1ty](https://togithub.com/Lev1ty) in
[bazelbuild/rules_rust#2606
- clippy: use --cap-lints=warn; apply clippy_flags when
capture_output=True by [@&#8203;goffrie](https://togithub.com/goffrie)
in
[bazelbuild/rules_rust#2451
- Added Rust 1.77.2 by
[@&#8203;UebelAndre](https://togithub.com/UebelAndre) in
[bazelbuild/rules_rust#2608
- Re-vendor crate_universe outputs by
[@&#8203;UebelAndre](https://togithub.com/UebelAndre) in
[bazelbuild/rules_rust#2609
- Release 0.42.0 by
[@&#8203;UebelAndre](https://togithub.com/UebelAndre) in
[bazelbuild/rules_rust#2610

#### New Contributors

- [@&#8203;Lev1ty](https://togithub.com/Lev1ty) made their first
contribution in
[bazelbuild/rules_rust#2606

**Full Changelog**:
bazelbuild/rules_rust@0.41.1...0.42.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/bazel-contrib/toolchains_llvm).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4yNjkuMiIsInVwZGF0ZWRJblZlciI6IjM3LjI2OS4yIiwidGFyZ2V0QnJhbmNoIjoibWFzdGVyIn0=-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krasimirgg krasimirgg krasimirgg left review comments

@scentini scentini scentini approved these changes

Assignees

@krasimirgg krasimirgg

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@goffrie @buchgr @krasimirgg @scentini