Skip to content

refactor(ci): consolidate clippy checks into one job #2695

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
Merged
merged 14 commits into from
Jul 30, 2025
Merged

refactor(ci): consolidate clippy checks into one job #2695

merged 14 commits into from
Jul 30, 2025

Conversation

boquan-fang
Copy link
Contributor

@boquan-fang boquan-fang commented Jul 3, 2025 •
edited
Loading

Release Summary:

Resolved issues:

related to #2643.

Description of changes:

As a first step to introduce the clippy bot, we should consolidate all clippy checks into one job. Currently, we have one clippy job running on the main crate, while other clippy jobs are scattering in other parts of ci.yml. This PR consolidate all of them together.

Call-outs:

  • We decide to remove the beta version of rust for clippy test. That test was not previously required, so for this consolidated clippy test, we will remove that.
  • I also update some rust-toolchain and version to v1.82.0 which correctly reflects our current MSRV.

Testing:

We should use the CI to do the testing. We should check two things:

  1. If the CI passes with the new clippy settings.
  2. Do we have full coverage for clippy checks.

Run this in my personal forked repo: https://github.com/boquan-fang/s2n-quic/actions/runs/16380488388/job/46290742025. Manually check that all the jobs that were previously running are also running for this PR.

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

@boquan-fang boquan-fang marked this pull request as ready for review July 3, 2025 22:22
@boquan-fang boquan-fang requested a review from maddeleine July 3, 2025 22:22
maddeleine
maddeleine reviewed Jul 11, 2025
View reviewed changes
@boquan-fang
address PR comments:
2667bcd 
* only run clippy on stable version of rust
@boquan-fang
Copy link
Contributor Author

Another Call Out:

I have addressed #2695 (comment). The original required job clippy (stable, -D warnings) is no longer run. Instead, this new clippy job will be ran for each PR: ci/ clippy (pull_request). We need someone with Admin access to remove the original clippy job from the Required list and add this new clippy job as the required job.

@boquan-fang boquan-fang requested a review from maddeleine July 12, 2025 07:00
maddeleine
maddeleine reviewed Jul 14, 2025
View reviewed changes
@boquan-fang
Copy link
Contributor Author

I ran my updated command and test it on my forked repo first: https://github.com/boquan-fang/s2n-quic/actions/runs/16380488388/job/46290742025. I checked and confirmed that clippy is running for all example crates:
previous change:

  Checking s2n-codec v0.61.0 (/home/runner/work/s2n-quic/s2n-quic/common/s2n-codec)
  Checking concurrent-queue v2.5.0
 Compiling bach v0.1.0
 Compiling s2n-quic-platform v0.61.0 (/home/runner/work/s2n-quic/s2n-quic/quic/s2n-quic-platform)
  Checking event-listener v5.4.0
  Checking criterion v0.6.0
  Checking event-listener-strategy v0.5.4
  Checking s2n-quic-events v0.1.0 (/home/runner/work/s2n-quic/s2n-quic/quic/s2n-quic-events)
  Checking s2n-quic-core v0.61.0 (/home/runner/work/s2n-quic/s2n-quic/quic/s2n-quic-core)
  Checking s2n-quic-crypto v0.61.0 (/home/runner/work/s2n-quic/s2n-quic/quic/s2n-quic-crypto)
  Checking s2n-quic-xdp v0.61.0 (/home/runner/work/s2n-quic/s2n-quic/tools/xdp/s2n-quic-xdp)
  Checking s2n-quic-transport v0.61.0 (/home/runner/work/s2n-quic/s2n-quic/quic/s2n-quic-transport)
  Checking s2n-quic-tls v0.61.0 (/home/runner/work/s2n-quic/s2n-quic/quic/s2n-quic-tls)
  Checking s2n-quic-rustls v0.61.0 (/home/runner/work/s2n-quic/s2n-quic/quic/s2n-quic-rustls)
  Checking s2n-quic-tls-default v0.61.0 (/home/runner/work/s2n-quic/s2n-quic/quic/s2n-quic-tls-default)
  Checking s2n-quic-bench v0.1.0 (/home/runner/work/s2n-quic/s2n-quic/quic/s2n-quic-bench)
  Checking s2n-quic v1.61.0 (/home/runner/work/s2n-quic/s2n-quic/quic/s2n-quic)
  Checking s2n-quic-h3 v0.1.0 (/home/runner/work/s2n-quic/s2n-quic/quic/s2n-quic-h3)
  Checking s2n-quic-qns v0.1.0 (/home/runner/work/s2n-quic/s2n-quic/quic/s2n-quic-qns)
  Checking s2n-quic-sim v0.1.0 (/home/runner/work/s2n-quic/s2n-quic/quic/s2n-quic-sim)

It doesn't actually check any example crates, but after the latest commit, it does go in and check for all example crates:

  ...
  Checking rustls-mtls v0.1.0 (/home/runner/work/s2n-quic/s2n-quic/examples/rustls-mtls)
  Checking jumbo-frame v0.1.0 (/home/runner/work/s2n-quic/s2n-quic/examples/jumbo-frame)
  Checking custom-congestion-controller v0.1.0 (/home/runner/work/s2n-quic/s2n-quic/examples/custom-congestion-controller)
  Checking dos-mitigation v0.1.0 (/home/runner/work/s2n-quic/s2n-quic/examples/dos-mitigation)
  Checking event-framework v0.1.0 (/home/runner/work/s2n-quic/s2n-quic/examples/event-framework)
  Checking echo v0.1.0 (/home/runner/work/s2n-quic/s2n-quic/examples/echo)
  Checking async-client-hello-callback v0.1.0 (/home/runner/work/s2n-quic/s2n-quic/examples/async-client-hello-callback)
  Checking unreliable-datagram v0.1.0 (/home/runner/work/s2n-quic/s2n-quic/examples/unreliable-datagram)
  Checking s2n-mtls v0.1.0 (/home/runner/work/s2n-quic/s2n-quic/examples/s2n-mtls)
  Checking turmoil-provider v0.1.0 (/home/runner/work/s2n-quic/s2n-quic/examples/turmoil-provider)
  Checking resumption v0.1.0 (/home/runner/work/s2n-quic/s2n-quic/examples/resumption)
  Checking rustls-provider v0.1.0 (/home/runner/work/s2n-quic/s2n-quic/examples/rustls-provider)
  Checking post-quantum v0.1.0 (/home/runner/work/s2n-quic/s2n-quic/examples/post-quantum)

@boquan-fang boquan-fang requested a review from maddeleine July 18, 2025 21:28
@boquan-fang
address PR comments:
17eb9d3 
* Combine all example into one workspace
@boquan-fang boquan-fang force-pushed the consolidate-clippy-ci branch from b03d742 to 17eb9d3 Compare July 18, 2025 21:30
maddeleine
maddeleine previously approved these changes Jul 21, 2025
View reviewed changes
@WesleyRosenblum
Copy link
Contributor

We decide to remove the beta version of rust for clippy test. That test was not previously required, so for this consolidated clippy test, we will remove that.

The purpose of having a non-required beta clippy test was to give us a heads up that there were new Clippy lints coming down the road (assuming they were included in the beta). That gives us a chance to fix them before they start blocking the CI.

@maddeleine
Copy link
Contributor

We decide to remove the beta version of rust for clippy test. That test was not previously required, so for this consolidated clippy test, we will remove that.

The purpose of having a non-required beta clippy test was to give us a heads up that there were new Clippy lints coming down the road (assuming they were included in the beta). That gives us a chance to fix them before they start blocking the CI.

Was anyone looking at this output though? It's not being looked at if it doesn't block PRs from getting merged.

@boquan-fang
Copy link
Contributor Author

We decide to remove the beta version of rust for clippy test. That test was not previously required, so for this consolidated clippy test, we will remove that.

The purpose of having a non-required beta clippy test was to give us a heads up that there were new Clippy lints coming down the road (assuming they were included in the beta). That gives us a chance to fix them before they start blocking the CI.

Was anyone looking at this output though? It's not being looked at if it doesn't block PRs from getting merged.

I agree with @maddeleine's comments. When a s2n-quic developer turns on auto merge after their PR is approved, they might not notice the clippy failure in the non-required run.

This PR is one step towards a fully automated clippybot, which will fix clippy issue for us. If a new stable version is released and the new lints breaks our CI, the bot will let us know about the failure and fix it for us. Personally, I think that is a better mechanism than running a job on beta version.

Moreover, even with the beta run, we still see new clippy failures which blocks our CI from time to time. That makes me wondering if the beta version run is actually helping. Let me know what you think about my take.

WesleyRosenblum
WesleyRosenblum reviewed Jul 22, 2025
View reviewed changes
@boquan-fang boquan-fang mentioned this pull request Jul 23, 2025
Merged
@boquan-fang
Merge branch 'main' into consolidate-clippy-ci
24a7048
@boquan-fang
Revert "address PR comments:"
2db83e6 
This reverts commit 17eb9d3.
@boquan-fang
remove exception for the consolidated clippy job:
00a11ab 
* We need to create a loop to run clippy
@boquan-fang
address PR comments:
afb771b 
* parse all example crates and loop through them with clippy
* add -D warnings to a job
@boquan-fang
address PR comments:
4002f70 
* fix clippy failure in xdp crate
@boquan-fang
Copy link
Contributor Author

Call-outs: by serializing the clippy run in example crates, it will take more time for clippy to finish than before, since example clippy jobs were running in parallel previously. The consolidated clippy job will run for about 14 minutes. Such runtime seems reasonable, because there are other jobs that ran much slower than this.

@WesleyRosenblum WesleyRosenblum merged commit 422d2c3 into aws:main Jul 30, 2025
121 checks passed
@boquan-fang boquan-fang deleted the consolidate-clippy-ci branch July 30, 2025 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WesleyRosenblum WesleyRosenblum WesleyRosenblum approved these changes

@maddeleine maddeleine Awaiting requested review from maddeleine

@camshaft camshaft Awaiting requested review from camshaft

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@boquan-fang @WesleyRosenblum @maddeleine @camshaft