Support OpenSSL 1.1.0 #2415

winksaville · 2017-12-13T01:26:34Z

This resolves issue #1838

winksaville · 2017-12-18T18:08:11Z

This should not be failing in Travis, the default is basically a "NOP" if OpenSSL 1.1 is not installed. Which I don't believe it is on and of the Travis enviornments. It does work for me on Arch Linux where OpenSSL 1.1 is installed.

In anycase it needs to be reviewed and I also need to create some tests, as such I'd like this to be marked with "Do not merge" for now.

EpicEric · 2017-12-18T20:36:40Z

The Travis failures have to do with network issues with the jobs, so this is likely not the reason for these errors.

I'll still try to get around testing this.

SeanTAllen · 2017-12-20T21:02:07Z

@jemc marking as needs discussion. want to talk this over with you. perhaps next sync.

winksaville · 2017-12-21T01:43:09Z

SG, I'll make my best attempt to attend the next sync.

mfelsche · 2017-12-27T22:24:58Z

Before we release this, we should add another part to our travis test runs, that tests the stdlib against openssl 1.1.0.

mfelsche · 2017-12-27T22:43:39Z

The only downside of this approach is that it adds the burden on the application programmer to define "OPENSSL_API_COMPAT_1_1_0" for every compile run of his pony program.

It would be nice if the detection of which ssl library is available and would be picked up by FFI could be done in the compiler so we can use it in ifdef statements to chose the right function.

mfelsche · 2017-12-28T20:42:55Z

We agreed on going with this approach for unlocking.
We would love to have one define called openssl_1.1.0 that corresponds to the current OPENSSL_API_COMPAT_1_1_0.

The automatic detection of the openssl version does not work for every platform and it is only necessary in the Makefile rules that compile the stdlib for testing, thus it should not error if no version could be detected.

The idea is to require the user to add a -Dopenssl_1.1.0 in case he wants to link against OpenSSL 1.1.0 on his platform and to also document this prominently both in the stdlib docs of the net/ssl and the crypto packages and in the tutorial.

A failsafe automatic OpenSSL version detection for convenience should be subject of another PR and discussion.

Can we ask you to give the doc part a try? If not, I could chime in to help here.

winksaville · 2017-12-28T22:30:36Z

I'll make the changes to the code today and follow with changes to the docs afterwards.

winksaville · 2017-12-29T02:46:04Z

So a first pass at trying to add a compiler ifdef, I'm not sure I've got this right but its "working" for me. I tried to define openssl_1.1.0 but I got a syntax error so I switched to openssl_1_1_0:

Linking libponyrt.benchmarks
build/release/ponyc -DOPENSSL_1.1.0 --checktree --verify packages/stdlib
Building builtin -> /home/wink/prgs/pony/ponyc/packages/builtin
Error:
/home/wink/prgs/pony/ponyc/packages/builtin/platform.pony:20:16: syntax error: expected ( after openssl_1
  fun openssl_1.1.0(): Bool => compile_intrinsic
               ^

winksaville · 2017-12-29T06:50:35Z

So at the moment openssl_1_1_0 is always defined, so I've done something totally wrong. Guidance appreciated.

mfelsche · 2017-12-29T07:54:55Z

Sorry if i was misleading.
I actually meant, changing the ifdef string value to "openssl_1.1.0" and set it accordingly in the makefile as a ponyc define with "-D" as you do right now with "OPENSSL_API_COMPAT_1_1_0". but only add it as define in the makefile if you detected openssl 1.1.0 and otherwise add no define.

winksaville · 2017-12-29T11:10:07Z

Ok, so as I understand it, the ifdef in the code would remain a string but would be defined as: ''' -D "openssl_1.1.0" '''' And tested with: ''' Ifdef "openssl_1.1.0" then foo.. else bar.. end '''

…

On Thu, Dec 28, 2017, 11:54 PM Matthias Wahl ***@***.***> wrote: Sorry if i was misleading. I actually meant, changing the ifdef string value to "openssl_1.1.0" and set it accordingly in the makefile as a ponyc define with "-D" as you do right now with "OPENSSL_API_COMPAT_1_1_0". but only add it as define in the makefile if you detected openssl 1.1.0 and otherwise add no define. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2415 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA-hHMosKWOt2yF0h5_MORIyHgSUscPhks5tFJrRgaJpZM4Q_4Lg> .

mfelsche · 2017-12-29T12:13:48Z

correct.

winksaville · 2017-12-29T19:27:58Z

@mfelsche, please see my latest changes and let me know what other changes you like to the code.

winksaville · 2018-01-05T01:16:47Z

I've made changes to README.md and rebased on master, squashed the commits. As requested I've got a PR for the FAQ page in the pony website.

winksaville · 2018-01-11T18:41:20Z

I needed to suppress the unused function error when default_openssl wasn't being used.

dipinhora · 2018-01-12T06:14:12Z

Makefile

@@ -859,10 +869,10 @@ benchmark: all
 	@$(PONY_BUILD_DIR)/libponyrt.benchmarks

 stdlib-debug: all
-	$(PONY_BUILD_DIR)/ponyc -d --checktree --verify packages/stdlib
+	$(PONY_BUILD_DIR)/ponyc $(OPENSSL) -d --checktree --verify packages/stdlib


The $(OPENSSL) argument to ponyc should no longer be required due to the use of default_openssl.

The use of default_openssl is optional so i believe these are still needed.

I lean towards recommending people use default_openssl similar to default_pic and removing these extra arguments but I don't feel very strongly about it.

I leave it to you and other other folks who decide to weigh in to make a decision about what to do.

I agree, its not needed if you assume you don't want change the value when using the Makefile. But one use I had was to use the Makefile to build the compiler and with a target of test-stdlib using default_openssl=openssl_1.1.0. This would compile the compiler and stdlib and then run it. I can then use the Makefile again to compile only stdlib using OPENSSL=-Dopenssl_0.9.0 to test that stdlib would not compile.

Probably not a normal case, but I have used it. I don't have a strong opinion either, but given that this is working and has some uses I'm inclined to leave.

dipinhora · 2018-01-12T06:14:51Z

src/libponyc/pkg/buildflagset.c

+  pony_assert(flags != NULL);
+
+  if(_user_flags == NULL)
+  {


It seems as though you should be able to return false; here if _user_flags == NULL since that means no user flags have been defined.

Yea, I actually coded it that way, but then thought its possible the initialization could potentially add flags and thought it best not to assume anything and just fall through.

No, initialization doesn't add any flags. All it does is initialize the hashmap. See: https://github.com/ponylang/ponyc/blob/master/src/libponyrt/ds/hash.c#L180-L202 for what flagtab_init does.

Note: I should clarify that the way the hashmap implementation works there's a bunch of template macros which effectively alias the hashmap functions for a given type/name. If you find a DECLARE_HASHMAP(flagtab, flagtab_t, flag_t); with the same prefix then that is what is happening and you can look up the relevant hashmap functions to see what is happening if you need to.

I absolutely agree that now we could return early, but I see no downside to letting the normal processing proceed. I also tend to like code that has only one exit point.

Okey, in that case you can change this logic to be if(_user_flags != NULL) { for... } and keep a single exit point. Or you can keep things as they are. Like the other item, I don't feel strongly about it........

Then unless overriden by higher powers, I'll leave it as is :)

dipinhora · 2018-01-12T06:16:01Z

src/libponyc/pkg/buildflagset.c

+  {
+    flag_t f1 = {stringtab(*next), false};
+    size_t index = HASHMAP_UNKNOWN;
+    flag_t* f2 = flagtab_get(_user_flags, &f1, &index);


You can replace the flagtab_get and flagtab_removeindex combination with flagtab_remove instead (see here for header definition of ponyint_hashmap_remove function: https://github.com/ponylang/ponyc/blob/master/src/libponyrt/ds/hash.h#L83-L88). flagtab_remove will return NULL if the element wasn't found or else will return the element. Please also remember to free the element after removing it to avoid a memory leak.

dipinhora · 2018-01-13T03:16:17Z

src/libponyc/pkg/buildflagset.c

+  for(const char** next = flags; *next != NULL; next += 1)
+  {
+    flag_t f1 = {stringtab(*next), false};
+    if(flagtab_remove(_user_flags, &f1) != NULL)


You need to save the return flag_t* from flagtab_remove and then free it via flag_free if it is not NULL to avoid a memory leak.

Good catch, fixing.

dipinhora · 2018-01-13T03:18:21Z

src/libponyc/options/options.c

+          if(validate_openssl_flag(s->arg_val))
+          { // User wants to add an openssl_flag,
+            // remove any existing openssl_flags.
+            remove_build_flags(valid_openssl_flags);


This line can be wrapped in an #if defined(PONY_DEFAULT_OPENSSL) since there wouldn't be a default openssl build flag to remove otherwise.

I'm in of favor minimizing assumptions and not adding conditionals if they aren't needed. So I'd rather leave it as is.

Okey dokey. Works for me.

dipinhora · 2018-01-13T03:21:40Z

Makefile

+  ifeq (ok,$(OPENSSL_VALID))
+    $(warning targeting openssl $(OPENSSL))
+  else
+    $(error OPENSSL=$(OPENSSL) is invalid, expecting one of -Dopenssl_0.9.0 or -Dopenssl_1.1.0)


This is likely to be confusing if someone specifies an invalid default_openssl value but the error message says that OPENSSL is invalid.

Good point, fixing.

winksaville · 2018-01-13T18:14:32Z

@dipinhora et al. I've made some additional changes. First, I slept on the discussion with Dipin about OPENSSL and decided it should be removed. And second, did some cleanup of default pic and openssl handling, I think the changes improve the PR but your opinions may differ :)

Please review again.

If your system has openssl 1.1.0 and you need to use the crypto or ssl packages you must define openssl_1.1.0 when invoking ponyc. Specifically, there are ifdef "openssl_1.1.0" statements in packages/crypto and packages/net/ssl which can use the openssl 1.1.0 API's. When compiling using ponyc/Makefile and targeting various tests you need to set OPENSSL=-Dopenssl_1.1.0. For instance, if targeting "test" the command line would be: make OPENSSL=-Dopenssl_1.1.0 test If you are compiling pony programs using ponyc in your own Makefile or directly on the command line and the program uses crypto or ssl then pass -Dopenssl_1.1.0 on the ponyc command line. For instance, to compile stdlib: ./build/release/ponyc -Dopenssl_1.1.0 packages/stdlib This resolves issue [ponylang#1838](ponylang#1838)

When compiling ponyc if default_openssl=openssl_1.1.0 or default_openssl=openssl_0.9.0 is passed on the make command line then that parameter will becomce the default when compiling packages/crypto or packages/net/ssl. This default maybe overridden by passing -Dopenssl_1.1.0 or -Dopenssl_0.9.0 on the ponyc command line.

Now openssl is validated in ponyc_opt_process and in the Makefile making it more likely to catch typos.

Dipin made a good suggestion to use flagtab_remove instead of flagtab_get and flagtab_removeindex.

Improve the error reporting in the Makefile for default_openssl and OPENSSL. Fix a memory leak when removing a build flag.

After sleeping on the discussion with Dipin and realizing that as I had it coded OPENSSL and default_openssl were mutually exclusive because default_openssl was overwriting OPENSSL, I think its simplest to just remove OPENSSL.

- Allow default_pic to be true or false and report an error in the Makefile if it isn't. - Add openssl to ponyc usage - Print pic and openssl defaults when printing version: $ ./build/release/ponyc --version 0.21.2-78fc1a87 [release] compiled with: llvm 5.0.1 -- cc (GCC) 7.2.1 20171224 Defaults: pic=true openssl=openssl_1.1.0

Remove the mention of the ponyc-rpm workaround Remove mention of pic errors and instead have command line provide default_pic and default_openssl.

winksaville · 2018-01-15T02:27:33Z

I created an issue for updating the FAQ when this is merged or another solution is found.

SeanTAllen · 2018-01-17T21:22:40Z

This looks good. @winksaville is going to let me know when he opens a PR to update the FAQ on the website and i'll merge both at the same time.

winksaville · 2018-01-17T22:27:52Z

I've got PR 235 for the FAQ.

winksaville force-pushed the support-openssl1.1 branch 3 times, most recently from 3bde61d to 230d7e6 Compare December 17, 2017 06:59

winksaville force-pushed the support-openssl1.1 branch from 230d7e6 to 64b64a2 Compare December 19, 2017 22:59

SeanTAllen added the needs discussion during sync label Dec 20, 2017

winksaville force-pushed the support-openssl1.1 branch from 64b64a2 to 018eaec Compare December 26, 2017 00:09

mfelsche mentioned this pull request Dec 27, 2017

Add Support for OpenSSL 1.1.0 #1838

Closed

winksaville force-pushed the support-openssl1.1 branch from 018eaec to 648b681 Compare December 28, 2017 19:20

winksaville force-pushed the support-openssl1.1 branch from 648b681 to 586e060 Compare December 29, 2017 02:41

winksaville force-pushed the support-openssl1.1 branch from 586e060 to 314ff9e Compare December 29, 2017 19:21

winksaville force-pushed the support-openssl1.1 branch 2 times, most recently from 0d35e17 to ce51a97 Compare January 2, 2018 22:24

SeanTAllen removed the needs discussion during sync label Jan 3, 2018

winksaville force-pushed the support-openssl1.1 branch from ce51a97 to 66a1ea1 Compare January 5, 2018 01:11

SeanTAllen added the changelog - added Automatically add "Added" CHANGELOG entry on merge label Jan 5, 2018

winksaville force-pushed the support-openssl1.1 branch from ffa7afb to 77f6d41 Compare January 11, 2018 18:37

winksaville force-pushed the support-openssl1.1 branch from 77f6d41 to dc111d6 Compare January 11, 2018 20:13

dipinhora reviewed Jan 12, 2018

View reviewed changes

winksaville force-pushed the support-openssl1.1 branch from dc111d6 to 825ce5f Compare January 12, 2018 18:55

dipinhora reviewed Jan 13, 2018

View reviewed changes

winksaville added 8 commits January 14, 2018 17:43

Change validation of openssl flags

3e31945

Now openssl is validated in ponyc_opt_process and in the Makefile making it more likely to catch typos.

Simplify remove_build_flags

47c5888

Dipin made a good suggestion to use flagtab_remove instead of flagtab_get and flagtab_removeindex.

Changes from Dipin's code review.

506765d

Improve the error reporting in the Makefile for default_openssl and OPENSSL. Fix a memory leak when removing a build flag.

Remove OPENSSL

e394896

After sleeping on the discussion with Dipin and realizing that as I had it coded OPENSSL and default_openssl were mutually exclusive because default_openssl was overwriting OPENSSL, I think its simplest to just remove OPENSSL.

Update README.md

b586b41

Remove the mention of the ponyc-rpm workaround Remove mention of pic errors and instead have command line provide default_pic and default_openssl.

winksaville force-pushed the support-openssl1.1 branch from 06ca4a2 to b586b41 Compare January 15, 2018 02:10

winksaville mentioned this pull request Jan 15, 2018

Update "FAQ" concerning undefined references to ssl symbols ponylang/ponylang-website#234

Closed

This was referenced Jan 17, 2018

undefined reference to ssl symbols when compiling with net/http package #2500

Closed

Add stdlib tests against OpenSSL 1.1.0 on CI #2501

Closed

winksaville mentioned this pull request Jan 17, 2018

Update FAQ with mentioning default_ssl support. ponylang/ponylang-website#235

Merged

SeanTAllen merged commit 3ee8fc4 into ponylang:master Jan 18, 2018

ponylang-main added a commit that referenced this pull request Jan 18, 2018

Update CHANGELOG for PR #2415 [skip ci]

c8bbed1

winksaville deleted the support-openssl1.1 branch January 18, 2018 18:30

Support OpenSSL 1.1.0 #2415

Support OpenSSL 1.1.0 #2415

Conversation

winksaville commented Dec 13, 2017

winksaville commented Dec 18, 2017

EpicEric commented Dec 18, 2017

SeanTAllen commented Dec 20, 2017

winksaville commented Dec 21, 2017

mfelsche commented Dec 27, 2017

mfelsche commented Dec 27, 2017

mfelsche commented Dec 28, 2017

winksaville commented Dec 28, 2017

winksaville commented Dec 29, 2017

winksaville commented Dec 29, 2017

mfelsche commented Dec 29, 2017

winksaville commented Dec 29, 2017 via email • edited Loading

mfelsche commented Dec 29, 2017

winksaville commented Dec 29, 2017

winksaville commented Jan 5, 2018

winksaville commented Jan 11, 2018

dipinhora Jan 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winksaville Jan 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winksaville commented Jan 13, 2018

winksaville commented Jan 15, 2018

SeanTAllen commented Jan 17, 2018

winksaville commented Jan 17, 2018

winksaville commented Dec 29, 2017 via email •

edited

Loading

dipinhora Jan 12, 2018 •

edited

Loading

winksaville Jan 13, 2018 •

edited

Loading