| Message ID | 16100eb6-bac8-2e4d-65f2-26333179f3b8@foxvalley.net |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [1/1] fix failure during 'make linux-menuconfig' when PATH env var has spaces | expand |
Dan, All, On 2021-02-28 19:40 -0700, Dan Raymond spake thusly: > From 4db6db355723bedb5918be214fbd6c6eb4a39257 Mon Sep 17 00:00:00 2001 > From: Dan Raymond <draymond@foxvalley.net> > Date: Sun, 28 Feb 2021 18:27:18 -0700 > Subject: [PATCH 1/1] fix failure during 'make linux-menuconfig' when PATH > env var has spaces Thanks for the patch. However, as I explained on IRC, this is far from sufficient. A lot of places do not expect spaces in PATH. For example, the config.guess from the gnuconfig package (i.e. a file we do bundle, and that is maintain elsewhere), and present in virtually all autotools-based packages, has this line: PATH=$PATH:/.attbin ; export PATH So, basically, all autotools packages are somewhat broken when PATH contains spaces. As a second example, package/fakedate/fakedate splits PATH on spaces: for P in `echo $PATH | tr ':' ' '`; do Fixing that is too cumbersome, and again, the vast majority of things is going to break when PATH contains spaces. Rather, as I suggested on IRC, please add a sanity check in support/dependencies/dependencies.sh, lines 31..44, that there indeed is no spaces in PATH. Besides, Buildroot does not even build when the top-level or output directories contain spaces. Regards, Yann E. MORIN. > Signed-off-by: Dan Raymond <draymond@foxvalley.net> > --- > package/pkg-kconfig.mk | 6 +++--- > support/misc/utils.mk | 6 ++++++ > 2 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/package/pkg-kconfig.mk b/package/pkg-kconfig.mk > index 2aecf2e203..6bf87fbead 100644 > --- a/package/pkg-kconfig.mk > +++ b/package/pkg-kconfig.mk > @@ -205,9 +205,9 @@ endif > # nconfig, gconfig, xconfig). > # So we simply remove our PATH and PKG_CONFIG_* variables. > $(2)_CONFIGURATOR_MAKE_ENV = \ > - $$(filter-out PATH=% PKG_CONFIG=% PKG_CONFIG_SYSROOT_DIR=% \ > - PKG_CONFIG_ALLOW_SYSTEM_CFLAGS=% PKG_CONFIG_ALLOW_SYSTEM_LIBS=% \ > - PKG_CONFIG_LIBDIR=%,$$($(2)_MAKE_ENV)) \ > + $$(call strip_env,PATH PKG_CONFIG PKG_CONFIG_SYSROOT_DIR \ > + PKG_CONFIG_ALLOW_SYSTEM_CFLAGS PKG_CONFIG_ALLOW_SYSTEM_LIBS \ > + PKG_CONFIG_LIBDIR,$$($(2)_MAKE_ENV)) \ > PKG_CONFIG_PATH="$(HOST_PKG_CONFIG_PATH)" > > # Configuration editors (menuconfig, ...) > diff --git a/support/misc/utils.mk b/support/misc/utils.mk > index dc60cad979..640e56c775 100644 > --- a/support/misc/utils.mk > +++ b/support/misc/utils.mk > @@ -135,3 +135,9 @@ define PRINTF > $(subst $(QUOTE),$(QUOTE)\$(QUOTE)$(QUOTE),\ > $(subst \,\\,$(1)))))\n' > endef > + > +# strip variables (quoted or unquoted) from an environment string: > +# MYENV = PATH="/bin:/dir with spaces" USER=root SHELL=/bin/sh > +# $(call strip_env,PATH SHELL,$(MYENV)) --> USER=root > +sed_cmds=$(foreach var,$(1),-e 's/$(var)="[^"]*" *//' -e 's/$(var)=[^ ]* > *//') > +strip_env=$(shell echo "$(subst ",\",$(2))" | sed $(call sed_cmds,$(1))) > -- > 2.25.1 > > _______________________________________________ > buildroot mailing list > buildroot@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot
Hi, Yann. While I was digging through the Makefiles hunting down this bug I saw plenty of evidence of spaces in environment variables (and quoting used to manage them). When using default buildroot and kernel options this was the only bug I encountered. After fixing it I am able to fully build all targets. There may be other bugs lurking in various packages I didn't build but that doesn't seem like a good reason not to fix this one. Also, note that the bug impacts more than just PATH. It impacts the following variables now (and any that may be added in the future): PATH PKG_CONFIG PKG_CONFIG_SYSROOT_DIR PKG_CONFIG_ALLOW_SYSTEM_CFLAGS PKG_CONFIG_ALLOW_SYSTEM_LIBS PKG_CONFIG_LIBDIR Since I spent the time debugging this and implementing/testing a clean and reliable fix why don't we merge this now and if there are future bug reports about spaces in environment variables we can discuss how to proceed then? On 3/1/2021 12:21 AM, Yann E. MORIN wrote: > Dan, All, > > On 2021-02-28 19:40 -0700, Dan Raymond spake thusly: >> From 4db6db355723bedb5918be214fbd6c6eb4a39257 Mon Sep 17 00:00:00 2001 >> From: Dan Raymond <draymond@foxvalley.net> >> Date: Sun, 28 Feb 2021 18:27:18 -0700 >> Subject: [PATCH 1/1] fix failure during 'make linux-menuconfig' when PATH >> env var has spaces > Thanks for the patch. > > However, as I explained on IRC, this is far from sufficient. A lot of > places do not expect spaces in PATH. For example, the config.guess from > the gnuconfig package (i.e. a file we do bundle, and that is maintain > elsewhere), and present in virtually all autotools-based packages, has > this line: > > PATH=$PATH:/.attbin ; export PATH > > So, basically, all autotools packages are somewhat broken when PATH > contains spaces. > > As a second example, package/fakedate/fakedate splits PATH on spaces: > > for P in `echo $PATH | tr ':' ' '`; do > > Fixing that is too cumbersome, and again, the vast majority of things is > going to break when PATH contains spaces. > > Rather, as I suggested on IRC, please add a sanity check in > support/dependencies/dependencies.sh, lines 31..44, that there indeed is > no spaces in PATH. > > Besides, Buildroot does not even build when the top-level or output > directories contain spaces. > > Regards, > Yann E. MORIN. > >> Signed-off-by: Dan Raymond <draymond@foxvalley.net> >> --- >> package/pkg-kconfig.mk | 6 +++--- >> support/misc/utils.mk | 6 ++++++ >> 2 files changed, 9 insertions(+), 3 deletions(-) >> >> diff --git a/package/pkg-kconfig.mk b/package/pkg-kconfig.mk >> index 2aecf2e203..6bf87fbead 100644 >> --- a/package/pkg-kconfig.mk >> +++ b/package/pkg-kconfig.mk >> @@ -205,9 +205,9 @@ endif >> # nconfig, gconfig, xconfig). >> # So we simply remove our PATH and PKG_CONFIG_* variables. >> $(2)_CONFIGURATOR_MAKE_ENV = \ >> - $$(filter-out PATH=% PKG_CONFIG=% PKG_CONFIG_SYSROOT_DIR=% \ >> - PKG_CONFIG_ALLOW_SYSTEM_CFLAGS=% PKG_CONFIG_ALLOW_SYSTEM_LIBS=% \ >> - PKG_CONFIG_LIBDIR=%,$$($(2)_MAKE_ENV)) \ >> + $$(call strip_env,PATH PKG_CONFIG PKG_CONFIG_SYSROOT_DIR \ >> + PKG_CONFIG_ALLOW_SYSTEM_CFLAGS PKG_CONFIG_ALLOW_SYSTEM_LIBS \ >> + PKG_CONFIG_LIBDIR,$$($(2)_MAKE_ENV)) \ >> PKG_CONFIG_PATH="$(HOST_PKG_CONFIG_PATH)" >> >> # Configuration editors (menuconfig, ...) >> diff --git a/support/misc/utils.mk b/support/misc/utils.mk >> index dc60cad979..640e56c775 100644 >> --- a/support/misc/utils.mk >> +++ b/support/misc/utils.mk >> @@ -135,3 +135,9 @@ define PRINTF >> $(subst $(QUOTE),$(QUOTE)\$(QUOTE)$(QUOTE),\ >> $(subst \,\\,$(1)))))\n' >> endef >> + >> +# strip variables (quoted or unquoted) from an environment string: >> +# MYENV = PATH="/bin:/dir with spaces" USER=root SHELL=/bin/sh >> +# $(call strip_env,PATH SHELL,$(MYENV)) --> USER=root >> +sed_cmds=$(foreach var,$(1),-e 's/$(var)="[^"]*" *//' -e 's/$(var)=[^ ]* >> *//') >> +strip_env=$(shell echo "$(subst ",\",$(2))" | sed $(call sed_cmds,$(1))) >> -- >> 2.25.1 >> >> _______________________________________________ >> buildroot mailing list >> buildroot@busybox.net >> http://lists.busybox.net/mailman/listinfo/buildroot
Hi Dan, On 01/03/2021 03:40, Dan Raymond wrote: > From 4db6db355723bedb5918be214fbd6c6eb4a39257 Mon Sep 17 00:00:00 2001 > From: Dan Raymond <draymond@foxvalley.net> > Date: Sun, 28 Feb 2021 18:27:18 -0700 > Subject: [PATCH 1/1] fix failure during 'make linux-menuconfig' when PATH env > var has spaces > > Signed-off-by: Dan Raymond <draymond@foxvalley.net> > --- > package/pkg-kconfig.mk | 6 +++--- > support/misc/utils.mk | 6 ++++++ > 2 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/package/pkg-kconfig.mk b/package/pkg-kconfig.mk > index 2aecf2e203..6bf87fbead 100644 > --- a/package/pkg-kconfig.mk > +++ b/package/pkg-kconfig.mk > @@ -205,9 +205,9 @@ endif > # nconfig, gconfig, xconfig). > # So we simply remove our PATH and PKG_CONFIG_* variables. > $(2)_CONFIGURATOR_MAKE_ENV = \ > - $$(filter-out PATH=% PKG_CONFIG=% PKG_CONFIG_SYSROOT_DIR=% \ > - PKG_CONFIG_ALLOW_SYSTEM_CFLAGS=% PKG_CONFIG_ALLOW_SYSTEM_LIBS=% \ > - PKG_CONFIG_LIBDIR=%,$$($(2)_MAKE_ENV)) \ > + $$(call strip_env,PATH PKG_CONFIG PKG_CONFIG_SYSROOT_DIR \ > + PKG_CONFIG_ALLOW_SYSTEM_CFLAGS PKG_CONFIG_ALLOW_SYSTEM_LIBS \ > + PKG_CONFIG_LIBDIR,$$($(2)_MAKE_ENV)) \ > PKG_CONFIG_PATH="$(HOST_PKG_CONFIG_PATH)" > > # Configuration editors (menuconfig, ...) > diff --git a/support/misc/utils.mk b/support/misc/utils.mk > index dc60cad979..640e56c775 100644 > --- a/support/misc/utils.mk > +++ b/support/misc/utils.mk > @@ -135,3 +135,9 @@ define PRINTF > $(subst $(QUOTE),$(QUOTE)\$(QUOTE)$(QUOTE),\ > $(subst \,\\,$(1)))))\n' > endef > + > +# strip variables (quoted or unquoted) from an environment string: > +# MYENV = PATH="/bin:/dir with spaces" USER=root SHELL=/bin/sh > +# $(call strip_env,PATH SHELL,$(MYENV)) --> USER=root > +sed_cmds=$(foreach var,$(1),-e 's/$(var)="[^"]*" *//' -e 's/$(var)=[^ ]* *//') > +strip_env=$(shell echo "$(subst ",\",$(2))" | sed $(call sed_cmds,$(1))) This is a bit of half-hearted quoting. It happens to work for spaces, but breaks down on double quotes, backslashes, dollar signs, and probably others as well. I don't like to solve one specific corner case but leaving the door wide open for different ones... Unfortunately, proper shell quoting is really really hard. Therefore, I'd propose to skirt around the issue entirely by overriding the environment variables directly. Something like this (untested): $(2)_CONFIGURATOR_MAKE_ENV = \ $$($(2)_MAKE_ENV) \ PATH="$$(BR_PATH)" \ PKG_CONFIG= \ ... Regards, Arnout
On 3/3/2021 10:11 AM, Arnout Vandecappelle wrote: > Hi Dan, > > On 01/03/2021 03:40, Dan Raymond wrote: >> $(2)_CONFIGURATOR_MAKE_ENV = \ >> - $$(filter-out PATH=% PKG_CONFIG=% PKG_CONFIG_SYSROOT_DIR=% \ >> - PKG_CONFIG_ALLOW_SYSTEM_CFLAGS=% PKG_CONFIG_ALLOW_SYSTEM_LIBS=% \ >> - PKG_CONFIG_LIBDIR=%,$$($(2)_MAKE_ENV)) \ >> + $$(call strip_env,PATH PKG_CONFIG PKG_CONFIG_SYSROOT_DIR \ >> + PKG_CONFIG_ALLOW_SYSTEM_CFLAGS PKG_CONFIG_ALLOW_SYSTEM_LIBS \ >> + PKG_CONFIG_LIBDIR,$$($(2)_MAKE_ENV)) \ >> PKG_CONFIG_PATH="$(HOST_PKG_CONFIG_PATH)" > This is a bit of half-hearted quoting. It happens to work for spaces, but > breaks down on double quotes, backslashes, dollar signs, and probably others as > well. > > I don't like to solve one specific corner case but leaving the door wide open > for different ones... > > Unfortunately, proper shell quoting is really really hard. > > Therefore, I'd propose to skirt around the issue entirely by overriding the > environment variables directly. Something like this (untested): > > $(2)_CONFIGURATOR_MAKE_ENV = \ > $$($(2)_MAKE_ENV) \ > PATH="$$(BR_PATH)" \ > PKG_CONFIG= \ > ... I like your idea. It is much simpler. I'll prepare a new patch and test it. But why are you setting PATH="$$(BR_PATH)"? Previously the PATH variable was stripped entirely.
On 3/3/2021 10:37 AM, Dan Raymond wrote: > On 3/3/2021 10:11 AM, Arnout Vandecappelle wrote: > >> Hi Dan, >> >> On 01/03/2021 03:40, Dan Raymond wrote: >>> $(2)_CONFIGURATOR_MAKE_ENV = \ >>> - $$(filter-out PATH=% PKG_CONFIG=% PKG_CONFIG_SYSROOT_DIR=% \ >>> - PKG_CONFIG_ALLOW_SYSTEM_CFLAGS=% >>> PKG_CONFIG_ALLOW_SYSTEM_LIBS=% \ >>> - PKG_CONFIG_LIBDIR=%,$$($(2)_MAKE_ENV)) \ >>> + $$(call strip_env,PATH PKG_CONFIG PKG_CONFIG_SYSROOT_DIR \ >>> + PKG_CONFIG_ALLOW_SYSTEM_CFLAGS PKG_CONFIG_ALLOW_SYSTEM_LIBS \ >>> + PKG_CONFIG_LIBDIR,$$($(2)_MAKE_ENV)) \ >>> PKG_CONFIG_PATH="$(HOST_PKG_CONFIG_PATH)" >> This is a bit of half-hearted quoting. It happens to work for spaces, >> but >> breaks down on double quotes, backslashes, dollar signs, and probably >> others as >> well. >> >> I don't like to solve one specific corner case but leaving the door >> wide open >> for different ones... >> >> Unfortunately, proper shell quoting is really really hard. >> >> Therefore, I'd propose to skirt around the issue entirely by >> overriding the >> environment variables directly. Something like this (untested): >> >> $(2)_CONFIGURATOR_MAKE_ENV = \ >> $$($(2)_MAKE_ENV) \ >> PATH="$$(BR_PATH)" \ >> PKG_CONFIG= \ >> ... > > I like your idea. It is much simpler. I'll prepare a new patch and > test it. But why are you setting PATH="$$(BR_PATH)"? Previously the > PATH variable was stripped entirely. OK, I see why we can't just set an empty path (none of the build tools will be found). I got the following to work. You can't quote $(BR_PATH) because it is already quoted and you only need one $. $(2)_CONFIGURATOR_MAKE_ENV = \ $$($(2)_MAKE_ENV) \ PATH=$(BR_PATH) \ PKG_CONFIG= \ PKG_CONFIG_SYSROOT_DIR= \ PKG_CONFIG_ALLOW_SYSTEM_CFLAGS= \ PKG_CONFIG_ALLOW_SYSTEM_LIBS= \ PKG_CONFIG_LIBDIR= \ PKG_CONFIG_PATH="$(HOST_PKG_CONFIG_PATH)" However the result is equivalent to leaving the PATH alone: $(2)_CONFIGURATOR_MAKE_ENV = \ $$($(2)_MAKE_ENV) \ PKG_CONFIG= \ PKG_CONFIG_SYSROOT_DIR= \ PKG_CONFIG_ALLOW_SYSTEM_CFLAGS= \ PKG_CONFIG_ALLOW_SYSTEM_LIBS= \ PKG_CONFIG_LIBDIR= \ PKG_CONFIG_PATH="$(HOST_PKG_CONFIG_PATH)"
On 3/3/2021 11:18 AM, Dan Raymond wrote: > OK, I see why we can't just set an empty path (none of the build tools > will be found). I got the following to work. You can't quote > $(BR_PATH) because it is already quoted and you only need one $. > > $(2)_CONFIGURATOR_MAKE_ENV = \ > $$($(2)_MAKE_ENV) \ > PATH=$(BR_PATH) \ > PKG_CONFIG= \ > PKG_CONFIG_SYSROOT_DIR= \ > PKG_CONFIG_ALLOW_SYSTEM_CFLAGS= \ > PKG_CONFIG_ALLOW_SYSTEM_LIBS= \ > PKG_CONFIG_LIBDIR= \ > PKG_CONFIG_PATH="$(HOST_PKG_CONFIG_PATH)" > > However the result is equivalent to leaving the PATH alone: > > $(2)_CONFIGURATOR_MAKE_ENV = \ > $$($(2)_MAKE_ENV) \ > PKG_CONFIG= \ > PKG_CONFIG_SYSROOT_DIR= \ > PKG_CONFIG_ALLOW_SYSTEM_CFLAGS= \ > PKG_CONFIG_ALLOW_SYSTEM_LIBS= \ > PKG_CONFIG_LIBDIR= \ > PKG_CONFIG_PATH="$(HOST_PKG_CONFIG_PATH)" My testing would imply that we don't need to strip the PATH at all here. Is there someone more familiar with this area that can clarify?
diff --git a/package/pkg-kconfig.mk b/package/pkg-kconfig.mk index 2aecf2e203..6bf87fbead 100644 --- a/package/pkg-kconfig.mk +++ b/package/pkg-kconfig.mk @@ -205,9 +205,9 @@ endif # nconfig, gconfig, xconfig). # So we simply remove our PATH and PKG_CONFIG_* variables. $(2)_CONFIGURATOR_MAKE_ENV = \ - $$(filter-out PATH=% PKG_CONFIG=% PKG_CONFIG_SYSROOT_DIR=% \ - PKG_CONFIG_ALLOW_SYSTEM_CFLAGS=% PKG_CONFIG_ALLOW_SYSTEM_LIBS=% \ - PKG_CONFIG_LIBDIR=%,$$($(2)_MAKE_ENV)) \ + $$(call strip_env,PATH PKG_CONFIG PKG_CONFIG_SYSROOT_DIR \ + PKG_CONFIG_ALLOW_SYSTEM_CFLAGS PKG_CONFIG_ALLOW_SYSTEM_LIBS \ + PKG_CONFIG_LIBDIR,$$($(2)_MAKE_ENV)) \ PKG_CONFIG_PATH="$(HOST_PKG_CONFIG_PATH)" # Configuration editors (menuconfig, ...) diff --git a/support/misc/utils.mk b/support/misc/utils.mk index dc60cad979..640e56c775 100644 --- a/support/misc/utils.mk +++ b/support/misc/utils.mk @@ -135,3 +135,9 @@ define PRINTF $(subst $(QUOTE),$(QUOTE)\$(QUOTE)$(QUOTE),\ $(subst \,\\,$(1)))))\n' endef + +# strip variables (quoted or unquoted) from an environment string: +# MYENV = PATH="/bin:/dir with spaces" USER=root SHELL=/bin/sh +# $(call strip_env,PATH SHELL,$(MYENV)) --> USER=root +sed_cmds=$(foreach var,$(1),-e 's/$(var)="[^"]*" *//' -e 's/$(var)=[^ ]* *//')