| Message ID | 20170418010011.GC4152@decadent.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Apr 18, 2017 at 02:00:11AM +0100, Ben Hutchings wrote: > This definition in Makefile.dtbinst: > > export dtbinst-root ?= $(obj) > > should define and export dtbinst-root when handling the root dts > directory, and do nothing in the subdirectories. However, the > variable does not reliably get exported to the environment, perhaps > because its name contains a hyphen. > > Rename the variable to dtbinst_root. Exported environment variables are usually in UPPPER CASE. And also listed in: Documentation/kbuild/kbuild.txt There are deviations from this already, so not something that is followed by all of kbuild. Sam -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Ben, 2017-04-18 10:00 GMT+09:00 Ben Hutchings <ben@decadent.org.uk>: > This definition in Makefile.dtbinst: > > export dtbinst-root ?= $(obj) > > should define and export dtbinst-root when handling the root dts > directory, and do nothing in the subdirectories. However, the > variable does not reliably get exported to the environment, perhaps > because its name contains a hyphen. > > Rename the variable to dtbinst_root. > > References: https://bugs.debian.org/833561 > Fixes: 323a028d39cdi ("dts, kbuild: Implement support for dtb vendor subdirs") > Signed-off-by: Ben Hutchings <ben@decadent.org.uk> > --- > --- a/scripts/Makefile.dtbinst > +++ b/scripts/Makefile.dtbinst > @@ -14,7 +14,7 @@ src := $(obj) > PHONY := __dtbs_install > __dtbs_install: > > -export dtbinst-root ?= $(obj) > +export dtbinst_root ?= $(obj) > > include include/config/auto.conf > include scripts/Kbuild.include > @@ -22,7 +22,7 @@ include $(src)/Makefile > > PHONY += __dtbs_install_prep > __dtbs_install_prep: > -ifeq ("$(dtbinst-root)", "$(obj)") > +ifeq ("$(dtbinst_root)", "$(obj)") > $(Q)mkdir -p $(INSTALL_DTBS_PATH) > endif > > @@ -33,7 +33,7 @@ dtbinst-dirs := $(dts-dirs) > quiet_cmd_dtb_install = INSTALL $< > cmd_dtb_install = mkdir -p $(2); cp $< $(2) > > -install-dir = $(patsubst $(dtbinst-root)%,$(INSTALL_DTBS_PATH)%,$(obj)) > +install-dir = $(patsubst $(dtbinst_root)%,$(INSTALL_DTBS_PATH)%,$(obj)) > > $(dtbinst-files) $(dtbinst-dirs): | __dtbs_install_prep > I tested dtbs_install once again by myself, but dtbinst-root is exported to the sub make and the vendor directories are created correctly. I checked the debian's forum you gave > References: https://bugs.debian.org/833561 In there, you mentioned: "This looks like a bug in make, but we can at least work around it by using a non-hyphenated variable name." Does this issue happen on a specific Make version? I tested GNU make 3.81, 3.82, 4.0, 4.1, 4.2, but I was not hit by the problem. In the last post in the thread, you concluded: "We believe that the bug you reported is fixed in the latest version of linux, which is due to be installed in the Debian FTP archive." If so, why is this patch here? How is the dtbs_install procedure different in the Debian package?
On Sun, 2017-04-23 at 15:47 +0900, Masahiro Yamada wrote: [...] > I tested dtbs_install once again by myself, but > dtbinst-root is exported to the sub make > and the vendor directories are created correctly. > > > I checked the debian's forum you gave > > References: https://bugs.debian.org/833561 > > In there, you mentioned: > "This looks like a bug in make, but we can at least work around it by > using a non-hyphenated variable name." > > > Does this issue happen on a specific Make version? > > I tested GNU make 3.81, 3.82, 4.0, 4.1, 4.2, > but I was not hit by the problem. I don't think this is make version dependent. I can't reproduce the issue today with make 4.1. But I would have been using the same version in August when I wrote that. What more can I say? Clearly the hyphenated variable gets passed to the sub-make in most cases. But it's not totally reliable because last year it wasn't working for us. > In the last post in the thread, you concluded: > "We believe that the bug you reported is fixed in the latest version of > linux, which is due to be installed in the Debian FTP archive." I didn't write that, it's a standard message generated for bugs marked as closed in a package changelog. :-) > If so, why is this patch here? > How is the dtbs_install procedure different in the Debian package? This is the patch I applied to the package. Ben.
Hi Ben, 2017-04-23 16:23 GMT+09:00 Ben Hutchings <ben@decadent.org.uk>: > On Sun, 2017-04-23 at 15:47 +0900, Masahiro Yamada wrote: > [...] >> I tested dtbs_install once again by myself, but >> dtbinst-root is exported to the sub make >> and the vendor directories are created correctly. >> >> >> I checked the debian's forum you gave >> > References: https://bugs.debian.org/833561 >> >> In there, you mentioned: >> "This looks like a bug in make, but we can at least work around it by >> using a non-hyphenated variable name." >> >> >> Does this issue happen on a specific Make version? >> >> I tested GNU make 3.81, 3.82, 4.0, 4.1, 4.2, >> but I was not hit by the problem. > > I don't think this is make version dependent. I can't reproduce the > issue today with make 4.1. But I would have been using the same > version in August when I wrote that. > > What more can I say? Clearly the hyphenated variable gets passed to > the sub-make in most cases. But it's not totally reliable because last > year it wasn't working for us. > >> In the last post in the thread, you concluded: >> "We believe that the bug you reported is fixed in the latest version of >> linux, which is due to be installed in the Debian FTP archive." > > I didn't write that, it's a standard message generated for bugs marked > as closed in a package changelog. :-) > >> If so, why is this patch here? >> How is the dtbs_install procedure different in the Debian package? > > This is the patch I applied to the package. > Do you still need this patch for Debian? If so, I can pick it up because it is apparently harmless (maybe safer as you said). However, I may add the following excuse in the git-log: [masahiro I cound not reproduce the issue, but at least this patch is harmless.]
On Sun, 2017-04-30 at 23:14 +0900, Masahiro Yamada wrote: > Hi Ben, > > > > 2017-04-23 16:23 GMT+09:00 Ben Hutchings <ben@decadent.org.uk>: > > On Sun, 2017-04-23 at 15:47 +0900, Masahiro Yamada wrote: > > [...] > > > I tested dtbs_install once again by myself, but > > > dtbinst-root is exported to the sub make > > > and the vendor directories are created correctly. > > > > > > > > > I checked the debian's forum you gave > > > > References: https://bugs.debian.org/833561 > > > > > > In there, you mentioned: > > > "This looks like a bug in make, but we can at least work around it by > > > using a non-hyphenated variable name." > > > > > > > > > Does this issue happen on a specific Make version? > > > > > > I tested GNU make 3.81, 3.82, 4.0, 4.1, 4.2, > > > but I was not hit by the problem. > > > > I don't think this is make version dependent. I can't reproduce the > > issue today with make 4.1. But I would have been using the same > > version in August when I wrote that. > > > > What more can I say? Clearly the hyphenated variable gets passed to > > the sub-make in most cases. But it's not totally reliable because last > > year it wasn't working for us. > > > > > In the last post in the thread, you concluded: > > > "We believe that the bug you reported is fixed in the latest version of > > > linux, which is due to be installed in the Debian FTP archive." > > > > I didn't write that, it's a standard message generated for bugs marked > > as closed in a package changelog. :-) > > > > > If so, why is this patch here? > > > How is the dtbs_install procedure different in the Debian package? > > > > This is the patch I applied to the package. > > > > Do you still need this patch for Debian? [...] I don't think so. I just don't know for sure. Ben.
Hi Ben, 2017-04-30 23:49 GMT+09:00 Ben Hutchings <ben@decadent.org.uk>: > On Sun, 2017-04-30 at 23:14 +0900, Masahiro Yamada wrote: >> Hi Ben, >> >> >> > 2017-04-23 16:23 GMT+09:00 Ben Hutchings <ben@decadent.org.uk>: >> > On Sun, 2017-04-23 at 15:47 +0900, Masahiro Yamada wrote: >> > [...] >> > > I tested dtbs_install once again by myself, but >> > > dtbinst-root is exported to the sub make >> > > and the vendor directories are created correctly. >> > > >> > > >> > > I checked the debian's forum you gave >> > > > References: https://bugs.debian.org/833561 >> > > >> > > In there, you mentioned: >> > > "This looks like a bug in make, but we can at least work around it by >> > > using a non-hyphenated variable name." >> > > >> > > >> > > Does this issue happen on a specific Make version? >> > > >> > > I tested GNU make 3.81, 3.82, 4.0, 4.1, 4.2, >> > > but I was not hit by the problem. >> > >> > I don't think this is make version dependent. I can't reproduce the >> > issue today with make 4.1. But I would have been using the same >> > version in August when I wrote that. >> > >> > What more can I say? Clearly the hyphenated variable gets passed to >> > the sub-make in most cases. But it's not totally reliable because last >> > year it wasn't working for us. >> > >> > > In the last post in the thread, you concluded: >> > > "We believe that the bug you reported is fixed in the latest version of >> > > linux, which is due to be installed in the Debian FTP archive." >> > >> > I didn't write that, it's a standard message generated for bugs marked >> > as closed in a package changelog. :-) >> > >> > > If so, why is this patch here? >> > > How is the dtbs_install procedure different in the Debian package? >> > >> > This is the patch I applied to the package. >> > >> >> Do you still need this patch for Debian? > [...] > > I don't think so. I just don't know for sure. > > Ben. I postpone this patch for now. Please re-post if this patch turns out to be really needed.
On Sun, 2017-04-30 at 15:49 +0100, Ben Hutchings wrote: > On Sun, 2017-04-30 at 23:14 +0900, Masahiro Yamada wrote: > > Hi Ben, > > > > > > > 2017-04-23 16:23 GMT+09:00 Ben Hutchings <ben@decadent.org.uk>: > > > On Sun, 2017-04-23 at 15:47 +0900, Masahiro Yamada wrote: > > > [...] > > > > I tested dtbs_install once again by myself, but > > > > dtbinst-root is exported to the sub make > > > > and the vendor directories are created correctly. > > > > > > > > > > > > I checked the debian's forum you gave > > > > > References: https://bugs.debian.org/833561 > > > > > > > > In there, you mentioned: > > > > "This looks like a bug in make, but we can at least work around it by > > > > using a non-hyphenated variable name." > > > > > > > > > > > > Does this issue happen on a specific Make version? > > > > > > > > I tested GNU make 3.81, 3.82, 4.0, 4.1, 4.2, > > > > but I was not hit by the problem. > > > > > > I don't think this is make version dependent. I can't reproduce the > > > issue today with make 4.1. But I would have been using the same > > > version in August when I wrote that. > > > > > > What more can I say? Clearly the hyphenated variable gets passed to > > > the sub-make in most cases. But it's not totally reliable because last > > > year it wasn't working for us. > > > > > > > In the last post in the thread, you concluded: > > > > "We believe that the bug you reported is fixed in the latest version of > > > > linux, which is due to be installed in the Debian FTP archive." > > > > > > I didn't write that, it's a standard message generated for bugs marked > > > as closed in a package changelog. :-) > > > > > > > If so, why is this patch here? > > > > How is the dtbs_install procedure different in the Debian package? > > > > > > This is the patch I applied to the package. > > > > > > > Do you still need this patch for Debian? > [...] > > I don't think so. I just don't know for sure. I've now seen a similar problem with the suffix-y variable exported from arch/sh/boot/Makefile to arch/sh/boot/compressed/Makefile: https://buildd.debian.org/status/fetch.php?pkg=linux&arch=sh4&ver=4.13%7Erc5-1%7Eexp1&stamp=1502943967&raw=0 Those Makefiles haven't changed recently, so there must be some difference in the build environment. Here's a Makefile that demonstrates the difference in behaviour between bash and dash: --- BEGIN --- ifdef WANTED_SHELL SHELL := $(WANTED_SHELL) endif test: @WANTED_SHELL=/bin/bash $(MAKE) test2 @WANTED_SHELL=/bin/dash $(MAKE) test2 @WANTED_SHELL= $(MAKE) test2 test2: export foo_bar := foo_bar test2: export foo-bar := foo-bar test2: @echo SHELL=$(SHELL) @$(MAKE) test3 test3: @echo foo_bar=$(foo_bar) @echo foo-bar=$(foo-bar) .PHONY: test test2 test3 --- END --- For me, this produces: $ make -s SHELL=/bin/bash foo_bar=foo_bar foo-bar=foo-bar SHELL=/bin/dash foo_bar=foo_bar foo-bar= SHELL=/bin/sh foo_bar=foo_bar foo-bar=foo-bar Note that the last two cases are different even though /bin/sh is dash here. It turns out that make avoids using the shell for simple commands, unless it has been changed from the default: https://sources.debian.net/src/make-dfsg/4.1-9.1/job.c/#L2575 Ben.
Hi Ben, Thanks for useful info. 2017-08-19 10:13 GMT+09:00 Ben Hutchings <ben@decadent.org.uk>: > On Sun, 2017-04-30 at 15:49 +0100, Ben Hutchings wrote: >> On Sun, 2017-04-30 at 23:14 +0900, Masahiro Yamada wrote: >> > Hi Ben, >> > >> > >> > > 2017-04-23 16:23 GMT+09:00 Ben Hutchings <ben@decadent.org.uk>: >> > > On Sun, 2017-04-23 at 15:47 +0900, Masahiro Yamada wrote: >> > > [...] >> > > > I tested dtbs_install once again by myself, but >> > > > dtbinst-root is exported to the sub make >> > > > and the vendor directories are created correctly. >> > > > >> > > > >> > > > I checked the debian's forum you gave >> > > > > References: https://bugs.debian.org/833561 >> > > > >> > > > In there, you mentioned: >> > > > "This looks like a bug in make, but we can at least work around it by >> > > > using a non-hyphenated variable name." >> > > > >> > > > >> > > > Does this issue happen on a specific Make version? >> > > > >> > > > I tested GNU make 3.81, 3.82, 4.0, 4.1, 4.2, >> > > > but I was not hit by the problem. >> > > >> > > I don't think this is make version dependent. I can't reproduce the >> > > issue today with make 4.1. But I would have been using the same >> > > version in August when I wrote that. >> > > >> > > What more can I say? Clearly the hyphenated variable gets passed to >> > > the sub-make in most cases. But it's not totally reliable because last >> > > year it wasn't working for us. >> > > >> > > > In the last post in the thread, you concluded: >> > > > "We believe that the bug you reported is fixed in the latest version of >> > > > linux, which is due to be installed in the Debian FTP archive." >> > > >> > > I didn't write that, it's a standard message generated for bugs marked >> > > as closed in a package changelog. :-) >> > > >> > > > If so, why is this patch here? >> > > > How is the dtbs_install procedure different in the Debian package? >> > > >> > > This is the patch I applied to the package. >> > > >> > >> > Do you still need this patch for Debian? >> [...] >> >> I don't think so. I just don't know for sure. > > I've now seen a similar problem with the suffix-y variable exported > from arch/sh/boot/Makefile to arch/sh/boot/compressed/Makefile: > https://buildd.debian.org/status/fetch.php?pkg=linux&arch=sh4&ver=4.13%7Erc5-1%7Eexp1&stamp=1502943967&raw=0 > > Those Makefiles haven't changed recently, so there must be some > difference in the build environment. Here's a Makefile that > demonstrates the difference in behaviour between bash and dash: > > --- BEGIN --- > ifdef WANTED_SHELL > SHELL := $(WANTED_SHELL) > endif > > test: > @WANTED_SHELL=/bin/bash $(MAKE) test2 > @WANTED_SHELL=/bin/dash $(MAKE) test2 > @WANTED_SHELL= $(MAKE) test2 > > test2: export foo_bar := foo_bar > test2: export foo-bar := foo-bar > test2: > @echo SHELL=$(SHELL) > @$(MAKE) test3 > > test3: > @echo foo_bar=$(foo_bar) > @echo foo-bar=$(foo-bar) > > .PHONY: test test2 test3 > --- END --- > > For me, this produces: > > $ make -s > SHELL=/bin/bash > foo_bar=foo_bar > foo-bar=foo-bar > SHELL=/bin/dash > foo_bar=foo_bar > foo-bar= > SHELL=/bin/sh > foo_bar=foo_bar > foo-bar=foo-bar > > Note that the last two cases are different even though /bin/sh is dash > here. It turns out that make avoids using the shell for simple > commands, unless it has been changed from the default: > https://sources.debian.net/src/make-dfsg/4.1-9.1/job.c/#L2575 Yes. This is also explained in O'Reilly GNU Make: http://www.oreilly.com/openbook/make3/book/ch05.pdf ------------------->8------------------------------ Commands are essentially one-line shell scripts. In effect, make grabs each line and passes it to a subshell for execution. In fact, make can optimize this (relatively) expensive fork/exec algorithm if it can guarantee that omitting the shell will not change the behavior of the program. It checks this by scanning each command line for shell special characters, such as wildcard characters and i/o redirection. If none are found, make directly executes the command without passing it to a subshell. By default, /bin/sh is used for the shell. This shell is controlled by the make variable SHELL but it is not inherited from the environment. When make starts, it imports all the variables from the user’s environment as make variables, except SHELL. This is because the user’s choice of shell should not cause a makefile (possibly included in some downloaded software package) to fail. If a user really wants to change the default shell used by make, he can set the SHELL variable explicitly in the makefile. We will discuss this issue in the section “Which Shell to Use” later in this chapter ------------------->8------------------------------ From your test case (the third one), if SHELL is unset, it should work whether /bin/sh is dash, bash, or whatever. (I use Ubuntu, and /bin/sh is dash. I see no problem for installing dtbs.) We see the problem only when SHELL is set to /bin/dash. foo-bar becomes empty if SHELL is set to /bin/dash in a Makefile that exports it. SHELL in a Makefile that references $(foo-bar) is don't care. The following is the test scripts. ---------------- Makefile ---------------- ifneq ($(WANTED_SHELL),) SHELL := $(WANTED_SHELL) endif export foo_bar := foo_bar export foo-bar := foo-bar test: @WANTED_SHELL= $(MAKE) test2 @WANTED_SHELL=/bin/dash $(MAKE) test2 test2: @echo SHELL of parent-Makefile is $(SHELL) $(MAKE) -C sub CHILD_SHELL=/bin/sh $(MAKE) -C sub CHILD_SHELL=/bin/dash ------------------------------------------- ------------------ sub/Makefile ----------- SHELL := $(CHILD_SHELL) sub: @echo SHELL of sub-Makefile is $(SHELL) @echo foo_bar $(foo_bar) @echo foo-bar $(foo-bar) .PHONY: sub ------------------------------------------- My test result was as follows: $ make --no-print-directory SHELL of parent-Makefile is /bin/sh make -C sub CHILD_SHELL=/bin/sh SHELL of sub-Makefile is /bin/sh foo_bar foo_bar foo-bar foo-bar make -C sub CHILD_SHELL=/bin/dash SHELL of sub-Makefile is /bin/dash foo_bar foo_bar foo-bar foo-bar SHELL of parent-Makefile is /bin/dash make -C sub CHILD_SHELL=/bin/sh SHELL of sub-Makefile is /bin/sh foo_bar foo_bar foo-bar make -C sub CHILD_SHELL=/bin/dash SHELL of sub-Makefile is /bin/dash foo_bar foo_bar foo-bar I did "grep SHELL" in kernel sources, but I could not find suspicious line. Is there anyone that sets SHELL in debian specific patches?
On Sun, 2017-08-20 at 02:37 +0900, Masahiro Yamada wrote: [...] > I did "grep SHELL" in kernel sources, but I could not > find suspicious line. > > > Is there anyone that sets SHELL > in debian specific patches? No, there isn't. But I finally worked out the trigger conditions: 1. Source directory name includes a shell special character. This is true in Debian whenever we build a release candidate, because the source directory is named after the package version, and we use ~ to represent a pre-release version (e.g. 4.13~rc5 sorts before 4.13). 2. Object directory is outside the source directory, or at least two levels below it (see the definition of srctree in the top level Makefile). This is always true in Debian package builds. Shell comamnds to reproduce this: rsync --archive --exclude /.git/ . /tmp/linux~/ cd /tmp/linux~ make mrproper make O=one/two ARCH=arm64 defconfig make -C one/two ARCH=arm64 dtbs make -C one/two ARCH=arm64 INSTALL_DTBS_PATH=dtbs~ dtbs_install Ben.
--- a/scripts/Makefile.dtbinst +++ b/scripts/Makefile.dtbinst @@ -14,7 +14,7 @@ src := $(obj) PHONY := __dtbs_install __dtbs_install: -export dtbinst-root ?= $(obj) +export dtbinst_root ?= $(obj) include include/config/auto.conf include scripts/Kbuild.include @@ -22,7 +22,7 @@ include $(src)/Makefile PHONY += __dtbs_install_prep __dtbs_install_prep: -ifeq ("$(dtbinst-root)", "$(obj)") +ifeq ("$(dtbinst_root)", "$(obj)") $(Q)mkdir -p $(INSTALL_DTBS_PATH) endif @@ -33,7 +33,7 @@ dtbinst-dirs := $(dts-dirs) quiet_cmd_dtb_install = INSTALL $< cmd_dtb_install = mkdir -p $(2); cp $< $(2) -install-dir = $(patsubst $(dtbinst-root)%,$(INSTALL_DTBS_PATH)%,$(obj)) +install-dir = $(patsubst $(dtbinst_root)%,$(INSTALL_DTBS_PATH)%,$(obj)) $(dtbinst-files) $(dtbinst-dirs): | __dtbs_install_prep
This definition in Makefile.dtbinst: export dtbinst-root ?= $(obj) should define and export dtbinst-root when handling the root dts directory, and do nothing in the subdirectories. However, the variable does not reliably get exported to the environment, perhaps because its name contains a hyphen. Rename the variable to dtbinst_root. References: https://bugs.debian.org/833561 Fixes: 323a028d39cdi ("dts, kbuild: Implement support for dtb vendor subdirs") Signed-off-by: Ben Hutchings <ben@decadent.org.uk> ---