ci: Switch macOS from Ventura to Monterey and add Valgrind

hebasto commented at 10:12 am on August 26, 2023: member

This PR switches the macOS native job from Ventura to Monterey, which allows to support Valgrind.

Both runners–macos-12 and macos-13–have the same clang compilers installed:

But Valgrind works fine on macOS Monterey, but not on Ventura.

The Homebrew’s Valgrind package is cached once it has been built (as it was before #1152). Therefore, the actions/cache@* action is needed to be added to the list of the allowed actions.

#1412 (comment):

By the way, this solves #1151.

hebasto cross-referenced this on Aug 26, 2023 from issue ci: Future of CI after Cirrus pricing change by real-or-random

real-or-random commented at 4:25 pm on August 27, 2023: contributor

nit: Perhaps rename the PR + commit to include Valgrind. We mostly add Valgrind here, which is useful in itself. Adding the ctime tests is just a side benefit of Valgrind, although an important one).

What I had in mind was initially to run CI for macOS only on Monterey, but yeah, maybe it’s not a bad idea to test on multiple Apple clangs…

real-or-random added the label ci on Aug 27, 2023

hebasto commented at 4:42 pm on August 27, 2023: member

What I had in mind was initially to run CI for macOS only on Monterey, but yeah, maybe it’s not a bad idea to test on multiple Apple clangs…

I guess, we are keeping the other macOS job for upcoming switching to arm64:https://github.com/bitcoin-core/secp256k1/blob/ea26b71c3a3e1a5c39a4fbdaf2f0b787c5139f56/.github/workflows/ci.yml#L580

Right?

hebasto force-pushed on Aug 27, 2023

hebasto renamed this:
~~ci: Add ctime tests for macOS Monterey~~
ci: Add macOS Monterey + Valgrind job
on Aug 27, 2023

hebasto commented at 4:44 pm on August 27, 2023: member

nit: Perhaps rename the PR + commit to include Valgrind. We mostly add Valgrind here, which is useful in itself. Adding the ctime tests is just a side benefit of Valgrind, although an important one).

Done.

real-or-random added the label assurance on Aug 28, 2023

in .github/workflows/ci.yml:596 in 97d0cd422c outdated

649+          - { WIDEMUL: 'int64',  RECOVERY: 'yes', ECDH: 'yes', SCHNORRSIG: 'yes', ELLSWIFT: 'yes' }
650+          - { WIDEMUL: 'int64',  RECOVERY: 'yes', ECDH: 'yes', SCHNORRSIG: 'yes', ELLSWIFT: 'yes', CC: 'gcc' }
651+          - { WIDEMUL: 'int128_struct', ECMULTGENPRECISION: 2, ECMULTWINDOW: 4 }
652+          - { WIDEMUL: 'int128',                  ECDH: 'yes', SCHNORRSIG: 'yes', ELLSWIFT: 'yes' }
653+          - { WIDEMUL: 'int128', RECOVERY: 'yes',              SCHNORRSIG: 'yes' }
654+          - { WIDEMUL: 'int128', RECOVERY: 'yes', ECDH: 'yes', SCHNORRSIG: 'yes', ELLSWIFT: 'yes', CC: 'gcc' }

real-or-random commented at 1:19 pm on August 28, 2023:

I suggest this merged matrix, which also includes real Valgrind jobs (see the main comment in my review for details):

 0        env_vars:
 1          - { WIDEMUL: 'int64',  RECOVERY: 'yes', ECDH: 'yes', SCHNORRSIG: 'yes', ELLSWIFT: 'yes' }
 2          - { WIDEMUL: 'int128_struct', ECMULTGENPRECISION: 2, ECMULTWINDOW: 4 }
 3          - { WIDEMUL: 'int128',                  ECDH: 'yes', SCHNORRSIG: 'yes', ELLSWIFT: 'yes' }
 4          - { WIDEMUL: 'int128', RECOVERY: 'yes' }
 5          - { WIDEMUL: 'int128', RECOVERY: 'yes', ECDH: 'yes', SCHNORRSIG: 'yes', ELLSWIFT: 'yes' }
 6          - { WIDEMUL: 'int128', RECOVERY: 'yes', ECDH: 'yes', SCHNORRSIG: 'yes', ELLSWIFT: 'yes', CC: 'gcc' }
 7          - { WIDEMUL: 'int128', RECOVERY: 'yes', ECDH: 'yes', SCHNORRSIG: 'yes', ELLSWIFT: 'yes',            WRAPPER_CMD: 'valgrind --exit-code=42' }
 8          - { WIDEMUL: 'int128', RECOVERY: 'yes', ECDH: 'yes', SCHNORRSIG: 'yes', ELLSWIFT: 'yes', CC: 'gcc', WRAPPER_CMD: 'valgrind --exit-code=42' }
 9          - { WIDEMUL: 'int128', RECOVERY: 'yes', ECDH: 'yes', SCHNORRSIG: 'yes', ELLSWIFT: 'yes', CPPFLAGS: '-DVERIFY' }
10          - BUILD: 'distcheck'

real-or-random commented at 2:18 pm on August 28, 2023:

By the way, I don’t like this style of having the same job with and without valgrind. Perhaps we should reintroduce a RUN_ON_VALGRIND variable (removed in fcfcb97e74b55a107290d44c81c049d6168e954f), but that should be a separate PR, and I don’t think it should hold up this PR here.

hebasto commented at 3:55 pm on August 28, 2023:

I suggest this merged matrix, which also includes real Valgrind jobs (see the main comment in my review for details):

Thanks! Implemented in #1412 (comment).

real-or-random commented at 4:35 pm on August 28, 2023:

Ah, I think we should set SECP256K1_TEST_ITERS=2 like for the other Valgrind jobs….

Perhaps it’s really cleaner to have a RUN_ON_VALGRIND, and the script then reduces the number of iterations automatically by a factor of 32 or 16…

but that should be a separate PR, and I don’t think it should hold up this PR here.

I stand by this, so I’d say do whatever you prefer. edit: Now that I think about, let’s really do this up in a separate PR. It will require further adjustments and more discussion, e.g., getting rid of separate “Sanitizers” jobs, and it’s not clear if we actually want this, etc.

hebasto commented at 4:51 pm on August 28, 2023:

Ah, I think we should set SECP256K1_TEST_ITERS=2 like for the other Valgrind jobs….

Sure. Done.

in .github/workflows/ci.yml:638 in 97d0cd422c outdated

631@@ -632,6 +632,61 @@ jobs:
632         run: env
633         if: ${{ always() }}
634 
635+  macos-monterey:
636+    name: "x86_64: macOS Monterey, ctime_tests"
637+    # See: https://github.com/actions/runner-images#available-images.
638+    runs-on: macos-12

real-or-random commented at 1:35 pm on August 28, 2023:

If we remove the Ventura jobs:

0  macos-native:
1    name: "x86_64: macOS Monterey"
2    # See: https://github.com/actions/runner-images#available-images.
3    runs-on: macos-12 # Use M1 once available https://github.com/github/roadmap/issues/528

hebasto commented at 3:56 pm on August 28, 2023:

Thanks! Done.

real-or-random commented at 1:36 pm on August 28, 2023: contributor

What I had in mind was initially to run CI for macOS only on Monterey, but yeah, maybe it’s not a bad idea to test on multiple Apple clangs…

Okay, I was wrong. The runners have the same Apple clang. And they also have the same gcc, so this is not a valid argument.

I guess, we are keeping the other macOS job for upcoming switching to arm64:

Hm, not sure then. I think we should switch to arm64 once it’s available. But when we do this, we’ll probably anyway want to rearrange the matrix again (e.g., this will depend on the other ARM jobs we have etc.)

So I suggest keeping only Monterey for now, and moving the “# Use M1 once available https://github.com/github/roadmap/issues/528" comment there.

nit: Perhaps rename the PR + commit to include Valgrind. We mostly add Valgrind here, which is useful in itself. Adding the ctime tests is just a side benefit of Valgrind, although an important one).

Okay, sorry, nevermind. I was wrong here, too… -.- When I wrote this, I assumed this PR adds also Valgrind jobs. But it’s a good idea to do this, and my suggested matrix has some Valgrind jobs…

real-or-random commented at 1:37 pm on August 28, 2023: contributor

By the way, this solves #1151.

hebasto force-pushed on Aug 28, 2023

hebasto renamed this:
~~ci: Add macOS Monterey + Valgrind job~~
ci: Switch macOS from Ventura to Monterey and add Valgrind
on Aug 28, 2023

hebasto force-pushed on Aug 28, 2023

hebasto commented at 3:53 pm on August 28, 2023: member

Updated 97d0cd422cf26d789b5652689585c6b06380fff6 -> 2ad93e81f845fa959514b8fb0cfd5df9f69d40da (pr1412.02 -> pr1412.04, diff):

addressed @real-or-random’s comments
the PR description and title have been updated

ci: Switch macOS from Ventura to Monterey and add Valgrind c223d7e33d

hebasto force-pushed on Aug 28, 2023

hebasto commented at 4:52 pm on August 28, 2023: member

Updated 2ad93e81f845fa959514b8fb0cfd5df9f69d40da -> c223d7e33d50b1da0b3ba617c83534185428b3d5 (pr1412.04 -> pr1412.05, diff):

addressed @real-or-random’s comment

real-or-random approved

real-or-random commented at 12:10 pm on August 29, 2023: contributor

ACK c223d7e33d50b1da0b3ba617c83534185428b3d5 I tested that a cttest failure makes CI fail: https://github.com/real-or-random/secp256k1/actions/runs/6010365844

real-or-random merged this on Aug 29, 2023

real-or-random closed this on Aug 29, 2023

hebasto deleted the branch on Aug 29, 2023

ci: Switch macOS from Ventura to Monterey and add Valgrind #1412