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.

Sign up for GitHub

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

Jump to bottom

Fix: compilation against OpenSSL 1.1.0 #4215

Merged
merged 1 commit into from
Mar 31, 2017
Merged

Fix: compilation against OpenSSL 1.1.0 #4215

merged 1 commit into from
Mar 31, 2017

Conversation

ysbaddaden
Copy link
Contributor

@ysbaddaden ysbaddaden commented Mar 30, 2017 •
edited
Loading

OpenSSL 1.1.0 introduced some breaking changes:

  • Initialization is now automatically handled (and init/load strings methods removed or renamed);
  • TLS_method has been added while SSLv23_method was removed.

fixes #4204

@jhass jhass self-requested a review March 30, 2017 08:35
jhass
jhass reviewed Mar 30, 2017
View reviewed changes
src/openssl/lib_crypto.cr Outdated
@@ -1,5 +1,8 @@
@[Link(ldflags: "`command -v pkg-config > /dev/null && pkg-config --libs libcrypto || printf %s '-lcrypto'`")]
lib LibCrypto
OPENSSL_102 = {% `command -v pkg-config > /dev/null && pkg-config --atleast-version 1.0.2 && printf %s true` == "true" %}
OPENSSL_110 = {% `command -v pkg-config > /dev/null && pkg-config --atleast-version 1.1.0 && printf %s true` == "true" %}
Copy link
Member

@jhass jhass Mar 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually the command is missing libcrypto, pkg-config libcrypto --atleast-version ....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I fixed the pkg-config commands, I hope they're fine, now.

jhass
jhass reviewed Mar 30, 2017
View reviewed changes
src/openssl/lib_ssl.cr Outdated
@@ -2,6 +2,9 @@ require "./lib_crypto"

@[Link(ldflags: "`command -v pkg-config > /dev/null && pkg-config --libs libssl || printf %s '-lssl -lcrypto'`")]
lib LibSSL
OPENSSL_102 = {% `command -v pkg-config > /dev/null && pkg-config --atleast-version=102 libssl && printf %s true` == "true" %}
OPENSSL_110 = {% `command -v pkg-config > /dev/null && pkg-config --atleast-version=110 libssl && printf %s true` == "true" %}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto with s/libcrypto/libssl/, also I wonder whether those should be --exact-version or --max-version.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I chose to have greater-than / lower-than triggers, which won't break on a new OpenSSL version (unless they removed symbols).

@ysbaddaden ysbaddaden force-pushed the std-fix-openssl-110 branch from a1ced87 to be8083e Compare March 30, 2017 12:48
jhass
jhass reviewed Mar 30, 2017
View reviewed changes
src/openssl/lib_crypto.cr Outdated
@@ -1,5 +1,8 @@
@[Link(ldflags: "`command -v pkg-config > /dev/null && pkg-config --libs libcrypto || printf %s '-lcrypto'`")]
lib LibCrypto
OPENSSL_102 = {% `command -v pkg-config > /dev/null && pkg-config --atleast-version=1.0.2 libcrypto && printf %s true` == "true" %}
OPENSSL_110 = {% `command -v pkg-config > /dev/null && pkg-config --atleast-version=1.1.0 libcrypto && printf %s true` == "true" %}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Figured out why CI fails, the assignment this way is runtime and thus always truthy at compile time, since it expands to the AST node. Doing an assignment to a constant at compile time seems to be not (no longer?) possible :(

jhass
jhass reviewed Mar 30, 2017
View reviewed changes
src/openssl/lib_crypto.cr
lib LibCrypto
OPENSSL_102 = false
end
{% end %}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I figured that out in the past and that's where this workaround comes from.

Copy link
Contributor Author

@ysbaddaden ysbaddaden Mar 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oookay... I'll revert and add a note about this trick so I won't​ inadvertently remove it again.

Thanks for the input!

@ysbaddaden ysbaddaden force-pushed the std-fix-openssl-110 branch 2 times, most recently from 8bfb10f to 0020328 Compare March 31, 2017 08:58
@ysbaddaden ysbaddaden force-pushed the std-fix-openssl-110 branch from 0020328 to 1658c31 Compare March 31, 2017 13:52
@ysbaddaden
Copy link
Contributor Author

ysbaddaden commented Mar 31, 2017 •
edited
Loading

After compiling different OpenSSL versions and a session of trial and error, I found out how to make things work:

  1. have a separate lib definition for versions, surrounded by {% begin %} {% end %} so it will be evaluated before a second lib definition, that contains the checks;
  2. make sure the backtick commands returns a success (exit code 0), hence the negative atleast || echo false.

So, now, I tested against OpenSSL 1.0.1, 1.0.2 and 1.1.0.

@jhass jhass merged commit a36e2b4 into crystal-lang:master Mar 31, 2017
@jhass
Copy link
Member

jhass commented Mar 31, 2017 •
edited
Loading

❤️ thank you!

@ysbaddaden ysbaddaden deleted the std-fix-openssl-110 branch March 31, 2017 15:53
@bcardiff bcardiff added this to the 0.22.0 milestone May 9, 2017
@nobilik nobilik mentioned this pull request Jul 3, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jhass jhass jhass approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.22.0
Development

Successfully merging this pull request may close these issues.

Fails to compile on OpenSSL 1.1.0
3 participants
@ysbaddaden @jhass @bcardiff