Add Github Actions CI #285
Conversation
|
Current status can be followed here: https://github.com/DaanDeMeyer/doctest/actions |
|
Do you want each platform to use its native build system? I'm using Ninja everywhere at the moment which is extremely convenient but it does reduce the testing surface as we're only building with a single CMake generator. On the other hand, doctest is a header-only library so the build system used to compile tests and examples shouldn't matter to the end user. (I'm strongly in favor of just using Ninja everywhere as it makes my life a whole lot easier) |
|
Are clang 3.5, 3.6 and 3.7 required for CI? LLVM does not provide Xenial apt repositories for those versions. If we want those I'll have to start juggling with ubuntu versions in the LLVM apt repository url's which is not impossible but makes stuff a little bit more complex. EDIT: nvm, Canonical bundles those in its own package repositories so I can just get them from there. |
|
Do I have to keep all the sanitizer builds separate? There's a comment in the Travis config on knowing exactly what caused the test run to fail but can't you see that from the sanitizer output? |
|
Hmmm I missed your comments here.
|
|
Yes, but currently you seem to do a separate build for each specific sanitizer in travis.yml (https://github.com/onqtam/doctest/blob/f13a00cc27ed3c1ec4f755572ab7556c4cb01716/.travis.yml#L372). I was wondering if I couldn't just do a single build with all sanitizers (asan, tsan and ubsan) combined (per compiler of course). |
|
I think ASAN and UBSAN can be combined, but am really not sure about TSAN... But I'm also not sure we need debug vs release builds with the sanitizers... perhaps debug is enough? |
|
I'm wondering if we can't just go one step further and do a single build with everything enabled (including static analysis later). For reproc I just do a debug build in CI instead of both debug and release. Is it often that there are differences in debug/release builds in terms of test or build results? |
|
the most common differences between debug/release have been warnings, so both build types are important. |
|
I'm kind of in an annoying situation where I don't want to make separate jobs for debug/release builds as it causes the job count to balloon but doing both in the same job requires an annoying amount of duplication because I can't use the same steps for Windows and POSIX due to Github Actions limitations and as a result have to add 3 steps each time I want to do something extra in a single job instead of a single one. I've reported the issue (https://github.community/t5/GitHub-Actions/Support-global-environment-variables/m-p/30894#M601) but I'm not sure if or when it will be fixed. |
|
Well for the duplication perhaps we could use a template engine to generate the actual github actions config file... Perhaps I should familiarize myself with the config format... I really don't have useful input at this point. |
|
I've made another post (https://github.community/t5/GitHub-Actions/Support-saving-environment-variables-between-steps/td-p/31373) detailing what would be needed to remove all duplication. For now, I'm going to wait and see if I get a positive reply there. While we can always work around the issue if needed, if the Github Actions team is open to fixing this it would results in a very simple CI script compared to the alternatives so I want to wait a while to see what happens. |
|
Let me know whenever this is ready for a review - seems to pass now :) |
|
Still need to add a lot of stuff. I've been on vacation this week so I didn't really have time to finish it yet. There's still a bit of duplication involved since we have to configure, build and test quite a few configurations so I might look into moving stuff into an actual Action so we can build a cross product of combinations with Javascript and reuse code for all those combinations. |
|
Also, while we do have 20 concurrent jobs, it does take quite a while for the second batch of remaining jobs to start after the first 20 complete so if we can remove some combinations in order to get the total amount of jobs down to 20 that would reduce waiting times for CI to complete somewhat. Of course, it depends on whether there are configurations that can be dropped. If they are all important, there's always the chance that the speed of Github Actions improves later on as its still in beta. |
|
I just checked and there are 31 different compiler jobs, so if all other actions/steps/builds are within a single per-compiler job then we would be left with 31 jobs in total, so 2 batches of GitHub Actions. I'm not sure it's worth optimizing around the '20' parallel jobs of Actions since that number could change in any direction in the future, and dropping compilers isn't ideal since we want as much testing as possible to minimize client issues/reports. |
|
I just force-pushed into dev (after rebasing it on top of master and rebranching dev from master) and it seems that didn't play well with this PR and there are some conflicts, but I don't think there was any overlap besides with the filter_2 test, so hopefully it's easy to fix. Let me know if I should do something about it. Perhaps my git practices are questionable... |
LLVM's does not provide Bionic apt repositories for older Clang versions that we want to test against.
valgrind does not support the version of macOS used by Github Actions yet.
|
The most important thing is to compile and run the tests on as much compilers/platforms as possible. Sanitizers don't need to be enabled everywhere - there are plenty of toolchain issues which are just a waste of time and effort... Most projects settle with 1-2 configurations/compilers being tested with sanitizers and even if we disable some sanitizers for 1/2 of the compilers it would still be more than enough - so feel free to cut out the configs which are problematic, and for those which still require the gold linker - use that if it's easy. |
|
I'm going to try a little bit more with this one as tests failing with sanitizers enabled on LTS Ubuntu with all major versions of GCC does not seem exactly confidence inspiring even if its an edge case feature. I setup a Ubuntu bionic container using systemd-nspawn and managed to reproduce the issue in there. It turns out the tests aren't being registered simply because the call to dlopen is failing in main.cpp in executable_plugin_and_dll. I still need to find out exactly why this is the case when sanitizers are enabled in GCC (and why it only happens on Ubuntu). |
|
FYI someone has already had a problem with GCC/sanitizers/plugins: #201 |
|
Adding Anyway, I'll just add |
|
address/undefined behaviour sanitizers are now enabled where possible. Should I add TSAN as well? I've always wondered why that one was needed for doctest. |
|
TSAN is necessary for the |
|
I think everything except coverage and static analysis is now running. Sanitizers are disabled on quite a few older compiler versions because those gave issues (See the Configure steps for the sanitizers). I also threw out the GCC 6 build on macOS as the concurrency test was flaky on that configuration. I also noticed we're now testing with a lot less xcode versions as the older ones Travis provides are not available on the Github Actions vm's. I recommend that we add static analysis later via CMake's integrated support for clang-tidy and cppcheck. I do the same in reproc (https://github.com/DaanDeMeyer/reproc/blob/4818cd138fceb6d3cb5ffd97634f7f9cad6e805f/cmake/reproc.cmake#L108) for clang-tidy and it's pretty trivial to set up. The advantage is that clang-tidy can seamlessly run on every build configuration during the build. This does increase build times but I don't think that's a big problem in this case as doctest builds pretty fast. This means that this is now ready for a review. I think the best way to go about this is you leave a comment on anything you don't immediately understand so I can add extra comments where needed. I already added quite a few comments but there might be stuff that I take for granted because I've been busy with this for quite a while. |
|
I'll take a look after at least 1 week since I have a really tight deadline for a presentation. I'll let you know as soon as possible. On the point about static analysis - I completely agree. Thanks for the effort - great work! |
|
it would be good to have |
nevermind - it's all in the detailed logs of the build because of the cmake config step :)
Thanks for the great work! Squashing and merging! |
|
I did notice that out of habit I did everything with 2 spaces indentation while the rest of doctest uses 4 space indentation. Do you want me to switch everything to 4 spaces? I'd also recommend adopting a .editorconfig file in the repository which allows most editors to pick up this stuff automatically. |
|
4 spaces sounds good (the 2 spaces thing I tried to stick to in travis.yml annoyed me quite a bit and I never even tried with 4 spaces...) an editorconfig file would be fine as well :) |
Opening the pull request so I can start testing the configuration. I've started by taking reproc's Github Actions configuration and removing everything reproc related. Once that works I'll start adding all doctest related configuration.