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

CI: Add windows clippy job and fix clippy errors #330

Merged
merged 3 commits into from
Mar 20, 2024

Conversation

joncinque
Copy link

Problem

#234 broke the Windows build, but we weren't able to detect it with CI until the windows build was started aftewards.

Summary of Changes

Add a step to the clippy CI to run clippy on a Windows machine. Here's a successful CI run: https://github.com/joncinque/solana/actions/runs/8348778898/job/22853151448

While testing this, I tried downgrading the mac machine from macos-latest-large to macos-latest, and the job is still pretty fast, taking 16m30s compared to 13m30s. And I don't think we would use minutes if we downgrade that machine.

Anyway, let me know what you think!

@joncinque joncinque requested a review from yihau March 20, 2024 00:23
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 92.85714% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 81.9%. Comparing base (8df80d9) to head (3cc02ca).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master     #330     +/-   ##
=========================================
- Coverage    81.9%    81.9%   -0.1%     
=========================================
  Files         837      837             
  Lines      226896   226896             
=========================================
- Hits       185901   185883     -18     
- Misses      40995    41013     +18     

Copy link
Member

@yihau yihau 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 for creating this one! 🫶 iirc I got stuck by libloading things in the geyser plugin but can't remember the reason.

about the mac clippy, I guess you hit cache 🤔 there is a random build from solana repo: https://github.com/solana-labs/solana/actions/runs/7421447588. both of them can take ~2x mins. in the beginning, I only updated nightly one due to the flaky results, then I found we still had budget so I did stable one as well for the symmetry 🙈 tbh, I'm okay to switch them back if we have the consensus!

@yihau
Copy link
Member

yihau commented Mar 20, 2024

btw, I try to have this one for saving some budget: #244
a tl;dr is that we run all clippy when a PR is present. I try to ensure linux success first, then other os.

@joncinque joncinque merged commit 261b3e9 into anza-xyz:master Mar 20, 2024
38 checks passed
@joncinque joncinque deleted the winclippy branch March 20, 2024 12:21
@joncinque
Copy link
Author

Ah cool, thanks for providing the info! I'll take a look at the other PR

codebender828 pushed a commit to codebender828/agave that referenced this pull request Oct 3, 2024
* CI: Run clippy on windows

* Update cargo-clippy-before-script.sh for Windows

* Pacify clippy
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.

3 participants