-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Restore libressl support and add CI for that #11400
Restore libressl support and add CI for that #11400
Conversation
src/openssl/lib_crypto.cr
Outdated
@@ -131,7 +131,7 @@ lib LibCrypto | |||
fun obj_obj2nid = OBJ_obj2nid(obj : ASN1_OBJECT) : Int | |||
fun obj_ln2nid = OBJ_ln2nid(ln : Char*) : Int | |||
fun obj_sn2nid = OBJ_sn2nid(sn : Char*) : Int | |||
{% if compare_versions(OPENSSL_VERSION, "1.0.2") >= 0 %} | |||
{% if compare_versions(OPENSSL_VERSION, "1.0.2") >= 0 || compare_versions(LIBRESSL_VERSION, "0.0.0") > 0 %} |
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.
Why "0.0.0"?
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.
This function should be available in all versions of libressl.
Could've picked the earliest libressl release (which is 2.0.0 AFAIK). But 0.0.0
makes it more obvious what this is about.
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.
But
0.0.0
makes it more obvious what this is about.
@straight-shoota It's rather pretty cryptic, I guess that was the main reason @beta-ziliani asked you about it :D
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.
It's fine, the relevant question is if we're compatible with all versions, and IIUC the answer is "yes"
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.
IMO more clear expression would be sth like !LIBRESSL_VERSION.empty?
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.
@Sija LIBRESSL_VERSION
is never empty. "0.0.0"
is the indicator for not being present: https://github.com/straight-shoota/crystal/blob/e813d044894e592171843f38af6d9c7f113a9d8f/src/openssl/lib_crypto.cr#L12
This allows using compare_versions
anywhere.
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.
I know, how about defining it in a constant LIB_VERSION_ANY
?
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.
LIBRESSL_VERSION != "0.0.0"
then - makes it immediately understandable what's the rationale behind it.
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.
@Sija sounds good.
@beta-ziliani That would complicate things A LOT. We need different paths for OpenSSL and LibreSSL. For example, most of the function names that changed in OpenSSL 1.1.0 or 3.0.0 still have the original names in LibreSSL 3.4.
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.
I was just saying to replace "0.0.0" by the constant. But never mind, it's clear now.
This patch improves
OpenSSL
support for linking against libressl. Bindings to some functions were missing for libressl. Together with #11374 this enables some missing functionality.But most of the changes are about fixing some specs to comply with libressl behaviour.
To ensure libressl support doesn't break in the future, new jobs are added to the openssl CI workflow (introduced in #11360) to test against libressl 3.1 and 3.4. These versions are easiest to obtain on the crystal alpine docker image.