I just realized the LIBRESSL_VERSION_NUMBER macro will return an empty string when OpenSSL is used. I need to fix that :o

I think I fixed it. Ready for another review :)

Thanks!

Maybe we could add some LIBRESSL_VERSION checks to enable some features, for example ALPN support was added in LibreSSL 2.5.0 (https://github.com/libressl-portable/openbsd/blob/libressl-v2.5.0/src/lib/libcrypto/opensslv.h).

@ysbaddaden another PR, please. Let's make it compile first before we add new libressl-spcific features.

Also, having library-specific features is absolutely the opposite of the direction I want to do. I want to remove the OpenSSL and make a library-inspecific, lowest common denominator, easy to use TLS module which works with openssl, libressl, libtls, gnutls, etc.

FYI, CI fails on OS X with <stdin>:1:10: fatal error: 'openssl/opensslv.h' file not found.

@RX14 Please reactor SSL in crystal, that would be awesome! Yet, there is no other way around the issue right now (and for the months to come).

Fixing LibreSSL compatibility is great, enabling ALPN on LibreSSL would be even better, doing it in this pull request be awesome.

I agree with @RX14 on the scope of this PR. I would like it merged until the next release so I can shave some lines off the Void Linux package for Crystal :)

@Sija I don't know anything about where library headers are located in OS X. Could you explain?

@ysbaddaden is alpn available in openssl? Do you mean you just need a different API to enable ALPN in libressl? In that case then yes, that's a good idea and would fit in this PR (but not required).

@LVMBDV It should be possible to get this to work by installing openssl using brew (if not already):

brew install openssl

and then setting the CPPFLAGS and LDFLAGS to point to the openssl lib from brew:

export CPPFLAGS=-I/usr/local/opt/openssl/include
export LDFLAGS=-L/usr/local/opt/openssl/lib

@Sija so everyone on mac is just going to get the same cryptic error as CI does unless you run a command nobody knows about? A solution that passes CI isn't enough. We should fall back to the old way if this way fails.

@Sija so everyone on mac is just going to get the same cryptic error as CI does unless you run a command nobody knows about? A solution that passes CI isn't enough.

Chill your boots dude. My comment refers to running this snippet on Mac OSX with openssl installed and was meant to help with this specific case, not all of 'em.

We should fall back to the old way if this way fails.

I agree, yet it's IMO different case; to which you haven't provided solutions either, yet.

@cc is replaced with an environment variable friendly alternative as @luislavena suggested.
+ "0" is removed from OPENSSL_VERSION as its #define will always be there since this is about openssl in the first place.

About the Mac issue, I think Crystal as a mere compiler is not responsible for finding include paths. I'm sure developers who use Macs are well aware of the platform's eccentricities. Also openssl not being installed is not crystal's problem either, it just has the binding for it in case you want to use openssl with it.

P.S. I guess crystal tool format got really mad at that one space I left at the end of those system() calls. It passed std_spec.

Fixed that one space, Travis is happy now. Ready for another review :)

@RX14 ALPN is in OpenSSL >= 1.0.2 and LibreSSL >= 2.5.0 with the same public API, and we already support it in Crystal for OpenSSL >= 1.0.2. It would be very kind to enable it for LibreSSL too. See: https://github.com/crystal-lang/crystal/blob/master/src/openssl/ssl/context.cr#L308

But beware of https://github.com/crystal-lang/crystal/blob/master/src/openssl/ssl/context.cr#L341 (and below) that may not be supported in LibreSSL.

@ysbaddaden ah, thanks, that makes a lot more sense. I thought you were proposing ALPN as a new feature, I was unaware it was already supported. My bad.

1. Either way we need to fix CI
2. I think that we should at the very least provide a better error message than this if we're going to fail to compile on OSX without the headers installed (common).
3. Should we just assume there's no libressl on osx and bypass this issue?

I really don't want to deal with the hundreds of bug reports we'll get if we merge this and everyone on OSX suddenly starts getting these errrors.

Fixed the header problem for OS X but a new one popped up. I think crystal is linking against system's (older) openssl libs while using brew's openssl headers (which are newer). This is mentioned briefly in this blog post. I'm trying to fix it while looking at results over at CircleCI, might take slightly longer than usual since I have to wait for it :)

P.S. I might look at supporting ALPN for LibreSSL as well, after this.

All checks passed! I was right about brew linking the system openssl instead of brew's more recent one. It must have been that way before I even started working on this. Anyways, I'll work on ALPN support with LibreSSL tomorrow.

Replaced hardcoded paths in darwin blocks with brew --prefix with /usr/local as a fallback.

Newest Mac OSX (High Sierra) comes with updated ssl/crypto libraries, there's no need (and in fact it might be wrong) to use homebrew for this.

I just tried changing lib LibSSL in mac to this:

@[Link("ssl")]
@[Link("crypto")]
lib LibSSL
  # ...
end

and it works fine.

But this didn't work well with a previous Mac version, so I don't know what advice to give on that. Maybe force users to update to the latest Mac OSX version is good.

@LVMBDV you can use brew --prefix openssl to obtain a full path.

@Sija Can I have the output of brew --prefix openssl on your mac?
@asterite Can I have the output of llvm-config --host-target on your (High Sierra) mac?

I tried my hand at ALPN support, it passes std_spec but I don't know enough about it to test it myself.

Moreover, I found out that Apple switched to LibreSSL as system default at High Sierra so I need to figure out a way to suppress the brew checks if the system is newer than or equal to High Sierra.

EDIT: OR modify behaviour with a compile time flag like OPENSSL_BREW instead, so users can choose which one to use.

A warning can be printed to let the user know about this issue.

{{ p "OpenSSL bindings may fail on older (< 10.13) OS X versions without a homebrew openssl. In such a case, install openssl and set OPENSSL_BREW (i.e. crystal build -D OPENSSL_BREW)." if flag?(:darwin) && !flag?(:OPENSSL_BREW) }}

Should be using brew be the default behaviour or vice versa? CircleCI uses El Capitan so the build will fail if the default is not using brew, so will the rest of Mac users with older versions.

I think you missed my comment that homebrew shouldn't be used for this.

Oh, sorry, nevermind

@LVMBDV here you go:

~ ❯ brew --prefix openssl
/usr/local/opt/openssl

Rebased but the deleted circle.yml contained a line to prepend the brew openssl directory to PKG_CONFIG_PATH, circleci fails without that.

@LVMBDV the circle.yml is now in .circleci/config.yml, but perhaps you can edit bin/ci instead?

Added the openssl path to PKG_CONFIG_PATH on bin/ci as @RX14 suggested :)

Seems like GTG, merge?

LGTM 👍

I compiled Crystal with this change, then tried to compile my program, got this:

Using compiled compiler at `.build/crystal'
_main.o: In function `__crystal_main':
/home/blaxpirit/repos/crystal/src/openssl/lib_ssl.cr:218: undefined reference to `SSL_library_init'
/home/blaxpirit/repos/crystal/src/openssl/lib_ssl.cr:218: undefined reference to `SSL_load_error_strings'
/home/blaxpirit/repos/crystal/src/openssl/lib_ssl.cr:218: undefined reference to `OPENSSL_add_all_algorithms_noconf'
/home/blaxpirit/repos/crystal/src/openssl/lib_ssl.cr:218: undefined reference to `ERR_load_crypto_strings'
O-penS-S-L-5858S-S-L-5858C-ontext.o: In function `default_method':
/home/blaxpirit/repos/crystal/src/openssl/ssl/context.cr:4: undefined reference to `SSLv23_method'
collect2: error: ld returned 1 exit status
Error: execution of command failed with code: 1: `cc "${@}" -o '/home/blaxpirit/projects/critter/critter'  -rdynamic  -lz `command -v pkg-config > /dev/null && pkg-config --libs --silence-errors libssl || printf %s '-lssl -lcrypto'` `command -v pkg-config > /dev/null && pkg-config --libs --silence-errors libcrypto || printf %s ' -lcrypto'` -lpcre -lm -lgc -lpthread /home/blaxpirit/repos/crystal/src/ext/libcrystal.a -levent -lrt -ldl -L/usr/lib -L/usr/local/lib`

I confirmed that this is the culprit

LLVM 6.0 on Arch Linux

Ugh, I kinda expected this.

Per @RX14's request

$ bin/crystal eval 'require "openssl"; {% pp LibCrypto::OPENSSL_VERSION, LibCrypto::LIBRESSL_VERSION %}'
Using compiled compiler at `.build/crystal'
LibCrypto::OPENSSL_VERSION  # => 269484175
LibCrypto::LIBRESSL_VERSION # => 0
_main.o: In function `__crystal_main':
/home/blaxpirit/repos/crystal/src/openssl/lib_ssl.cr:218: undefined reference to `SSL_library_init'
/home/blaxpirit/repos/crystal/src/openssl/lib_ssl.cr:218: undefined reference to `SSL_load_error_strings'
/home/blaxpirit/repos/crystal/src/openssl/lib_ssl.cr:218: undefined reference to `OPENSSL_add_all_algorithms_noconf'
/home/blaxpirit/repos/crystal/src/openssl/lib_ssl.cr:218: undefined reference to `ERR_load_crypto_strings'
collect2: error: ld returned 1 exit status
Error: execution of command failed with code: 1: `cc "${@}" -o '/tmp/cache-blaxpirit/crystal/crystal-run-eval.tmp'  -rdynamic  `command -v pkg-config > /dev/null && pkg-config --libs --silence-errors libssl || printf %s '-lssl -lcrypto'` `command -v pkg-config > /dev/null && pkg-config --libs --silence-errors libcrypto || printf %s ' -lcrypto'` -lpcre -lgc -lpthread /home/blaxpirit/repos/crystal/src/ext/libcrystal.a -levent -lrt -ldl -L/usr/lib -L/usr/local/lib`