gh-143750: Compile OpenSSL with TSan for TSan CI #143752
Conversation
- Also fix "Install dependencies" step so that we use the installed Clang. We can use clang-20 on both ASan and TSan now.
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| os: [ubuntu-24.04] | ||
| openssl_ver: [3.5.4] | ||
| runs-on: ${{ matrix.os }} |
There was a problem hiding this comment.
Plz move these to inputs: and host the matrix on the calling side (in build.yml). This is the separation I was going for when I first introduced the reusable-*.yml module pattern — no matrices in modules.
There was a problem hiding this comment.
This is a convenient way to define a variable that's reused a few times in the workflow. It could be an env, but it's not actually needed in the environment.
There was a problem hiding this comment.
I know. I designed the modular approach with consistency in mind (which includes only allowing matrices on the top level). Having a matrix in reusable workflows sometimes has awkward side effects in a few places on GH, which I'd rather we avoid.
FWIW, you can use inputs with default values the same way — for var declarations — just a bit more verbosely. Although, I'd argue that being able to fill out descriptions for the inputs would be beneficial by making the purpose documented explicitly.
There was a problem hiding this comment.
It could be an
env, but it's not actually needed in the environment.
I noticed you're setting OPENSSL_VER already so it's already in there.
And I know that for os, people in other PRs were unhappy to see it in the matrix 🤷♂️
Actually, in #136729 (comment), Hugo suggested using env.IMAGE_OS_VERSION / ${IMAGE_OS_VERSION} in the steps, making the matrix entry redundant.
There was a problem hiding this comment.
Another reason for using env is that it'll trip Zizmor checks — they are probably suppressed right now but once re-enabled, we'll need to deal with the interpolation on GHA vs. Bash level.
I'll need to work on migrating everything to env vars at some point and making sure Zizmor enforces that.
| && ' (free-threading)' | ||
| || '' | ||
| }} |
There was a problem hiding this comment.
This will also need to interpolate new matrix factors since the job names will be confusing if there's ever more than one job.
There was a problem hiding this comment.
There's the same jobs as before. The TSan ones just now compile OpenSSL with TSan
There was a problem hiding this comment.
Right, so there shouldn't be a matrix, then: #143752 (comment)
| run: | | ||
| if [ "${SANITIZER}" = "TSan" ]; then |
There was a problem hiding this comment.
We can move this conditional to the GHA step so that skipping is more prominent.
| run: | | |
| if [ "${SANITIZER}" = "TSan" ]; then | |
| if: inputs.sanitizer == 'TSan' | |
| run: | |
Also drop fi and the second conditional block below too.
There was a problem hiding this comment.
I merged the if blocks, but you can't lift the conditional the GHA step because you still need the:
else
echo "UBSAN_OPTIONS=${SAN_LOG_OPTION}" >> "$GITHUB_ENV"
fiand the unconditional
echo "CC=clang" >> "$GITHUB_ENV"
echo "CXX=clang++" >> "$GITHUB_ENV"There was a problem hiding this comment.
@colesbury what I'd recommend is splitting these into two steps so that it'd be possible to move the conditional and it'd be prominent in the log. Alternatively, it might make sense to just move CC and CXX into the declarative per-step env: since they aren't computed dynamically anyway. Though, looking more into it, it sound like they probably belong in the Install dependencies which makes clang available and is already setting up the env to use it — just like these env vars.
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
Also fix "Install dependencies" step so that we use the installed Clang. We can use clang-20 on both TSan and UBSan now.