[CI] Add macos testing

[samcunliffe]

Got it working on 🍎 !

Thanks for @giordano 's top-tip to use this tmate action to help debugging.

Unfortunately it's yet another separate build with a YML file that is 90% the same as the Linux and Windows ones. But on the plus: we get another badge.

Good news: it found a genuine (minor) bug. 🐛

[t-young31]

🍎 🚀

My only comment is: perhaps ~3h of CI time is a lot per run for this. I wonder if, like the Windows CI, we should just check a successful compile on pull_request (and run everything on push: main)

[samcunliffe]

🍎 🚀

My only comment is: perhaps ~3h of CI time is a lot per run for this. I wonder if, like the Windows CI, we should just check a successful compile on pull_request (and run everything on push: main)

Oh agreed. I didn't notice that the system tests were at ~35mins each. (Why so slow?)

[giordano]

It looks overall ok to me (apart from the comments below), but I must say I'm thrilled about the idea of adding yet another build script which is for 75% identical to the linux one. I hadn't had the opportunity to comment about this when the Windows job was split out of the Linux one. My main problem is that changing the scripts in the future to, say, add one more step or use a different configuration option for CMake will require changing three different files. Yes, conditionals aren't great, but I think their ugliness is outweighed by the cost of keeping in sync three different files.

[giordano]

Instead of setting OMP_NUM_THREADS manually in each step (and I'd use

        env:
          OMP_NUM_THREADS: 1

instead), I think you can also set the variable globally for the entire job. This has also the advantage of being shell-independent (not that on macOS specifically we'd use much different shells, but I'm thinking in the optic of unifying the build scripts).

[t-young31]

tangential to this point: it may be nice to have a test e.g. just test_arc_01.py that runs with 2 threads, to test the parallelisation hasn't been broken?

[giordano]

Sounds good. It could be done with an additional matrix entry, and then you can do

env:
  OMP_NUM_THREADS: ${{ matrix.num_threads }}

We do it for example at https://github.com/Team-RADDISH/ParticleDA.jl/blob/410605a29b17d2ec2f3e86848e62a4ab6ff73102/.github/workflows/ci.yml#L12-L13

[samcunliffe]

Instead of setting OMP_NUM_THREADS manually in each step (and I'd use

        env:
          OMP_NUM_THREADS: 1

instead), I think you can also set the variable globally for the entire job. This has also the advantage of being shell-independent (not that on macOS specifically we'd use much different shells, but I'm thinking in the optic of unifying the build scripts).

Ah. This was just brainlessly copied over from the Windows one which was the starting point.

[samcunliffe]

Sounds good. It could be done with an additional matrix entry, and then you can do

env:
  OMP_NUM_THREADS: ${{ matrix.num_threads }}

We do it for example at https://github.com/Team-RADDISH/ParticleDA.jl/blob/410605a29b17d2ec2f3e86848e62a4ab6ff73102/.github/workflows/ci.yml#L12-L13

I would do this in a separate issue/PR (to avoid snowballing). And let's do it to the linux tests (only)?