Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Draft
colesbury wants to merge4 commits intopython:main
base:main
Choose a base branch
Loading
fromcolesbury:gh-143750-openssl-tsan
Draft
Show file tree
Hide file tree
Changes fromall commits
Commits
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view

Some comments aren't visible on the classic Files Changed page.

43 changes: 30 additions & 13 deletions.github/workflows/reusable-san.yml
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -23,8 +23,17 @@ jobs:
&& ' (free-threading)'
|| ''
}}
Comment on lines 23 to 25
Copy link
Member

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.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

There's the same jobs as before. The TSan ones just now compile OpenSSL with TSan

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Right, so there shouldn't be a matrix, then:#143752 (comment)

runs-on: ubuntu-24.04
strategy:
fail-fast: false
matrix:
os: [ubuntu-24.04]
openssl_ver: [3.5.4]
runs-on: ${{ matrix.os }}
Comment on lines +26 to +31
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Plz move these toinputs: and host the matrix on the calling side (inbuild.yml). This is the separation I was going for when I first introduced thereusable-*.yml module pattern — no matrices in modules.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This is a convenient way to define a variable that's reused a few times in the workflow. It could be anenv, but it's not actually needed in the environment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

It could be anenv, but it's not actually needed in the environment.

I noticed you're settingOPENSSL_VER already so it's already in there.

And I know that foros, people in other PRs were unhappy to see it in the matrix 🤷‍♂️
Actually, in#136729 (comment), Hugo suggested usingenv.IMAGE_OS_VERSION /${IMAGE_OS_VERSION} in the steps, making the matrix entry redundant.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Another reason for usingenv 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.

timeout-minutes: 60
env:
OPENSSL_VER: ${{ matrix.openssl_ver }}
MULTISSL_DIR: ${{ github.workspace }}/multissl
OPENSSL_DIR: ${{ github.workspace }}/multissl/openssl/${{ matrix.openssl_ver }}
steps:
- uses: actions/checkout@v4
with:
Expand All@@ -37,22 +46,19 @@ jobs:
# Install clang
wget https://apt.llvm.org/llvm.sh
chmod +x llvm.sh

if [ "${SANITIZER}" = "TSan" ]; then
sudo ./llvm.sh 17 # gh-121946: llvm-18 package is temporarily broken
sudo update-alternatives --install /usr/bin/clang clang /usr/bin/clang-17 100
sudo update-alternatives --set clang /usr/bin/clang-17
sudo update-alternatives --install /usr/bin/clang++ clang++ /usr/bin/clang++-17 100
sudo update-alternatives --set clang++ /usr/bin/clang++-17
# Reduce ASLR to avoid TSan crashing
sudo sysctl -w vm.mmap_rnd_bits=28
else
sudo ./llvm.sh 20
fi
sudo ./llvm.sh 20
sudo update-alternatives --install /usr/bin/clang clang /usr/bin/clang-20 100
sudo update-alternatives --set clang /usr/bin/clang-20
sudo update-alternatives --install /usr/bin/clang++ clang++ /usr/bin/clang++-20 100
sudo update-alternatives --set clang++ /usr/bin/clang++-20
sudo update-alternatives --install /usr/bin/llvm-symbolizer llvm-symbolizer /usr/bin/llvm-symbolizer-20 100
sudo update-alternatives --set llvm-symbolizer /usr/bin/llvm-symbolizer-20

- name: Sanitizer option setup
run: |
if [ "${SANITIZER}" = "TSan" ]; then
# Reduce ASLR to avoid TSan crashing
sudo sysctl -w vm.mmap_rnd_bits=28
echo "TSAN_OPTIONS=${SAN_LOG_OPTION} suppressions=${GITHUB_WORKSPACE}/Tools/tsan/suppressions${{
fromJSON(inputs.free-threading)
&& '_free_threading'
Expand All@@ -69,6 +75,16 @@ jobs:
- name: Add ccache to PATH
run: |
echo "PATH=/usr/lib/ccache:$PATH" >> "$GITHUB_ENV"
- name: 'Restore OpenSSL build (TSan)'
id: cache-openssl
uses: actions/cache@v4
if: inputs.sanitizer == 'TSan'
with:
path: ./multissl/openssl/${{ env.OPENSSL_VER }}
key: ${{ matrix.os }}-multissl-openssl-tsan-${{ env.OPENSSL_VER }}
- name: Install OpenSSL (TSan)
if: steps.cache-openssl.outputs.cache-hit != 'true' && inputs.sanitizer == 'TSan'
run: python3 Tools/ssl/multissltests.py --steps=library --base-directory "$MULTISSL_DIR" --openssl "$OPENSSL_VER" --system Linux --tsan
- name: Configure CPython
run: >-
./configure
Expand All@@ -79,6 +95,7 @@ jobs:
|| '--with-undefined-behavior-sanitizer'
}}
--with-pydebug
${{ inputs.sanitizer == 'TSan' && '--with-openssl="$OPENSSL_DIR" --with-openssl-rpath=auto' || '' }}
${{ fromJSON(inputs.free-threading) && '--disable-gil' || '' }}
- name: Build CPython
run: make -j4
Expand Down
8 changes: 8 additions & 0 deletionsTools/ssl/multissltests.py
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -158,6 +158,12 @@
dest='keep_sources',
help="Keep original sources for debugging."
)
parser.add_argument(
'--tsan',
action='store_true',
dest='tsan',
help="Build with thread sanitizer. (Disables fips in OpenSSL 3.x)."
)


class AbstractBuilder(object):
Expand DownExpand Up@@ -312,6 +318,8 @@ def _build_src(self, config_args=()):
"""Now build openssl"""
log.info("Running build in {}".format(self.build_dir))
cwd = self.build_dir
if self.args.tsan:
config_args += ("-fsanitize=thread",)
cmd = [
"./config", *config_args,
"shared", "--debug",
Expand Down
Loading
[8]ページ先頭
©2009-2026 Movatter.jp