botan
botan copied to clipboard
randombit
CI: Use GitHub Actions for Windows builds
Description
This did escalate quite a bit from "just adding windows" to "overhauling the CI". So please don't hesitate to call me out for going too far with it. I'm using the opportunity to do a deep-dive on GitHub Actions and splitting this up into several PRs or rolling back most of the changes shouldn't be too hard.
As a side-note: While experimenting, I found a few buggy build configurations and documented them below. I would suggest to keep them in mind and fix them after this is integrated.
The pipeline as it is now should provide at least as much coverage as before, plus some extra configurations. Some new bits and pieces bump the minimal requirements up, though. Below is an overview:
Requirements
-
Boost 1.69 (released on christmas 2018)
- no more linking of
libboost_system(header-only as of 1.69)
- no more linking of
-
Visual Studio 2019
- because of
/external:I
- because of
-
Not sure about clang's
--system-header-prefix=- though that is only used in the CI not in production
Build Configurations
Basic Matrices
- Windows 32-Bit/64-Bit, Linux, macOS
-
static,shared,amalgamation - including most external dependency integrations on all platforms (particularly boost)
- some configs are disabled (see "Build Failures" below)
Specials and Metrics
- coverage build (with basically everything included)
- MinGW smoke test
- sanitizer (windows)
- minimized
- bsi
- fuzzers
- valgrind
- emscripten
- baremetal
- lint
Cross-Compilation
- same configurations as before
Components
setup_gh_actions.ps1/sh
This didn't change much. The windows flavour of the script basically just installs jom and sccache. One thing was added: The scripts now find out where the compiler cache is located and communicates this upstream via an environment variable. Hence, no more hard-coded cache paths in actions/cache.
setup_gh_actions_after_vsvars.ps1
A new script installing 3rd-party dependencies via nuget (is there a better way?) after the Visual Studio environment was set up. That setup should also facilitate caching the windows dependencies, as nuget takes like 4 to 5 minutes to install boost. -.-
Multiple Matrices
Windows brought more complexity into the ci.yml than expected. Mainly due to platform-dependent setup steps, handling vcvarsall.bat, extra build configuration variables (make_tool, arch) and their inter-dependency. Also the list of build configurations became hard to grasp, so I grouped the configurations into several build matrices. As a side effect, we can now actually use the "matrix" feature effectively. E.g. to build static, shared and amalgamation on all major platforms.
Custom Actions
VS-Shell
This is a small 3rd-party action by @egor-tensin. I figured that it would be easier to copy the code and adapt it here, instead of relying on the 3rd-party repo. It basically locates vsvarsall.bat and slurps it into the build agent's environment. Currently, the newest Visual Studio version available is selected. Though, it should be extensible to configure older VS versions as well.
Setup Build Agent
This is a tiny custom action that I extracted to re-use the platform dependent agent setup steps. Mainly, it takes care of interacting with the setup_gh_actions script, set up compiler caching and calling VS-Shell (see above) on Windows.
ci_build.py
As the only new thing, this script can now auto-detect sccache on Windows machines.
Production Code Changes
To make this compile, a few changes to the production code were required. Most notably, the boost module does not link against libboost_system anymore. This boost library is effectively header-only since Boost 1.69 and only shipped with an empty compatibity library we were linking against. For reference: Boost 1.69 was released in the end of 2018 making this effectively the oldest supported version.
Build Infrastructure Changes
This introduces a notion of "system headers" (-isystem on gcc/clang and /external:I on msvc). That way, we can suppress warnings in external header files like boost.
TODO
- [x] Remove trailing dash from cache restore key
- [x] Make sure Boost works on windows
- [x] Add older versions of Visual Studio to the pipeline
- [x] Clean-up the commit history
- [x] Sunset AppVeyor
TODO -- Build Failures in Specific Configurations
- [ ] Argon2id seems to fail on Windows 32 bit builds
- [ ] 32 bit build on windows causes an unreachable code warning
- [ ] macOS fails to build with gcc (lacking
BOTAN_PARALLEL_SIMD_FOR) - [ ] Amalgamation build fails on ubuntu (as well as windows and macOS with clang) with clang (abstract class marked
final)
Nice to have
- [ ] Clang sanitizers
- [ ] Custom error/warning messages in the Checks summary
- [x] Maybe group individual build jobs
- e.g. "standard builds" (static, shared, amalg.), "analysis" (coverage, static analy. linting, ...), "cross compilation"
- [ ] Maybe have a "smoke test" stage run first, and deeper analysis later, to save on free-tier GitHub runners?
- [ ] Cache installed dependency (particularly
nuget install boosttakes long)
Giving up for today.
Current problem: The shell that executes ci_build.py does not have the Visual Studio vcvarsall.bat context. Hence, cl.exe is not found and the build fails.
There's a third-party action to pull the Visual Studio context into the GH Actions workflow (ilammy/msvc-dev-cmd), but this repo's security settingr prevent it from running. I tried it in the R&S fork and it worked well.
Nevertheless, I'm all in favor of pulling in as little 3rdparty code as possible. Will look into this next week. Frankly, I think its terrible that Microsoft's GitHub Actions make it so hard to compile stuff with MSVC. :(
Thanks for looking into this. AppVeyor's serial execution really hurts CI times and as a result over time the number of Windows configurations being tested has been pruned.
Codecov Report
Merging #3007 (34d26ab) into master (c6870fe) will decrease coverage by
0.01%. The diff coverage isn/a.
:exclamation: Current head 34d26ab differs from pull request most recent head ae97dff. Consider uploading reports for the commit ae97dff to get more accurate results
@@ Coverage Diff @@
## master #3007 +/- ##
==========================================
- Coverage 92.62% 92.60% -0.02%
==========================================
Files 596 596
Lines 69776 69776
Branches 6614 6614
==========================================
- Hits 64627 64616 -11
- Misses 5116 5127 +11
Partials 33 33
| Impacted Files | Coverage Δ | |
|---|---|---|
| src/lib/utils/cpuid/cpuid_x86.cpp | 76.62% <0.00%> (-11.69%) |
:arrow_down: |
| src/lib/pubkey/elgamal/elgamal.cpp | 95.94% <0.00%> (-1.36%) |
:arrow_down: |
| src/lib/pubkey/mce/mceliece_key.cpp | 83.24% <0.00%> (-1.05%) |
:arrow_down: |
| src/lib/tls/tls12/tls_channel_impl_12.cpp | 89.33% <0.00%> (+0.28%) |
:arrow_up: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing dataPowered by Codecov. Last update c6870fe...ae97dff. Read the comment docs.
This is getting to a state where some feedback would be appreciated. :) Cleanup still needs to be done and the last configurations still need some T.L.C, but I'm confident that this is now converging and provides at least as much coverage as before. As mentioned above: I think it isn't a big deal to pull those changes apart into multiple patches.
That's pretty much done from my side, now.
If you wish, @randombit, I could make that into three PRs, namely:
- Changes somewhat unrelated to the CI:
- CI Refactorings and Improvements
- Moving Windows build configurations to Github Actions
@randombit This is now based on #3010 and #3011. Note that I integrated the changes in my self-review into the respective commits.