Bug 209793 - www/node*: Use ports dependencies where possible
Summary: www/node*: Use ports dependencies where possible
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Many People
Assignee: Adam Weinberger
URL:
Keywords: needs-qa, patch
: 203528 (view as bug list)
Depends on:
Blocks: 203528
  Show dependency treegraph
 
Reported: 2016-05-27 13:19 UTC by Bradley T. Hughes
Modified: 2016-06-14 20:29 UTC (History)
1 user (show)

See Also:


Attachments
patch from a git commit, apply with patch -p1 (17.03 KB, text/plain)
2016-05-27 13:19 UTC, Bradley T. Hughes
no flags Details
patch from a git commit, apply with patch -p1 (22.26 KB, patch)
2016-05-27 22:46 UTC, Bradley T. Hughes
no flags Details | Diff
patch from a git commit, apply with patch -p1 (25.10 KB, patch)
2016-05-31 22:31 UTC, Bradley T. Hughes
no flags Details | Diff
patch from a git commit, apply with patch -p1 (25.41 KB, text/plain)
2016-06-01 19:18 UTC, Bradley T. Hughes
no flags Details
patch from a git commit, apply with patch -p1 (25.40 KB, patch)
2016-06-14 10:28 UTC, Bradley T. Hughes
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Bradley T. Hughes freebsd_committer 2016-05-27 13:19:57 UTC
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.
Comment 1 Adam Weinberger freebsd_committer 2016-05-27 13:54:28 UTC
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.
Comment 2 Bradley T. Hughes freebsd_committer 2016-05-27 20:36:59 UTC
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?
Comment 3 Adam Weinberger freebsd_committer 2016-05-27 20:42:00 UTC
(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.
Comment 4 Adam Weinberger freebsd_committer 2016-05-27 20:44:46 UTC
*** Bug 203528 has been marked as a duplicate of this bug. ***
Comment 5 Adam Weinberger freebsd_committer 2016-05-27 20:49:09 UTC
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.
Comment 6 Bradley T. Hughes freebsd_committer 2016-05-27 21:39:11 UTC
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.
Comment 7 Bradley T. Hughes freebsd_committer 2016-05-27 22:46:42 UTC
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. :)
Comment 8 Adam Weinberger freebsd_committer 2016-05-29 15:04:40 UTC
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.
Comment 9 Bradley T. Hughes freebsd_committer 2016-05-31 08:19:32 UTC
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 :)
Comment 10 Adam Weinberger freebsd_committer 2016-05-31 13:57:03 UTC
Nothing says you can't have two common.mk includes if you want to :-)
Comment 11 Bradley T. Hughes freebsd_committer 2016-05-31 19:59:53 UTC
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.
Comment 12 Bradley T. Hughes freebsd_committer 2016-05-31 22:31:22 UTC
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.
Comment 13 Bradley T. Hughes freebsd_committer 2016-05-31 22:33:34 UTC
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.
--
Comment 14 Adam Weinberger freebsd_committer 2016-06-01 01:10:07 UTC
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"?
Comment 15 Bradley T. Hughes freebsd_committer 2016-06-01 05:05:45 UTC
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. :)
Comment 16 Adam Weinberger freebsd_committer 2016-06-01 14:59:02 UTC
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?
Comment 17 Bradley T. Hughes freebsd_committer 2016-06-01 18:19:08 UTC
Argh. Yes, it should be. New patch incoming shortly.
Comment 18 Bradley T. Hughes freebsd_committer 2016-06-01 19:18:48 UTC
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.
Comment 19 Adam Weinberger freebsd_committer 2016-06-02 16:05:19 UTC
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).
Comment 20 Bradley T. Hughes freebsd_committer 2016-06-02 18:02:28 UTC
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 :)
Comment 21 Adam Weinberger freebsd_committer 2016-06-02 18:05:28 UTC
(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?
Comment 22 Bradley T. Hughes freebsd_committer 2016-06-02 18:07:46 UTC
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.
Comment 23 Bradley T. Hughes freebsd_committer 2016-06-06 13:08:39 UTC
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.
Comment 24 Bradley T. Hughes freebsd_committer 2016-06-14 10:28:07 UTC
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 :)
Comment 25 Adam Weinberger freebsd_committer 2016-06-14 15:49:34 UTC
Works perfectly for me, Bradley. Really good work here. I'm very pleased to commit this.
Comment 26 commit-hook freebsd_committer 2016-06-14 15:54:41 UTC
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
Comment 27 commit-hook freebsd_committer 2016-06-14 15:59:42 UTC
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
Comment 28 Adam Weinberger freebsd_committer 2016-06-14 16:01:07 UTC
I also added a head's up in UPDATING.

Thanks for all your work on this patch, Bradley.
Comment 29 Bradley T. Hughes freebsd_committer 2016-06-14 20:29:27 UTC
Thanks to you as well, for encouraging me to make it work with LibreSSL installed. :)