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

WillChilds-Klein · 2025-07-16T20:05:29Z

Notes

Please see #136728. Changes to build.yml are directly adapted from @hugovk's proof-of-concept here.

Testing

this PR's CI
PR CI on my fork.

Issue: Refactor build.yml CI ssltests config and multissltests.py #136728

@hugovk

Suggested by @hugovk [1]. [1]: hugovk@a3f2ba9

This reverts commit 1fcb49f.

picnixz

If possible, I want multissltests to be refactored separately.

picnixz · 2025-08-02T10:37:24Z

.github/workflows/build.yml

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


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.

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

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

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

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

.github/workflows/build.yml

picnixz · 2025-08-02T10:44:39Z

.github/workflows/build.yml

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


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.

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.

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.

.github/workflows/build.yml

picnixz · 2025-08-02T10:48:59Z

.github/workflows/build.yml

        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'


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?

picnixz · 2025-08-02T10:50:05Z

Tools/ssl/multissltests.py

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


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.

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

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.

picnixz · 2025-08-02T10:53:38Z

Tools/ssl/multissltests.py

+    )
+
+    versions = []
+    ssl_libs = AbstractBuilder.__subclasses__()


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

Tools/ssl/multissltests.py

picnixz · 2025-08-02T10:54:25Z

Tools/ssl/multissltests.py

+        format="*** %(levelname)s %(message)s"
+    )
+
+    versions = []


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

Tools/ssl/multissltests.py

.github/workflows/build.yml

AA-Turner · 2025-08-05T13:12:51Z

.github/workflows/build.yml

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


All jobs run on the same OS, this is simpler:

Suggested change

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

runs-on: ubuntu-24.04

We use matrix.os in the cache key so better not hardcode it here?

Tools/ssl/multissltests.py

AA-Turner · 2025-08-05T13:22:46Z

Tools/ssl/multissltests.py

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


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

WillChilds-Klein · 2025-08-25T18:32:02Z

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

hugovk · 2026-01-16T21:04:34Z

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>

hugovk · 2026-01-16T21:51:43Z

Let's continue in #143940.

WillChilds-Klein added 8 commits July 14, 2025 21:19

Adapt @hugovk's proof-of-concept CI definition refactor

a104f8a

Suggested by @hugovk [1]. [1]: hugovk@a3f2ba9

Preliminary refactor of multissltests.py, TODO migrate to classes

3fcbe0d

Fix aws-lc/awslc lib name discrepancy

5d8ec9a

Migrate AbstractBuilder to abc

991c6b2

Fix imports

7b51499

Complete ABC refactor

6638127

Colorize parser

4e0a8ca

Adjust compatibility comment

1a90e0c

bedevere-app bot mentioned this pull request Jul 16, 2025

Refactor build.yml CI ssltests config and multissltests.py #136728

Open

zware added the skip news label Jul 16, 2025

StanFromIreland added the skip news label Jul 16, 2025

blurb-it bot and others added 4 commits July 16, 2025 20:08

📜🤖 Added by blurb_it.

1fcb49f

Revert "📜🤖 Added by blurb_it."

dd969fb

This reverts commit 1fcb49f.

Merge branch 'main' into refactor-ssl-tests

1edce1c

Include old versions when using default versions

3343120

WillChilds-Klein marked this pull request as ready for review July 16, 2025 20:39

WillChilds-Klein requested review from AA-Turner, ezio-melotti, gpshead, hugovk and picnixz as code owners July 16, 2025 20:39

bedevere-app bot added the awaiting review label Jul 16, 2025

picnixz reviewed Aug 2, 2025

View reviewed changes

hugovk reviewed Aug 5, 2025

View reviewed changes

Tools/ssl/multissltests.py Outdated Show resolved Hide resolved

Tools/ssl/multissltests.py Outdated Show resolved Hide resolved

.github/workflows/build.yml Show resolved Hide resolved

.github/workflows/build.yml Show resolved Hide resolved

AA-Turner reviewed Aug 5, 2025

View reviewed changes

Tools/ssl/multissltests.py Show resolved Hide resolved

AA-Turner reviewed Aug 5, 2025

View reviewed changes

Tools/ssl/multissltests.py Outdated Show resolved Hide resolved

AA-Turner reviewed Aug 5, 2025

View reviewed changes

Tools/ssl/multissltests.py Outdated Show resolved Hide resolved

AA-Turner reviewed Aug 5, 2025

View reviewed changes

Apply suggestions from code review

66414c3

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

hugovk mentioned this pull request Jan 16, 2026

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

Draft

hugovk closed this Jan 16, 2026

Uh oh!

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

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

Uh oh!

Conversation

WillChilds-Klein commented Jul 16, 2025 • edited by bedevere-app bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Notes

Testing

Uh oh!

picnixz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

picnixz Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WillChilds-Klein commented Aug 25, 2025

Uh oh!

hugovk commented Jan 16, 2026

Uh oh!

hugovk commented Jan 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

20 participants

WillChilds-Klein commented Jul 16, 2025 •

edited by bedevere-app bot

Loading

picnixz Aug 5, 2025 •

edited

Loading