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

feat(debug): improved debug messages #55

Merged
merged 2 commits into from
Oct 11, 2024
Merged

feat(debug): improved debug messages #55

merged 2 commits into from
Oct 11, 2024

Conversation

RufusJWB
Copy link
Collaborator

Motivation

Simplify debugging

Proposed Changes

Improving log messages

Test Plan

n/a

@RufusJWB RufusJWB requested a review from DDvO August 15, 2024 09:20
Copy link

sonarcloud bot commented Aug 15, 2024

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.

Thank you for the improvements suggested.
Here are some requests for tweaking them.

OpenSSL_version.c Outdated Show resolved Hide resolved
OpenSSL_version.c Outdated Show resolved Hide resolved
OpenSSL_version.c Outdated Show resolved Hide resolved
OpenSSL_version.mk Outdated Show resolved Hide resolved
OpenSSL_version.mk Outdated Show resolved Hide resolved
OpenSSL_version.mk Outdated Show resolved Hide resolved
OpenSSL_version.mk Outdated Show resolved Hide resolved
OpenSSL_version.mk Outdated Show resolved Hide resolved
OpenSSL_version.mk Outdated Show resolved Hide resolved
OpenSSL_version.mk Outdated Show resolved Hide resolved
@RufusJWB
Copy link
Collaborator Author

Thank you for the improvements suggested. Here are some requests for tweaking them.

All implemented. Thank you for the proposals

@RufusJWB RufusJWB closed this Sep 18, 2024
@RufusJWB RufusJWB reopened this Sep 18, 2024
@RufusJWB RufusJWB requested a review from DDvO September 18, 2024 20:58
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 - thank you for the contribution!

@DDvO
Copy link
Member

DDvO commented Sep 19, 2024

@RufusJWB could you please squash you commits,
while keeping the unrelated change to .gitignore in a separate commit,
and rebase to handle the merge conflict (simply letting your changes override the recent change
of -Wl,-rpath,= to -Wl,-rpath,. which has become also part of this PR).

@RufusJWB
Copy link
Collaborator Author

while keeping the unrelated change to .gitignore in a separate commit

Moved this to #61

@RufusJWB RufusJWB force-pushed the openssl34 branch 2 times, most recently from 11c9731 to 99a7d3b Compare September 19, 2024 20:49
@RufusJWB
Copy link
Collaborator Author

@RufusJWB could you please squash you commits,

done

@RufusJWB RufusJWB requested a review from DDvO September 19, 2024 20:51
@DDvO
Copy link
Member

DDvO commented Sep 24, 2024

Please check the output of the CI test_all run:

make -f Makefile_tests test_Mock CMPCLIENT="./cmpClient" OPENSSL=openssl OPENSSL_VERSION=[DEBUG] LIB is set to header [DEBUG] OPENSSL_VERSION_MAJOR found in OPENSSLV_H [DEBUG] OS is not MacOS [TRACE] After OS check in header: OPENSSL_VERSION=3.0 [TRACE] OPENSSL_NUMBER_SEL=head -n 1 | sed -r 's/.*OpenSSL //' | awk '{print ($0+0)}' [TRACE] OPENSSLV_H=/usr/include/openssl/opensslv.h [TRACE] OPENSSL_VERSION=3.0 3.0
awk: fatal: cannot open file `[TRACE]' for reading: No such file or directory
make[1]: *** No rule to make target 'LIB'.  Stop.

Somehow the new diagnostic output confuses a Makefile used for the tests.

OpenSSL_version.mk Outdated Show resolved Hide resolved
@RufusJWB RufusJWB requested a review from DDvO September 25, 2024 11:52
@RufusJWB
Copy link
Collaborator Author

Please check the output of the CI test_all run:

make -f Makefile_tests test_Mock CMPCLIENT="./cmpClient" OPENSSL=openssl OPENSSL_VERSION=[DEBUG] LIB is set to header [DEBUG] OPENSSL_VERSION_MAJOR found in OPENSSLV_H [DEBUG] OS is not MacOS [TRACE] After OS check in header: OPENSSL_VERSION=3.0 [TRACE] OPENSSL_NUMBER_SEL=head -n 1 | sed -r 's/.*OpenSSL //' | awk '{print ($0+0)}' [TRACE] OPENSSLV_H=/usr/include/openssl/opensslv.h [TRACE] OPENSSL_VERSION=3.0 3.0
awk: fatal: cannot open file `[TRACE]' for reading: No such file or directory
make[1]: *** No rule to make target 'LIB'.  Stop.

Somehow the new diagnostic output confuses a Makefile used for the tests.

looks good now
image

@DDvO
Copy link
Member

DDvO commented Sep 25, 2024

Hmm, the test_all CI run still fails the same way.
I suspect that also the output of OPENSSL_VERSION_MAJOR needs to be tweaked.

@DDvO
Copy link
Member

DDvO commented Sep 25, 2024

BTW, would be nice not to have merge commits.
Best practice is to use fixup commits (e.g., on the command line, git commit --fixup @).

@DDvO
Copy link
Member

DDvO commented Sep 25, 2024

The CI still fails, supposedly because there is still one output line containing the string OPENSSL_VERSION:
$(info [TRACE] After OS check in header: OPENSSL_VERSION=$(OPENSSL_VERSION))

@DDvO
Copy link
Member

DDvO commented Oct 11, 2024

Argh, just recalled that this PR was still in trouble,
and meanwhile I understand what went wrong:
The new diagnostics output gets prepended to the version info expected by Makefile_v1.

Fixed by directing all the new diagnostic output to stderr.
Also updated README.md

Copy link

sonarcloud bot commented Oct 11, 2024

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.

Also CI confirms that the issue is fixed, finally.

@DDvO DDvO merged commit 8871aa6 into master Oct 11, 2024
6 checks passed
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