ci, gha: Run "x86_64: macOS Ventura" job on GitHub Actions

hebasto commented at 3:35 pm on August 5, 2023: member

This PR solves one item in #1392.

hebasto commented at 3:42 pm on August 5, 2023: member

For macOS, we need to take also #1153 into account.

#1153#issue-1450259053:

maybe it makes sense to reduce the number of MacOS jobs a little bit.

I’m opening for suggestion regarding that.

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

real-or-random commented at 5:23 pm on August 8, 2023: contributor

The problem with this is that it removes all our tests on native ARM.

We could instead run some Linux tasks on ARM, using Bitcoin Core’s ARM runner on Cirrus. But if we do this, then this makes it less much attractive to move Linux x86_64 tasks to GitHub Actions. I don’t know, it’s all related to each other. :/

hebasto commented at 5:26 pm on August 8, 2023: member

The problem with this is that it removes all our tests on native ARM.

Want me to drop the second commit?

hebasto commented at 10:27 am on August 9, 2023: member

Converting to draft for now.

Please review #1389 first.

hebasto marked this as a draft on Aug 9, 2023

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

in .github/workflows/ci.yml:55 in 0298f8fb20 outdated

50+
51+    strategy:
52+      fail-fast: false
53+      matrix:
54+        include:
55+          - { widemul:  'int64', recovery: 'yes', ecdh: 'yes', schnorrsig: 'yes', ellswift: 'yes' }

real-or-random commented at 4:26 pm on August 9, 2023:

According to https://stackoverflow.com/a/75447432 you can use

0      matrix:
1        include:
2           - env: ...

AFAIU this will avoid all the env settings like BUILD: ${{ matrix.build != '' && matrix.build || env.BUILD }} below.

hebasto commented at 6:53 pm on August 9, 2023:

Reworked.

real-or-random commented at 4:46 pm on August 9, 2023: contributor

The problem with this is that it removes all our tests on native ARM.

Want me to drop the second commit?

Hm, to be honest, I’m not sure what I want (or what others think). But yeah, for now, I lean towards keeping the Cirrus tasks.

There are basically three independent dimensions we want to test: CPU architectures, OSes, and compilers. If you ask me, testing on architectures like ARM is crucial, so it’s good to have a good number of (ideally native) ARM jobs. Testing on a particular OS like macOS can be restricted to a few configurations: It’s mostly to make sure that the build works etc., because the interaction of the actual library with the OS is very limited (basically just one printf and one abort in case of error). BUT: macOS is a bit of an exception because they have Apple clang, so macOS means not only a different OS but also a different compiler…

One way forward is to keep the Cirrus CI jobs for now, and then move those to the ARM Linux runner (with the same config) in a second PR before Sep 1st. None of this should be too controversial, since it’s just adding jobs, just possibly overshooting a bit. And then we could have a third PR that cleans up the overshooting, and here we can then see what exactly we want to remove.

hebasto force-pushed on Aug 9, 2023

hebasto marked this as ready for review on Aug 9, 2023

hebasto commented at 6:54 pm on August 9, 2023: member

Rebased and dropped the second commit.

hebasto commented at 7:17 pm on August 9, 2023: member

Related: https://github.com/github/roadmap/issues/528.

hebasto force-pushed on Aug 11, 2023

hebasto commented at 7:24 pm on August 11, 2023: member

Updated 10037078145b2b94b8fbd49c1a7c7c0aa5ac31b3 -> 59ae09ba4838634b486fdf630e0befa0544941cc (pr1394.02 -> pr1394.03, diff):

reworked this based on feedback from https://github.com/bitcoin-core/secp256k1/pull/1398

hebasto renamed this:
~~ci: Run "x86_64: macOS Ventura" job on GitHub Actions~~
ci, gha: Run "x86_64: macOS Ventura" job on GitHub Actions
on Aug 12, 2023

hebasto force-pushed on Aug 12, 2023

hebasto commented at 1:57 pm on August 12, 2023: member

Updated 59ae09ba4838634b486fdf630e0befa0544941cc -> 10180bf7cb50ddcdfe0f6c50a85f0b63f42f0708 (pr1394.03 -> pr1394.04, diff):

dropped the Set environment variables step in favor of direct env: ${{ matrix.env_vars }} assignment

real-or-random commented at 9:03 am on August 16, 2023: contributor

This uses the wrong GCC, namely Clang: https://github.com/bitcoin-core/secp256k1/actions/runs/5841706589/job/15842131976?pr=1394#step:4:43

But we have the same bug on Cirrus CI: :disappointed: https://cirrus-ci.com/task/5276055561306112?logs=test#L17

This is so easy to get wrong on macOS. I suggest we just replace the gcc symlink to point to the actual gcc…

hebasto force-pushed on Aug 16, 2023

hebasto cross-referenced this on Aug 16, 2023 from issue ci: Use Homebrew's gcc in native macOS task by hebasto

hebasto commented at 10:33 am on August 16, 2023: member

This uses the wrong GCC, namely Clang: bitcoin-core/secp256k1/actions/runs/5841706589/job/15842131976?pr=1394#step:4:43

But we have the same bug on Cirrus CI: 😞 cirrus-ci.com/task/5276055561306112?logs=test#L17

This is so easy to get wrong on macOS. I suggest we just replace the gcc symlink to point to the actual gcc…

Fixed here.

Also see #1402.

real-or-random referenced this in commit b327abfcea on Aug 16, 2023

hebasto force-pushed on Aug 16, 2023

real-or-random commented at 3:23 pm on August 16, 2023: contributor

I’m currently looking into the failure, trying to figure out the right path. :)

hebasto commented at 3:24 pm on August 16, 2023: member

I’m currently looking into the failure, trying to figure out the right path. :)

Doing the same on my macOS :)

hebasto force-pushed on Aug 16, 2023

ci, gha: Run "x86_64: macOS Ventura" job on GitHub Actions 8e54a346d2

in .github/workflows/ci.yml:75 in c43eef36d6 outdated

70+        env:
71+          HOMEBREW_NO_AUTO_UPDATE: 1
72+          HOMEBREW_NO_INSTALL_CLEANUP: 1
73+        run: |
74+          brew install automake libtool gcc
75+          ln -s /usr/local/bin/gcc-?? /usr/local/bin/gcc

hebasto commented at 3:41 pm on August 16, 2023:

@real-or-random

By default, there are gcc-13, gcc-12 and gcc-11 are pre-installed. That breaks ?? usage.

hebasto commented at 3:49 pm on August 16, 2023:

Fixed.

real-or-random commented at 3:58 pm on August 16, 2023:

Oh sure, they even say it in the docs…

We could use a (simplified) variant of this: https://github.com/bitcoin-core/secp256k1/blob/b327abfcea90394b5c63890406d6a5c54d02212e/ci/linux-debian.Dockerfile#L52-L57

That is, this should do it:

0ln -s $(ls /usr/local/bin/gcc-?? | sort | tail -1) /usr/ocal/bin/gcc)

real-or-random commented at 4:00 pm on August 16, 2023:

Ah nice, your ln -s $(brew --prefix gcc)/bin/gcc-?? /usr/local/bin/gcc is better. I was trying to remember brew --prefix but I couldn’t find it in the docs anymore.

hebasto force-pushed on Aug 16, 2023

real-or-random approved

real-or-random commented at 4:03 pm on August 16, 2023: contributor

ACK 8e54a346d2fa5aeedd6ba5201fcb084c281cf6a7

That’s really clean now, I think.

real-or-random commented at 4:17 pm on August 16, 2023: contributor

@ Reviewers:

I think what still needs conceptual discussion when it comes to macOS is whether

we follow the approach I suggested above, namely “moving” some of the existing macOS jobs to self-hosted ARM Linux Cirrus, in order to keep some ARM testing,
or whether we do something else, e.g., add just ARM QEMU jobs. It’s possible to run both the ctimetests (using MSan) and the normal tests in QEMU. The only caveat is that we can run the ctimetests only for gcc, because gcc has no MSan.

Not entirely sure. Having everything on GHA., and not depending on the self-hosted runners, makes our infrastructure simpler. But on the other hand, having the cttimetest on GCC/ARM is probably a good idea. We could at least move as much as possible to GHA, and just have a single Cirrus job that covers that particular cttimetest.

Anyway, all of this should probably happen after this PR here.

real-or-random cross-referenced this on Aug 16, 2023 from issue test: Silent noisy clang warnings about Valgrind code on macOS x86_64 by hebasto

MarcoFalke commented at 7:13 am on August 17, 2023: none

cttimetest on GCC/ARM

If this is just a single task running on a single CPU for 5 minutes, it may fit in the “free” Cirrus budget, or someone may be willing to sponsor the Cirrus Credits needed for the task, until it can be moved elsewhere.

real-or-random commented at 7:52 am on August 17, 2023: contributor

cttimetest on GCC/ARM

If this is just a single task running on a single CPU for 5 minutes, it may fit in the “free” Cirrus budget, […] until it can be moved elsewhere.

Indeed, it should be. It’s basically just the compilation. The actual test runs in one second (!) on my machine…

I think my comment was primarily about simplicity and not about the money (one CI provider is simpler than two), but I agree, it’s a good observation. So yep, I vote for keeping this single job on Cirrus for now, and we can re-evaluate when/if GitHub will have macOS M1 (and perhaps at that time valgrind-macos will support it.

edit: I still think all of this belongs to a separate PR, we should first get the additional GHA jobs merged here.

jonasnick commented at 11:05 am on August 17, 2023: contributor

ACK 8e54a346d2fa5aeedd6ba5201fcb084c281cf6a7

jonasnick merged this on Aug 17, 2023

jonasnick closed this on Aug 17, 2023

hebasto deleted the branch on Aug 17, 2023

real-or-random commented at 1:43 pm on August 17, 2023: contributor

I vote for keeping this single job on Cirrus for now @jonasnick pointed out to me that we don’t run the cttimetests on ARM currently (not before and not after this PR). I lost track of that apparently when I wrote that above… (Sorry, too many platforms and special cases…) To clarify, this means that removing the existing macOS tasks on Cirrus would be a loss only in the sense that run fewer normal tests on ARM. What we currently do for ARM is to run some tests in QEMU. My suggestion is to run more tasks in QEMU to mitigate this loss. We currently run them on Cirrus, but GHA should work equally.

Independently of this, it still makes sense to add a job that runs the cttimetests on ARM, and the only feasible way to do this right now is using Cirrus CI. I’ll pick up the work #970 to implement this.

MarcoFalke commented at 1:48 pm on August 17, 2023: none

So I guess the Cirrus macos task can be removed, and a comment be added to switch to M1, once it is there? Similar to how it was done in Bitcoin Core:

0runs-on: macos-13  # Use M1 once available https://github.com/github/roadmap/issues/528

real-or-random cross-referenced this on Aug 17, 2023 from issue ci: Remove "arm64: macOS Ventura" task from Cirrus CI by real-or-random

real-or-random commented at 2:05 pm on August 17, 2023: contributor

So I guess the Cirrus macos task can be removed, and a comment be added to switch to M1, once it is there?

Yes, indeed. See #1404.

ci, gha: Run “x86_64: macOS Ventura” job on GitHub Actions #1394