-
Notifications
You must be signed in to change notification settings - Fork 167
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
Enhance printing OpenSSL versions. #662
Conversation
It is printed like this. |
d4c7d67
to
63363ca
Compare
Rebased with updating the value of the In the OpenSSL case, the
And in the LibreSSL case, the value is printed as follows. https://github.com/ruby/openssl/actions/runs/5866211628/job/15904567048?pr=662#step:13:33
|
63363ca
to
ff14d38
Compare
ff14d38
to
a1656cd
Compare
@rhenium I got an idea. Right now the Lines 1141 to 1148 in db633c5
So, maybe we can implement as follows, removing the the implementatio n of the
What do you think? |
IMO I have no objection to adding |
Right. If we change the value of the
All right! So, let me fix this PR. |
7881a2f
to
9a03080
Compare
*/ | ||
rb_define_const(mOSSL, "OPENSSL_VERSION_NUMBER", INT2NUM(OPENSSL_VERSION_NUMBER)); | ||
|
||
#if defined(LIBRESSL_VERSION_NUMBER) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note the reason why the #if defined(LIBRESSL_VERSION_NUMBER)
is above the comment, not below the comment is because Rdoc cannot parse the comment if #if defined(LIBRESSL_VERSION_NUMBER)
is written below the comment.
@rhenium I rebased this PR fixing the issues mentioned in the review. Now the OpenSSL case:
LibreSSL case:
|
05cfcf3
to
c6db8a5
Compare
* Updated the `OpenSSL::OPENSSL_VERSION_NUMBER` comment explaining the format. * Added the `OpenSSL::LIBRESSL_VERSION_NUMBER` to print LibreSSL version number, in the case that Ruby OpenSSL binding is compiled with LibreSSL. Note `test/openssl/utils.rb#libressl?` is not using this value in it for now. * Update `rake debug` to print the values in a readable way, adding `OpenSSL::OPENSSL_VERSION_NUMBER` and `OpenSSL::LIBRESSL_VERSION_NUMBER`.
c6db8a5
to
d19e636
Compare
Everything looks good to me! |
Thanks for reviewing! |
This PR is to enhance the feature of printing OpenSSL versions. This is useful when running tests conditionally depending on OpenSSL and LibreSSL versions. In my case, it was useful to fix pending tests in FIPS case. See my working branch wip/fip-test-debug commits to fix pending tests. The last 4 commits are mine. And the 3rd newest commit is the same with this PR's commit.
For the
OPENSSL_VERSION_NUMBER
format, the OPENSSL_VERSION_NUMBER(3) documents were helpful for me. You can check each OpenSSL version's format by clicking the master, 3.1, 3.0, 1.1.1, 1.0.2 links. For adding theOpenSSL::LIBRESSL_VERSION_NUMBER
, perhaps it is controversial. Because thetest/openssl/utils.rb#libressl?
doesn't depend on theLIBRESSL_VERSION_NUMBER
. But I thought it was useful to debug C files.OpenSSL::OPENSSL_VERSION_NUMBER
comment explaining the format.OpenSSL::LIBRESSL_VERSION_NUMBER
. It's useful to debug the C language code. Notetest/openssl/utils.rb#libressl?
is not using this value.rake debug
to printOpenSSL::OPENSSL_VERSION_NUMBER
andOpenSSL::LIBRESSL_VERSION_NUMBER.