chore(ci): remove clippy.sh #1917

giladchase · 2024-11-10T13:57:51Z

Only one source of truth for lints, main Cargo.toml, making clippy.sh unnecessary.
Warnings no longer denied, only in the CI. To preserve existing behaviors users can add -Dwarnings to local RUSTFLAGS.

We still need to enforce in workspace tests that all crates inherit workspace lints.

reviewable-StarkWare · 2024-11-10T13:58:00Z

This change is

github-actions · 2024-11-10T13:59:24Z

Artifacts upload triggered. View details here

github-actions · 2024-11-10T14:04:53Z

Artifacts upload triggered. View details here

github-actions · 2024-11-10T14:12:02Z

Artifacts upload triggered. View details here

giladchase

See this example PR that rejects a commit that violated nonstandard-style, which is now just a warning locally, but generates an error in the CI.

Note: unused-imports, the main suspect, is already warning by default.

Reviewable status: 0 of 3 files reviewed, all discussions resolved

scripts/run_tests.py line 49 at r1 (raw file):

            # Don't remove, or warnings will merge.
            # TODO: add this to package args instead of here, it is a rustc flag, not a clippy flag.
            clippy_args += ["--", "-Dwarnings"]

This is suboptimal, would have preferred to add this to package_args but that one's already used for options, whereas -Dwarnings as shown below is an operand.

Technically this implies adding a package_operands array but I didn't want to overengineer this task unprompted.

Code quote:

            # TODO: add this to package args instead of here, it is a rustc flag, not a clippy flag.
            clippy_args += ["--", "-Dwarnings"]

github-actions · 2024-11-10T14:22:39Z

Benchmark movements:
full_committer_flow performance improved 😺
full_committer_flow time: [29.536 ms 29.632 ms 29.733 ms]
change: [-2.2675% -1.8785% -1.4904%] (p = 0.00 < 0.05)
Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
2 (2.00%) low mild
1 (1.00%) high severe

codecov · 2024-11-10T14:36:46Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.50%. Comparing base (e3165c4) to head (2e7756a).
Report is 344 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #1917       +/-   ##
===========================================
+ Coverage   40.10%   76.50%   +36.39%     
===========================================
  Files          26      373      +347     
  Lines        1895    39965    +38070     
  Branches     1895    39965    +38070     
===========================================
+ Hits          760    30575    +29815     
- Misses       1100     7114     +6014     
- Partials       35     2276     +2241

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

dorimedini-starkware

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @giladchase)

scripts/run_tests.py line 49 at r1 (raw file):

Previously, giladchase wrote…

This is suboptimal, would have preferred to add this to package_args but that one's already used for options, whereas -Dwarnings as shown below is an operand.

Technically this implies adding a package_operands array but I didn't want to overengineer this task unprompted.

can you explain? (maybe offline)

Cargo.toml line 259 at r1 (raw file):

# See [here](https://github.com/taiki-e/cargo-llvm-cov/issues/370) for a discussion on why this is
# needed (from rust 1.80).
unexpected_cfgs = { level = "warn", check-cfg = ['cfg(coverage_nightly)'] }

please add instruction (here is a good place?)

Suggestion:

# To deny warnings during local development: BLA BLA

[workspace.lints.rust]
future-incompatible = "warn"
nonstandard-style = "warn"
rust-2018-idioms = "warn"
# See [here](https://github.com/taiki-e/cargo-llvm-cov/issues/370) for a discussion on why this is
# needed (from rust 1.80).
unexpected_cfgs = { level = "warn", check-cfg = ['cfg(coverage_nightly)'] }

github-actions · 2024-11-12T11:20:46Z

Artifacts upload triggered. View details here

scripts/run_tests.py

github-actions · 2024-11-12T11:30:54Z

Benchmark movements:
tree_computation_flow performance regressed!
tree_computation_flow time: [35.393 ms 35.853 ms 36.394 ms]
change: [+1.0161% +2.3708% +3.7222%] (p = 0.00 < 0.05)
Performance has regressed.
Found 8 outliers among 100 measurements (8.00%)
1 (1.00%) high mild
7 (7.00%) high severe

github-actions · 2024-11-12T11:31:44Z

Artifacts upload triggered. View details here

giladchase

Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware)

Cargo.toml line 259 at r1 (raw file):

Previously, dorimedini-starkware wrote…

please add instruction (here is a good place?)

Done.

scripts/run_tests.py line 49 at r1 (raw file):

Previously, dorimedini-starkware wrote…

can you explain? (maybe offline)

Fixed, a bit repetitive, can it be better?

giladchase

Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware and @graphite-app[bot])

scripts/run_tests.py

giladchase

Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware and @graphite-app[bot])

scripts/run_tests.py

github-actions · 2024-11-12T11:36:15Z

Artifacts upload triggered. View details here

giladchase

Dismissed @graphite-app[bot] from a discussion.
Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware and @graphite-app[bot])

github-actions · 2024-11-12T11:48:25Z

Artifacts upload triggered. View details here

github-actions · 2024-11-12T11:55:54Z

Artifacts upload triggered. View details here

scripts/run_tests.py

dorimedini-starkware

Reviewed 1 of 1 files at r2, 1 of 1 files at r3, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @giladchase)

scripts/run_tests.py line 49 at r1 (raw file):

Previously, giladchase wrote…

Fixed, a bit repetitive, can it be better?

make the if-else set the base command, then add the operand args outside the if-else blocks?

scripts/run_tests.py

github-actions · 2024-11-12T12:33:01Z

Artifacts upload triggered. View details here

Cargo.toml

giladchase

Dismissed @graphite-app[bot] from a discussion.
Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware)

scripts/run_tests.py line 49 at r1 (raw file):

Previously, dorimedini-starkware wrote…

make the if-else set the base command, then add the operand args outside the if-else blocks?

Not sure that would work, inner branches might want to add args/operands, and you have to add them args first, operands later...
Maybe I misunderstand what you mean, can you give a rough psuedo?

scripts/run_tests.py

dorimedini-starkware

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @giladchase)

scripts/run_tests.py line 49 at r1 (raw file):

Previously, giladchase wrote…

Not sure that would work, inner branches might want to add args/operands, and you have to add them args first, operands later...
Maybe I misunderstand what you mean, can you give a rough psuedo?

ok now that review-bot made the RUSTFMT case a special case, my way no longer works anyway :P

giladchase · 2024-11-12T12:48:53Z

It's messier than i thought, even though it's a rustc flag, rustfmt and doc don't respect it.
Trying a different tactic.

giladchase · 2024-11-12T12:49:58Z

reopening

github-actions · 2024-11-12T12:50:15Z

Artifacts upload triggered. View details here

github-actions · 2024-11-12T12:50:27Z

Artifacts upload triggered. View details here

dorimedini-starkware

Reviewed 4 of 4 files at r9, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @giladchase)

a discussion (no related file):
Q: what about other CI workflows?
example: blockifier_ci builds the blockifier with the native feature; do we need to inject this -D everywhere?

.github/workflows/main.yml line 21 at r9 (raw file):

env:
  CI: 1
  RUSTFLAGS: "-D warnings"

oooh... much better

Code quote:

RUSTFLAGS: "-D warnings"

giladchase

Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware and @graphite-app[bot])

a discussion (no related file):

Previously, dorimedini-starkware wrote…

Q: what about other CI workflows?
example: blockifier_ci builds the blockifier with the native feature; do we need to inject this -D everywhere?

Don't need really, it's just easier and less error-prone to have this in place rather than in multiple places.
It is unlikely to cause anyone any issues, I saw other popular crates using this pattern in ci.yml.
Moved it closer to callsites.

scripts/run_tests.py line 49 at r1 (raw file):

Previously, dorimedini-starkware wrote…

ok now that review-bot made the RUSTFMT case a special case, my way no longer works anyway :P

Ya rustfmt/doc can't handle this operand, which is weird cause they can handle it through RUSTFLAGS alright 🤷

Cargo.toml

github-actions · 2024-11-12T13:37:06Z

Artifacts upload triggered. View details here

dorimedini-starkware

Reviewed 1 of 1 files at r10, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @graphite-app[bot])

dorimedini-starkware

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @graphite-app[bot])

- Only one source of truth for lints, main Cargo.toml, making clippy.sh unnecessary. - Warnings no longer denied, only in the CI. To preserve existing behaviors users can add `-Dwarnings` to local RUSTFLAGS. We still need to enforce in workspace tests that all crates inherit workspace lints.

github-actions · 2024-11-13T05:49:37Z

Artifacts upload triggered. View details here

giladchase

Dismissed @graphite-app[bot] from a discussion.
Reviewable status: 2 of 4 files reviewed, all discussions resolved (waiting on @dorimedini-starkware)

giladchase

Reviewed 1 of 4 files at r9, 1 of 1 files at r10, 2 of 2 files at r11, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @giladchase)

giladchase force-pushed the gilad/remove-clippy-sh branch from 337de5c to c9436fe Compare November 10, 2024 14:04

giladchase force-pushed the gilad/remove-clippy-sh branch from c9436fe to f3383ae Compare November 10, 2024 14:11

giladchase commented Nov 10, 2024

View reviewed changes

giladchase requested a review from dorimedini-starkware November 11, 2024 08:55

dorimedini-starkware requested changes Nov 12, 2024

View reviewed changes

giladchase force-pushed the gilad/remove-clippy-sh branch from f3383ae to 9144e78 Compare November 12, 2024 11:20

graphite-app bot reviewed Nov 12, 2024

View reviewed changes