briansmith / ring

Safe, fast, small crypto using Rust
Other
3.65k stars 684 forks source link

Changed toolchain to stable for coverage #1987

Open Raghav-Bell opened 4 months ago

Raghav-Bell commented 4 months ago

It fixes #1902

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 96.80%. Comparing base (ee1e217) to head (6d493b1). Report is 64 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1987 +/- ## ========================================== + Coverage 96.32% 96.80% +0.48% ========================================== Files 137 143 +6 Lines 20704 20434 -270 Branches 226 226 ========================================== - Hits 19943 19782 -161 + Misses 728 618 -110 - Partials 33 34 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

briansmith commented 4 months ago

From the CI run:

qemu: uncaught target signal 11 (Segmentation fault) - core dumped
/home/runner/work/ring/ring/mk/runner: line 21:  6499 Segmentation fault      (core dumped) $*
error: test failed, to rerun pass `-p ring --lib`

This kind of crash seems to happen when the clang version doesn't match the LLVM version that Rust uses. It seems like we upgraded clang in CI to what Nightly Rust uses. In order to move forward with this change, we may either need to downgrade to the earlier version of clang that matches stable Rust's LLVM version, or wait until the next stable Rust is released.

You might try using channel beta to see if the beta channel is using the new version of LLVM to get an idea of how long we'd need to wait.

Raghav-Bell commented 4 months ago

image

@briansmith You are right stable needs LLVM $\geq$ 16 beta & nightly are compatible with LLVM ==18. I will push it to beta for now, later we can move it to stable. Thanks

briansmith commented 2 months ago

@Raghav-Bell Do you want to try again now?

briansmith commented 2 months ago

OK, this seems to be working now on stable. Could you please do the following?:

  1. Rebase this on top of the latest main. Your PR branch is 64 commits behind the main branch, so it is hard to compare its effect on code coverage measurement as it is. If you rebase, then we should get a useful report from codecov.io.
  2. Squash the three commits into one.
  3. Improve the commit message. Something like this:

CI: Use stable Rust toolchain for code coverage.

Use the stable Rust toolchain for code coverage in CI. This should make coverage measurements more stable day-to-day and also make it easier for people to replicate coverage measurements on locally.

briansmith commented 2 months ago

Since PR #2056 was merged, we'll need to wait for Rust 1.80 (IIUC) to become stable, or else add a wokaround that forces the use of nightly for the armv7 target.