[Buildroot] [PATCH v2 1/1] package/nodejs: use system-icu for host-nodejs when available

[Buildroot] [PATCH v2 1/1] package/nodejs: use system-icu for host-nodejs when available

[James Hilliard]

Removed LDFLAGS.host as it doesn't appear to be needed.

Set CXXFLAGS.target to -DU_DISABLE_RENAMING=1 when building with
system-icu since host-icu is built with --disable-renaming.

Fixes:
 - http://autobuild.buildroot.net/results/1ef947553ec762dba6a6202b1cfc84ceed75dbb2/

Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
Changes v1 -> v2:
  - removed LDFLAGS.host
  - Rework style for setting
---
 package/nodejs/nodejs.mk | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/package/nodejs/nodejs.mk b/package/nodejs/nodejs.mk
index e6eb73d576..432f47c66a 100644
--- a/package/nodejs/nodejs.mk
+++ b/package/nodejs/nodejs.mk
@@ -25,6 +25,17 @@ NODEJS_CONF_OPTS = \
 	--cross-compiling \
 	--dest-os=linux
 
+HOST_NODEJS_CONF_OPTS = \
+	--prefix=$(HOST_DIR) \
+	--without-snapshot \
+	--without-dtrace \
+	--without-etw \
+	--shared-openssl \
+	--shared-openssl-includes=$(HOST_DIR)/include/openssl \
+	--shared-openssl-libpath=$(HOST_DIR)/lib \
+	--shared-zlib \
+	--no-cross-compiling
+
 ifeq ($(BR2_PACKAGE_OPENSSL),y)
 NODEJS_DEPENDENCIES += openssl
 NODEJS_CONF_OPTS += --shared-openssl
@@ -35,8 +46,11 @@ endif
 ifeq ($(BR2_PACKAGE_ICU),y)
 NODEJS_DEPENDENCIES += icu
 NODEJS_CONF_OPTS += --with-intl=system-icu
+HOST_NODEJS_CONF_OPTS += --with-intl=system-icu
+HOST_NODEJS_CXXFLAGS = -DU_DISABLE_RENAMING=1
 else
 NODEJS_CONF_OPTS += --with-intl=none
+HOST_NODEJS_CONF_OPTS += --with-intl=small-icu
 endif
 
 ifneq ($(BR2_PACKAGE_NODEJS_NPM),y)
@@ -56,16 +70,7 @@ define HOST_NODEJS_CONFIGURE_CMDS
 		PATH=$(@D)/bin:$(BR_PATH) \
 		PYTHON=$(HOST_DIR)/bin/python2 \
 		$(HOST_DIR)/bin/python2 ./configure \
-		--prefix=$(HOST_DIR) \
-		--without-snapshot \
-		--without-dtrace \
-		--without-etw \
-		--shared-openssl \
-		--shared-openssl-includes=$(HOST_DIR)/include/openssl \
-		--shared-openssl-libpath=$(HOST_DIR)/lib \
-		--shared-zlib \
-		--no-cross-compiling \
-		--with-intl=small-icu \
+		$(HOST_NODEJS_CONF_OPTS) \
 	)
 endef
 
@@ -80,7 +85,7 @@ define HOST_NODEJS_BUILD_CMDS
 	$(HOST_MAKE_ENV) PYTHON=$(HOST_DIR)/bin/python2 \
 		$(MAKE) -C $(@D) \
 		$(HOST_CONFIGURE_OPTS) \
-		LDFLAGS.host="$(HOST_LDFLAGS)" \
+		$(if $(HOST_NODEJS_CXXFLAGS),CXXFLAGS.target="$(HOST_NODEJS_CXXFLAGS)") \
 		NO_LOAD=cctest.target.mk \
 		PATH=$(@D)/bin:$(BR_PATH)
 endef
@@ -89,7 +94,7 @@ define HOST_NODEJS_INSTALL_CMDS
 	$(HOST_MAKE_ENV) PYTHON=$(HOST_DIR)/bin/python2 \
 		$(MAKE) -C $(@D) install \
 		$(HOST_CONFIGURE_OPTS) \
-		LDFLAGS.host="$(HOST_LDFLAGS)" \
+		$(if $(HOST_NODEJS_CXXFLAGS),CXXFLAGS.target="$(HOST_NODEJS_CXXFLAGS)") \
 		NO_LOAD=cctest.target.mk \
 		PATH=$(@D)/bin:$(BR_PATH)
 
-- 
2.20.1

[Buildroot] [PATCH v2 1/1] package/nodejs: use system-icu for host-nodejs when available

[Thomas Petazzoni]

On Sun, 26 Jan 2020 17:55:39 -0700
James Hilliard <james.hilliard1@gmail.com> wrote:

> Removed LDFLAGS.host as it doesn't appear to be needed.

LDFLAGS.host is definitely needed, see commit
f4abcbe112a0a45b87545f32981be87212116e94. At least, you must verify
that the issue fixed by this commit no longer exists if you drop
LDFLAGS.host. You must check that the host tools built by nodejs have a
correct RPATH. If they don't, then LDFLAGS.host is still needed.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH v2 1/1] package/nodejs: use system-icu for host-nodejs when available

[James Hilliard]

On Mon, Jan 27, 2020 at 2:59 AM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> On Sun, 26 Jan 2020 17:55:39 -0700
> James Hilliard <james.hilliard1@gmail.com> wrote:
>
> > Removed LDFLAGS.host as it doesn't appear to be needed.
>
> LDFLAGS.host is definitely needed, see commit
> f4abcbe112a0a45b87545f32981be87212116e94. At least, you must verify
> that the issue fixed by this commit no longer exists if you drop
> LDFLAGS.host. You must check that the host tools built by nodejs have a
> correct RPATH. If they don't, then LDFLAGS.host is still needed.
Hmm, well it seemed to work fine with a full rebuild with and without host-icu
I looked at that commit which is apparently fixing this issue:
http://autobuild.buildroot.net/results/a1f5e336ddaf386ba08eb5a7a299a48e2bdfe2d9/build-end.log
however that log is from a much older version of nodejs(10.16.3) build so
I'm assuming some of the node build system changes eliminated the issue
since I wasn't able to reproduce the build failure with the current 12.14.1.
>
> Best regards,
>
> Thomas
> --
> Thomas Petazzoni, CTO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

[Buildroot] [PATCH v2 1/1] package/nodejs: use system-icu for host-nodejs when available

[Thomas Petazzoni]

On Mon, 27 Jan 2020 03:31:54 -0700
James Hilliard <james.hilliard1@gmail.com> wrote:

> Hmm, well it seemed to work fine with a full rebuild with and without host-icu
> I looked at that commit which is apparently fixing this issue:
> http://autobuild.buildroot.net/results/a1f5e336ddaf386ba08eb5a7a299a48e2bdfe2d9/build-end.log
> however that log is from a much older version of nodejs(10.16.3) build so
> I'm assuming some of the node build system changes eliminated the issue
> since I wasn't able to reproduce the build failure with the current 12.14.1.

"It builds for you" doesn't mean it is correct. The build issue we had
only occurred on machines that don't have OpenSSL installed
system-wide, which is unlikely to be the case on your system. The issue
was that the host tools built by NodeJS did not had a RPATH defined,
and therefore when executed they didn't know they should use the
OpenSSL library installed in $(HOST_DIR)/lib.

In machines that had a close enough version of OpenSSL installed
system-wide, it was not visible, as those host tools were happily using
the system-wide OpenSSL. But in the general case, it doesn't work.

So, no "it builds for me" is not sufficient. We need to ensure that the
host tools installed by host-nodejs do have a correct RPATH.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH v2 1/1] package/nodejs: use system-icu for host-nodejs when available

[James Hilliard]

On Mon, Jan 27, 2020 at 6:31 AM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> On Mon, 27 Jan 2020 03:31:54 -0700
> James Hilliard <james.hilliard1@gmail.com> wrote:
>
> > Hmm, well it seemed to work fine with a full rebuild with and without host-icu
> > I looked at that commit which is apparently fixing this issue:
> > http://autobuild.buildroot.net/results/a1f5e336ddaf386ba08eb5a7a299a48e2bdfe2d9/build-end.log
> > however that log is from a much older version of nodejs(10.16.3) build so
> > I'm assuming some of the node build system changes eliminated the issue
> > since I wasn't able to reproduce the build failure with the current 12.14.1.
>
> "It builds for you" doesn't mean it is correct. The build issue we had
> only occurred on machines that don't have OpenSSL installed
> system-wide, which is unlikely to be the case on your system. The issue
> was that the host tools built by NodeJS did not had a RPATH defined,
> and therefore when executed they didn't know they should use the
> OpenSSL library installed in $(HOST_DIR)/lib.
>
> In machines that had a close enough version of OpenSSL installed
> system-wide, it was not visible, as those host tools were happily using
> the system-wide OpenSSL. But in the general case, it doesn't work.
>
> So, no "it builds for me" is not sufficient. We need to ensure that the
> host tools installed by host-nodejs do have a correct RPATH.
Ah, ok I'll resend with that reverted.
>
> Best regards,
>
> Thomas
> --
> Thomas Petazzoni, CTO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com