Message ID | CABJxTWFfyu_KLnSCzTkhEO8N9wGb8EZtr6srw_Oj=Nab66Jo3g@mail.gmail.com |
---|---|
State | Changes Requested |
Headers | show |
Series | PATCH package/nodejs: do not write in $HOME dir | expand |
Hello Kiril, On Wed, 10 Feb 2021 19:01:13 +0100 Kiril Maler <kiril.maler@gmail.com> 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. > > --- 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. > 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 to > HOST_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. > > 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. > + > --- 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
# 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 --- 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" 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 to HOST_DIR/config + in BASE_DIR endif --- /dev/null +++ b/package/nodejs/Config.in.host @@ -0,0 +1,3 @@ +config BR2_PACKAGE_HOST_NODEJS + bool + --- 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" +endif + # nodejs build system is based on python, but only support python-2.6 or # python-2.7. So, we have to enforce PYTHON interpreter to be python2. define HOST_NODEJS_CONFIGURE_CMDS