Re: [Buildroot] [PATCH v2] packages/nodejs: install npm packages on host

["Yann E. MORIN" <yann.morin.1998@free.fr>]

Tom, All,

Sorry for the delay; here's a quick review...

On 2022-01-25 14:05 +0100, Tom Marcuzzi spake thusly:
> Installing npm packages on host allows to use JavaScript tools that
> runs on NodeJS (such as webpack) to build web applications.
> 
> Signed-off-by: Tom Marcuzzi <tom.marcuzzi@orolia.com>
> ---
[--SNIP--]
> diff --git a/package/nodejs/Config.in.host b/package/nodejs/Config.in.host
> index 4ceaf0c73e..0b67a59c55 100644
> --- a/package/nodejs/Config.in.host
> +++ b/package/nodejs/Config.in.host
> @@ -16,3 +16,35 @@ config BR2_PACKAGE_HOST_NODEJS
>  comment "host nodejs needs a host gcc >= 8"
>  	depends on BR2_PACKAGE_HOST_QEMU_USER_ARCH_SUPPORTS
>  	depends on !BR2_HOST_GCC_AT_LEAST_8
> +
> +if BR2_PACKAGE_HOST_NODEJS
> +
> +config BR2_PACKAGE_HOST_NODEJS_MODULES_ADDITIONAL
> +	string "Additional modules"
> +	help
> +	  List of space-separated nodejs modules to install via npm
> +	  on host. See https://npmjs.org/ to find modules and
> +	  'npm help install' for available installation methods.
> +	  For repeatable builds, download and save tgz files or
> +	  clone git repos for the components you care about.
> +
> +	  Example:
> +	  serialport uglify-js@1.3.4 /my/module/mymodule.tgz \
> +	  git://github.com/someuser/somemodule.git#v1.2
> +
> +	  This would install the serialport module (at the newest
> +	  version), the uglify-js module at 1.3.4, a module from a
> +	  filesystem path, and a module from a git repository.
> +
> +config BR2_PACKAGE_HOST_NODEJS_MODULES_ADDITIONAL_DEPS
> +	string "Additional module dependencies"

I know you modelled this after the equivalent code for the target
variant, but I think BR2_PACKAGE_HOST_NODEJS_MODULES_ADDITIONAL_DEPS
should have a dependency on BR2_PACKAGE_HOST_NODEJS_MODULES_ADDITIONAL
not being empty (and a similar dependency should be added for the target
case), something like:

    depends on BR2_PACKAGE_HOST_NODEJS_MODULES_ADDITIONAL != ""

[--SNIP--]
> diff --git a/package/nodejs/nodejs.mk b/package/nodejs/nodejs.mk
> index 727af6dc50..8a4a2ff0cf 100644
> --- a/package/nodejs/nodejs.mk
> +++ b/package/nodejs/nodejs.mk
[--SNIP--]
> @@ -96,6 +97,35 @@ NODEJS_HOST_TOOLS = $(NODEJS_HOST_TOOLS_V8) $(NODEJS_HOST_TOOLS_NODE)
> 
>  HOST_NODEJS_CXXFLAGS = $(HOST_CXXFLAGS) -DU_DISABLE_RENAMING=1
> 
> +#
> +# Build the list of modules to install on host.
> +#
> +HOST_NODEJS_MODULES_LIST= $(call qstrip,\
> +	$(BR2_PACKAGE_HOST_NODEJS_MODULES_ADDITIONAL))
> +
> +# Define NPMHOST for other packages to use
> +NPMHOST = $(HOST_CONFIGURE_OPTS) \
> +	LDFLAGS="$(HOST_LDFLAGS)" \
> +	LD="$(HOST_CXX)" \
> +	npm_config_arch=$(NODEJS_CPU) \
> +	npm_config_target_arch=$(NODEJS_CPU) \

NODEJS_CPU is the target CPU. This is most probably incorrect for the
host npm. You can use $(HOSTARCH) to decide what kind of CPU to use,
something like:

    ifeq ($(HOSTARCH),x86)
    HOST_NODEJS_CPU = ia32
    else ifeq ($(HOSTARCH),x86_64)
    HOST_NODEJS_CPU = x84
    ...
    fi

Or maybe we just don't need it at all, since we do not specify it at all
when configuring the host nodejs...

> +	npm_config_build_from_source=true \
> +	npm_config_nodedir=$(HOST_DIR)/nodejs-$(NODEJS_VERSION) \

For the target variant, we point that to the nodejs build directory:

    npm_config_nodedir=$(BUILD_DIR)/nodejs-$(NODEJS_VERSION)

So for the host we should probably do something similar:

    npm_config_nodedir=$(BUILD_DIR)/host-nodejs-$(NODEJS_VERSION)

Otherwise, please explain that in the commit log.

And note that it should probably be better to use the actual variable:

    npm_config_nodedir=$(HOST_NODEJS_BUILDDIR)

and that the target variant should probably also be fixed.

> +	npm_config_prefix=$(HOST_DIR)/usr \
> +	npm_config_cache=$(HOST_DIR)/.npm-cache \

Ditto, this should point to $(BUILD_DIR)/.host-npm-cache

Regards,
Yann E. MORIN.

> +	$(HOST_DIR)/bin/npm
> +#
> +# We can only call NPMHOST if there's something to install.
> +#
> +ifneq ($(HOST_NODEJS_MODULES_LIST),)
> +define HOST_NODEJS_INSTALL_MODULES
> +	# If you're having trouble with module installation, adding -d to the
> +	# npm install call below and setting npm_config_rollback=false can both
> +	# help in diagnosing the problem.
> +	$(NPMHOST) install -g $(HOST_NODEJS_MODULES_LIST)
> +endef
> +endif
> +
>  define HOST_NODEJS_BUILD_CMDS
>  	$(HOST_MAKE_ENV) PYTHON=$(HOST_DIR)/bin/python3 \
>  		$(MAKE) -C $(@D) \
> @@ -110,6 +140,8 @@ define HOST_NODEJS_INSTALL_CMDS
>  	$(foreach f,$(NODEJS_HOST_TOOLS), \
>  		$(INSTALL) -m755 -D $(@D)/out/Release/$(f) $(HOST_DIR)/bin/$(f)
>  	)
> +
> +	$(HOST_NODEJS_INSTALL_MODULES)
>  endef
> 
>  ifeq ($(BR2_i386),y)
> --
> 2.25.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/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.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot