-
-
Notifications
You must be signed in to change notification settings - Fork 33.1k
gh-136728: Refactor build.yml CI config and multissltests.py #136729
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?
Changes from all commits
a104f8a
3fcbe0d
5d8ec9a
991c6b2
7b51499
6638127
4e0a8ca
1a90e0c
1fcb49f
dd969fb
1edce1c
3343120
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -260,84 +260,28 @@ jobs: | |
free-threading: ${{ matrix.free-threading }} | ||
os: ${{ matrix.os }} | ||
|
||
build-ubuntu-ssltests-openssl: | ||
name: 'Ubuntu SSL tests with OpenSSL' | ||
build-ubuntu-ssltests: | ||
name: 'Ubuntu SSL tests' | ||
runs-on: ${{ matrix.os }} | ||
timeout-minutes: 60 | ||
needs: build-context | ||
if: needs.build-context.outputs.run-tests == 'true' | ||
strategy: | ||
fail-fast: false | ||
matrix: | ||
os: [ubuntu-24.04] | ||
openssl_ver: [3.0.16, 3.1.8, 3.2.4, 3.3.3, 3.4.1] | ||
include: | ||
- { os: ubuntu-24.04, ssl: openssl, ssl_ver: 3.0.16 } | ||
- { os: ubuntu-24.04, ssl: openssl, ssl_ver: 3.1.8 } | ||
- { os: ubuntu-24.04, ssl: openssl, ssl_ver: 3.2.4 } | ||
- { os: ubuntu-24.04, ssl: openssl, ssl_ver: 3.3.3 } | ||
- { os: ubuntu-24.04, ssl: openssl, ssl_ver: 3.4.1 } | ||
- { os: ubuntu-24.04, ssl: awslc, ssl_ver: 1.55.0 } | ||
Comment on lines
+273
to
+278
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can remove the matrix:
os: [ubuntu-24.04]
include:
- { ssl: openssl, ssl_ver: 3.0.16 }
- ... It would be possible to merge two matrices but we'll need a pre-step and I don't think it's more maintainable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm sorry but what would There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @picnixz I'm not sure I follow, the suggestion is just to rename There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh! I thought you suggested something like this: matrix:
os: [ubuntu-24.04]
include:
- { ssl_lib: openssl | awslc, ssl_ver: 3.0.16 } and I didn't know what it would means. Yes, we can have |
||
# See Tools/ssl/make_ssl_data.py for notes on adding a new version | ||
env: | ||
OPENSSL_VER: ${{ matrix.openssl_ver }} | ||
MULTISSL_DIR: ${{ github.workspace }}/multissl | ||
OPENSSL_DIR: ${{ github.workspace }}/multissl/openssl/${{ matrix.openssl_ver }} | ||
LD_LIBRARY_PATH: ${{ github.workspace }}/multissl/openssl/${{ matrix.openssl_ver }}/lib | ||
steps: | ||
- uses: actions/checkout@v4 | ||
with: | ||
persist-credentials: false | ||
- name: Runner image version | ||
run: echo "IMAGE_OS_VERSION=${ImageOS}-${ImageVersion}" >> "$GITHUB_ENV" | ||
- name: Restore config.cache | ||
uses: actions/cache@v4 | ||
with: | ||
path: config.cache | ||
key: ${{ github.job }}-${{ env.IMAGE_OS_VERSION }}-${{ needs.build-context.outputs.config-hash }} | ||
- name: Register gcc problem matcher | ||
run: echo "::add-matcher::.github/problem-matchers/gcc.json" | ||
- name: Install dependencies | ||
run: sudo ./.github/workflows/posix-deps-apt.sh | ||
- name: Configure OpenSSL env vars | ||
run: | | ||
echo "MULTISSL_DIR=${GITHUB_WORKSPACE}/multissl" >> "$GITHUB_ENV" | ||
echo "OPENSSL_DIR=${GITHUB_WORKSPACE}/multissl/openssl/${OPENSSL_VER}" >> "$GITHUB_ENV" | ||
echo "LD_LIBRARY_PATH=${GITHUB_WORKSPACE}/multissl/openssl/${OPENSSL_VER}/lib" >> "$GITHUB_ENV" | ||
- name: 'Restore OpenSSL build' | ||
id: cache-openssl | ||
uses: actions/cache@v4 | ||
with: | ||
path: ./multissl/openssl/${{ env.OPENSSL_VER }} | ||
key: ${{ matrix.os }}-multissl-openssl-${{ env.OPENSSL_VER }} | ||
- name: Install OpenSSL | ||
if: steps.cache-openssl.outputs.cache-hit != 'true' | ||
run: python3 Tools/ssl/multissltests.py --steps=library --base-directory "$MULTISSL_DIR" --openssl "$OPENSSL_VER" --system Linux | ||
- name: Add ccache to PATH | ||
run: | | ||
echo "PATH=/usr/lib/ccache:$PATH" >> "$GITHUB_ENV" | ||
- name: Configure ccache action | ||
uses: hendrikmuhs/[email protected] | ||
with: | ||
save: false | ||
- name: Configure CPython | ||
run: ./configure CFLAGS="-fdiagnostics-format=json" --config-cache --enable-slower-safety --with-pydebug --with-openssl="$OPENSSL_DIR" | ||
- name: Build CPython | ||
run: make -j4 | ||
- name: Display build info | ||
run: make pythoninfo | ||
- name: SSL tests | ||
run: ./python Lib/test/ssltests.py | ||
|
||
build-ubuntu-ssltests-awslc: | ||
name: 'Ubuntu SSL tests with AWS-LC' | ||
runs-on: ${{ matrix.os }} | ||
timeout-minutes: 60 | ||
needs: build-context | ||
if: needs.build-context.outputs.run-tests == 'true' | ||
strategy: | ||
fail-fast: false | ||
matrix: | ||
os: [ubuntu-24.04] | ||
awslc_ver: [1.55.0] | ||
env: | ||
AWSLC_VER: ${{ matrix.awslc_ver}} | ||
SSL_VER: ${{ matrix.ssl_ver }} | ||
MULTISSL_DIR: ${{ github.workspace }}/multissl | ||
OPENSSL_DIR: ${{ github.workspace }}/multissl/aws-lc/${{ matrix.awslc_ver }} | ||
LD_LIBRARY_PATH: ${{ github.workspace }}/multissl/aws-lc/${{ matrix.awslc_ver }}/lib | ||
SSL_DIR: ${{ github.workspace }}/multissl/${{ matrix.ssl }}/${{ matrix.ssl_ver }} | ||
LD_LIBRARY_PATH: ${{ github.workspace }}/multissl/${{ matrix.ssl }}/${{ matrix.ssl_ver }}/lib | ||
steps: | ||
- uses: actions/checkout@v4 | ||
with: | ||
|
@@ -356,22 +300,18 @@ jobs: | |
- name: Configure SSL lib env vars | ||
run: | | ||
echo "MULTISSL_DIR=${GITHUB_WORKSPACE}/multissl" >> "$GITHUB_ENV" | ||
echo "OPENSSL_DIR=${GITHUB_WORKSPACE}/multissl/aws-lc/${AWSLC_VER}" >> "$GITHUB_ENV" | ||
echo "LD_LIBRARY_PATH=${GITHUB_WORKSPACE}/multissl/aws-lc/${AWSLC_VER}/lib" >> "$GITHUB_ENV" | ||
- name: 'Restore AWS-LC build' | ||
id: cache-aws-lc | ||
echo "SSL_DIR=${GITHUB_WORKSPACE}/multissl/${{ matrix.ssl }}/${SSL_VER}" >> "$GITHUB_ENV" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't we just use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. zizmor says using
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's ask zizmor author @woodruffw. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, so zizmor kwnows that it's not attackable but recommends against it for shell expansion purposes: https://github.com/woodruffw/gha-hazmat/blob/e4094ceabb0068a2a5b81ecc8e1ee919ba52a050/.github/workflows/template-injection.yml#L77. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. However! we can do another trick https://github.com/woodruffw/gha-hazmat/blob/e4094ceabb0068a2a5b81ecc8e1ee919ba52a050/.github/workflows/template-injection.yml#L67-L72. I guess we could directly use the environment variable name then without needing to make GH variable expansion. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I'd recommend using the environment variable directly instead of intermediating it through a template (e.g. The reason There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some more docs here too (the first tip in that section mentions why |
||
echo "LD_LIBRARY_PATH=${GITHUB_WORKSPACE}/multissl/${{ matrix.ssl }}/${SSL_VER}/lib" >> "$GITHUB_ENV" | ||
- name: 'Restore SSL build' | ||
id: cache-ssl | ||
uses: actions/cache@v4 | ||
with: | ||
path: ./multissl/aws-lc/${{ matrix.awslc_ver }} | ||
key: ${{ matrix.os }}-multissl-aws-lc-${{ matrix.awslc_ver }} | ||
- name: Install AWS-LC | ||
if: steps.cache-aws-lc.outputs.cache-hit != 'true' | ||
path: ./multissl/${{ env.SSL }}/${{ env.SSL_VER }} | ||
key: ${{ matrix.os }}-multissl-${{ env.SSL }}-${{ env.SSL_VER }} | ||
- name: Install SSL | ||
if: steps.cache-ssl.outputs.cache-hit != 'true' | ||
run: | | ||
python3 Tools/ssl/multissltests.py \ | ||
--steps=library \ | ||
--base-directory "$MULTISSL_DIR" \ | ||
--awslc ${{ matrix.awslc_ver }} \ | ||
--system Linux | ||
python3 Tools/ssl/multissltests.py --steps=library --base-directory "$MULTISSL_DIR" --system Linux --ssl ${{ matrix.ssl }} --ssl-versions ${{ matrix.ssl_ver }} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you make it multiline as before? please There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note we can use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it would be better. We don't care about literal new lines here. |
||
- name: Add ccache to PATH | ||
run: | | ||
echo "PATH=/usr/lib/ccache:$PATH" >> "$GITHUB_ENV" | ||
|
@@ -381,18 +321,18 @@ jobs: | |
save: false | ||
- name: Configure CPython | ||
run: | | ||
./configure CFLAGS="-fdiagnostics-format=json" \ | ||
--config-cache \ | ||
--enable-slower-safety \ | ||
--with-pydebug \ | ||
--with-openssl="$OPENSSL_DIR" \ | ||
--with-builtin-hashlib-hashes=blake2 \ | ||
--with-ssl-default-suites=openssl | ||
CMD=(./configure CFLAGS="-fdiagnostics-format=json" --config-cache --enable-slower-safety --with-pydebug --with-openssl="$SSL_DIR") | ||
if [ "${{ matrix.ssl }}" = "openssl" ]; then | ||
"${CMD[@]}" | ||
else | ||
"${CMD[@]}" --with-builtin-hashlib-hashes=blake2 --with-ssl-default-suites=openssl | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we populate a variable called "CONFIGURE_FLAGS" instead and use a switch/case so that we exactly match the SSL libname. It will become easier in the future if we add BoringSSL or LibreSSL tests. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At this point I'd almost suggest using a small Python script to calculate flags. We should avoid complex shell logic in workflow files as much as possible, I don't want to read a bash switch-case in YAML! An if-statement is simpler to understand here for me, though the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh yeah, a small Python script is also fine; here what matters to me is to be able to easily add a new library without having 3km long of options. As for |
||
fi | ||
- name: Build CPython | ||
run: make -j | ||
run: make -j4 | ||
picnixz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
- name: Display build info | ||
run: make pythoninfo | ||
- name: Verify python is linked to AWS-LC | ||
if: matrix.ssl == 'aws-lc' | ||
Comment on lines
334
to
+335
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should, if possible, do it for OpenSSL as well. |
||
run: ./python -c 'import ssl; print(ssl.OPENSSL_VERSION)' | grep AWS-LC | ||
- name: SSL tests | ||
run: ./python Lib/test/ssltests.py | ||
|
@@ -435,7 +375,7 @@ jobs: | |
key: ${{ runner.os }}-multissl-openssl-${{ env.OPENSSL_VER }} | ||
- name: Install OpenSSL | ||
if: steps.cache-openssl.outputs.cache-hit != 'true' | ||
run: python3 Tools/ssl/multissltests.py --steps=library --base-directory "$MULTISSL_DIR" --openssl "$OPENSSL_VER" --system Linux | ||
run: python3 Tools/ssl/multissltests.py --steps=library --base-directory "$MULTISSL_DIR" --ssl 'openssl' --ssl-versions "$OPENSSL_VER" --system Linux | ||
- name: Add ccache to PATH | ||
run: | | ||
echo "PATH=/usr/lib/ccache:$PATH" >> "$GITHUB_ENV" | ||
|
@@ -567,7 +507,7 @@ jobs: | |
key: ${{ matrix.os }}-multissl-openssl-${{ env.OPENSSL_VER }} | ||
- name: Install OpenSSL | ||
if: steps.cache-openssl.outputs.cache-hit != 'true' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With |
||
run: python3 Tools/ssl/multissltests.py --steps=library --base-directory "$MULTISSL_DIR" --openssl "$OPENSSL_VER" --system Linux | ||
run: python3 Tools/ssl/multissltests.py --steps=library --base-directory "$MULTISSL_DIR" --ssl 'openssl' --ssl-versions "$OPENSSL_VER" --system Linux | ||
- name: Add ccache to PATH | ||
run: | | ||
echo "PATH=/usr/lib/ccache:$PATH" >> "$GITHUB_ENV" | ||
|
@@ -703,8 +643,7 @@ jobs: | |
- build-windows-msi | ||
- build-macos | ||
- build-ubuntu | ||
- build-ubuntu-ssltests-awslc | ||
- build-ubuntu-ssltests-openssl | ||
- build-ubuntu-ssltests | ||
- build-wasi | ||
- test-hypothesis | ||
- build-asan | ||
|
@@ -719,8 +658,7 @@ jobs: | |
with: | ||
allowed-failures: >- | ||
build-windows-msi, | ||
build-ubuntu-ssltests-awslc, | ||
build-ubuntu-ssltests-openssl, | ||
build-ubuntu-ssltests, | ||
test-hypothesis, | ||
cifuzz, | ||
allowed-skips: >- | ||
|
@@ -738,8 +676,7 @@ jobs: | |
check-generated-files, | ||
build-macos, | ||
build-ubuntu, | ||
build-ubuntu-ssltests-awslc, | ||
build-ubuntu-ssltests-openssl, | ||
build-ubuntu-ssltests, | ||
build-wasi, | ||
test-hypothesis, | ||
build-asan, | ||
|
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.
All jobs run on the same OS, this is simpler:
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 use
matrix.os
in the cache key so better not hardcode it here?