Re: The who needs reviews anyways [PATCH]
On Fri, Feb 09, 2007 at 12:35:04PM +0100, Roman Zippel wrote: [] > > > +$(error bash is required to build the kernel) > > > +endif > > > +SHELL := $(CONFIG_SHELL) > > > > here is policy to have `bash' introduced, so due to original > > issue, where `root' users ended with removed /dev/null, may policy to have > > `non root' user to build kernel be added? Thus > > I doubt gentoo user will like you for that and above is more a de facto > requirement that bash is needed for kbuild. The alternative is to > introduce a new policy that everything is POSIX clean. Bad thing is, that `man bash' has no clean line between `sh' and bash-specific features. POSIX it or `common practice' doesn't matter, one just must try to test shell scripts with `busybox' or any other `sh' compatible shell. > > this: > > > + rm -f "$$TMP") > > > > may be removed, and to make TMP=/dev/null? And to forget currently about > > my silly symlinks, and this crappy sets of output files? > > This still wouldn't work, as these tests are also done when running 'make > install'. and/or "make modules_install", and this must be fixed: to have only one configuration-run. And then to use its results. > > > -as-instr = $(call checker-shell,\ > > > - printf "$(1)" | $(CC) $(AFLAGS) -c -xassembler -o $$OUT -,$(2),$(3)) > > > > `printf $(1)' is pretty enough. > > As Andreas suggested 'printf "%b" "$(1)"' would be the other alternative. It will not help against single double quote in $(1) - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: The who needs reviews anyways [PATCH]
Hi, On Fri, 9 Feb 2007, Oleg Verych wrote: > > - else if [ -x /bin/bash ]; then echo /bin/bash; \ > > - else echo sh; fi ; fi) > > + else if [ -x /bin/bash ]; then echo /bin/bash; fi; fi) > > +ifeq ($(CONFIG_SHELL),) > > +$(error bash is required to build the kernel) > > +endif > > +SHELL := $(CONFIG_SHELL) > > here is policy to have `bash' introduced, so due to original > issue, where `root' users ended with removed /dev/null, may policy to have > `non root' user to build kernel be added? Thus I doubt gentoo user will like you for that and above is more a de facto requirement that bash is needed for kbuild. The alternative is to introduce a new policy that everything is POSIX clean. > this: > > + rm -f "$$TMP") > > may be removed, and to make TMP=/dev/null? And to forget currently about > my silly symlinks, and this crappy sets of output files? This still wouldn't work, as these tests are also done when running 'make install'. > > -as-instr = $(call checker-shell,\ > > - printf "$(1)" | $(CC) $(AFLAGS) -c -xassembler -o $$OUT -,$(2),$(3)) > > `printf $(1)' is pretty enough. As Andreas suggested 'printf "%b" "$(1)"' would be the other alternative. bye, Roman - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: The who needs reviews anyways [PATCH]
Hi, On Fri, 9 Feb 2007, Oleg Verych wrote: - else if [ -x /bin/bash ]; then echo /bin/bash; \ - else echo sh; fi ; fi) + else if [ -x /bin/bash ]; then echo /bin/bash; fi; fi) +ifeq ($(CONFIG_SHELL),) +$(error bash is required to build the kernel) +endif +SHELL := $(CONFIG_SHELL) here is policy to have `bash' introduced, so due to original issue, where `root' users ended with removed /dev/null, may policy to have `non root' user to build kernel be added? Thus I doubt gentoo user will like you for that and above is more a de facto requirement that bash is needed for kbuild. The alternative is to introduce a new policy that everything is POSIX clean. this: + rm -f $$TMP) may be removed, and to make TMP=/dev/null? And to forget currently about my silly symlinks, and this crappy sets of output files? This still wouldn't work, as these tests are also done when running 'make install'. -as-instr = $(call checker-shell,\ - printf $(1) | $(CC) $(AFLAGS) -c -xassembler -o $$OUT -,$(2),$(3)) `printf $(1)' is pretty enough. As Andreas suggested 'printf %b $(1)' would be the other alternative. bye, Roman - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: The who needs reviews anyways [PATCH]
On Fri, Feb 09, 2007 at 12:35:04PM +0100, Roman Zippel wrote: [] +$(error bash is required to build the kernel) +endif +SHELL := $(CONFIG_SHELL) here is policy to have `bash' introduced, so due to original issue, where `root' users ended with removed /dev/null, may policy to have `non root' user to build kernel be added? Thus I doubt gentoo user will like you for that and above is more a de facto requirement that bash is needed for kbuild. The alternative is to introduce a new policy that everything is POSIX clean. Bad thing is, that `man bash' has no clean line between `sh' and bash-specific features. POSIX it or `common practice' doesn't matter, one just must try to test shell scripts with `busybox' or any other `sh' compatible shell. this: + rm -f $$TMP) may be removed, and to make TMP=/dev/null? And to forget currently about my silly symlinks, and this crappy sets of output files? This still wouldn't work, as these tests are also done when running 'make install'. and/or make modules_install, and this must be fixed: to have only one configuration-run. And then to use its results. -as-instr = $(call checker-shell,\ - printf $(1) | $(CC) $(AFLAGS) -c -xassembler -o $$OUT -,$(2),$(3)) `printf $(1)' is pretty enough. As Andreas suggested 'printf %b $(1)' would be the other alternative. It will not help against single double quote in $(1) - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: The who needs reviews anyways [PATCH]
On Thu, Feb 08, 2007 at 10:48:51PM +0100, Roman Zippel wrote: [] > - printf has other side effects, instead stop pretending we support > something else than bash More on printf, `sh', tmpfiles. As we know original problem is: something from binutils is removing output files on failure. > - else if [ -x /bin/bash ]; then echo /bin/bash; \ > - else echo sh; fi ; fi) > + else if [ -x /bin/bash ]; then echo /bin/bash; fi; fi) > +ifeq ($(CONFIG_SHELL),) > +$(error bash is required to build the kernel) > +endif > +SHELL := $(CONFIG_SHELL) here is policy to have `bash' introduced, so due to original issue, where `root' users ended with removed /dev/null, may policy to have `non root' user to build kernel be added? Thus this: > +# output directory for tests below > +TMPOUT := $(if $(KBUILD_EXTMOD),$(firstword $(KBUILD_EXTMOD))/) [] > +# try-run > +# Usage: option = $(call try-run, $(CC)...-o "$$TMP",option-ok,otherwise) > +# Exit code chooses option. "$$TMP" is can be used as temporary file and > +# is automatically cleaned up. > +try-run = $(shell set -e;\ this: > + TMP="$(TMPOUT)..tmp"; \ [] > + if ($(1)) >/dev/null 2>&1; \ > + then echo "$(2)"; \ > + else echo "$(3)"; \ > + fi; \ this: > + rm -f "$$TMP") may be removed, and to make TMP=/dev/null? And to forget currently about my silly symlinks, and this crappy sets of output files? As for `printf', as i've wrote, only in case of % and quotes in arguments, something else must be added to handle that. But i think, it's paranoia. > -as-instr = $(call checker-shell,\ > - printf "$(1)" | $(CC) $(AFLAGS) -c -xassembler -o $$OUT -,$(2),$(3)) `printf $(1)' is pretty enough. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: The who needs reviews anyways [PATCH]
On Fri, 09 Feb 2007 00:20:49 +0100, Roman Zippel said: > > The point is, neither $BASH nor /bin/bash may be set. > > Is that really a problem? I think any system that has bash without > /bin/bash is simply broken. If you're trying to bootstrap a Linux box onto a new platform from some non-Linux Unixoid, it's possible that bash lives in $TOOLCHAIN/bin/bash. But I see that even Solaris 9 has a bash 2.05 in /bin/bash, so I might be talking out some odd orifice here. pgpA6X9IC9WHY.pgp Description: PGP signature
Re: The who needs reviews anyways [PATCH]
Hi, On Fri, 9 Feb 2007, Andreas Schwab wrote: > Roman Zippel <[EMAIL PROTECTED]> writes: > > > - printf has other side effects, instead stop pretending we support > > something else than bash > > printf is a much better echo, but you need to use it properly as well. > Either use %s to print a literal string or %b to let it interpret escape > sequences. Hmm, I didn't know about %b, which would indeed be another possibility here, although I think it would only make a real difference if we decided to make (and more importantly keep) all our scripts POSIX clean. bye, Roman - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: The who needs reviews anyways [PATCH]
Roman Zippel <[EMAIL PROTECTED]> writes: > - printf has other side effects, instead stop pretending we support > something else than bash printf is a much better echo, but you need to use it properly as well. Either use %s to print a literal string or %b to let it interpret escape sequences. Andreas. -- Andreas Schwab, SuSE Labs, [EMAIL PROTECTED] SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany PGP key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different." - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: The who needs reviews anyways [PATCH]
Hi, On Thu, 8 Feb 2007, Linus Torvalds wrote: > On Thu, 8 Feb 2007, Roman Zippel wrote: > > > I don't quite understand, the Makefile doesn't care anymore about /bin/sh > > with this patch, the Makefile checks only for $BASH and /bin/bash > > Exactly. > > The point is, neither $BASH nor /bin/bash may be set. Is that really a problem? I think any system that has bash without /bin/bash is simply broken. > If you run make while running tcsh, "BASH" won't be set. We've always just > defaulted to doing /bin/sh. Doesn't really seem to be a problem. There are chances this is already broken anyway. We call external scripts using $(shell $(CONFIG_SHELL) ...), which ignores the "#! ..." setting. bye, Roman - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: The who needs reviews anyways [PATCH]
On Thu, 8 Feb 2007, Roman Zippel wrote: > I don't quite understand, the Makefile doesn't care anymore about /bin/sh > with this patch, the Makefile checks only for $BASH and /bin/bash Exactly. The point is, neither $BASH nor /bin/bash may be set. If you run make while running tcsh, "BASH" won't be set. We've always just defaulted to doing /bin/sh. Doesn't really seem to be a problem. Linus - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Kbuild refactoring (Re: The who needs reviews anyways [PATCH])
On Thu, Feb 08, 2007 at 10:48:51PM +0100, Roman Zippel wrote: [] > - printf has other side effects, instead stop pretending we support > something else than bash Yes. With `%' in option strings there will be side effects. I would suggest to use printf %s "$(1)" with "paranoia mode on", and hope to do not forcing `bash'. > - proper quoting > - proper indentation > > bye, Roman Thanks. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: The who needs reviews anyways [PATCH]
Hi, On Thu, 8 Feb 2007, Linus Torvalds wrote: > Historically, people used to do: > - /bin/sh was the "standard shell" (bash) > - /bin/[t]csh is what clueless weenies use for interactive work. > > (Yeah, I'm not a [t]csh fan ;) > > And you did break that. > > It's quite possible that all modern distributions will install /bin/bash > as a link to /bin/sh, but I don't see the point of that particular change. > We aren't even all that bash-centric any more. If you have a > POSIX-compatible shell in /bin/sh, it really _should_ work. It just can't > be something really broken. I don't quite understand, the Makefile doesn't care anymore about /bin/sh with this patch, the Makefile checks only for $BASH and /bin/bash (equivalent to adding "#! /bin/bash" to scripts) and if the latter fails it's possible some of our scripts will fail. We could make sure that all our scripts are POSIX clean, but is it really worth the effort? It would make casual kbuild hacking only even more difficult, as one has to check it works with the various shells. > > - proper quoting > > - proper indentation > > One thing I'm wondering about is whether we could have a "does this warn" > test. I guess you can do it with -Werror, but it might be nice to have > some tests for "does the -Wxyzzy flag warn also for proper code" Adding something like try-compile should be possible, but we should be careful with this, the more checks we add the more work is done at every make invocation. bye, Roman - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: The who needs reviews anyways [PATCH]
On Thu, 8 Feb 2007, Roman Zippel wrote: > > Sorry for the delay, but the git server were gone. > > - the define command is inappropriate (it's primarily for rule > definitions) Looks fine. Especially considering the strange whitespace issues. > - execute commands in the current dir as all other commands > - .*.tmp (but not .*.null) files are also removed up by "make clean" > - printf has other side effects, instead stop pretending we support > something else than bash However, this one I have problems with .The problem is, many people probably _do_ have bash, but it's in /bin/sh. That used to be a fairly common setup a long time ago. Maybe it's not any more, but the whole "fall back to sh" actually came from that. The $BASH variable is only defined if you use bash as your *interactive* shell, ie if you started "make" from a bash shell. Historically, people used to do: - /bin/sh was the "standard shell" (bash) - /bin/[t]csh is what clueless weenies use for interactive work. (Yeah, I'm not a [t]csh fan ;) And you did break that. It's quite possible that all modern distributions will install /bin/bash as a link to /bin/sh, but I don't see the point of that particular change. We aren't even all that bash-centric any more. If you have a POSIX-compatible shell in /bin/sh, it really _should_ work. It just can't be something really broken. > - proper quoting > - proper indentation One thing I'm wondering about is whether we could have a "does this warn" test. I guess you can do it with -Werror, but it might be nice to have some tests for "does the -Wxyzzy flag warn also for proper code" Linus - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: The who needs reviews anyways [PATCH]
On Thu, Feb 08, 2007 at 10:48:51PM +0100, Roman Zippel wrote: > Hi, > > This adds the remaining changes which should have been part of the review > process. Oleg could have learned something in process, but who needs that > if wasting everyones time is so much more fun... > > Sorry for the delay, but the git server were gone. > > - the define command is inappropriate (it's primarily for rule > definitions) > - execute commands in the current dir as all other commands > - .*.tmp (but not .*.null) files are also removed up by "make clean" > - printf has other side effects, instead stop pretending we support > something else than bash > - proper quoting > - proper indentation > > bye, Roman > > Signed-off-by: Roman Zippel <[EMAIL PROTECTED]> Acked-by: Sam Ravnborg <[EMAIL PROTECTED]> Acked as "patch reviewed and looks good". PS. Will be unresponsive for next 12 days due to vacation. Sam - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
The who needs reviews anyways [PATCH]
Hi,
This adds the remaining changes which should have been part of the review
process. Oleg could have learned something in process, but who needs that
if wasting everyones time is so much more fun...
Sorry for the delay, but the git server were gone.
- the define command is inappropriate (it's primarily for rule
definitions)
- execute commands in the current dir as all other commands
- .*.tmp (but not .*.null) files are also removed up by "make clean"
- printf has other side effects, instead stop pretending we support
something else than bash
- proper quoting
- proper indentation
bye, Roman
Signed-off-by: Roman Zippel <[EMAIL PROTECTED]>
---
Makefile |9 --
scripts/Kbuild.include | 72 -
2 files changed, 42 insertions(+), 39 deletions(-)
Index: linux-2.6/Makefile
===
--- linux-2.6.orig/Makefile
+++ linux-2.6/Makefile
@@ -192,8 +192,11 @@ KCONFIG_CONFIG ?= .config
# SHELL used by kbuild
CONFIG_SHELL := $(shell if [ -x "$$BASH" ]; then echo $$BASH; \
- else if [ -x /bin/bash ]; then echo /bin/bash; \
- else echo sh; fi ; fi)
+ else if [ -x /bin/bash ]; then echo /bin/bash; fi; fi)
+ifeq ($(CONFIG_SHELL),)
+$(error bash is required to build the kernel)
+endif
+SHELL := $(CONFIG_SHELL)
HOSTCC = gcc
HOSTCXX = g++
@@ -321,7 +324,7 @@ KERNELRELEASE = $(shell cat include/conf
KERNELVERSION = $(VERSION).$(PATCHLEVEL).$(SUBLEVEL)$(EXTRAVERSION)
export VERSION PATCHLEVEL SUBLEVEL KERNELRELEASE KERNELVERSION
-export ARCH CONFIG_SHELL HOSTCC HOSTCFLAGS CROSS_COMPILE AS LD CC
+export ARCH CONFIG_SHELL SHELL HOSTCC HOSTCFLAGS CROSS_COMPILE AS LD CC
export CPP AR NM STRIP OBJCOPY OBJDUMP MAKE AWK GENKSYMS PERL UTS_MACHINE
export HOSTCXX HOSTCXXFLAGS LDFLAGS_MODULE CHECK CHECKFLAGS
Index: linux-2.6/scripts/Kbuild.include
===
--- linux-2.6.orig/scripts/Kbuild.include
+++ linux-2.6/scripts/Kbuild.include
@@ -1,7 +1,7 @@
# kbuild: Generic definitions
-# Convenient constants
+# Convenient variables
comma := ,
squote := '
empty :=
@@ -56,44 +56,48 @@ endef
# gcc support functions
# See documentation in Documentation/kbuild/makefiles.txt
-# checker-shell
-# Usage: option = $(call checker-shell,$(CC)...-o $$OUT,option-ok,otherwise)
-# Exit code chooses option. $$OUT is safe location for needless output.
-define checker-shell
-$(shell set -e; \
- DIR=$(KBUILD_EXTMOD); \
- cd $${DIR:-$(objtree)}; \
- OUT=$$PWD/..null; \
- if $(1) >/dev/null 2>&1; \
-then echo "$(2)"; \
-else echo "$(3)"; \
- fi; \
- rm -f $$OUT)
-endef
+# output directory for tests below
+TMPOUT := $(if $(KBUILD_EXTMOD),$(firstword $(KBUILD_EXTMOD))/)
+
+# try-run
+# Usage: option = $(call try-run, $(CC)...-o "$$TMP",option-ok,otherwise)
+# Exit code chooses option. "$$TMP" is can be used as temporary file and
+# is automatically cleaned up.
+try-run = $(shell set -e; \
+ TMP="$(TMPOUT)..tmp"; \
+ if ($(1)) >/dev/null 2>&1; \
+ then echo "$(2)"; \
+ else echo "$(3)"; \
+ fi; \
+ rm -f "$$TMP")
# as-option
# Usage: cflags-y += $(call as-option,-Wa$(comma)-isa=foo,)
-as-option = $(call checker-shell,\
- $(CC) $(CFLAGS) $(1) -c -xassembler /dev/null -o $$OUT,$(1),$(2))
+
+as-option = $(call try-run,\
+ $(CC) $(CFLAGS) $(1) -c -xassembler /dev/null -o "$$TMP",$(1),$(2))
# as-instr
# Usage: cflags-y += $(call as-instr,instr,option1,option2)
-as-instr = $(call checker-shell,\
- printf "$(1)" | $(CC) $(AFLAGS) -c -xassembler -o $$OUT -,$(2),$(3))
+
+as-instr = $(call try-run,\
+ echo -e "$(1)" | $(CC) $(AFLAGS) -c -xassembler -o "$$TMP" -,$(2),$(3))
# cc-option
# Usage: cflags-y += $(call cc-option,-march=winchip-c6,-march=i586)
-cc-option = $(call checker-shell,\
- $(CC) $(CFLAGS) $(if $(3),$(3),$(1)) -S -xc /dev/null -o $$OUT,$(1),$(2))
+
+cc-option = $(call try-run,\
+ $(CC) $(CFLAGS) $(1) -S -xc /dev/null -o "$$TMP",$(1),$(2))
# cc-option-yn
# Usage: flag := $(call cc-option-yn,-march=winchip-c6)
-cc-option-yn = $(call cc-option,"y","n",$(1))
+cc-option-yn = $(call try-run,\
+ $(CC) $(CFLAGS) $(1) -S -xc /dev/null -o "$$TMP",y,n)
# cc-option-align
# Prefix align with either -falign or -malign
cc-option-align = $(subst -functions=0,,\
- $(call cc-option,-falign-functions=0,-malign-functions=0))
+ $(call cc-option,-falign-functions=0,-malign-functions=0))
# cc-version
# Usage gcc-ver := $(call cc-version,$(CC))
@@ -105,24 +109,22 @@ cc-ifversion = $(shell [ $(call cc-versi
# ld-option
# Usage: ldflags += $(call ld-option, -Wl$(comma)--hash-style=both)
-ld-option = $(call checker-shell,\
- $(CC) $(1) -nostdlib -xc /dev/null -o $$OUT,$(1),$(2))
+ld-option = $(call try-run,\
+
The who needs reviews anyways [PATCH]
Hi,
This adds the remaining changes which should have been part of the review
process. Oleg could have learned something in process, but who needs that
if wasting everyones time is so much more fun...
Sorry for the delay, but the git server were gone.
- the define command is inappropriate (it's primarily for rule
definitions)
- execute commands in the current dir as all other commands
- .*.tmp (but not .*.null) files are also removed up by make clean
- printf has other side effects, instead stop pretending we support
something else than bash
- proper quoting
- proper indentation
bye, Roman
Signed-off-by: Roman Zippel [EMAIL PROTECTED]
---
Makefile |9 --
scripts/Kbuild.include | 72 -
2 files changed, 42 insertions(+), 39 deletions(-)
Index: linux-2.6/Makefile
===
--- linux-2.6.orig/Makefile
+++ linux-2.6/Makefile
@@ -192,8 +192,11 @@ KCONFIG_CONFIG ?= .config
# SHELL used by kbuild
CONFIG_SHELL := $(shell if [ -x $$BASH ]; then echo $$BASH; \
- else if [ -x /bin/bash ]; then echo /bin/bash; \
- else echo sh; fi ; fi)
+ else if [ -x /bin/bash ]; then echo /bin/bash; fi; fi)
+ifeq ($(CONFIG_SHELL),)
+$(error bash is required to build the kernel)
+endif
+SHELL := $(CONFIG_SHELL)
HOSTCC = gcc
HOSTCXX = g++
@@ -321,7 +324,7 @@ KERNELRELEASE = $(shell cat include/conf
KERNELVERSION = $(VERSION).$(PATCHLEVEL).$(SUBLEVEL)$(EXTRAVERSION)
export VERSION PATCHLEVEL SUBLEVEL KERNELRELEASE KERNELVERSION
-export ARCH CONFIG_SHELL HOSTCC HOSTCFLAGS CROSS_COMPILE AS LD CC
+export ARCH CONFIG_SHELL SHELL HOSTCC HOSTCFLAGS CROSS_COMPILE AS LD CC
export CPP AR NM STRIP OBJCOPY OBJDUMP MAKE AWK GENKSYMS PERL UTS_MACHINE
export HOSTCXX HOSTCXXFLAGS LDFLAGS_MODULE CHECK CHECKFLAGS
Index: linux-2.6/scripts/Kbuild.include
===
--- linux-2.6.orig/scripts/Kbuild.include
+++ linux-2.6/scripts/Kbuild.include
@@ -1,7 +1,7 @@
# kbuild: Generic definitions
-# Convenient constants
+# Convenient variables
comma := ,
squote := '
empty :=
@@ -56,44 +56,48 @@ endef
# gcc support functions
# See documentation in Documentation/kbuild/makefiles.txt
-# checker-shell
-# Usage: option = $(call checker-shell,$(CC)...-o $$OUT,option-ok,otherwise)
-# Exit code chooses option. $$OUT is safe location for needless output.
-define checker-shell
-$(shell set -e; \
- DIR=$(KBUILD_EXTMOD); \
- cd $${DIR:-$(objtree)}; \
- OUT=$$PWD/..null; \
- if $(1) /dev/null 21; \
-then echo $(2); \
-else echo $(3); \
- fi; \
- rm -f $$OUT)
-endef
+# output directory for tests below
+TMPOUT := $(if $(KBUILD_EXTMOD),$(firstword $(KBUILD_EXTMOD))/)
+
+# try-run
+# Usage: option = $(call try-run, $(CC)...-o $$TMP,option-ok,otherwise)
+# Exit code chooses option. $$TMP is can be used as temporary file and
+# is automatically cleaned up.
+try-run = $(shell set -e; \
+ TMP=$(TMPOUT)..tmp; \
+ if ($(1)) /dev/null 21; \
+ then echo $(2); \
+ else echo $(3); \
+ fi; \
+ rm -f $$TMP)
# as-option
# Usage: cflags-y += $(call as-option,-Wa$(comma)-isa=foo,)
-as-option = $(call checker-shell,\
- $(CC) $(CFLAGS) $(1) -c -xassembler /dev/null -o $$OUT,$(1),$(2))
+
+as-option = $(call try-run,\
+ $(CC) $(CFLAGS) $(1) -c -xassembler /dev/null -o $$TMP,$(1),$(2))
# as-instr
# Usage: cflags-y += $(call as-instr,instr,option1,option2)
-as-instr = $(call checker-shell,\
- printf $(1) | $(CC) $(AFLAGS) -c -xassembler -o $$OUT -,$(2),$(3))
+
+as-instr = $(call try-run,\
+ echo -e $(1) | $(CC) $(AFLAGS) -c -xassembler -o $$TMP -,$(2),$(3))
# cc-option
# Usage: cflags-y += $(call cc-option,-march=winchip-c6,-march=i586)
-cc-option = $(call checker-shell,\
- $(CC) $(CFLAGS) $(if $(3),$(3),$(1)) -S -xc /dev/null -o $$OUT,$(1),$(2))
+
+cc-option = $(call try-run,\
+ $(CC) $(CFLAGS) $(1) -S -xc /dev/null -o $$TMP,$(1),$(2))
# cc-option-yn
# Usage: flag := $(call cc-option-yn,-march=winchip-c6)
-cc-option-yn = $(call cc-option,y,n,$(1))
+cc-option-yn = $(call try-run,\
+ $(CC) $(CFLAGS) $(1) -S -xc /dev/null -o $$TMP,y,n)
# cc-option-align
# Prefix align with either -falign or -malign
cc-option-align = $(subst -functions=0,,\
- $(call cc-option,-falign-functions=0,-malign-functions=0))
+ $(call cc-option,-falign-functions=0,-malign-functions=0))
# cc-version
# Usage gcc-ver := $(call cc-version,$(CC))
@@ -105,24 +109,22 @@ cc-ifversion = $(shell [ $(call cc-versi
# ld-option
# Usage: ldflags += $(call ld-option, -Wl$(comma)--hash-style=both)
-ld-option = $(call checker-shell,\
- $(CC) $(1) -nostdlib -xc /dev/null -o $$OUT,$(1),$(2))
+ld-option = $(call try-run,\
+ $(CC) $(1) -nostdlib -xc /dev/null -o
Re: The who needs reviews anyways [PATCH]
On Thu, Feb 08, 2007 at 10:48:51PM +0100, Roman Zippel wrote: Hi, This adds the remaining changes which should have been part of the review process. Oleg could have learned something in process, but who needs that if wasting everyones time is so much more fun... Sorry for the delay, but the git server were gone. - the define command is inappropriate (it's primarily for rule definitions) - execute commands in the current dir as all other commands - .*.tmp (but not .*.null) files are also removed up by make clean - printf has other side effects, instead stop pretending we support something else than bash - proper quoting - proper indentation bye, Roman Signed-off-by: Roman Zippel [EMAIL PROTECTED] Acked-by: Sam Ravnborg [EMAIL PROTECTED] Acked as patch reviewed and looks good. PS. Will be unresponsive for next 12 days due to vacation. Sam - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: The who needs reviews anyways [PATCH]
On Thu, 8 Feb 2007, Roman Zippel wrote: Sorry for the delay, but the git server were gone. - the define command is inappropriate (it's primarily for rule definitions) Looks fine. Especially considering the strange whitespace issues. - execute commands in the current dir as all other commands - .*.tmp (but not .*.null) files are also removed up by make clean - printf has other side effects, instead stop pretending we support something else than bash However, this one I have problems with .The problem is, many people probably _do_ have bash, but it's in /bin/sh. That used to be a fairly common setup a long time ago. Maybe it's not any more, but the whole fall back to sh actually came from that. The $BASH variable is only defined if you use bash as your *interactive* shell, ie if you started make from a bash shell. Historically, people used to do: - /bin/sh was the standard shell (bash) - /bin/[t]csh is what clueless weenies use for interactive work. (Yeah, I'm not a [t]csh fan ;) And you did break that. It's quite possible that all modern distributions will install /bin/bash as a link to /bin/sh, but I don't see the point of that particular change. We aren't even all that bash-centric any more. If you have a POSIX-compatible shell in /bin/sh, it really _should_ work. It just can't be something really broken. - proper quoting - proper indentation One thing I'm wondering about is whether we could have a does this warn test. I guess you can do it with -Werror, but it might be nice to have some tests for does the -Wxyzzy flag warn also for proper code Linus - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: The who needs reviews anyways [PATCH]
Hi, On Thu, 8 Feb 2007, Linus Torvalds wrote: Historically, people used to do: - /bin/sh was the standard shell (bash) - /bin/[t]csh is what clueless weenies use for interactive work. (Yeah, I'm not a [t]csh fan ;) And you did break that. It's quite possible that all modern distributions will install /bin/bash as a link to /bin/sh, but I don't see the point of that particular change. We aren't even all that bash-centric any more. If you have a POSIX-compatible shell in /bin/sh, it really _should_ work. It just can't be something really broken. I don't quite understand, the Makefile doesn't care anymore about /bin/sh with this patch, the Makefile checks only for $BASH and /bin/bash (equivalent to adding #! /bin/bash to scripts) and if the latter fails it's possible some of our scripts will fail. We could make sure that all our scripts are POSIX clean, but is it really worth the effort? It would make casual kbuild hacking only even more difficult, as one has to check it works with the various shells. - proper quoting - proper indentation One thing I'm wondering about is whether we could have a does this warn test. I guess you can do it with -Werror, but it might be nice to have some tests for does the -Wxyzzy flag warn also for proper code Adding something like try-compile should be possible, but we should be careful with this, the more checks we add the more work is done at every make invocation. bye, Roman - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Kbuild refactoring (Re: The who needs reviews anyways [PATCH])
On Thu, Feb 08, 2007 at 10:48:51PM +0100, Roman Zippel wrote: [] - printf has other side effects, instead stop pretending we support something else than bash Yes. With `%' in option strings there will be side effects. I would suggest to use printf %s $(1) with paranoia mode on, and hope to do not forcing `bash'. - proper quoting - proper indentation bye, Roman Thanks. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: The who needs reviews anyways [PATCH]
On Thu, 8 Feb 2007, Roman Zippel wrote: I don't quite understand, the Makefile doesn't care anymore about /bin/sh with this patch, the Makefile checks only for $BASH and /bin/bash Exactly. The point is, neither $BASH nor /bin/bash may be set. If you run make while running tcsh, BASH won't be set. We've always just defaulted to doing /bin/sh. Doesn't really seem to be a problem. Linus - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: The who needs reviews anyways [PATCH]
Hi, On Thu, 8 Feb 2007, Linus Torvalds wrote: On Thu, 8 Feb 2007, Roman Zippel wrote: I don't quite understand, the Makefile doesn't care anymore about /bin/sh with this patch, the Makefile checks only for $BASH and /bin/bash Exactly. The point is, neither $BASH nor /bin/bash may be set. Is that really a problem? I think any system that has bash without /bin/bash is simply broken. If you run make while running tcsh, BASH won't be set. We've always just defaulted to doing /bin/sh. Doesn't really seem to be a problem. There are chances this is already broken anyway. We call external scripts using $(shell $(CONFIG_SHELL) ...), which ignores the #! ... setting. bye, Roman - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: The who needs reviews anyways [PATCH]
Roman Zippel [EMAIL PROTECTED] writes: - printf has other side effects, instead stop pretending we support something else than bash printf is a much better echo, but you need to use it properly as well. Either use %s to print a literal string or %b to let it interpret escape sequences. Andreas. -- Andreas Schwab, SuSE Labs, [EMAIL PROTECTED] SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany PGP key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 And now for something completely different. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: The who needs reviews anyways [PATCH]
Hi, On Fri, 9 Feb 2007, Andreas Schwab wrote: Roman Zippel [EMAIL PROTECTED] writes: - printf has other side effects, instead stop pretending we support something else than bash printf is a much better echo, but you need to use it properly as well. Either use %s to print a literal string or %b to let it interpret escape sequences. Hmm, I didn't know about %b, which would indeed be another possibility here, although I think it would only make a real difference if we decided to make (and more importantly keep) all our scripts POSIX clean. bye, Roman - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: The who needs reviews anyways [PATCH]
On Fri, 09 Feb 2007 00:20:49 +0100, Roman Zippel said: The point is, neither $BASH nor /bin/bash may be set. Is that really a problem? I think any system that has bash without /bin/bash is simply broken. If you're trying to bootstrap a Linux box onto a new platform from some non-Linux Unixoid, it's possible that bash lives in $TOOLCHAIN/bin/bash. But I see that even Solaris 9 has a bash 2.05 in /bin/bash, so I might be talking out some odd orifice here. pgpA6X9IC9WHY.pgp Description: PGP signature
Re: The who needs reviews anyways [PATCH]
On Thu, Feb 08, 2007 at 10:48:51PM +0100, Roman Zippel wrote: [] - printf has other side effects, instead stop pretending we support something else than bash More on printf, `sh', tmpfiles. As we know original problem is: something from binutils is removing output files on failure. - else if [ -x /bin/bash ]; then echo /bin/bash; \ - else echo sh; fi ; fi) + else if [ -x /bin/bash ]; then echo /bin/bash; fi; fi) +ifeq ($(CONFIG_SHELL),) +$(error bash is required to build the kernel) +endif +SHELL := $(CONFIG_SHELL) here is policy to have `bash' introduced, so due to original issue, where `root' users ended with removed /dev/null, may policy to have `non root' user to build kernel be added? Thus this: +# output directory for tests below +TMPOUT := $(if $(KBUILD_EXTMOD),$(firstword $(KBUILD_EXTMOD))/) [] +# try-run +# Usage: option = $(call try-run, $(CC)...-o $$TMP,option-ok,otherwise) +# Exit code chooses option. $$TMP is can be used as temporary file and +# is automatically cleaned up. +try-run = $(shell set -e;\ this: + TMP=$(TMPOUT)..tmp; \ [] + if ($(1)) /dev/null 21; \ + then echo $(2); \ + else echo $(3); \ + fi; \ this: + rm -f $$TMP) may be removed, and to make TMP=/dev/null? And to forget currently about my silly symlinks, and this crappy sets of output files? As for `printf', as i've wrote, only in case of % and quotes in arguments, something else must be added to handle that. But i think, it's paranoia. -as-instr = $(call checker-shell,\ - printf $(1) | $(CC) $(AFLAGS) -c -xassembler -o $$OUT -,$(2),$(3)) `printf $(1)' is pretty enough. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
