gh-136728: Refactor build.yml CI config and multissltests.py #143940
Conversation
This reverts commit 1fcb49f.
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
| build-ubuntu-ssltests-awslc, | ||
| build-ubuntu-ssltests-openssl, | ||
| build-ubuntu-ssltests, | ||
| build-wasi, |
There was a problem hiding this comment.
wasi? why is this a part of the patch?
There was a problem hiding this comment.
Could be because of conflicts or out-of-date PR?
| run: | | ||
| echo "PATH=/usr/lib/ccache:$PATH" >> "$GITHUB_ENV" | ||
| - name: Configure CPython | ||
| run: | |
There was a problem hiding this comment.
>- makes EOL escaping redundant while allowing for nice line separation of long/logically self-contained components:
| run: | | |
| env: | |
| CONFIGURE_TRAILING_ARGS: >- | |
| ${{ | |
| matrix.ssl != 'openssl' | |
| && '--with-builtin-hashlib-hashes=blake2 --with-ssl-default-suites=openssl' | |
| || '' | |
| }} | |
| run: >- |
There was a problem hiding this comment.
I prefer \ becauseI find them easier to read. I find the >- harder to read honestly.
| 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 | ||
| fi |
There was a problem hiding this comment.
| 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 | |
| fi | |
| ./configure | |
| CFLAGS="-fdiagnostics-format=json" | |
| --config-cache | |
| --enable-slower-safety | |
| --with-pydebug | |
| --with-openssl="$SSL_DIR" | |
| ${CONFIGURE_TRAILING_ARGS} |
There was a problem hiding this comment.
I prefer the if/else approach. I do not like the notion of 'trailing' args.
|
|
||
| @property | ||
| @abstractmethod | ||
| def library(self=None): |
There was a problem hiding this comment.
So as I said previously, we should just have annotated properties. It will complain at runtime if we are missing one.
| assert len(libs) == 1 | ||
| cls = libs.pop() | ||
| if args.ssl_versions: | ||
| versions.extend((cls, v) for v in args.ssl_versions]) |
There was a problem hiding this comment.
| versions.extend((cls, v) for v in args.ssl_versions]) | |
| versions.extend((cls, v) for v in args.ssl_versions) |
| if args.ssl_versions: | ||
| versions.extend((cls, v) for v in args.ssl_versions]) | ||
| else: | ||
| versions.extend([(cls, v) for v in cls.recent_versions.fget()]) |
There was a problem hiding this comment.
No need for the additional list constructor. You can just tuse the generator directly in extend()
4 participants
Continuation of #136729.
Original description:
Notes
Please see #136728. Changes to
build.ymlare directly adapted from @hugovk's proof-of-concept here.Testing