Created attachment 170726 [details] patch from a git commit, apply with patch -p1 Do not statically link bundled libraries into node.js. It requires openssl 1.0.2, so pull that in from ports when the base version is not new enough. While all versions use c-ares, only www/node has a configure knob to enable building against an unbundled version. All other dependencies are specified in www/node/common.mk and used by all 3 ports. This removes the BUNDLED_SSL option from the www/node5 port. This option was accidentially included when the port was created. The www/node010 and www/node012 ports have not been done since both are deprecated and have very old bundled dependencies that are not available in ports.
Wait, so now if you have LibreSSL installed you cannot have node at all? That's a terrible idea. Lots of us use LibreSSL. Please, turn it back into an OPTION.
That's correct. I had thought this was the intent behind comment 11 in PR 203528, https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=203528#c11, but I understand now that I missed the point about wanting both libressl and node.js at the same time. How about the knob defaulting to off, so that normal package builds and people not using libressl will get the external dependency? Or an implicit knob that enables the bundled openssl if libressl is enabled?
(In reply to Bradley T. Hughes from comment #2) I'm totally happy with it defaulting to external dependency. I see where I was being unclear in the other PR. My point was that ftp/curl detects when it's being compiled against LibreSSL and produces a message saying that you need to disable the TLS_SRP option, and that node should also do that.
*** Bug 203528 has been marked as a duplicate of this bug. ***
Sorry, forgot to address your last point in the previous comment. It needs to be an explicit knob. poudriere needs to be able to determine the dependency chain in advance.
I'll make it explicit. FWWO, when I suggested an implicit knob, I was thinking about overloading OPENSSL_PORT=security/libressl in make.conf (or similar) to mean "use the bundled openssl in node.js". But as soon as I suggested it, I changed my mind. Explicit it is.
Created attachment 170738 [details] patch from a git commit, apply with patch -p1 Updated patch to address comments. Test builds are running, but my puny 2-cpu EC2 instance isn't exactly the fastest. I will report back once the test builds are finished. :)
This doesn't work. PORT_OPTIONS is always unset until bsd.port.options.mk or bsd.port.pre.mk is included, so this check is always a no-op. Furthermore, OPENSSLINC/OPENSSLLIB/etc. have to come after bsd.port.pre.mk.
Indeed. I discovered that while testing builds with the option on/off in poudriere. I've got another patch almost ready, need to test it a bit more. I've given up on the idea of a common.mk between the various node ports in this PR as well. I might come back to it in the future, though. Thanks for the help and feedback so far, I really appreciate it :)
Nothing says you can't have two common.mk includes if you want to :-)
I'm sure there isn't. But my changes weren't working, so I decided to remove the extra variable. I think I have something now, a few more poudriere testport runs and I'll push it up.
Created attachment 170891 [details] patch from a git commit, apply with patch -p1 Updated patch. I don't have 11-CURRENT/ALPHA1 anywhere, so I haven't tested that yet. 10.3-RELEASE build works, though, pulling in security/openssl as a dependency when the BUNDLED_SSL is off.
My git commit message, if useful: -- www/node, www/node5, www/node4: use ports dependencies where possible Do not statically link bundled c-ares, libuv, and OpenSSL libraries into node.js, if possible. Only www/node has a configure knob to enable building against an unbundled dns/c-ares version. All 3 ports depend on devel/libuv. Bump PORTREVISION as a result of the dependency changes. OpenSSL 1.0.2 is required by node.js. Pull in OpenSSL from ports if the base version is not new enough. Note that node.js cannot be built with LibreSSL. Introduce the BUNDLED_SSL option (defaults to off) to allow statically linking the bundled OpenSSL library. The www/node010 and www/node012 ports have not been changed since both are deprecated and have very old bundled dependencies that are not available in ports. --
Good work, Bradley. I'll run tests and aim to get it committed tomorrow morning. Two things I'd like you to consider: 1) The OpenSSL stuff (OPENSSLLIB/OPENSSLINC) really should come after bsd.port.pre.mk. It likely works fine in this case after bsd.port.options.mk, but it could break in the future and definitely won't work if you do anything with OPENSSLLIB/INC other than assign it to something else. I'm not opposed to the way you have it now, because it works, so really everything else is gravy. 2) The message says "Cannot build node.js with LibreSSL. Consider enabling BUNDLED_SSL." Enabling BUNDLED_SSL really isn't optional here. Do you have any objections to "Cannot build node.js with LibreSSL. You must enable BUNDLED_SSL"?
1) This is what I struggled with the most while trying to get this working. Including bsd.port.pre.mk pulls in bsd.openss.mk, which evaluates current values of the USE_OPENSSL and WITH_OPENSSL_PORT variables. Changing (especially) WITH_OPENSSL_PORT afterwards doesn't add the OpenSSL port dependency nor does it change OPENSSLINC/OPENSSLLIB. I saw some ports, e.g. security/hpenc, that tried to re-include bsd.openssl.mk, but that hack didn't work for me. Thankfully, bsd.port.options.mk sets OSVERSION so I can set WITH_OPENSSL_PORT accordingly before bsd.port.pre.mk pulls in bsd.openssl.mk. 2) No, I have no objections to using that wording. :)
I think there's a logic problem here. .if empty(PORT_OPTIONS:MBUNDLED_SSL) [...] .elif defined(OPENSSL_PORT) && (${OPENSSL_PORT} == "security/libressl" || ${OPENSSL_PORT} == "security/libressl-devel") IGNORE= Cannot build node.js with LibreSSL. Consider enabling BUNDLED_SSL. .endif The error only gets produced if BUNDLED_SSL *is* enabled. I think you want that .elif to be a nested .if?
Argh. Yes, it should be. New patch incoming shortly.
Created attachment 170923 [details] patch from a git commit, apply with patch -p1 Hopefully the final patch. I moved the check for security/libressl to after bsd.port.pre.mk has been included, since its only then that I can really be sure OPENSSL_PORT is set. I also updated the IGNORE message.
Sorry buddy, I really thought we had it this time. I'm getting build failures when LibreSSL is installed. See https://people.freebsd.org/~adamw/node-libressl.log.xz for a full poudriere log. Also, https://people.freebsd.org/~adamw/node-6.2.0.log.xz for a full poudriere log of the same jail successfully building node 6.2.0. I'm not immediately clear on why the build is failing. Let me know how I can help. Also: the IGNORE lines shouldn't end with a period (one gets added automatically).
Looking at the failing build, I can see why it's failing: -I include path ordering. You've enabled BUNDLED_SSL and have LibreSSL installed. The failure in tls_wrap.cc and node_crypto.cc is due to the include path order including -I/usr/local/include (to be able to find c-ares and libuv) first, before -I../deps/openssl/openssl/include. This means that it's also picking up your LibreSSL headers. Fun! I'll dig into the node.js build process to see how to deal with that. I've got some contacts in the node.js build team that can probably help me out. Thanks for testing this out, btw :)
(In reply to Bradley T. Hughes from comment #20) Then I'll put two bugs in your ear: 1) Can you just reorder the DEPS to bring SSL_CFLAGS before LIBUV_CFLAGS? (I haven't actually looked at the Makefile so those names will be wrong) 2) Does it need libuv and c-ares CFLAGS when it's building tls_wrap.cc and node_crypto.cc?
Way ahead of you on #1 ;) As for #2, I suspect it probably needs the libuv, since that's the platform abstraction layer. I'll be able to answer that for sure once I dig into the code.
I've dug a bit into the build system, node-gyp, trying to find out where and why the include path order is what it is. Unfortunately it's not obvious where and what needs to be changed, and I've been thinking about alternative solutions, such as USES+=localbase instead of explicitly specifying ${LOCALBASE} in configure options. I'm doing some test builds to see if it would work.
Created attachment 171418 [details] patch from a git commit, apply with patch -p1 Final patch that adds USES+=localbase. I am going to get in touch with my upstream contact about the include path ordering to see if its something that needs fixing. Thanks for the input and patience so far :)
Works perfectly for me, Bradley. Really good work here. I'm very pleased to commit this.
A commit references this bug: Author: adamw Date: Tue Jun 14 15:53:54 UTC 2016 New revision: 416894 URL: https://svnweb.freebsd.org/changeset/ports/416894 Log: Prefer external libraries to the ones bundled with node. Do not statically link bundled libraries into node.js. It requires openssl 1.0.2, so pull that in from ports when the base version is not new enough. While all versions use c-ares, only www/node has a configure knob to enable building against an unbundled version. All other dependencies are specified in www/node/common.mk and used by all 3 ports. node cannot build against LibreSSL, so if it's the chosen SSL provider, the BUNDLED_SSL option must be enabled. The www/node010 and www/node012 ports have not been done since both are deprecated and have very old bundled dependencies that are not available in ports. PR: 209793 Submitted by: Bradley T. Hughes (maintainer) Changes: head/www/node/Makefile head/www/node/pkg-plist head/www/node4/Makefile head/www/node4/pkg-plist head/www/node5/Makefile head/www/node5/pkg-plist
A commit references this bug: Author: adamw Date: Tue Jun 14 15:59:02 UTC 2016 New revision: 416895 URL: https://svnweb.freebsd.org/changeset/ports/416895 Log: Add a note about the need to enable BUNDLED_SSL in node* when LibreSSL is the SSL provider. PR: 209793 Changes: head/UPDATING
I also added a head's up in UPDATING. Thanks for all your work on this patch, Bradley.
Thanks to you as well, for encouraging me to make it work with LibreSSL installed. :)