[PATCH] make it possible to keep Config/Kbuild snippets in *.c files
Rob Landley
rob at landley.net
Sun May 9 05:31:48 UTC 2010
On Saturday 08 May 2010 21:04:35 Denys Vlasenko wrote:
> Please review.
> --
> vda
Well, obviously I'm in favor of the concept, since toybox does something
similar. :)
> diff -ad -urpN busybox.0/findutils/Config.in busybox.1/findutils/Config.in
> --- busybox.0/findutils/Config.in 2010-04-09 20:01:59.000000000 +0200
> +++ busybox.1/findutils/Config.in 1970-01-01 01:00:00.000000000 +0100
> @@ -1,253 +0,0 @@
> -#
> -# For a description of the syntax of this configuration file,
> -# see scripts/kbuild/config-language.txt.
> -#
> -
> -menu "Finding Utilities"
...
> -endmenu
Zapped the whole file, ok...
> diff -ad -urpN busybox.0/findutils/Config.src
> busybox.1/findutils/Config.src ---
> busybox.0/findutils/Config.src 1970-01-01 01:00:00.000000000 +0100 +++
> busybox.1/findutils/Config.src 2010-05-09 01:43:41.000000000 +0200 @@
> -0,0 +1,10 @@
> +#
> +# For a description of the syntax of this configuration file,
> +# see scripts/kbuild/config-language.txt.
> +#
> +
> +menu "Finding Utilities"
> +
> +INSERT
> +
> +endmenu
If you're going to have some commentary in this file already, you might mention
that Config.in is generated from Config.src file by gen_build_files.sh.
Structural comment: I separated the generated code from the source code,
putting it all in a "generated" directory.
I believe you're changing "Config.in" from a file that's tracked in the
repository to a file that doesn't exist in the repository but is generated in
situ as a temp file? (And presumably goes away with "make clean"?) People may
not notice that, and may waste time editing a temp file. If the file moves,
people will be forced to notice. (I find it also makes the dependencies and
cleaning logic easier to follow, but that could just be me...)
> diff -ad -urpN busybox.0/findutils/find.c busybox.1/findutils/find.c
> --- busybox.0/findutils/find.c 2010-04-27 07:20:34.000000000 +0200
> +++ busybox.1/findutils/find.c 2010-05-09 01:52:43.000000000 +0200
> @@ -53,6 +53,181 @@
> * diff -u /tmp/std_find /tmp/bb_find && echo Identical
> */
>
> +// kbuild:lib-$(CONFIG_FIND) += find.o
> +// config:
> +// config:config FIND
> +// config: bool "find"
> +// config: default n
> +// config: help
> +// config: find is used to search your system to find specified
...
> +// config: Support the 'find -links' option for matching number of
> links. +
> #include <fnmatch.h>
> #include "libbb.h"
> #if ENABLE_FEATURE_FIND_REGEX
A little on the verbose side as syntaxes go, but fairly straightforward.
What do you do if somebody typos "comfig" or similar? With the syntax I chose,
if there was a screw-up the build would generally break. In this case, a
single line of could silently drop out in a way that wouldn't necessarily be
immediately obvious...
I note that CONFIG_APPNAME += appname.o is the common case, which could
presumably be generated automatically for 90% of the commands. (You only
really need to specify something like this when there's more than one .o file,
or if the .o file is a different name...)
> diff -ad -urpN busybox.0/findutils/grep.c busybox.1/findutils/grep.c
> --- busybox.0/findutils/grep.c 2010-04-30 09:30:10.000000000 +0200
> +++ busybox.1/findutils/grep.c 2010-05-09 01:49:28.000000000 +0200
> @@ -19,6 +19,41 @@
> * (C) 2006 Jac Goudsmit added -o option
> */
>
> +// kbuild:lib-$(CONFIG_GREP) += grep.o
> +// config:
> +// config:config GREP
> +// config: bool "grep"
> +// config: default n
...
> +// config: depends on GREP
> +// config: help
> +// config: Print the specified number of leading (-B) and/or
> trailing (-A) +// config: context surrounding our matching lines.
> +// config: Print the specified number of context lines (-C).
> +
> #include "libbb.h"
> #include "xregex.h"
More of the same, ok.
> diff -ad -urpN busybox.0/findutils/Kbuild busybox.1/findutils/Kbuild
> --- busybox.0/findutils/Kbuild 2010-04-09 20:01:59.000000000 +0200
> +++ busybox.1/findutils/Kbuild 1970-01-01 01:00:00.000000000 +0100
> @@ -1,10 +0,0 @@
> -# Makefile for busybox
> -#
> -# Copyright (C) 1999-2005 by Erik Andersen <andersen at codepoet.org>
> -#
> -# Licensed under the GPL v2, see the file LICENSE in this tarball.
> -
> -lib-y:=
> -lib-$(CONFIG_FIND) += find.o
> -lib-$(CONFIG_GREP) += grep.o
> -lib-$(CONFIG_XARGS) += xargs.o
Deleting whole file, ok.
> diff -ad -urpN busybox.0/findutils/Kbuild.src
> busybox.1/findutils/Kbuild.src ---
> busybox.0/findutils/Kbuild.src 1970-01-01 01:00:00.000000000 +0100 +++
> busybox.1/findutils/Kbuild.src 2010-05-09 03:54:53.000000000 +0200 @@
> -0,0 +1,9 @@
> +# Makefile for busybox
> +#
> +# Copyright (C) 1999-2005 by Erik Andersen <andersen at codepoet.org>
> +#
> +# Licensed under the GPL v2, see the file LICENSE in this tarball.
> +
> +lib-y:=
> +
> +INSERT
More boilerplate.
Again, this looks like a default case. If you're going to do this for more
than one directory, possibly this should be emitted by the script rather than
appended to, and only non-default stuff should go in here.
The Config.src file had a reason to exist because it was naming the menu, but
this one isn't actually doing anything.
Also, since these files are generated you don't really need one per directory.
You could just make one big one. (Which brings us back to the generated
directory, mentioned earlier.)
Still, that sort of thing could easily be done in stages. (Cleanup on top of
cleanup, this is fine for now...) The important thing is getting the syntax
that goes into the .c files right, so that conversion doesn't have to be
redone.
> diff -ad -urpN busybox.0/findutils/xargs.c busybox.1/findutils/xargs.c
> --- busybox.0/findutils/xargs.c 2010-04-09 20:01:59.000000000 +0200
> +++ busybox.1/findutils/xargs.c 2010-05-09 01:49:25.000000000 +0200
> @@ -17,6 +17,47 @@
> *
> */
>
> +// kbuild:lib-$(CONFIG_XARGS) += xargs.o
> +// config:
> +// config:config XARGS
...
> +// config: are not special.
> +
> #include "libbb.h"
>
> /* This is a NOEXEC applet. Be very careful! */
More of the same, ok.
> diff -ad -urpN busybox.0/Makefile busybox.1/Makefile
> --- busybox.0/Makefile 2010-04-09 20:01:58.000000000 +0200
> +++ busybox.1/Makefile 2010-05-09 03:53:15.000000000 +0200
> @@ -377,6 +377,11 @@ ifneq ($(KBUILD_SRC),)
> $(srctree) $(objtree) $(VERSION) $(PATCHLEVEL)
> endif
>
> +# This target generates Kbuild's and Config.in's from *.c files
> +PHONY += gen_build_files
> +gen_build_files:
> + $(Q)$(srctree)/scripts/gen_build_files.sh $(srctree) $(objtree)
> +
This rebuilds every time. Wouldn't it be better to do dependencies on the .c
files, ala:
gen_build_files: findutils/*.c
> # To make sure we do not include .config for any of the *config targets
> # catch them early, and hand them over to scripts/kconfig/Makefile
> # It is allowed to specify more targets when calling make, including
> @@ -428,7 +433,7 @@ ifeq ($(config-targets),1)
> -include $(srctree)/arch/$(ARCH)/Makefile
> export KBUILD_DEFCONFIG
>
> -config %config: scripts_basic outputmakefile FORCE
> +config %config: scripts_basic outputmakefile gen_build_files FORCE
> $(Q)mkdir -p include
> $(Q)$(MAKE) $(build)=scripts/kconfig $@
> $(Q)$(MAKE) -C $(srctree) KBUILD_SRC= .kernelrelease
> @@ -443,7 +448,7 @@ ifeq ($(KBUILD_EXTMOD),)
> # Carefully list dependencies so we do not try to build scripts twice
> # in parrallel
> PHONY += scripts
> -scripts: scripts_basic include/config/MARKER
> +scripts: gen_build_files scripts_basic include/config/MARKER
> $(Q)$(MAKE) $(build)=$(@)
>
> scripts_basic: include/autoconf.h
It seems like that first hunk would have been sufficient, but the busybox
makefiles have always been a bit overcomplicated, haven't they?
> diff -ad -urpN busybox.0/scripts/gen_build_files.sh
> busybox.1/scripts/gen_build_files.sh ---
> busybox.0/scripts/gen_build_files.sh 1970-01-01 01:00:00.000000000
> +0100 +++ busybox.1/scripts/gen_build_files.sh 2010-05-09
> 03:58:09.000000000 +0200 @@ -0,0 +1,51 @@
> +#!/bin/sh
> +
> +test $# -ge 2 || exit 1
> +
Might want to emit a usage message there. Just a thought.
> +# cd to objtree
> +cd "$2" || exit 1
> +
> +srctree="$1"
> +
> +find -type d \
> +| while read; do
> + d="$REPLY"
find -type d | while read d
do
> + test -f "$srctree/$d"/Kbuild.src && {
Possibly just a style thing, but it seems more shell-ish to do:
if [ -f "$srctree/$d/Kbuild.src" ]
then
*shrug*
> + echo " CHK $d/Kbuild"
> + s=`grep -h '^// kbuild:' "$d"/*.c | sed 's^// kbuild:^^'`
You don't need both grep and sed, probably you want something like:
s="$(sed -n 's@^// kbuild:@@p' "$d"/*.c)"
> + {
> + while read; do
> + test x"$REPLY" = x"INSERT" && REPLY="$s"
> + printf "%s\n" "$REPLY"
> + done <"$srctree/$d"/Kbuild.src
> + } >"$d"/Kbuild.$$.tmp
Um, where is this read loop getting input from? I missed a curve. (The outer
read loop had something piped into it, but this one is listening to stdin?)
So we pass along most lines unmodified, but if the line we read was "INSERT"
then we grab the data sed accumlated above. Currently, the INSERT line is at
the end of the file. If that's always going to be the case (and this is part
of the declarative code of the makefile so order shouldn't matter), can't we
just do:
(cat; sed -n 's@^// kbuild:@@p' "$d"/*.c) > "$d"/Kbuild.tmp
> + if cmp -s "$d"/Kbuild.$$.tmp "$d"/Kbuild; then
> + rm "$d"/Kbuild.$$.tmp
> + else
> + echo " GEN $d/Kbuild"
> + mv "$d"/Kbuild.$$.tmp "$d"/Kbuild
> + fi
> + }
If you get the dependencies right you won't regenerate these when you don't
need to.
> + test -f "$srctree/$d"/Config.src && {
> + echo " CHK $d/Config.in"
> + s=`grep -h '^// config:' "$d"/*.c | sed 's^// config:^^'`
> + {
> + while read; do
> + test x"$REPLY" = x"INSERT" && REPLY="$s"
> + printf "%s\n" "$REPLY"
> + done <"$srctree/$d"/Config.src
> + } >"$d"/Config.in.$$.tmp
> + if cmp -s "$d"/Config.in.$$.tmp "$d"/Config.in; then
> + rm "$d"/Config.in.$$.tmp
> + else
> + echo " GEN $d/Config.in"
> + mv "$d"/Config.in.$$.tmp "$d"/Config.in
> + fi
> + }
And the same again for Config.in.
I note that bash is a wonderful thing, and in theory you should be able to do
most of the above with something like (off the top of my head, untested):
sed '/^INSERT$//r '<$(sed 's@^// config:@@p' "$d"/*.c) > "$d"/Config.in
*shrug*
> +done
> +
> +# Last read failed. This is normal. Don't exit with its error code:
> +exit 0
Looks like it'll probably work.
Rob
--
Latency is more important than throughput. It's that simple. - Linus Torvalds
More information about the busybox
mailing list