Fixed OpenSSL bindings to work with LibreSSL #6917
Conversation
|
The fail on build 12969 seems irrelevant. Ready for a review :) |
|
Build restarted. I would prefer to use |
Is there a string representation of the library version available that would fit with |
|
There is a string representation of the library version inside the same header file. I could use the MAJOR.MINOR.PATCH parts of both if you prefer, but I think |
|
Even if there is no string representation, I don't see the value of using custom integer value vs initializing manually a string semver value. It's better to keep one single idiom for version. So either we needed to extract the semver from |
|
The official version check in OpenSSL is the hexadecimal check. As ugly as it may look, we should follow it. What really bothers me is the amount and complexity of macro run calls to extract 2 definitions from the C headers :-( |
|
I reworked the checks to work with Edit: Let's try to |
|
The second solution seems to pass too. It's a bit simpler as it uses the header file only to check for LibreSSL and uses Both commits are valid solutions, pick your favorite :) |
src/openssl/lib_ssl.cr
Outdated
| OPENSSL_VERSION = "0.0.0" | ||
| {% else %} | ||
| LIBRESSL_VERSION = "0.0.0" | ||
| OPENSSL_VERSION = {{ `command -v pkg-config > /dev/null && pkg-config --silence-errors --modversion libssl || printf %s 0.0.0`.split.last.gsub(/[^0-9.]/, "") }} |
There was a problem hiding this comment.
hash pkg-config 2>/dev/null is shorter and has the same effect than command -v pkg-config >/dev/null.
You can also do this command once, before {% if from_libressl %}:
{% ssl_version = `hash pkg-config 2>/dev/null && pkg-config --silence-errors --modversion libssl || printf %s 0.0.0`.split.last.gsub(/[^0-9.]/, "") %}
And then:
LIBRESSL_VERSION = {{ ssl_version }}
OPENSSL_VERSION = {{ ssl_version }}
There was a problem hiding this comment.
Yeah, you're right. I remember thinking about storing the libssl version last night, wonder why it slipped my mind later. I will do that when I get on my computer.
src/openssl/lib_crypto.cr
Outdated
| OPENSSL_102 = {{ `command -v pkg-config > /dev/null && pkg-config --atleast-version=1.0.2 libcrypto || printf %s false`.stringify != "false" }} | ||
| {% from_libressl = (`hash pkg-config 2> /dev/null || printf %s false` != "false") && | ||
| (`test -f $(pkg-config --silence-errors --variable=includedir libcrypto)/openssl/opensslv.h || printf %s false` != "false") && | ||
| (`printf %s "#include <openssl/opensslv.h>\nlibressl_version_number" | #{(env("cc") || "cc").id} #{`pkg-config --cflags --silence-errors libcrypto || true`.chomp} -E -`.chomp.split('\n').last == "libressl_version_number") %} |
There was a problem hiding this comment.
perhaps use $(pkg-config ...) here instead of #{`pkg-config ...`}
There was a problem hiding this comment.
#{(env("cc") || "cc").id} could be inserted directly into the command, as well.
There was a problem hiding this comment.
Maybe the entire boolean expression could just be evaluated in the shell... I'm not sure if that brings any benefits but it would avoid conceptually switching between shell and macro interpreter back and forth.
|
The fail on build 13549 seems irrelevant to me. Is it from master? |
|
@LVMBDV no it's just a transient OOM error, I've restarted the build. |
|
Tested on my machine, this works well. Has this been tested on alpine? |
Somewhat agreed, perhaps it's worth adding 3rd argument which would specify the operator for comparison, turning your example into: |
|
To test this features, it would be really nice to have an Alpine Linux CI when the language is compiled with:
|
pkg-configif the header files are inaccessible.OPENSSL_110with actual version number values e.g.OPENSSL_VERSIONis0x10102000when its version is1.1.2.pkg-config, we assume OpenSSL is installed and set the version values accordingly. Although this behavior is a bit iffy for systems that have LibreSSL installed without the headers but the current behavior also works exactly like this AFAIU.