[Buildroot] PATCH package/nodejs: do not write in $HOME dir
flat view
From: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Date: 2021-02-13 22:46:03
Hello Kiril, On Wed, 10 Feb 2021 19:01:13 +0100 Kiril Maler [off-list ref] wrote:
host-nodejs will write a config file in $HOME/.config dir. When BR2_PACKAGE_NODEJS_NPM_HOSTCONFIG_IN_BASEDIR is selected, the config file will be written in $HOST_DIR/config
Thanks for your patch. However, it is not sent in the proper format: it should be inline in the e-mail, and your commit log should have a Signed-off-by: line. Also, I have a number of comments on the patch, so I'm quoting it below.
# The package nodejs provides tool $(HOST_DIR)/bin/npm # During host install, nodejs will write a json config file # to $XDG_CONFIG_HOME, which is normally $HOME/.config of $USER # # Add a default enabled option, that the json file is written # in $HOST_DIR/config
This should be part of the commit log. I'm not sure how you generated your patch.
quoted hunk ↗ jump to hunk
--- a/package/Config.in.host +++ b/package/Config.in.host@@ -52,6 +52,7 @@ menu "Host utilities" source "package/mtd/Config.in.host" source "package/mtools/Config.in.host" source "package/mxsldr/Config.in.host" + source "package/nodejs/Config.in.host"
This is not needed.
quoted hunk ↗ jump to hunk
source "package/odb/Config.in.host" source "package/omap-u-boot-utils/Config.in.host" source "package/openocd/Config.in.host"--- a/package/nodejs/Config.in +++ b/package/nodejs/Config.in@@ -77,5 +77,15 @@ config BR2_PACKAGE_NODEJS_MODULES_ADDITI specify 'libcurl' here, to ensure that buildroot builds the libcurl package, and does so before building your node modules. +endif +if BR2_PACKAGE_NODEJS || BR2_PACKAGE_HOST_NODEJS +config BR2_PACKAGE_NODEJS_NPM_HOSTCONFIG_IN_BASEDIR + bool "npm host config in HOST_DIR" + default y + help + The host NPM config is stored in a JSON file located in + $XDG_CONFIG_HOME or $HOME/.config. + When selected, the XDG_CONFIG_HOME is set toHOST_DIR/config + in BASE_DIR
Why are you adding an option for this? Buildroot's nodejs should not write to $HOME, so what you've proposed should be done unconditionally.
quoted hunk ↗ jump to hunk
endif--- /dev/null +++ b/package/nodejs/Config.in.host@@ -0,0 +1,3 @@ +config BR2_PACKAGE_HOST_NODEJS + bool
What is this option for ? It seems unrelated to this patch.
quoted hunk ↗ jump to hunk
+--- a/package/nodejs/nodejs.mk +++ b/package/nodejs/nodejs.mk@@ -44,6 +44,10 @@ ifneq ($(BR2_PACKAGE_NODEJS_NPM),y) NODEJS_CONF_OPTS += --without-npm endif +ifeq ($(BR2_PACKAGE_NODEJS_NPM_HOSTCONFIG_IN_BASEDIR),y) +HOST_MAKE_ENV += XDG_CONFIG_HOME="$(HOST_DIR)/config"
Overriding HOST_MAKE_ENV is not a good idea: it is a global variable. You rather want to pass this option specifically in the make environment of the nodejs build process. Unless we decide that XDG_CONFIG_HOME is a generic variable that we should pass globally, in which case it should be done in package/Makefile.in, where HOST_MAKE_ENV is defined. I'm not sure we want to do that, though. Could you rework your patch according to those suggestions ? Thanks a lot! Thomas -- Thomas Petazzoni, CTO, Bootlin Embedded Linux and Kernel engineering https://bootlin.com