-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add clang and latest gcc to CI system #154
Conversation
36596be
to
4cf9f7b
Compare
4d770a6
to
245b5e9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approve in principle. Just a couple of questions.
@@ -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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume the new linters pick this kind of stuff out automatically now, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't add new linters -- just new compilers -- but, yes, clang noticed that this variable was unused even though gcc didn't.
.github/workflows/ci.yml
Outdated
@@ -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') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be matrix.platform == 'linux'
, or am I being dumb? I'm not sure I completely understand these lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like a good option
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made a couple of comments, but looks good otherwsie.
- platform: linux | ||
os: ubuntu-22.04 | ||
compiler: gcc-latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What a nice explanation - worth a blogpost ;) . I hadn't realised that this runs only in pushes to main. Ok, no comments, then.
.github/workflows/ci.yml
Outdated
@@ -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') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 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.