Skip to content

Use stable rust in clippy check and move to a shell script#1492

Merged
plebhash merged 3 commits into
stratum-mining:mainfrom
jbesraa:2025-02-18/ci-clippy
Mar 5, 2025
Merged

Use stable rust in clippy check and move to a shell script#1492
plebhash merged 3 commits into
stratum-mining:mainfrom
jbesraa:2025-02-18/ci-clippy

Conversation

@jbesraa

@jbesraa jbesraa commented Feb 18, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Part of #1272

The most notable fix/change in this pr is adding this https://github.com/stratum-mining/stratum/pull/1492/files#diff-40acd8e96b9e30ae1cfa538fb7ae0cdf80358930dd6f4167375f85980ee019e4 because tracing feature is used withing the code but did not exist in Cargo.toml

Probably eventually whats called now Clippy-lint.yml would become rust-checks and we will have in it clippy/fmt/test/.. all cargo/rust stuff that we want in the same place

@codecov

codecov Bot commented Feb 18, 2025 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 12.50000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 19.09%. Comparing base (a32c988) to head (9d5d9a3).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
protocols/v2/roles-logic-sv2/src/routing_logic.rs 0.00% 4 Missing ⚠️
protocols/v1/src/utils.rs 33.33% 2 Missing ⚠️
...d-client/src/lib/job_declarator/message_handler.rs 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1492      +/-   ##
==========================================
+ Coverage   17.87%   19.09%   +1.21%     
==========================================
  Files         132      131       -1     
  Lines       10069     9433     -636     
==========================================
+ Hits         1800     1801       +1     
+ Misses       8269     7632     -637
Flag Coverage Δ
binary_codec_sv2-coverage 0.00% <0.00%> (ø)
binary_sv2-coverage 8.59% <0.00%> (+1.63%) ⬆️
bip32_derivation-coverage 0.00% <ø> (ø)
buffer_sv2-coverage 37.68% <ø> (ø)
codec_sv2-coverage 0.03% <0.00%> (+<0.01%) ⬆️
common_messages_sv2-coverage 0.21% <0.00%> (+0.03%) ⬆️
const_sv2-coverage 0.00% <0.00%> (ø)
error_handling-coverage 0.00% <ø> (ø)
framing_sv2-coverage 0.45% <0.00%> (+0.08%) ⬆️
jd_client-coverage 0.44% <0.00%> (+0.02%) ⬆️
jd_server-coverage 14.56% <ø> (+1.49%) ⬆️
job_declaration_sv2-coverage 0.00% <0.00%> (ø)
key-utils-coverage 3.61% <ø> (ø)
mining-coverage 3.89% <0.00%> (+0.73%) ⬆️
mining_device-coverage 0.00% <ø> (ø)
mining_proxy_sv2-coverage 0.96% <ø> (+0.13%) ⬆️
noise_sv2-coverage 7.30% <0.00%> (+1.51%) ⬆️
pool_sv2-coverage 2.76% <ø> (+0.29%) ⬆️
protocols 27.15% <14.28%> (+3.26%) ⬆️
roles 7.01% <0.00%> (+0.04%) ⬆️
roles_logic_sv2-coverage 13.54% <0.00%> (+1.95%) ⬆️
sv2_ffi-coverage 0.00% <0.00%> (ø)
template_distribution_sv2-coverage 0.00% <0.00%> (ø)
translator_sv2-coverage 9.96% <ø> (+0.37%) ⬆️
utils 36.39% <ø> (ø)
v1-coverage 3.74% <14.28%> (+0.62%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@plebhash

plebhash commented Feb 19, 2025

Copy link
Copy Markdown
Member

@jbesraa #1272 (comment) here you said:

Consider running each job with twice once with rust 1.75(our MSRV) and once with rust stable

but here we're just abandoning MSRV for stable? what's the rationale?

@jbesraa

jbesraa commented Feb 20, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@jbesraa #1272 (comment) here you said:

Consider running each job with twice once with rust 1.75(our MSRV) and once with rust stable

but here we're just abandoning MSRV for stable? what's the rationale?

Added msrv back, so now it is stable + msrv. Got confused a bit.

I think we need to change the github action settings in the repo because the name of the check stopped being only "clippy-check" but "Clippy Lint / clippy-check ($version)". I think that's why clippy-checks are returning "Waiting for status to be reported"

@plebhash

plebhash commented Feb 27, 2025

Copy link
Copy Markdown
Member

can we fix the commit history here?

a few issues are making the review a bit confusing:

  • the top commit message is contradicting the latest changes in the PR strategy
  • the top commit is titled "Use stable and MSRV versions in clippy CI check", but the other commit is also making changes under the same scope

@jbesraa

jbesraa commented Feb 28, 2025

Copy link
Copy Markdown
Contributor Author

can we fix the commit history here?

a few issues are making the review a bit confusing:

* the top commit message is contradicting the latest changes in the PR strategy

* the top commit is titled "Use stable and MSRV versions in clippy CI check", but the other commit is also making changes under the same scope

My bad, updated commits.
Note that I left only stable version and the rationale is explained in 851f169. I also decided to leave both macos and ubuntu in and not remove macos, mainly due to https://github.com/stratum-mining/stratum/pull/1508/files#diff-adaf0b6cc7ee6f3aca68632c496a4bd7cead4d5ae214049da0881486a50050f5R50 fix that was discovered on macos initially.

Shourya742
Shourya742 reviewed Feb 28, 2025
View reviewed changes

@Shourya742 Shourya742 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Finally, the Clippy fixes are in! I've been encountering these for a while. Should we also consider upgrading the MSRV?

Comment thread scripts/rust/clippy.sh Outdated
@jbesraa

jbesraa commented Mar 1, 2025

Copy link
Copy Markdown
Contributor Author

Finally, the Clippy fixes are in! I've been encountering these for a while. Should we also consider upgrading the MSRV?

sorry, not sure what you mean by "upgrading msrv"?

@jbesraa

jbesraa commented Mar 3, 2025

Copy link
Copy Markdown
Contributor Author

This is ready for review

@plebhash

plebhash commented Mar 5, 2025

Copy link
Copy Markdown
Member

fmt CI is broken

@plebhash plebhash added the ready-to-be-merged triggers auto rebase bot label Mar 5, 2025
jbesraa added 3 commits March 5, 2025 14:00
@jbesraa
Move clippy check to a shell script
bf8798c 
.. This is part of unifying the CI checks and eventually combining the
rust checks into a single/few scripts.
@jbesraa
Use stable cargo clippy in CI
01f6b9e 
Previously msrv was used which prevented us from using newer features
from later clippy versions although it does not affect our actual msrv.
Here we change msrv to stable to benifit from recent clippy features.

This change should be safe unless there is a regression in cargo clippy
i.e., a feature exist in 1.75 but removed in stable but this is very
unlikely.
@jbesraa
Fix cargo clippy warnings
9d5d9a3
@plebhash plebhash mentioned this pull request Mar 5, 2025
Closed
@plebhash plebhash merged commit 4a353a5 into stratum-mining:main Mar 5, 2025
@jbesraa jbesraa mentioned this pull request Apr 17, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Shourya742 Shourya742 Shourya742 left review comments
@plebhash plebhash plebhash approved these changes

Assignees

No one assigned

Labels

ready-to-be-merged triggers auto rebase bot

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jbesraa @plebhash @Shourya742