Add clang and latest gcc to CI system #154

alexdewar · 2023-07-11T15:20:20Z

This PR adds clang and the latest version of gcc to the compilers tested on the CI (both running on Ubuntu), which should increase our coverage.

Closes #76. Closes #84.

Closes #76.

Closes #84.

jamesturner246

Approve in principle. Just a couple of questions.

jamesturner246 · 2023-07-13T16:01:26Z

src/HealthGPS/energy_balance_model.cpp

@@ -51,7 +51,7 @@ void EnergyBalanceModel::update_risk_factors(RuntimeContext &context) {
            current_risk_factors.at(age_key) = model_age;
        }

-        double energy_intake = 0.0;
+        [[maybe_unused]] double energy_intake = 0.0;


I assume the new linters pick this kind of stuff out automatically now, right?

I didn't add new linters -- just new compilers -- but, yes, clang noticed that this variable was unused even though gcc didn't.

jamesturner246 · 2023-07-13T16:05:58Z

.github/workflows/ci.yml

@@ -79,20 +108,20 @@ jobs:
        run: ctest --preset=core-test-${{ matrix.platform }}

      - name: Zip output folder
-        if: startsWith(github.ref, 'refs/tags/')
+        if: startsWith(github.ref, 'refs/tags/') && (matrix.platform != 'linux' || matrix.compiler == 'gcc-default')


Should it be matrix.platform == 'linux', or am I being dumb? I'm not sure I completely understand these lines.

It's not obvious... I should probably add a comment.

This step and the one after it are only run for new releases. They zip up the binaries then upload them to GitHub as release artifacts. We want to run this step for every platform, but we only want to upload one set of binaries for Linux, even though we're building it several times with different compilers.

I think that mixing platforms and compiles muddy the explanation a bit. Maybe use, so you choose what to upload based on the compiler only.

Suggested change

if: startsWith(github.ref, 'refs/tags/') && (matrix.platform != 'linux' || matrix.compiler == 'gcc-default')

if: startsWith(github.ref, 'refs/tags/') && (matrix.compiler == 'msvc' || matrix.compiler == 'gcc-default')

This would work now, but won't work when I add macOS to the runners. I agree this is a bit ugly though. How about I add another parameter which indicates whether the compiler is the "main" one or not (e.g. a matrix.is_main boolean value you could check instead)?

Sounds like a good option

dalonsoa

I've made a couple of comments, but looks good otherwsie.

dalonsoa · 2023-07-14T04:15:16Z

.github/workflows/ci.yml

+          - platform: linux
+            os: ubuntu-22.04
+            compiler: gcc-latest


C++ is supposed to be backwards compatible, so if things work with an older gcc version, they should work with the latest gcc version, right? In that sense, even though I understand this might come with some extra warnings and niceties, I'm not totally sure it is worth having another branch for it - at least not with every push.

I'm not sure how much you got in the standup yesterday about being environmental-aware when setting our CI systems (your connection was a bit unstable), so we don't waste resources and energy unnecessary.

I didn't hear much so I was left wondering what you'd said 😆.

Yes, C++ is in theory backwards compatible. In practice though, you do often get compile errors when you move to a new compiler, usually because one header in your compiler's implementation of the standard library used to #include another and now it doesn't (e.g. #73). Compilers do often vary in how much of the various standards they implement and how faithfully too, which can also cause breakage (I just learned that the latest version of Apple's clang doesn't implement STL parallel algorithms at all, even though that's a C++17 feature, which means you can't compile Health-GPS with it). That's why I think it makes sense to have some minimum version of gcc that you test -- we're using Ubuntu LTS for the runner, so the default version included with that seems a sensible candidate -- but also to test against the latest version. That way we can be fairly confident that anyone using any of the intervening versions will be alright too. The extra warnings are a nice bonus, but also the Ubuntu folks tend to ship fairly old versions of packages, including gcc, so I do think it makes sense to test against the latest version of gcc too for people who use other distros with a less ancient version. (@jamesturner246 uses Fedora and I use Arch, which both ship newer versions than you find in Ubuntu LTS.) It also means that we don't have to fix a bunch of warnings and errors whenever we update the version of the Ubuntu runner. Admittedly, that's not a big job, but it would be better to fix the problems in advance, both for people using a newer version of gcc and also because compiler warnings can indicate things like serious memory corruption bugs caused by undefined behaviour -- unfortunately there are lots of ways you can shoot yourself in the foot with C++.

We are currently only running the CI for pushes to main and for PRs, so hopefully the resource usage isn't too terrible. The other thing we could do is restrict ourselves to building a release version of the code, not the debug version (see #122).

What a nice explanation - worth a blogpost ;) . I hadn't realised that this runs only in pushes to main. Ok, no comments, then.

dalonsoa · 2023-07-14T04:20:13Z

.github/workflows/ci.yml

@@ -79,20 +108,20 @@ jobs:
        run: ctest --preset=core-test-${{ matrix.platform }}

      - name: Zip output folder
-        if: startsWith(github.ref, 'refs/tags/')
+        if: startsWith(github.ref, 'refs/tags/') && (matrix.platform != 'linux' || matrix.compiler == 'gcc-default')


I think that mixing platforms and compiles muddy the explanation a bit. Maybe use, so you choose what to upload based on the compiler only.

Suggested change

if: startsWith(github.ref, 'refs/tags/') && (matrix.platform != 'linux' || matrix.compiler == 'gcc-default')

if: startsWith(github.ref, 'refs/tags/') && (matrix.compiler == 'msvc' || matrix.compiler == 'gcc-default')

alexdewar marked this pull request as draft July 11, 2023 15:20

alexdewar force-pushed the ci_clang branch 4 times, most recently from 36596be to 4cf9f7b Compare July 12, 2023 07:24

alexdewar added 4 commits July 12, 2023 08:45

Add pre-commit hook for checking GitHub workflows config

d11f5f5

Use matrix.platform cf. os in a few places

28c4924

CI: Also compile with clang on Ubuntu

8d625cb

Closes #76.

Mark variable as unused to appease clang

245b5e9

alexdewar force-pushed the ci_clang branch 3 times, most recently from 4d770a6 to 245b5e9 Compare July 12, 2023 08:45

CI: Also try compiling with latest gcc

edfcc1e

Closes #84.

alexdewar force-pushed the ci_clang branch from 6d9477a to edfcc1e Compare July 13, 2023 14:48

alexdewar requested review from jamesturner246 and dalonsoa July 13, 2023 14:48

alexdewar marked this pull request as ready for review July 13, 2023 14:50

jamesturner246 approved these changes Jul 13, 2023

View reviewed changes

alexdewar changed the title ~~Add clang to CI system~~ Add clang and latest gcc to CI system Jul 13, 2023

dalonsoa approved these changes Jul 14, 2023

View reviewed changes

CI: Add is_main variable to matrix for readability

fc34609

alexdewar merged commit 7341988 into main Jul 14, 2023
5 checks passed

alexdewar deleted the ci_clang branch July 14, 2023 09:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add clang and latest gcc to CI system #154

Add clang and latest gcc to CI system #154

alexdewar commented Jul 11, 2023 •

edited

Loading

jamesturner246 left a comment

jamesturner246 Jul 13, 2023

alexdewar Jul 13, 2023

jamesturner246 Jul 13, 2023

alexdewar Jul 13, 2023

dalonsoa Jul 14, 2023

alexdewar Jul 14, 2023

dalonsoa Jul 14, 2023

dalonsoa left a comment

dalonsoa Jul 14, 2023

alexdewar Jul 14, 2023

dalonsoa Jul 14, 2023

dalonsoa Jul 14, 2023

	if: startsWith(github.ref, 'refs/tags/') && (matrix.platform != 'linux' \|\| matrix.compiler == 'gcc-default')
	if: startsWith(github.ref, 'refs/tags/') && (matrix.compiler == 'msvc' \|\| matrix.compiler == 'gcc-default')

Add clang and latest gcc to CI system #154

Add clang and latest gcc to CI system #154

Conversation

alexdewar commented Jul 11, 2023 • edited Loading

jamesturner246 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dalonsoa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexdewar commented Jul 11, 2023 •

edited

Loading