* [Buildroot] [PATCH 1/1] package/nodejs: use system-icu for host-nodejs when available
@ 2020-01-26 7:58 James Hilliard
2020-01-26 16:26 ` Yann E. MORIN
2020-01-29 17:28 ` Thomas Preston
0 siblings, 2 replies; 6+ messages in thread
From: James Hilliard @ 2020-01-26 7:58 UTC (permalink / raw)
To: buildroot
Fixes:
- http://autobuild.buildroot.net/results/1ef947553ec762dba6a6202b1cfc84ceed75dbb2/
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
package/nodejs/nodejs.mk | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/package/nodejs/nodejs.mk b/package/nodejs/nodejs.mk
index e6eb73d576..abc868c364 100644
--- a/package/nodejs/nodejs.mk
+++ b/package/nodejs/nodejs.mk
@@ -65,7 +65,7 @@ define HOST_NODEJS_CONFIGURE_CMDS
--shared-openssl-libpath=$(HOST_DIR)/lib \
--shared-zlib \
--no-cross-compiling \
- --with-intl=small-icu \
+ --with-intl=$(if $(BR2_PACKAGE_ICU),system-icu,small-icu) \
)
endef
@@ -80,6 +80,7 @@ define HOST_NODEJS_BUILD_CMDS
$(HOST_MAKE_ENV) PYTHON=$(HOST_DIR)/bin/python2 \
$(MAKE) -C $(@D) \
$(HOST_CONFIGURE_OPTS) \
+ $(if $(BR2_PACKAGE_ICU),CXXFLAGS.target="-DU_DISABLE_RENAMING=1") \
LDFLAGS.host="$(HOST_LDFLAGS)" \
NO_LOAD=cctest.target.mk \
PATH=$(@D)/bin:$(BR_PATH)
@@ -89,6 +90,7 @@ define HOST_NODEJS_INSTALL_CMDS
$(HOST_MAKE_ENV) PYTHON=$(HOST_DIR)/bin/python2 \
$(MAKE) -C $(@D) install \
$(HOST_CONFIGURE_OPTS) \
+ $(if $(BR2_PACKAGE_ICU),CXXFLAGS.target="-DU_DISABLE_RENAMING=1") \
LDFLAGS.host="$(HOST_LDFLAGS)" \
NO_LOAD=cctest.target.mk \
PATH=$(@D)/bin:$(BR_PATH)
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [Buildroot] [PATCH 1/1] package/nodejs: use system-icu for host-nodejs when available
2020-01-26 7:58 [Buildroot] [PATCH 1/1] package/nodejs: use system-icu for host-nodejs when available James Hilliard
@ 2020-01-26 16:26 ` Yann E. MORIN
2020-01-26 20:57 ` James Hilliard
2020-01-28 9:20 ` Thomas Petazzoni
2020-01-29 17:28 ` Thomas Preston
1 sibling, 2 replies; 6+ messages in thread
From: Yann E. MORIN @ 2020-01-26 16:26 UTC (permalink / raw)
To: buildroot
James, All,
On 2020-01-26 00:58 -0700, James Hilliard spake thusly:
> Fixes:
> - http://autobuild.buildroot.net/results/1ef947553ec762dba6a6202b1cfc84ceed75dbb2/
Pointing to an autobuild failure is nice, but you really need to provide
a bit more context and explanations on what you are doing here...
> Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> ---
> package/nodejs/nodejs.mk | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/package/nodejs/nodejs.mk b/package/nodejs/nodejs.mk
> index e6eb73d576..abc868c364 100644
> --- a/package/nodejs/nodejs.mk
> +++ b/package/nodejs/nodejs.mk
> @@ -65,7 +65,7 @@ define HOST_NODEJS_CONFIGURE_CMDS
> --shared-openssl-libpath=$(HOST_DIR)/lib \
> --shared-zlib \
> --no-cross-compiling \
> - --with-intl=small-icu \
> + --with-intl=$(if $(BR2_PACKAGE_ICU),system-icu,small-icu) \
We already have a conditional block about icu, lines 35-40.
> )
> endef
>
> @@ -80,6 +80,7 @@ define HOST_NODEJS_BUILD_CMDS
> $(HOST_MAKE_ENV) PYTHON=$(HOST_DIR)/bin/python2 \
> $(MAKE) -C $(@D) \
> $(HOST_CONFIGURE_OPTS) \
> + $(if $(BR2_PACKAGE_ICU),CXXFLAGS.target="-DU_DISABLE_RENAMING=1") \
Why do you need to account for a target package when building the host
variant of nodejs? If this is needed, then explain it in the commit log.
Then, the way we usually pass such options is to append to the existing
CXXFLAGS. With the above comment, that would probably yield something
like (e.g. for target, ditto for host):
NODEJS_CXXFLAGS = $(TARGET_CXXFLAGS)
ifeq ($(BR2_PACKAGE_ICU),y)
NODEJS_DEPENDENCIES += icu
NODEJS_CONF_OPTS += --with-intl=system-icu
NODEJS_CXXFLAGS += -DU_DISABLE_RENAMING=1
else
NODEJS_CONF_OPTS += --with-intl=small-icu
endif
[...]
$(TARGET_MAKE_ENV) PYTHON=$(HOST_DIR)/bin/python2 \
$(MAKE) -C $(@D) \
$(TARGET_CONFIGURE_OPTS) \
CXXFLAGS="$(NODEJS_CXXFLAGS)" \
[...]
Regards,
Yann E. MORIN.
> LDFLAGS.host="$(HOST_LDFLAGS)" \
> NO_LOAD=cctest.target.mk \
> PATH=$(@D)/bin:$(BR_PATH)
> @@ -89,6 +90,7 @@ define HOST_NODEJS_INSTALL_CMDS
> $(HOST_MAKE_ENV) PYTHON=$(HOST_DIR)/bin/python2 \
> $(MAKE) -C $(@D) install \
> $(HOST_CONFIGURE_OPTS) \
> + $(if $(BR2_PACKAGE_ICU),CXXFLAGS.target="-DU_DISABLE_RENAMING=1") \
> LDFLAGS.host="$(HOST_LDFLAGS)" \
> NO_LOAD=cctest.target.mk \
> PATH=$(@D)/bin:$(BR_PATH)
> --
> 2.20.1
>
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
--
.-----------------.--------------------.------------------.--------------------.
| Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ |
| +33 561 099 427 `------------.-------: X AGAINST | \e/ There is no |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. |
'------------------------------^-------^------------------^--------------------'
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Buildroot] [PATCH 1/1] package/nodejs: use system-icu for host-nodejs when available
2020-01-26 16:26 ` Yann E. MORIN
@ 2020-01-26 20:57 ` James Hilliard
2020-01-28 9:20 ` Thomas Petazzoni
1 sibling, 0 replies; 6+ messages in thread
From: James Hilliard @ 2020-01-26 20:57 UTC (permalink / raw)
To: buildroot
On Sun, Jan 26, 2020 at 9:27 AM Yann E. MORIN <yann.morin.1998@free.fr> wrote:
>
> James, All,
>
> On 2020-01-26 00:58 -0700, James Hilliard spake thusly:
> > Fixes:
> > - http://autobuild.buildroot.net/results/1ef947553ec762dba6a6202b1cfc84ceed75dbb2/
>
> Pointing to an autobuild failure is nice, but you really need to provide
> a bit more context and explanations on what you are doing here...
>
> > Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> > ---
> > package/nodejs/nodejs.mk | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/package/nodejs/nodejs.mk b/package/nodejs/nodejs.mk
> > index e6eb73d576..abc868c364 100644
> > --- a/package/nodejs/nodejs.mk
> > +++ b/package/nodejs/nodejs.mk
> > @@ -65,7 +65,7 @@ define HOST_NODEJS_CONFIGURE_CMDS
> > --shared-openssl-libpath=$(HOST_DIR)/lib \
> > --shared-zlib \
> > --no-cross-compiling \
> > - --with-intl=small-icu \
> > + --with-intl=$(if $(BR2_PACKAGE_ICU),system-icu,small-icu) \
>
> We already have a conditional block about icu, lines 35-40.
>
> > )
> > endef
> >
> > @@ -80,6 +80,7 @@ define HOST_NODEJS_BUILD_CMDS
> > $(HOST_MAKE_ENV) PYTHON=$(HOST_DIR)/bin/python2 \
> > $(MAKE) -C $(@D) \
> > $(HOST_CONFIGURE_OPTS) \
> > + $(if $(BR2_PACKAGE_ICU),CXXFLAGS.target="-DU_DISABLE_RENAMING=1") \
>
> Why do you need to account for a target package when building the host
> variant of nodejs? If this is needed, then explain it in the commit log.
I was doing that because the target icu package determines if host-icu
is built, which breaks the small-icu host-nodejs build.
>
> Then, the way we usually pass such options is to append to the existing
> CXXFLAGS. With the above comment, that would probably yield something
> like (e.g. for target, ditto for host):
I tried that approach first but it didn't seem to work seemingly due to the
weird nodejs build system.
>
> NODEJS_CXXFLAGS = $(TARGET_CXXFLAGS)
>
> ifeq ($(BR2_PACKAGE_ICU),y)
> NODEJS_DEPENDENCIES += icu
> NODEJS_CONF_OPTS += --with-intl=system-icu
> NODEJS_CXXFLAGS += -DU_DISABLE_RENAMING=1
> else
> NODEJS_CONF_OPTS += --with-intl=small-icu
> endif
>
> [...]
>
> $(TARGET_MAKE_ENV) PYTHON=$(HOST_DIR)/bin/python2 \
> $(MAKE) -C $(@D) \
> $(TARGET_CONFIGURE_OPTS) \
> CXXFLAGS="$(NODEJS_CXXFLAGS)" \
> [...]
>
> Regards,
> Yann E. MORIN.
>
> > LDFLAGS.host="$(HOST_LDFLAGS)" \
> > NO_LOAD=cctest.target.mk \
> > PATH=$(@D)/bin:$(BR_PATH)
> > @@ -89,6 +90,7 @@ define HOST_NODEJS_INSTALL_CMDS
> > $(HOST_MAKE_ENV) PYTHON=$(HOST_DIR)/bin/python2 \
> > $(MAKE) -C $(@D) install \
> > $(HOST_CONFIGURE_OPTS) \
> > + $(if $(BR2_PACKAGE_ICU),CXXFLAGS.target="-DU_DISABLE_RENAMING=1") \
> > LDFLAGS.host="$(HOST_LDFLAGS)" \
> > NO_LOAD=cctest.target.mk \
> > PATH=$(@D)/bin:$(BR_PATH)
> > --
> > 2.20.1
> >
> > _______________________________________________
> > buildroot mailing list
> > buildroot at busybox.net
> > http://lists.busybox.net/mailman/listinfo/buildroot
>
> --
> .-----------------.--------------------.------------------.--------------------.
> | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
> | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ |
> | +33 561 099 427 `------------.-------: X AGAINST | \e/ There is no |
> | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. |
> '------------------------------^-------^------------------^--------------------'
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Buildroot] [PATCH 1/1] package/nodejs: use system-icu for host-nodejs when available
2020-01-26 16:26 ` Yann E. MORIN
2020-01-26 20:57 ` James Hilliard
@ 2020-01-28 9:20 ` Thomas Petazzoni
2020-01-28 9:57 ` James Hilliard
1 sibling, 1 reply; 6+ messages in thread
From: Thomas Petazzoni @ 2020-01-28 9:20 UTC (permalink / raw)
To: buildroot
On Sun, 26 Jan 2020 17:26:59 +0100
"Yann E. MORIN" <yann.morin.1998@free.fr> wrote:
> James, All,
>
> On 2020-01-26 00:58 -0700, James Hilliard spake thusly:
> > Fixes:
> > - http://autobuild.buildroot.net/results/1ef947553ec762dba6a6202b1cfc84ceed75dbb2/
>
> Pointing to an autobuild failure is nice, but you really need to provide
> a bit more context and explanations on what you are doing here...
>
> > Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> > ---
> > package/nodejs/nodejs.mk | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/package/nodejs/nodejs.mk b/package/nodejs/nodejs.mk
> > index e6eb73d576..abc868c364 100644
> > --- a/package/nodejs/nodejs.mk
> > +++ b/package/nodejs/nodejs.mk
> > @@ -65,7 +65,7 @@ define HOST_NODEJS_CONFIGURE_CMDS
> > --shared-openssl-libpath=$(HOST_DIR)/lib \
> > --shared-zlib \
> > --no-cross-compiling \
> > - --with-intl=small-icu \
> > + --with-intl=$(if $(BR2_PACKAGE_ICU),system-icu,small-icu) \
>
> We already have a conditional block about icu, lines 35-40.
>
> > )
> > endef
> >
> > @@ -80,6 +80,7 @@ define HOST_NODEJS_BUILD_CMDS
> > $(HOST_MAKE_ENV) PYTHON=$(HOST_DIR)/bin/python2 \
> > $(MAKE) -C $(@D) \
> > $(HOST_CONFIGURE_OPTS) \
> > + $(if $(BR2_PACKAGE_ICU),CXXFLAGS.target="-DU_DISABLE_RENAMING=1") \
>
> Why do you need to account for a target package when building the host
> variant of nodejs? If this is needed, then explain it in the commit log.
>
> Then, the way we usually pass such options is to append to the existing
> CXXFLAGS. With the above comment, that would probably yield something
> like (e.g. for target, ditto for host):
>
> NODEJS_CXXFLAGS = $(TARGET_CXXFLAGS)
>
> ifeq ($(BR2_PACKAGE_ICU),y)
> NODEJS_DEPENDENCIES += icu
> NODEJS_CONF_OPTS += --with-intl=system-icu
> NODEJS_CXXFLAGS += -DU_DISABLE_RENAMING=1
> else
> NODEJS_CONF_OPTS += --with-intl=small-icu
> endif
I think you're missing the fact that James is tweaking
HOST_NODEJS_CONF_OPTS, depending on the value of BR2_PACKAGE_ICU, which
looks very wrong.
The issue is apparently that when host-icu has been built, the
host-nodejs internal small-icu causes a build failure. However, while
BR2_PACKAGE_ICU=y implies that host-icu will be built, host-icu can
potentially be built even with BR2_PACKAGE_ICU is disabled. It is not
the case today in Buildroot, because host-icu is only in the build
dependencies of the target icu package, but relying on this assumption
seems wrong.
Best regards,
Thomas
--
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Buildroot] [PATCH 1/1] package/nodejs: use system-icu for host-nodejs when available
2020-01-28 9:20 ` Thomas Petazzoni
@ 2020-01-28 9:57 ` James Hilliard
0 siblings, 0 replies; 6+ messages in thread
From: James Hilliard @ 2020-01-28 9:57 UTC (permalink / raw)
To: buildroot
On Tue, Jan 28, 2020 at 2:20 AM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> On Sun, 26 Jan 2020 17:26:59 +0100
> "Yann E. MORIN" <yann.morin.1998@free.fr> wrote:
>
> > James, All,
> >
> > On 2020-01-26 00:58 -0700, James Hilliard spake thusly:
> > > Fixes:
> > > - http://autobuild.buildroot.net/results/1ef947553ec762dba6a6202b1cfc84ceed75dbb2/
> >
> > Pointing to an autobuild failure is nice, but you really need to provide
> > a bit more context and explanations on what you are doing here...
> >
> > > Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> > > ---
> > > package/nodejs/nodejs.mk | 4 +++-
> > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/package/nodejs/nodejs.mk b/package/nodejs/nodejs.mk
> > > index e6eb73d576..abc868c364 100644
> > > --- a/package/nodejs/nodejs.mk
> > > +++ b/package/nodejs/nodejs.mk
> > > @@ -65,7 +65,7 @@ define HOST_NODEJS_CONFIGURE_CMDS
> > > --shared-openssl-libpath=$(HOST_DIR)/lib \
> > > --shared-zlib \
> > > --no-cross-compiling \
> > > - --with-intl=small-icu \
> > > + --with-intl=$(if $(BR2_PACKAGE_ICU),system-icu,small-icu) \
> >
> > We already have a conditional block about icu, lines 35-40.
> >
> > > )
> > > endef
> > >
> > > @@ -80,6 +80,7 @@ define HOST_NODEJS_BUILD_CMDS
> > > $(HOST_MAKE_ENV) PYTHON=$(HOST_DIR)/bin/python2 \
> > > $(MAKE) -C $(@D) \
> > > $(HOST_CONFIGURE_OPTS) \
> > > + $(if $(BR2_PACKAGE_ICU),CXXFLAGS.target="-DU_DISABLE_RENAMING=1") \
> >
> > Why do you need to account for a target package when building the host
> > variant of nodejs? If this is needed, then explain it in the commit log.
> >
> > Then, the way we usually pass such options is to append to the existing
> > CXXFLAGS. With the above comment, that would probably yield something
> > like (e.g. for target, ditto for host):
> >
> > NODEJS_CXXFLAGS = $(TARGET_CXXFLAGS)
> >
> > ifeq ($(BR2_PACKAGE_ICU),y)
> > NODEJS_DEPENDENCIES += icu
> > NODEJS_CONF_OPTS += --with-intl=system-icu
> > NODEJS_CXXFLAGS += -DU_DISABLE_RENAMING=1
> > else
> > NODEJS_CONF_OPTS += --with-intl=small-icu
> > endif
>
> I think you're missing the fact that James is tweaking
> HOST_NODEJS_CONF_OPTS, depending on the value of BR2_PACKAGE_ICU, which
> looks very wrong.
>
> The issue is apparently that when host-icu has been built, the
> host-nodejs internal small-icu causes a build failure. However, while
> BR2_PACKAGE_ICU=y implies that host-icu will be built, host-icu can
> potentially be built even with BR2_PACKAGE_ICU is disabled. It is not
> the case today in Buildroot, because host-icu is only in the build
> dependencies of the target icu package, but relying on this assumption
> seems wrong.
Is there a way to make it depend on host-icu directly?
>
> Best regards,
>
> Thomas
> --
> Thomas Petazzoni, CTO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Buildroot] [PATCH 1/1] package/nodejs: use system-icu for host-nodejs when available
2020-01-26 7:58 [Buildroot] [PATCH 1/1] package/nodejs: use system-icu for host-nodejs when available James Hilliard
2020-01-26 16:26 ` Yann E. MORIN
@ 2020-01-29 17:28 ` Thomas Preston
1 sibling, 0 replies; 6+ messages in thread
From: Thomas Preston @ 2020-01-29 17:28 UTC (permalink / raw)
To: buildroot
On 26/01/2020 07:58, James Hilliard wrote:
> Fixes:
> - http://autobuild.buildroot.net/results/1ef947553ec762dba6a6202b1cfc84ceed75dbb2/
>
> Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> ---
> package/nodejs/nodejs.mk | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/package/nodejs/nodejs.mk b/package/nodejs/nodejs.mk
> index e6eb73d576..abc868c364 100644
> --- a/package/nodejs/nodejs.mk
> +++ b/package/nodejs/nodejs.mk
> @@ -65,7 +65,7 @@ define HOST_NODEJS_CONFIGURE_CMDS
> --shared-openssl-libpath=$(HOST_DIR)/lib \
> --shared-zlib \
> --no-cross-compiling \
> - --with-intl=small-icu \
> + --with-intl=$(if $(BR2_PACKAGE_ICU),system-icu,small-icu) \
> )
> endef
>
> @@ -80,6 +80,7 @@ define HOST_NODEJS_BUILD_CMDS
> $(HOST_MAKE_ENV) PYTHON=$(HOST_DIR)/bin/python2 \
> $(MAKE) -C $(@D) \
> $(HOST_CONFIGURE_OPTS) \
> + $(if $(BR2_PACKAGE_ICU),CXXFLAGS.target="-DU_DISABLE_RENAMING=1") \
> LDFLAGS.host="$(HOST_LDFLAGS)" \
> NO_LOAD=cctest.target.mk \
> PATH=$(@D)/bin:$(BR_PATH)
> @@ -89,6 +90,7 @@ define HOST_NODEJS_INSTALL_CMDS
> $(HOST_MAKE_ENV) PYTHON=$(HOST_DIR)/bin/python2 \
> $(MAKE) -C $(@D) install \
> $(HOST_CONFIGURE_OPTS) \
> + $(if $(BR2_PACKAGE_ICU),CXXFLAGS.target="-DU_DISABLE_RENAMING=1") \
> LDFLAGS.host="$(HOST_LDFLAGS)" \
> NO_LOAD=cctest.target.mk \
> PATH=$(@D)/bin:$(BR_PATH)
>
I was actually able to fix this by removing `--shared-zlib`, see below.
commit 8ddaef98e72449c41a3255374a5f61da20a167df
Author: Thomas Preston <thomas.preston@codethink.co.uk>
Date: Fri Jan 24 13:40:49 2020 +0000
package/nodejs: Remove --shared-zlib configure arg
The nodejs configure.py file orders zlib headers before the bundled ICU
headers. The zlib headers happen to be located in the system include
directory, next to some system ICU headers (not bundled). If these are
built before nodejs is, nodejs will get confused and try to use the
system ICU headers instead of the bundled ones.
Fix this by removing the --shared-zlib configure argument, since we
search in the system include directory after bundled ICU headers anyway.
Signed-off-by: Thomas Preston <thomas.preston@codethink.co.uk>
diff --git a/package/nodejs/nodejs.mk b/package/nodejs/nodejs.mk
index e6eb73d576..205e8a8bd5 100644
--- a/package/nodejs/nodejs.mk
+++ b/package/nodejs/nodejs.mk
@@ -63,7 +63,6 @@ define HOST_NODEJS_CONFIGURE_CMDS
--shared-openssl \
--shared-openssl-includes=$(HOST_DIR)/include/openssl \
--shared-openssl-libpath=$(HOST_DIR)/lib \
- --shared-zlib \
--no-cross-compiling \
--with-intl=small-icu \
)
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-01-29 17:28 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-26 7:58 [Buildroot] [PATCH 1/1] package/nodejs: use system-icu for host-nodejs when available James Hilliard
2020-01-26 16:26 ` Yann E. MORIN
2020-01-26 20:57 ` James Hilliard
2020-01-28 9:20 ` Thomas Petazzoni
2020-01-28 9:57 ` James Hilliard
2020-01-29 17:28 ` Thomas Preston
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.