ci, gha: Run “x86_64: macOS Ventura” job on GitHub Actions #1394
pull hebasto wants to merge 1 commits into bitcoin-core:master from hebasto:230805-gha-macos changing 1 files +85 −0-
hebasto commented at 3:35 pm on August 5, 2023: memberThis PR solves one item in #1392.
-
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 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 likeBUILD: ${{ 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: contributorThe 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 oneabort
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, 2023hebasto marked this as ready for review on Aug 9, 2023hebasto commented at 6:54 pm on August 9, 2023: memberRebased and dropped the second commit.hebasto commented at 7:17 pm on August 9, 2023: memberhebasto force-pushed on Aug 11, 2023hebasto commented at 7:24 pm on August 11, 2023: memberUpdated 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, 2023hebasto force-pushed on Aug 12, 2023real-or-random commented at 9:03 am on August 16, 2023: contributorThis 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, 2023hebasto cross-referenced this on Aug 16, 2023 from issue ci: Use Homebrew's gcc in native macOS task by hebastohebasto commented at 10:33 am on August 16, 2023: memberThis 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, 2023hebasto force-pushed on Aug 16, 2023real-or-random commented at 3:23 pm on August 16, 2023: contributorI’m currently looking into the failure, trying to figure out the right path. :)hebasto commented at 3:24 pm on August 16, 2023: memberI’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, 2023ci, gha: Run "x86_64: macOS Ventura" job on GitHub Actions 8e54a346d2in .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:By default, there are
gcc-13
,gcc-12
andgcc-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, yourln -s $(brew --prefix gcc)/bin/gcc-?? /usr/local/bin/gcc
is better. I was trying to rememberbrew --prefix
but I couldn’t find it in the docs anymore.hebasto force-pushed on Aug 16, 2023real-or-random approvedreal-or-random commented at 4:03 pm on August 16, 2023: contributorACK 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 hebastoMarcoFalke commented at 7:13 am on August 17, 2023: nonecttimetest 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: contributorcttimetest 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: contributorACK 8e54a346d2fa5aeedd6ba5201fcb084c281cf6a7jonasnick merged this on Aug 17, 2023jonasnick closed this on Aug 17, 2023
hebasto deleted the branch on Aug 17, 2023real-or-random commented at 1:43 pm on August 17, 2023: contributorI 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: noneSo 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-randomreal-or-random commented at 2:05 pm on August 17, 2023: contributorSo 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.
hebasto real-or-random MarcoFalke jonasnickLabels
ci
This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-30 01:15 UTC
More mirrored repositories can be found on mirror.b10c.me