-
-
Notifications
You must be signed in to change notification settings - Fork 33.9k
gh-143750: Compile OpenSSL with TSan for TSan CI #143752
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
base: main
Are you sure you want to change the base?
Conversation
- Also fix "Install dependencies" step so that we use the installed Clang. We can use clang-20 on both ASan and TSan now.
webknjaz
left a comment
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.
(early GHA feedback)
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| && ' (free-threading)' | ||
| || '' | ||
| }} |
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.
This will also need to interpolate new matrix factors since the job names will be confusing if there's ever more than one job.
| run: | | ||
| if [ "${SANITIZER}" = "TSan" ]; then |
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.
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.
| || '--with-undefined-behavior-sanitizer' | ||
| }} | ||
| --with-pydebug | ||
| ${{ inputs.sanitizer == 'TSan' && ' --with-openssl="$OPENSSL_DIR" --with-openssl-rpath=auto' || '' }} |
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.
(nit) no need for the whitespace since >- in YAML already adds whitespaces when it joins the lines on parsing.
| ${{ inputs.sanitizer == 'TSan' && ' --with-openssl="$OPENSSL_DIR" --with-openssl-rpath=auto' || '' }} | |
| ${{ inputs.sanitizer == 'TSan' && '--with-openssl="$OPENSSL_DIR" --with-openssl-rpath=auto' || '' }} |
Also fix "Install dependencies" step so that we use the installed Clang. We can use clang-20 on both TSan and UBSan now.