-
Notifications
You must be signed in to change notification settings - Fork 42
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
Refactor CI to return better results faster #406
Conversation
I prefer the 9 jobs personally. Definitely a bit unwieldy in the UI to deal with more tasks, but I think it's nice to isolate since it makes clear at the top level what is failing or what is yet to be run. There might also be some parallelization benefits to having them separate? Not familiar enough with actions to know how that works. |
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.
No show stoppers (so approved), but I have some comments.
This PR replaces #398, which started out small and turned into kind of a disaster. This PR restructures the MLSpp CI. Before, we did a single Linux build first and then ran clang-tidy build on all three platforms:
With this PR, we immediately launch build + unit test jobs on all three platforms, with all three choices of crypto library. Clang-tidy is done only on Ubuntu (the most available platform, and unlike macOS, doesn't hang), and interop testing is only done once (also using Ubuntu).
Along the way, I made some smaller changes that make the YAML easier to read:
The only change to code is a minor change to zeroization and its test in
lib/bytes/
. The Zeroization test in that file deliberately reads freed memory in an attempt to verify that memory is zeroized before being freed. This test was already disabled when ASAN was enabled (since its whole purpose is to detect UAF!), but we also disabled on Windows because it seemed to be causing problems. While I was researching Windows memory issues, I discovered theSecureZeroMemory()
function, which seemed safer than thestd::fill()
call that was there before, so I added code to use that.Nonetheless, Windows CI builds still seem to hang during the unit tests when they are built without
SANITIZERS=ON
, so that flag is always on in the CI builds, which slows them down a little.My only real remaining question is whether we should continue to run the different crypto libraries matrix-style, or whether we should just run the three of them manually in one job. I find the 9 jobs a little aesthetically unpleasing, and caching might work a little better.