Skip to content
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

CI: OpenSSL compatibility check and macOS #67

Merged
merged 2 commits into from
Jan 24, 2025

Conversation

rajeev-0
Copy link
Collaborator

Motivation

issue #46

Proposed Changes

Add new CI pipeline.

Test Plan

Test is not required

@rajeev-0 rajeev-0 changed the title Openssl ver compatabilty check WIP - Openssl ver compatabilty check Oct 16, 2024
@rajeev-0 rajeev-0 force-pushed the openssl_ver_compatabilty_check branch 3 times, most recently from cd48e8c to d0ea0d1 Compare October 22, 2024 15:26
Copy link

@DDvO
Copy link
Member

DDvO commented Nov 7, 2024

Would this work if rebased on #68?

@rajeev-0 rajeev-0 force-pushed the openssl_ver_compatabilty_check branch 3 times, most recently from 76ce89b to dd1eb06 Compare January 16, 2025 13:21
@rajeev-0 rajeev-0 marked this pull request as ready for review January 16, 2025 13:21
@rajeev-0 rajeev-0 self-assigned this Jan 16, 2025
@rajeev-0 rajeev-0 requested a review from DDvO January 16, 2025 14:04
Copy link
Member

@DDvO DDvO left a comment

Choose a reason for hiding this comment

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

Looks nice.
Some small points for improvements.

.github/workflows/mac-compatability.yml Outdated Show resolved Hide resolved
cd build-with-libcmp
USE_LIBCMP=1 cmake -S .. -B .
make clean build
DESTDIR=tmp make install uninstall
Copy link
Member

Choose a reason for hiding this comment

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

Please also add

cd ..

make -f Makefile_v1 test_Mock

Copy link
Member

@DDvO DDvO Jan 17, 2025

Choose a reason for hiding this comment

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

Argh, with macOS the HTTP-based tests hang,
I believe due to waiting for interactive approval for the server to receive input on a local port.

If we do not quickly find way of fixing this, I suggest replacing

make -f Makefile_v1 test_Mock`

by just

make -f Makefile_v1
./cmpClient --help

.github/workflows/version_compatability.yml Outdated Show resolved Hide resolved
.github/workflows/mac-compatability.yml Outdated Show resolved Hide resolved
.github/workflows/mac-compatability.yml Outdated Show resolved Hide resolved
.github/workflows/mac-compatability.yml Outdated Show resolved Hide resolved
.github/workflows/version_compatability.yml Outdated Show resolved Hide resolved
run: |
make -f Makefile_v1
make -f Makefile_v1 clean_all
USE_LIBCMP=1 STATIC_LIBCMP=1 make -f Makefile_v1 build_no_tls
Copy link
Member

Choose a reason for hiding this comment

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

please add:

make -f Makefile_v1 test_Mock

Copy link
Member

@DDvO DDvO Jan 17, 2025

Choose a reason for hiding this comment

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

I believe the current build failure is due to inconsistency of the build configuration after
USE_LIBCMP=1 STATIC_LIBCMP=1 make -f Makefile_v1 build_no_tls.

So instead of adding make -f Makefile_v1 test_Mock thereafter,
one could replace the earlier make -f Makefile_v1 by make -f Makefile_v1 test_Mock.
or insert make -f Makefile_v1 clean_all before the mock test build,
of more efficiently:
augment the new make -f Makefile_v1 test_Mock by
USE_LIBCMP=1 STATIC_LIBCMP=1 make -f Makefile_v1 test_Mock

@DDvO
Copy link
Member

DDvO commented Jan 17, 2025

One of the two typos is also in the file name version_compatability.yml.
Anyway, better include "OpenSSL" in the name, and may simply leave out that difficult word "compatibility",
e.g. OpenSSL_versions.yml.

@rajeev-0 rajeev-0 changed the title WIP - Openssl ver compatabilty check OpenSSL compatibility check Jan 17, 2025
@rajeev-0 rajeev-0 force-pushed the openssl_ver_compatabilty_check branch 6 times, most recently from 29f67f3 to 3857dca Compare January 20, 2025 13:27
@rajeev-0 rajeev-0 requested a review from DDvO January 20, 2025 13:32
.github/workflows/mac-compatibility.yml Outdated Show resolved Hide resolved
.github/workflows/mac-compatibility.yml Outdated Show resolved Hide resolved
.github/workflows/mac-compatibility.yml Outdated Show resolved Hide resolved
.github/workflows/OpenSSL_versions.yml Outdated Show resolved Hide resolved
.github/workflows/OpenSSL_versions.yml Outdated Show resolved Hide resolved
.github/workflows/OpenSSL_versions.yml Outdated Show resolved Hide resolved
.github/workflows/mac-compatibility.yml Outdated Show resolved Hide resolved
.github/workflows/mac-compatibility.yml Outdated Show resolved Hide resolved
.github/workflows/mac-compatibility.yml Outdated Show resolved Hide resolved
@rajeev-0 rajeev-0 force-pushed the openssl_ver_compatabilty_check branch 2 times, most recently from 2c4f453 to 4853603 Compare January 20, 2025 17:20
Copy link
Member

@DDvO DDvO left a comment

Choose a reason for hiding this comment

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

for some reason, steps.cache-openssl.outputs.cache-hit
seems to nave yield 'true'.
I suggest printing the actual value to help find out what went wrong.

.github/workflows/OpenSSL_versions.yml Outdated Show resolved Hide resolved
.github/workflows/OpenSSL_versions.yml Outdated Show resolved Hide resolved
.github/workflows/OpenSSL_versions.yml Outdated Show resolved Hide resolved
.github/workflows/OpenSSL_versions.yml Show resolved Hide resolved
.github/workflows/OpenSSL_versions.yml Outdated Show resolved Hide resolved
.github/workflows/OpenSSL_versions.yml Show resolved Hide resolved
.github/workflows/OpenSSL_versions.yml Outdated Show resolved Hide resolved
.github/workflows/macos.yml Show resolved Hide resolved
@rajeev-0 rajeev-0 force-pushed the openssl_ver_compatabilty_check branch from 33b93c8 to 701861f Compare January 23, 2025 15:17
Copy link
Member

@DDvO DDvO left a comment

Choose a reason for hiding this comment

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

LGTM

@DDvO
Copy link
Member

DDvO commented Jan 23, 2025

Ready to merge -
before doing so, please (auto-)squash each of the two commit threads before,
as GitHub does not offer this feature (it can only squash all commits, which is not what we want).

@rajeev-0 rajeev-0 force-pushed the openssl_ver_compatabilty_check branch from 701861f to b5840ef Compare January 24, 2025 10:12
@rajeev-0 rajeev-0 merged commit 7b8e42a into master Jan 24, 2025
13 checks passed
@rajeev-0 rajeev-0 deleted the openssl_ver_compatabilty_check branch January 24, 2025 10:51
@DDvO DDvO changed the title OpenSSL compatibility check CI: OpenSSL compatibility check and macOS Jan 27, 2025
@DDvO DDvO mentioned this pull request Jan 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants