Skip to content
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

Merged
merged 17 commits into from
Nov 3, 2023
Merged

Refactor CI to return better results faster #406

merged 17 commits into from
Nov 3, 2023

Conversation

bifurcation
Copy link
Contributor

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:

image

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).

image

Along the way, I made some smaller changes that make the YAML easier to read:

  • Make CMAKE_TOOLCHAIN_FILE an environment variable
  • Make OpenSSL 1.1 parallel to other crypto libraries
  • Simplify out the vcpkg-dir variables
  • Factor out build preparation steps into a composite action

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 the SecureZeroMemory() function, which seemed safer than the std::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.

@bifurcation bifurcation mentioned this pull request Nov 2, 2023
@birarda
Copy link
Contributor

birarda commented Nov 3, 2023

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.

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.

Copy link
Contributor

@glhewett glhewett left a 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.

.github/actions/build/action.yml Outdated Show resolved Hide resolved
.github/workflows/main_ci.yml Outdated Show resolved Hide resolved
.github/workflows/main_ci.yml Show resolved Hide resolved
.github/workflows/main_ci.yml Show resolved Hide resolved
.github/workflows/main_ci.yml Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
lib/bytes/test/bytes.cpp Outdated Show resolved Hide resolved
@bifurcation bifurcation merged commit 3a84133 into main Nov 3, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants