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,\
+ $(CC) $(1) -nostdlib -xc /dev/null -o "$$TMP",$(1),$(2))
######
+###
# Shorthand for $(Q)$(MAKE) -f scripts/Makefile.build obj=
# Usage:
# $(Q)$(MAKE) $(build)=dir
build := -f $(if $(KBUILD_SRC),$(srctree)/)scripts/Makefile.build obj
-# Prefix -I with $(srctree) if it is not an absolute path,
-# add original to the end
-addtree = $(if \
- $(filter-out -I/%,$(1)),$(patsubst -I%,-I$(srctree)/%,$(1))) $(1)
+# Prefix -I with $(srctree) if it is not an absolute path.
+addtree = $(if $(filter-out -I/%,$(1)),$(patsubst -I%,-I$(srctree)/%,$(1))) $(1)
# Find all -I options and call addtree
-flags = $(foreach o,$($(1)),\
- $(if $(filter -I%,$(o)),$(call addtree,$(o)),$(o)))
+flags = $(foreach o,$($(1)),$(if $(filter -I%,$(o)),$(call addtree,$(o)),$(o)))
# echo command.
# Short version is used, if $(quiet) equals `quiet_', otherwise full one.
@@ -144,7 +146,7 @@ objectify = $(foreach o,$(1),$(if $(filt
# See Documentation/kbuild/makefiles.txt for more info
ifneq ($(KBUILD_NOCMDDEP),1)
-# Check if both arguments has same arguments. Result is empty string, if equal.
+# Check if both arguments has same arguments. Result is empty string if equal.
# User may override this check using make KBUILD_NOCMDDEP=1
arg-check = $(strip $(filter-out $(cmd_$(1)), $(cmd_$@)) \
$(filter-out $(cmd_$@), $(cmd_$(1))) )
@@ -168,7 +170,6 @@ if_changed = $(if $(strip $(any-prereq)
echo 'cmd_$@ := $(make-cmd)' > $(dot-target).cmd)
# Execute the command and also postprocess generated .d dependencies file.
-#
if_changed_dep = $(if $(strip $(any-prereq) $(arg-check) ), \
@set -e; \
$(echo-cmd) $(cmd_$(1)); \
@@ -176,10 +177,9 @@ if_changed_dep = $(if $(strip $(any-prer
rm -f $(depfile); \
mv -f $(dot-target).tmp $(dot-target).cmd)
-# Will check if $(cmd_foo) changed, or any of the prerequisites changed,
-# and if so will execute $(rule_foo).
# Usage: $(call if_changed_rule,foo)
-#
+# Will check if $(cmd_foo) or any of the prerequisites changed,
+# and if so will execute $(rule_foo).
if_changed_rule = $(if $(strip $(any-prereq) $(arg-check) ), \
@set -e; \
$(rule_$(1)))
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
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
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
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.
____
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
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
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."
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
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.
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.
____
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
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)
____