Skip to content

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.

Sign up for GitHub

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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

gh-136728: Refactor build.yml CI config and multissltests.py #136729

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
a104f8a
Adapt @hugovk's proof-of-concept CI definition refactor
WillChilds-Klein Jul 14, 2025
3fcbe0d
Preliminary refactor of multissltests.py, TODO migrate to classes
WillChilds-Klein Jul 14, 2025
5d8ec9a
Fix aws-lc/awslc lib name discrepancy
WillChilds-Klein Jul 14, 2025
991c6b2
Migrate AbstractBuilder to abc
WillChilds-Klein Jul 14, 2025
7b51499
Fix imports
WillChilds-Klein Jul 14, 2025
6638127
Complete ABC refactor
WillChilds-Klein Jul 16, 2025
4e0a8ca
Colorize parser
WillChilds-Klein Jul 16, 2025
1a90e0c
Adjust compatibility comment
WillChilds-Klein Jul 16, 2025
1fcb49f
📜🤖 Added by blurb_it.
blurb-it[bot] Jul 16, 2025
dd969fb
Revert "📜🤖 Added by blurb_it."
WillChilds-Klein Jul 16, 2025
1edce1c
Merge branch 'main' into refactor-ssl-tests
WillChilds-Klein Jul 16, 2025
3343120
Include old versions when using default versions
WillChilds-Klein Jul 16, 2025
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
131 changes: 34 additions & 97 deletions .github/workflows/build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }}
Copy link
Member

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:

Suggested change
runs-on: ${{ matrix.os }}
runs-on: ubuntu-24.04

Copy link
Member

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?

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
Copy link
Member

Choose a reason for hiding this comment

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

You can remove the os from the include and have:

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.

Copy link
Member

Choose a reason for hiding this comment

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

Could we do ssl_lib: openssl | awslc? It would be easier to follow later on than just matrix.ssl everywhere.

Copy link
Member

Choose a reason for hiding this comment

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

I'm sorry but what would ssl_lib: openssl | awslc in this case? does it run on both and if it doesn't work for one, it picks the other? (also, how would we use it below?)

Copy link
Member

Choose a reason for hiding this comment

The 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 matrix.ssl to matrix.ssl_lib. openssl | awslc are the valid values for either name.

Copy link
Member

Choose a reason for hiding this comment

The 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 matrix.ssl_lib

# 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:
Expand All @@ -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"
Copy link
Member

Choose a reason for hiding this comment

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

Can't we just use ${{ env.ssldir }}? I don't understand why we're defining it here and in the env section

Copy link
Member

Choose a reason for hiding this comment

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

zizmor says using ${{ env.ssldir }} can result in template injection:

help[template-injection]: code injection via template expansion
   --> ./.github/workflows/build.yml:324:134
    |
323 |       run: |
    |       --- help: this run block
324 |         CMD=(./configure CFLAGS="-fdiagnostics-format=json" --config-cache --enable-slower-safety --with-pydebug --with-openssl="${{ env.ssldir }...
    |                                                                                                                                      ---------- help: may expand into attacker-controllable code

Copy link
Member

@picnixz picnixz Aug 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Isn't env a procteced namespace that cannot be accessed from outside? but if zizmor flags it as possibly vulnerable, let's be verbose.

Copy link
Member

Choose a reason for hiding this comment

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

Let's ask zizmor author @woodruffw.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@picnixz picnixz Aug 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

The 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. $FOO instead of ${{ env.FOO }}). That should be equivalent in terms of intended behavior without the injection risk 🙂

The reason zizmor flags these is because they can be exploitable in some subtle situations: most of the time env.* is static and not attacker-controllable, but sometimes it comes from GITHUB_ENV or another dynamic source that zizmor can't follow.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 env.* is not preferred versus direct expansion): https://docs.zizmor.sh/audits/#template-injection

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 }}
Copy link
Member

Choose a reason for hiding this comment

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

Can you make it multiline as before? please

Copy link
Member

Choose a reason for hiding this comment

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

Note we can use run: > syntax to avoid the trailing backslashes

Copy link
Member

Choose a reason for hiding this comment

The 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"
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

The 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 ${CMD[@]} syntax is arcane.

Copy link
Member

Choose a reason for hiding this comment

The 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 ${CMD[@]}, it expands the array's content, so you end up with ./configure [...]. Usually, it's more common to expand the options and hardcode the command rather than expand the entire array containing the command as well.

fi
- name: Build CPython
run: make -j
run: make -j4
- 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
Copy link
Member

Choose a reason for hiding this comment

The 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
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -567,7 +507,7 @@ jobs:
key: ${{ matrix.os }}-multissl-openssl-${{ env.OPENSSL_VER }}
- name: Install OpenSSL
if: steps.cache-openssl.outputs.cache-hit != 'true'
Copy link
Member

Choose a reason for hiding this comment

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

With steps.cache-openssl here, we won't be hitting the cache-ssl entry right? maybe it's better to store a cache hit with steps.cache-${{ matrix.ssl }} if it's possible?

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"
Expand Down Expand Up @@ -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
Expand All @@ -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: >-
Expand All @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/reusable-ubuntu.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ jobs:
key: ${{ inputs.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"
Expand Down
Loading
Loading