The who needs reviews anyways [PATCH]

The who needs reviews anyways [PATCH]

[Roman Zippel]

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 <zippel@linux-m68k.org>

---
 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)))

Re: The who needs reviews anyways [PATCH]

[Sam Ravnborg]

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 <zippel@linux-m68k.org>
Acked-by: Sam Ravnborg <sam@ravnborg.org>

Acked as "patch reviewed and looks good".

PS. Will be unresponsive for next 12 days due to vacation.

	Sam

Re: The who needs reviews anyways [PATCH]

[Linus Torvalds]

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

Re: The who needs reviews anyways [PATCH]

[Roman Zippel]

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

Re: The who needs reviews anyways [PATCH]

[Linus Torvalds]

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

Re: The who needs reviews anyways [PATCH]

[Roman Zippel]

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

Re: The who needs reviews anyways [PATCH]

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 474 bytes --]

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.



[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Kbuild refactoring (Re: The who needs reviews anyways [PATCH])

[Oleg Verych]

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.

____

Re: The who needs reviews anyways [PATCH]

[Andreas Schwab]

Roman Zippel <zippel@linux-m68k.org> 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, schwab@suse.de
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."

Re: The who needs reviews anyways [PATCH]

[Roman Zippel]

Hi,

On Fri, 9 Feb 2007, Andreas Schwab wrote:

> Roman Zippel <zippel@linux-m68k.org> 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

Re: The who needs reviews anyways [PATCH]

[Oleg Verych]

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.

____

Re: The who needs reviews anyways [PATCH]

[Roman Zippel]

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

Re: The who needs reviews anyways [PATCH]

[Oleg Verych]

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)

____