| Message ID | 20200127005539.125369-1-james.hilliard1@gmail.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v2,1/1] package/nodejs: use system-icu for host-nodejs when available | expand |
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
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
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
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
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)
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(-)