gh-136728: Refactor build.yml CI config and multissltests.py by WillChilds-Klein · Pull Request #136729 · python/cpython

WillChilds-Klein

Choose a reason for hiding this comment

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

If possible, I want multissltests to be refactored separately.

Comment on lines +273 to +278

		- { 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 }

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.

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.

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?)

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.

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

		if [ "${{ matrix.ssl }}" = "openssl" ]; then
		"${CMD[@]}"
		else
		"${CMD[@]}" --with-builtin-hashlib-hashes=blake2 --with-ssl-default-suites=openssl

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.

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.

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.

		@@ -567,7 +507,7 @@ jobs:
		key: ${{ matrix.os }}-multissl-openssl-${{ env.OPENSSL_VER }}
		- name: Install OpenSSL
		if: steps.cache-openssl.outputs.cache-hit != 'true'

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?

Comment on lines +143 to +146

		@property
		@abstractmethod
		def library(self=None):
		pass

Choose a reason for hiding this comment

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

Please revert this. As we can't have abstract class methods, I prefer having real class variables where we would raise if they are not set.

Choose a reason for hiding this comment

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

Yep, better to raise NotImplementedError. That's treated equivalently to an abstract method.

Choose a reason for hiding this comment

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

I think it's just better to have abstract class variables here. The reason is that I don't think we would ever need a non-static computation for most of those abstract properties. If they must be abstract class properties that are represented by dynamic values that need a bit more work to compute (but only needed to be computed once), just a @classmethod together with NotImplementedError.

		)

		versions = []
		ssl_libs = AbstractBuilder.__subclasses__()

Choose a reason for hiding this comment

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

Please, don't use this. It's harder to maintain if we do it like this. Just hardcode the classes.

		format="*** %(levelname)s %(message)s"
		)

		versions = []

Choose a reason for hiding this comment

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

You could have a dict of classes to a list of versions instead of storing a tuple of (class, version)

		name: 'Ubuntu SSL tests with OpenSSL'
		build-ubuntu-ssltests:
		name: 'Ubuntu SSL tests'
		runs-on: ${{ matrix.os }}

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:

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

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?

		"LibreSSL versions, defaults to '{}' (ancient: '{}') if no "
		"OpenSSL and LibreSSL versions are given."
		).format(LIBRESSL_RECENT_VERSIONS, LIBRESSL_OLD_VERSIONS)
		'--ssl',

Choose a reason for hiding this comment

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

Again I'd suggest --ssl-library or etc

Hi folks, sorry for my delayed responses. Hoping to incorporate feedback here later this week or next.

All commit authors signed the Contributor License Agreement.

hugovk removed request for a team, JelleZijlstra, ambv, barneygale, berkerpeksag, encukou, erlend-aasland, ethanfurman, ezio-melotti, gaogaotiantian, giampaolo, gpshead, hauntsaninja, jaraco, lysnikolaou, pablogsal, rhettinger, vsajip and warsaw

January 16, 2026 21:04

Oh yuck, sorry for the pings, this knotty merge resolution pulled in a lot of code...

I'll apply some of the suggestions to the branch, then close the PR and open a new one.

Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>