All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] kconfig: Add merge_config.sh script
@ 2011-11-17 21:58 john stultz
  2011-11-17 22:44 ` Arnaud Lacombe
                   ` (3 more replies)
  0 siblings, 4 replies; 20+ messages in thread
From: john stultz @ 2011-11-17 21:58 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Sam Ravnborg, gthelen, tartler, Dmitry Fink, Darren Hart,
	Eric B Munson, Bruce Ashfield, Michal Marek, linux-kbuild

Hey Andrew, 
	I've tried sending this via Michal a few times, but haven't heard much
back. So I wanted to check if you would consider merging it via your
tree, or if you had any suggestions of who would be better to
review/merge this.

thanks
-john



After noticing almost every distro has their own method of managing
config fragments, I went looking at some best practices, and wanted
to try to consolidate some of the different approaches so this fairly
simple infrastructure can be shared (and new distros/build systems
don't have to implement yet another config fragment merge script).

This script is most influenced by the Windriver tools used in
the Yocto Project, reusing some portions found there.

This script merges multiple config fragments, warning on any
overrided values. It then sets any unspecified values to their
default, then finally checks to make sure no specified value was
dropped due to unsatisfied dependencies.

I'm sure this implementation won't work for everyone, and I expect
it will need to evolve to adapt for various use cases. But I think
its a reasonable starting point.

v2:
  * Reworked to use alldefconfig instead of the proposed
olddefconfig as suggested by Sam Ravnborg.

v3:
  * Script improvements from Dmitri.
  * allnoconfig option from Darren
  * pre-make exit option from Darren
  * lots of other fixes/cleanups from Darren.
  * Fix final check to not compain about config values in comments

Please let me know if you have any comments or thoughts!

CC: Sam Ravnborg <sam@ravnborg.org>
CC: gthelen@google.com
CC: tartler@cs.fau.de
CC: Dmitry Fink <Dmitry.Fink@palm.com>
CC: Darren Hart <dvhart@linux.intel.com>
CC: Eric B Munson <ebmunson@us.ibm.com>
CC: Bruce Ashfield <Bruce.Ashfield@windriver.com>
CC: Michal Marek <mmarek@suse.cz>
CC: linux-kbuild@vger.kernel.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 scripts/kconfig/merge_config.sh |  117 +++++++++++++++++++++++++++++++++++++++
 1 files changed, 117 insertions(+), 0 deletions(-)
 create mode 100755 scripts/kconfig/merge_config.sh

diff --git a/scripts/kconfig/merge_config.sh b/scripts/kconfig/merge_config.sh
new file mode 100755
index 0000000..890276b
--- /dev/null
+++ b/scripts/kconfig/merge_config.sh
@@ -0,0 +1,117 @@
+#!/bin/sh
+#  merge_config.sh - Takes a list of config fragment values, and merges
+#  them one by one. Provides warnings on overridden values, and specified
+#  values that did not make it to the resulting .config file (due to missed
+#  dependencies or config symbol removal).
+#
+#  Portions reused from kconf_check and generate_cfg:
+#  http://git.yoctoproject.org/cgit/cgit.cgi/yocto-kernel-tools/tree/tools/kconf_check
+#  http://git.yoctoproject.org/cgit/cgit.cgi/yocto-kernel-tools/tree/tools/generate_cfg
+#
+#  Copyright (c) 2009-2010 Wind River Systems, Inc.
+#  Copyright 2011 Linaro
+#
+#  This program is free software; you can redistribute it and/or modify
+#  it under the terms of the GNU General Public License version 2 as
+#  published by the Free Software Foundation.
+#
+#  This program is distributed in the hope that it will be useful,
+#  but WITHOUT ANY WARRANTY; without even the implied warranty of
+#  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
+#  See the GNU General Public License for more details.
+
+clean_up() {
+       rm -f $TMP_FILE
+       exit
+}
+trap clean_up SIGHUP SIGINT SIGTERM
+
+usage() {
+	echo "Usage: $0 [OPTIONS] [CONFIG [...]]"
+	echo "  -h    display this help text"
+	echo "  -m    only merge the fragments, do not execute the make command"
+	echo "  -n    use allnoconfig instead of alldefconfig"
+}
+
+MAKE=true
+ALLTARGET=alldefconfig
+
+while true; do
+	case $1 in
+	"-n")
+		ALLTARGET=allnoconfig
+		shift
+		continue
+		;;
+	"-m")
+		MAKE=false
+		shift
+		continue
+		;;
+	"-h")
+		usage
+		exit
+		;;
+	*)
+		break
+		;;
+	esac
+done
+
+
+
+MERGE_LIST=$*
+SED_CONFIG_EXP="s/^\(# \)\{0,1\}\(CONFIG_[a-zA-Z0-9_]*\)[= ].*/\2/p"
+TMP_FILE=$(mktemp ./.tmp.config.XXXXXXXXXX)
+
+# Merge files, printing warnings on overrided values
+for MERGE_FILE in $MERGE_LIST ; do
+	echo "Merging $MERGE_FILE"
+	CFG_LIST=$(sed -n "$SED_CONFIG_EXP" $MERGE_FILE)
+
+	for CFG in $CFG_LIST ; do
+		grep -q -w $CFG $TMP_FILE
+		if [ $? -eq 0 ] ; then
+			PREV_VAL=$(grep -w $CFG $TMP_FILE)
+			NEW_VAL=$(grep -w $CFG $MERGE_FILE)
+			if [ "x$PREV_VAL" != "x$NEW_VAL" ] ; then
+			echo Value of $CFG is redefined by fragment $MERGE_FILE:
+			echo Previous  value: $PREV_VAL
+			echo New value:       $NEW_VAL
+			echo
+			fi
+			sed -i "/$CFG[ =]/d" $TMP_FILE
+		fi
+	done
+	cat $MERGE_FILE >> $TMP_FILE
+done
+
+if [ "$MAKE" = "false" ]; then
+	cp $TMP_FILE .config
+	echo "#"
+	echo "# merged configuration written to .config (needs make)"
+	echo "#"
+	clean_up
+	exit
+fi
+
+# Use the merged file as the starting point for:
+# alldefconfig: Fills in any missing symbols with Kconfig default
+# allnoconfig: Fills in any missing symbols with # CONFIG_* is not set
+make KCONFIG_ALLCONFIG=$TMP_FILE $ALLTARGET
+
+
+# Check all specified config values took (might have missed-dependency issues)
+for CFG in $(sed -n "$SED_CONFIG_EXP" $TMP_FILE); do
+
+	REQUESTED_VAL=$(sed -n "$SED_CONFIG_EXP" $TMP_FILE | grep -w -e "$CFG")
+	ACTUAL_VAL=$(sed -n "$SED_CONFIG_EXP" .config | grep -w -e "$CFG")
+	if [ "x$REQUESTED_VAL" != "x$ACTUAL_VAL" ] ; then
+		echo "Value requested for $CFG not in final .config"
+		echo "Requested value:  $REQUESTED_VAL"
+		echo "Actual value:     $ACTUAL_VAL"
+		echo ""
+	fi
+done
+
+clean_up
-- 
1.7.3.2.146.gca209




^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH] kconfig: Add merge_config.sh script
  2011-11-17 21:58 [PATCH] kconfig: Add merge_config.sh script john stultz
@ 2011-11-17 22:44 ` Arnaud Lacombe
  2011-11-17 22:54   ` john stultz
  2011-11-19  6:51 ` Arnaud Lacombe
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 20+ messages in thread
From: Arnaud Lacombe @ 2011-11-17 22:44 UTC (permalink / raw)
  To: john stultz
  Cc: Andrew Morton, Sam Ravnborg, gthelen, tartler, Dmitry Fink,
	Darren Hart, Eric B Munson, Bruce Ashfield, Michal Marek,
	linux-kbuild

Hi,

On Thu, Nov 17, 2011 at 4:58 PM, john stultz <johnstul@us.ibm.com> wrote:
> Hey Andrew,
>        I've tried sending this via Michal a few times, but haven't heard much
> back. So I wanted to check if you would consider merging it via your
> tree, or if you had any suggestions of who would be better to
> review/merge this.
>
One of the worry I would have is that the script is merging config
blindly, ie. there is no dependency checking done. I have some some
work-in-progress to help resolving this, but still lots of thought to
be implemented.

That said, I don't really mind for any gap-filling solution.

Regards,
 - Arnaud

> thanks
> -john
>
>
>
> After noticing almost every distro has their own method of managing
> config fragments, I went looking at some best practices, and wanted
> to try to consolidate some of the different approaches so this fairly
> simple infrastructure can be shared (and new distros/build systems
> don't have to implement yet another config fragment merge script).
>
> This script is most influenced by the Windriver tools used in
> the Yocto Project, reusing some portions found there.
>
> This script merges multiple config fragments, warning on any
> overrided values. It then sets any unspecified values to their
> default, then finally checks to make sure no specified value was
> dropped due to unsatisfied dependencies.
>
> I'm sure this implementation won't work for everyone, and I expect
> it will need to evolve to adapt for various use cases. But I think
> its a reasonable starting point.
>
> v2:
>  * Reworked to use alldefconfig instead of the proposed
> olddefconfig as suggested by Sam Ravnborg.
>
> v3:
>  * Script improvements from Dmitri.
>  * allnoconfig option from Darren
>  * pre-make exit option from Darren
>  * lots of other fixes/cleanups from Darren.
>  * Fix final check to not compain about config values in comments
>
> Please let me know if you have any comments or thoughts!
>
> CC: Sam Ravnborg <sam@ravnborg.org>
> CC: gthelen@google.com
> CC: tartler@cs.fau.de
> CC: Dmitry Fink <Dmitry.Fink@palm.com>
> CC: Darren Hart <dvhart@linux.intel.com>
> CC: Eric B Munson <ebmunson@us.ibm.com>
> CC: Bruce Ashfield <Bruce.Ashfield@windriver.com>
> CC: Michal Marek <mmarek@suse.cz>
> CC: linux-kbuild@vger.kernel.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>  scripts/kconfig/merge_config.sh |  117 +++++++++++++++++++++++++++++++++++++++
>  1 files changed, 117 insertions(+), 0 deletions(-)
>  create mode 100755 scripts/kconfig/merge_config.sh
>
> diff --git a/scripts/kconfig/merge_config.sh b/scripts/kconfig/merge_config.sh
> new file mode 100755
> index 0000000..890276b
> --- /dev/null
> +++ b/scripts/kconfig/merge_config.sh
> @@ -0,0 +1,117 @@
> +#!/bin/sh
> +#  merge_config.sh - Takes a list of config fragment values, and merges
> +#  them one by one. Provides warnings on overridden values, and specified
> +#  values that did not make it to the resulting .config file (due to missed
> +#  dependencies or config symbol removal).
> +#
> +#  Portions reused from kconf_check and generate_cfg:
> +#  http://git.yoctoproject.org/cgit/cgit.cgi/yocto-kernel-tools/tree/tools/kconf_check
> +#  http://git.yoctoproject.org/cgit/cgit.cgi/yocto-kernel-tools/tree/tools/generate_cfg
> +#
> +#  Copyright (c) 2009-2010 Wind River Systems, Inc.
> +#  Copyright 2011 Linaro
> +#
> +#  This program is free software; you can redistribute it and/or modify
> +#  it under the terms of the GNU General Public License version 2 as
> +#  published by the Free Software Foundation.
> +#
> +#  This program is distributed in the hope that it will be useful,
> +#  but WITHOUT ANY WARRANTY; without even the implied warranty of
> +#  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> +#  See the GNU General Public License for more details.
> +
> +clean_up() {
> +       rm -f $TMP_FILE
> +       exit
> +}
> +trap clean_up SIGHUP SIGINT SIGTERM
> +
> +usage() {
> +       echo "Usage: $0 [OPTIONS] [CONFIG [...]]"
> +       echo "  -h    display this help text"
> +       echo "  -m    only merge the fragments, do not execute the make command"
> +       echo "  -n    use allnoconfig instead of alldefconfig"
> +}
> +
> +MAKE=true
> +ALLTARGET=alldefconfig
> +
> +while true; do
> +       case $1 in
> +       "-n")
> +               ALLTARGET=allnoconfig
> +               shift
> +               continue
> +               ;;
> +       "-m")
> +               MAKE=false
> +               shift
> +               continue
> +               ;;
> +       "-h")
> +               usage
> +               exit
> +               ;;
> +       *)
> +               break
> +               ;;
> +       esac
> +done
> +
> +
> +
> +MERGE_LIST=$*
> +SED_CONFIG_EXP="s/^\(# \)\{0,1\}\(CONFIG_[a-zA-Z0-9_]*\)[= ].*/\2/p"
> +TMP_FILE=$(mktemp ./.tmp.config.XXXXXXXXXX)
> +
> +# Merge files, printing warnings on overrided values
> +for MERGE_FILE in $MERGE_LIST ; do
> +       echo "Merging $MERGE_FILE"
> +       CFG_LIST=$(sed -n "$SED_CONFIG_EXP" $MERGE_FILE)
> +
> +       for CFG in $CFG_LIST ; do
> +               grep -q -w $CFG $TMP_FILE
> +               if [ $? -eq 0 ] ; then
> +                       PREV_VAL=$(grep -w $CFG $TMP_FILE)
> +                       NEW_VAL=$(grep -w $CFG $MERGE_FILE)
> +                       if [ "x$PREV_VAL" != "x$NEW_VAL" ] ; then
> +                       echo Value of $CFG is redefined by fragment $MERGE_FILE:
> +                       echo Previous  value: $PREV_VAL
> +                       echo New value:       $NEW_VAL
> +                       echo
> +                       fi
> +                       sed -i "/$CFG[ =]/d" $TMP_FILE
> +               fi
> +       done
> +       cat $MERGE_FILE >> $TMP_FILE
> +done
> +
> +if [ "$MAKE" = "false" ]; then
> +       cp $TMP_FILE .config
> +       echo "#"
> +       echo "# merged configuration written to .config (needs make)"
> +       echo "#"
> +       clean_up
> +       exit
> +fi
> +
> +# Use the merged file as the starting point for:
> +# alldefconfig: Fills in any missing symbols with Kconfig default
> +# allnoconfig: Fills in any missing symbols with # CONFIG_* is not set
> +make KCONFIG_ALLCONFIG=$TMP_FILE $ALLTARGET
> +
> +
> +# Check all specified config values took (might have missed-dependency issues)
> +for CFG in $(sed -n "$SED_CONFIG_EXP" $TMP_FILE); do
> +
> +       REQUESTED_VAL=$(sed -n "$SED_CONFIG_EXP" $TMP_FILE | grep -w -e "$CFG")
> +       ACTUAL_VAL=$(sed -n "$SED_CONFIG_EXP" .config | grep -w -e "$CFG")
> +       if [ "x$REQUESTED_VAL" != "x$ACTUAL_VAL" ] ; then
> +               echo "Value requested for $CFG not in final .config"
> +               echo "Requested value:  $REQUESTED_VAL"
> +               echo "Actual value:     $ACTUAL_VAL"
> +               echo ""
> +       fi
> +done
> +
> +clean_up
> --
> 1.7.3.2.146.gca209
>
>
>
> --
> 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
>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] kconfig: Add merge_config.sh script
  2011-11-17 22:44 ` Arnaud Lacombe
@ 2011-11-17 22:54   ` john stultz
  2011-11-19  6:24     ` Arnaud Lacombe
  0 siblings, 1 reply; 20+ messages in thread
From: john stultz @ 2011-11-17 22:54 UTC (permalink / raw)
  To: Arnaud Lacombe
  Cc: Andrew Morton, Sam Ravnborg, gthelen, tartler, Dmitry Fink,
	Darren Hart, Eric B Munson, Bruce Ashfield, Michal Marek,
	linux-kbuild

On Thu, 2011-11-17 at 17:44 -0500, Arnaud Lacombe wrote:
> Hi,
> 
> On Thu, Nov 17, 2011 at 4:58 PM, john stultz <johnstul@us.ibm.com> wrote:
> > Hey Andrew,
> >        I've tried sending this via Michal a few times, but haven't heard much
> > back. So I wanted to check if you would consider merging it via your
> > tree, or if you had any suggestions of who would be better to
> > review/merge this.
> >
> One of the worry I would have is that the script is merging config
> blindly, ie. there is no dependency checking done. I have some some
> work-in-progress to help resolving this, but still lots of thought to
> be implemented.

So the script actually does warn you if a specified option is dropped
due to missing dependencies or if the option is removed.  So, I guess
could you clarify your concern a bit more?

thanks
-john


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] kconfig: Add merge_config.sh script
  2011-11-17 22:54   ` john stultz
@ 2011-11-19  6:24     ` Arnaud Lacombe
  2011-11-21 18:22       ` Darren Hart
                         ` (2 more replies)
  0 siblings, 3 replies; 20+ messages in thread
From: Arnaud Lacombe @ 2011-11-19  6:24 UTC (permalink / raw)
  To: john stultz
  Cc: Andrew Morton, Sam Ravnborg, gthelen, tartler, Dmitry Fink,
	Darren Hart, Eric B Munson, Bruce Ashfield, Michal Marek,
	linux-kbuild

Hi,

On Thu, Nov 17, 2011 at 5:54 PM, john stultz <johnstul@us.ibm.com> wrote:
> On Thu, 2011-11-17 at 17:44 -0500, Arnaud Lacombe wrote:
>> Hi,
>>
>> On Thu, Nov 17, 2011 at 4:58 PM, john stultz <johnstul@us.ibm.com> wrote:
>> > Hey Andrew,
>> >        I've tried sending this via Michal a few times, but haven't heard much
>> > back. So I wanted to check if you would consider merging it via your
>> > tree, or if you had any suggestions of who would be better to
>> > review/merge this.
>> >
>> One of the worry I would have is that the script is merging config
>> blindly, ie. there is no dependency checking done. I have some some
>> work-in-progress to help resolving this, but still lots of thought to
>> be implemented.
>
> So the script actually does warn you if a specified option is dropped
> due to missing dependencies or if the option is removed.  So, I guess
> could you clarify your concern a bit more?
>
well, assuming the following Kconfig's snippet:

choice
        bool "choice"
config A
        bool "A"
config B
        bool "B"
endchoice

and trying to merge:

 - `config1':

CONFIG_A=y

 - `config2':

Result in:

 % sh scripts/kconfig/merge_config.sh config1 config2
Merging config1
Merging config2
scripts/kconfig/conf --alldefconfig Kconfig
./.tmp.config.uMY8Z97l9T:2:warning: override: B changes choice state
#
# configuration written to .config
#

% cat .config
#
# Automatically generated file; DO NOT EDIT.
# Linux Kernel Configuration
#
# CONFIG_A is not set
CONFIG_B=y

so we still get the warning from the incantation of `alldefconfig',
but the one in the script is defeated.

Moreover, there might still be performance optimization to realize,
considering a base 'defconfig' amended by disabling
CONFIG_DECOMPRESS_GZIP, we get:

% time sh scripts/kconfig/merge_config.sh base amend
Merging base
Merging amend
Value of CONFIG_DECOMPRESS_GZIP is redefined by fragment amend:
Previous value: CONFIG_DECOMPRESS_GZIP=y
New value: # CONFIG_DECOMPRESS_GZIP is not set

scripts/kconfig/conf --alldefconfig Kconfig
#
# configuration written to .config
#
sh scripts/kconfig/merge_config.sh base amend  158.20s user 30.19s
system 100% cpu 3:07.86 total

Just by getting rid of the two `| grep ...' in the last step:

--- a/scripts/kconfig/merge_config.sh
+++ b/scripts/kconfig/merge_config.sh
@@ -104,8 +104,8 @@ make KCONFIG_ALLCONFIG=$TMP_FILE $ALLTARGET
 # Check all specified config values took (might have missed-dependency issues)
 for CFG in $(sed -n "$SED_CONFIG_EXP" $TMP_FILE); do

-       REQUESTED_VAL=$(sed -n "$SED_CONFIG_EXP" $TMP_FILE | grep -w -e "$CFG")
-       ACTUAL_VAL=$(sed -n "$SED_CONFIG_EXP" .config | grep -w -e "$CFG")
+       REQUESTED_VAL=$(sed -n "/\<$CFG\>/!d; $SED_CONFIG_EXP" $TMP_FILE)
+       ACTUAL_VAL=$(sed -n "/\<$CFG\>/!d; $SED_CONFIG_EXP" .config)
        if [ "x$REQUESTED_VAL" != "x$ACTUAL_VAL" ] ; then
                echo "Value requested for $CFG not in final .config"
                echo "Requested value:  $REQUESTED_VAL"

we get:

sh scripts/kconfig/merge_config.sh base amend  23.20s user 22.30s
system 94% cpu 48.033 total

Btw, there is other nits in the scripts I'll comment separately.

 - Arnaud

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] kconfig: Add merge_config.sh script
  2011-11-17 21:58 [PATCH] kconfig: Add merge_config.sh script john stultz
  2011-11-17 22:44 ` Arnaud Lacombe
@ 2011-11-19  6:51 ` Arnaud Lacombe
  2011-11-21 18:32   ` Darren Hart
                     ` (2 more replies)
  2011-11-20 12:56 ` Michal Marek
  2011-11-21 19:42 ` [PATCH 1/2] merge_config.sh: use signal names compatible with dash and bash Darren Hart
  3 siblings, 3 replies; 20+ messages in thread
From: Arnaud Lacombe @ 2011-11-19  6:51 UTC (permalink / raw)
  To: John Stultz
  Cc: Arnaud Lacombe, Sam Ravnborg, gthelen, tartler, Dmitry Fink,
	Darren Hart, Eric B Munson, Bruce Ashfield, Michal Marek,
	linux-kbuild

Hi,

On Thu, Nov 17, 2011 at 4:58 PM, john stultz <johnstul@us.ibm.com> wrote:
> [...]
>
> v2:
>  * Reworked to use alldefconfig instead of the proposed
> olddefconfig as suggested by Sam Ravnborg.
>
> v3:
>  * Script improvements from Dmitri.
>  * allnoconfig option from Darren
>  * pre-make exit option from Darren
>  * lots of other fixes/cleanups from Darren.
>  * Fix final check to not compain about config values in comments
>
If Dmitri and Darren have direct contribution to the script, shouldn't their
Signed-off-by tag be present ?

> Please let me know if you have any comments or thoughts!
>
> CC: Sam Ravnborg <sam@ravnborg.org>
> CC: gthelen@google.com
> CC: tartler@cs.fau.de
> CC: Dmitry Fink <Dmitry.Fink@palm.com>
> CC: Darren Hart <dvhart@linux.intel.com>
> CC: Eric B Munson <ebmunson@us.ibm.com>
> CC: Bruce Ashfield <Bruce.Ashfield@windriver.com>
> CC: Michal Marek <mmarek@suse.cz>
> CC: linux-kbuild@vger.kernel.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>

You'll find below some more nits

1) bail out early on error.

This fixes handling of non-existant file:

Before:
    % sh scripts/kconfig/merge_config.sh non existant files
    Merging non
    sed: can't read non: No such file or directory
    cat: non: No such file or directory
    Merging existant
    sed: can't read existant: No such file or directory
    cat: existant: No such file or directory
    Merging files
    sed: can't read files: No such file or directory
    cat: files: No such file or directory
    scripts/kconfig/conf --alldefconfig Kconfig
    #
    # configuration written to .config
    #

After:

    % sh scripts/kconfig/merge_config.sh non existant files
    Merging non
    sed: can't read non: No such file or directory

2) re-implement argument parsing using sh(1) getopts builtin

3) verify that the script was given enough argument to proceed. There isn't
much point running the script with less than 2 arguments.

CC: Sam Ravnborg <sam@ravnborg.org>
CC: gthelen@google.com
CC: tartler@cs.fau.de
CC: Dmitry Fink <Dmitry.Fink@palm.com>
CC: Darren Hart <dvhart@linux.intel.com>
CC: Eric B Munson <ebmunson@us.ibm.com>
CC: Bruce Ashfield <Bruce.Ashfield@windriver.com>
CC: Michal Marek <mmarek@suse.cz>
CC: linux-kbuild@vger.kernel.org
Signed-off-by: Arnaud Lacombe <lacombar@gmail.com>
---
 scripts/kconfig/merge_config.sh |   24 ++++++++++++------------
 1 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/scripts/kconfig/merge_config.sh b/scripts/kconfig/merge_config.sh
index 890276b..abfd8b2 100755
--- a/scripts/kconfig/merge_config.sh
+++ b/scripts/kconfig/merge_config.sh
@@ -20,6 +20,8 @@
 #  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
 #  See the GNU General Public License for more details.
 
+set -e
+
 clean_up() {
        rm -f $TMP_FILE
        exit
@@ -36,29 +38,27 @@ usage() {
 MAKE=true
 ALLTARGET=alldefconfig
 
-while true; do
-	case $1 in
-	"-n")
+while getopts "nmh" opt; do
+	case ${opt} in
+	n)
 		ALLTARGET=allnoconfig
-		shift
-		continue
 		;;
-	"-m")
+	m)
 		MAKE=false
-		shift
-		continue
 		;;
-	"-h")
+	h)
 		usage
 		exit
 		;;
-	*)
-		break
-		;;
 	esac
 done
 
+shift $(expr $OPTIND - 1)
 
+if [ $# -lt 2 ]; then
+	usage
+	exit 1
+fi
 
 MERGE_LIST=$*
 SED_CONFIG_EXP="s/^\(# \)\{0,1\}\(CONFIG_[a-zA-Z0-9_]*\)[= ].*/\2/p"
-- 
1.7.6.153.g78432


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH] kconfig: Add merge_config.sh script
  2011-11-17 21:58 [PATCH] kconfig: Add merge_config.sh script john stultz
  2011-11-17 22:44 ` Arnaud Lacombe
  2011-11-19  6:51 ` Arnaud Lacombe
@ 2011-11-20 12:56 ` Michal Marek
  2011-11-20 18:05   ` Arnaud Lacombe
  2011-11-21 19:42 ` [PATCH 1/2] merge_config.sh: use signal names compatible with dash and bash Darren Hart
  3 siblings, 1 reply; 20+ messages in thread
From: Michal Marek @ 2011-11-20 12:56 UTC (permalink / raw)
  To: john stultz
  Cc: Andrew Morton, Sam Ravnborg, gthelen, tartler, Dmitry Fink,
	Darren Hart, Eric B Munson, Bruce Ashfield, linux-kbuild

On 17.11.2011 22:58, john stultz wrote:
> Hey Andrew, 
> 	I've tried sending this via Michal a few times, but haven't heard much
> back. So I wanted to check if you would consider merging it via your
> tree, or if you had any suggestions of who would be better to
> review/merge this.

My concern was that the script works around missing features in kconfig,
instead of fixing kconfig. Don't get me wrong, I do believe that the
script is useful, but if you are submitting to mainline anyway, why
don't you fix/enhance kconfig in mainline and have a much simpler script
making use of the enhanced kconfig?

Michal

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] kconfig: Add merge_config.sh script
  2011-11-20 12:56 ` Michal Marek
@ 2011-11-20 18:05   ` Arnaud Lacombe
  2011-11-21 18:34     ` Darren Hart
  0 siblings, 1 reply; 20+ messages in thread
From: Arnaud Lacombe @ 2011-11-20 18:05 UTC (permalink / raw)
  To: Michal Marek
  Cc: john stultz, Andrew Morton, Sam Ravnborg, gthelen, tartler,
	Dmitry Fink, Darren Hart, Eric B Munson, Bruce Ashfield,
	linux-kbuild

Hi,

On Sun, Nov 20, 2011 at 7:56 AM, Michal Marek <mmarek@suse.cz> wrote:
> On 17.11.2011 22:58, john stultz wrote:
>> Hey Andrew,
>>       I've tried sending this via Michal a few times, but haven't heard much
>> back. So I wanted to check if you would consider merging it via your
>> tree, or if you had any suggestions of who would be better to
>> review/merge this.
>
> My concern was that the script works around missing features in kconfig,
> instead of fixing kconfig. Don't get me wrong, I do believe that the
> script is useful, but if you are submitting to mainline anyway, why
> don't you fix/enhance kconfig in mainline and have a much simpler script
> making use of the enhanced kconfig?
>
FWIW, as much as I agree with you, kconfig is currently a bit too
static, with too much assumption, do any advanced stuff. I have WIP in
the area, but I prefer not giving ETA. In the mean time, I think
John's script could fill the gap.

 - Arnaud

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] kconfig: Add merge_config.sh script
  2011-11-19  6:24     ` Arnaud Lacombe
@ 2011-11-21 18:22       ` Darren Hart
  2011-11-21 23:29       ` john stultz
  2011-11-21 23:41       ` john stultz
  2 siblings, 0 replies; 20+ messages in thread
From: Darren Hart @ 2011-11-21 18:22 UTC (permalink / raw)
  To: Arnaud Lacombe
  Cc: john stultz, Andrew Morton, Sam Ravnborg, gthelen, tartler,
	Dmitry Fink, Eric B Munson, Bruce Ashfield, Michal Marek,
	linux-kbuild



On 11/18/2011 10:24 PM, Arnaud Lacombe wrote:
> Hi,
> 
> On Thu, Nov 17, 2011 at 5:54 PM, john stultz <johnstul@us.ibm.com> wrote:
>> On Thu, 2011-11-17 at 17:44 -0500, Arnaud Lacombe wrote:
>>> Hi,
>>>
>>> On Thu, Nov 17, 2011 at 4:58 PM, john stultz <johnstul@us.ibm.com> wrote:
>>>> Hey Andrew,
>>>>        I've tried sending this via Michal a few times, but haven't heard much
>>>> back. So I wanted to check if you would consider merging it via your
>>>> tree, or if you had any suggestions of who would be better to
>>>> review/merge this.
>>>>
>>> One of the worry I would have is that the script is merging config
>>> blindly, ie. there is no dependency checking done. I have some some
>>> work-in-progress to help resolving this, but still lots of thought to
>>> be implemented.
>>
>> So the script actually does warn you if a specified option is dropped
>> due to missing dependencies or if the option is removed.  So, I guess
>> could you clarify your concern a bit more?
>>
> well, assuming the following Kconfig's snippet:
> 
> choice
>         bool "choice"
> config A
>         bool "A"
> config B
>         bool "B"
> endchoice
> 
> and trying to merge:
> 
>  - `config1':
> 
> CONFIG_A=y
> 
>  - `config2':
> 
> Result in:
> 
>  % sh scripts/kconfig/merge_config.sh config1 config2
> Merging config1
> Merging config2
> scripts/kconfig/conf --alldefconfig Kconfig
> ./.tmp.config.uMY8Z97l9T:2:warning: override: B changes choice state
> #
> # configuration written to .config
> #
> 
> % cat .config
> #
> # Automatically generated file; DO NOT EDIT.
> # Linux Kernel Configuration
> #
> # CONFIG_A is not set
> CONFIG_B=y
> 
> so we still get the warning from the incantation of `alldefconfig',
> but the one in the script is defeated.
> 
> Moreover, there might still be performance optimization to realize,
> considering a base 'defconfig' amended by disabling
> CONFIG_DECOMPRESS_GZIP, we get:
> 
> % time sh scripts/kconfig/merge_config.sh base amend
> Merging base
> Merging amend
> Value of CONFIG_DECOMPRESS_GZIP is redefined by fragment amend:
> Previous value: CONFIG_DECOMPRESS_GZIP=y
> New value: # CONFIG_DECOMPRESS_GZIP is not set
> 
> scripts/kconfig/conf --alldefconfig Kconfig
> #
> # configuration written to .config
> #
> sh scripts/kconfig/merge_config.sh base amend  158.20s user 30.19s
> system 100% cpu 3:07.86 total
> 
> Just by getting rid of the two `| grep ...' in the last step:
> 
> --- a/scripts/kconfig/merge_config.sh
> +++ b/scripts/kconfig/merge_config.sh
> @@ -104,8 +104,8 @@ make KCONFIG_ALLCONFIG=$TMP_FILE $ALLTARGET
>  # Check all specified config values took (might have missed-dependency issues)
>  for CFG in $(sed -n "$SED_CONFIG_EXP" $TMP_FILE); do
> 
> -       REQUESTED_VAL=$(sed -n "$SED_CONFIG_EXP" $TMP_FILE | grep -w -e "$CFG")
> -       ACTUAL_VAL=$(sed -n "$SED_CONFIG_EXP" .config | grep -w -e "$CFG")
> +       REQUESTED_VAL=$(sed -n "/\<$CFG\>/!d; $SED_CONFIG_EXP" $TMP_FILE)
> +       ACTUAL_VAL=$(sed -n "/\<$CFG\>/!d; $SED_CONFIG_EXP" .config)
>         if [ "x$REQUESTED_VAL" != "x$ACTUAL_VAL" ] ; then
>                 echo "Value requested for $CFG not in final .config"
>                 echo "Requested value:  $REQUESTED_VAL"

These look like good changes to me. We made several optimizations along
these lines, but I confess performance was not my top priority while
working with John on this script.

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] kconfig: Add merge_config.sh script
  2011-11-19  6:51 ` Arnaud Lacombe
@ 2011-11-21 18:32   ` Darren Hart
  2011-11-21 22:06   ` John Stultz
  2011-11-21 22:48   ` john stultz
  2 siblings, 0 replies; 20+ messages in thread
From: Darren Hart @ 2011-11-21 18:32 UTC (permalink / raw)
  To: Arnaud Lacombe
  Cc: John Stultz, Sam Ravnborg, gthelen, tartler, Dmitry Fink,
	Eric B Munson, Bruce Ashfield, Michal Marek, linux-kbuild

Hi Arnaud,

On 11/18/2011 10:51 PM, Arnaud Lacombe wrote:
> Hi,
> 
> On Thu, Nov 17, 2011 at 4:58 PM, john stultz <johnstul@us.ibm.com> wrote:
>> [...]
>>
>> v2:
>>  * Reworked to use alldefconfig instead of the proposed
>> olddefconfig as suggested by Sam Ravnborg.
>>
>> v3:
>>  * Script improvements from Dmitri.
>>  * allnoconfig option from Darren
>>  * pre-make exit option from Darren
>>  * lots of other fixes/cleanups from Darren.
>>  * Fix final check to not compain about config values in comments
>>
> If Dmitri and Darren have direct contribution to the script, shouldn't their
> Signed-off-by tag be present ?

Yes, that would be more correct.

> 
>> Please let me know if you have any comments or thoughts!
>>
>> CC: Sam Ravnborg <sam@ravnborg.org>
>> CC: gthelen@google.com
>> CC: tartler@cs.fau.de
>> CC: Dmitry Fink <Dmitry.Fink@palm.com>
>> CC: Darren Hart <dvhart@linux.intel.com>
>> CC: Eric B Munson <ebmunson@us.ibm.com>
>> CC: Bruce Ashfield <Bruce.Ashfield@windriver.com>
>> CC: Michal Marek <mmarek@suse.cz>
>> CC: linux-kbuild@vger.kernel.org
>> Signed-off-by: John Stultz <john.stultz@linaro.org>
> 
> You'll find below some more nits
> 
> 1) bail out early on error.
> 
> This fixes handling of non-existant file:
> 
> Before:
>     % sh scripts/kconfig/merge_config.sh non existant files
>     Merging non
>     sed: can't read non: No such file or directory
>     cat: non: No such file or directory
>     Merging existant
>     sed: can't read existant: No such file or directory
>     cat: existant: No such file or directory
>     Merging files
>     sed: can't read files: No such file or directory
>     cat: files: No such file or directory
>     scripts/kconfig/conf --alldefconfig Kconfig
>     #
>     # configuration written to .config
>     #
> 
> After:
> 
>     % sh scripts/kconfig/merge_config.sh non existant files
>     Merging non
>     sed: can't read non: No such file or directory

An early test (-f) of each non-option argument would be easy enough to
add, and would provide better error handling/reporting.

> 
> 2) re-implement argument parsing using sh(1) getopts builtin

I was trying to keep it simple - and I was also concerned about
colliding with dash/bash inconsistencies. I have no objection to getopt
if it works consistently in dash and bash. Did you test with dash as
well as bash?

> 
> 3) verify that the script was given enough argument to proceed. There isn't
> much point running the script with less than 2 arguments.

Sure.

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] kconfig: Add merge_config.sh script
  2011-11-20 18:05   ` Arnaud Lacombe
@ 2011-11-21 18:34     ` Darren Hart
  0 siblings, 0 replies; 20+ messages in thread
From: Darren Hart @ 2011-11-21 18:34 UTC (permalink / raw)
  To: Arnaud Lacombe
  Cc: Michal Marek, john stultz, Andrew Morton, Sam Ravnborg, gthelen,
	tartler, Dmitry Fink, Eric B Munson, Bruce Ashfield,
	linux-kbuild



On 11/20/2011 10:05 AM, Arnaud Lacombe wrote:
> Hi,
> 
> On Sun, Nov 20, 2011 at 7:56 AM, Michal Marek <mmarek@suse.cz> wrote:
>> On 17.11.2011 22:58, john stultz wrote:
>>> Hey Andrew,
>>>       I've tried sending this via Michal a few times, but haven't heard much
>>> back. So I wanted to check if you would consider merging it via your
>>> tree, or if you had any suggestions of who would be better to
>>> review/merge this.
>>
>> My concern was that the script works around missing features in kconfig,
>> instead of fixing kconfig. Don't get me wrong, I do believe that the
>> script is useful, but if you are submitting to mainline anyway, why
>> don't you fix/enhance kconfig in mainline and have a much simpler script
>> making use of the enhanced kconfig?
>>
> FWIW, as much as I agree with you, kconfig is currently a bit too
> static, with too much assumption, do any advanced stuff. I have WIP in
> the area, but I prefer not giving ETA. In the mean time, I think
> John's script could fill the gap.

The script is also at least partly usable in older kernel trees without
having to modify Kconfig there. It has some value as a standalone
script. Not to say that we shouldn't also see about updating Kconfig as
well.

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 1/2] merge_config.sh: use signal names compatible with dash and bash
  2011-11-17 21:58 [PATCH] kconfig: Add merge_config.sh script john stultz
                   ` (2 preceding siblings ...)
  2011-11-20 12:56 ` Michal Marek
@ 2011-11-21 19:42 ` Darren Hart
  2011-11-21 19:42   ` [PATCH 2/2] merge_config.sh: whitespace cleanup Darren Hart
  2011-11-21 20:05   ` [PATCH 1/2] merge_config.sh: use signal names compatible with dash and bash John Stultz
  3 siblings, 2 replies; 20+ messages in thread
From: Darren Hart @ 2011-11-21 19:42 UTC (permalink / raw)
  To: Linux Kernel Mailing List
  Cc: John Stultz, Andrew Morton, Sam Ravnborg, gthelen, Dmitry Fink,
	Eric B Munson, Bruce Ashfield, Michal Marek, linux-kbuild,
	Richard Tartler, Darren Hart

The SIGHUP SIGINT and SIGTERM names caused failures when running
merge_config.sh with the dash shell. Dropping the "SIG" component
makes the script work in both bash and dash.

Signed-off-by: Darren Hart <dvhart@linux.intel.com>
---
 scripts/kconfig/merge_config.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/scripts/kconfig/merge_config.sh b/scripts/kconfig/merge_config.sh
index 890276b..b91015d 100755
--- a/scripts/kconfig/merge_config.sh
+++ b/scripts/kconfig/merge_config.sh
@@ -24,7 +24,7 @@ clean_up() {
        rm -f $TMP_FILE
        exit
 }
-trap clean_up SIGHUP SIGINT SIGTERM
+trap clean_up HUP INT TERM
 
 usage() {
 	echo "Usage: $0 [OPTIONS] [CONFIG [...]]"
-- 
1.7.6.4


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 2/2] merge_config.sh: whitespace cleanup
  2011-11-21 19:42 ` [PATCH 1/2] merge_config.sh: use signal names compatible with dash and bash Darren Hart
@ 2011-11-21 19:42   ` Darren Hart
  2011-11-21 20:14     ` John Stultz
  2011-11-21 20:05   ` [PATCH 1/2] merge_config.sh: use signal names compatible with dash and bash John Stultz
  1 sibling, 1 reply; 20+ messages in thread
From: Darren Hart @ 2011-11-21 19:42 UTC (permalink / raw)
  To: Linux Kernel Mailing List
  Cc: John Stultz, Andrew Morton, Sam Ravnborg, gthelen, Dmitry Fink,
	Eric B Munson, Bruce Ashfield, Michal Marek, linux-kbuild,
	Richard Tartler, Darren Hart

Fix whitespace usage in the clean_up routine.

Signed-off-by: Darren Hart <dvhart@linux.intel.com>
---
 scripts/kconfig/merge_config.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/kconfig/merge_config.sh b/scripts/kconfig/merge_config.sh
index b91015d..07bb886 100755
--- a/scripts/kconfig/merge_config.sh
+++ b/scripts/kconfig/merge_config.sh
@@ -21,8 +21,8 @@
 #  See the GNU General Public License for more details.
 
 clean_up() {
-       rm -f $TMP_FILE
-       exit
+	rm -f $TMP_FILE
+	exit
 }
 trap clean_up HUP INT TERM
 
-- 
1.7.6.4


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/2] merge_config.sh: use signal names compatible with dash and bash
  2011-11-21 19:42 ` [PATCH 1/2] merge_config.sh: use signal names compatible with dash and bash Darren Hart
  2011-11-21 19:42   ` [PATCH 2/2] merge_config.sh: whitespace cleanup Darren Hart
@ 2011-11-21 20:05   ` John Stultz
  1 sibling, 0 replies; 20+ messages in thread
From: John Stultz @ 2011-11-21 20:05 UTC (permalink / raw)
  To: Darren Hart
  Cc: Linux Kernel Mailing List, Andrew Morton, Sam Ravnborg, gthelen,
	Dmitry Fink, Eric B Munson, Bruce Ashfield, Michal Marek,
	linux-kbuild, Richard Tartler

On Mon, 2011-11-21 at 11:42 -0800, Darren Hart wrote:
> The SIGHUP SIGINT and SIGTERM names caused failures when running
> merge_config.sh with the dash shell. Dropping the "SIG" component
> makes the script work in both bash and dash.
> 
> Signed-off-by: Darren Hart <dvhart@linux.intel.com>
Acked-by: John Stultz <john.stultz@linaro.org>

> ---
>  scripts/kconfig/merge_config.sh |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/scripts/kconfig/merge_config.sh b/scripts/kconfig/merge_config.sh
> index 890276b..b91015d 100755
> --- a/scripts/kconfig/merge_config.sh
> +++ b/scripts/kconfig/merge_config.sh
> @@ -24,7 +24,7 @@ clean_up() {
>         rm -f $TMP_FILE
>         exit
>  }
> -trap clean_up SIGHUP SIGINT SIGTERM
> +trap clean_up HUP INT TERM
> 
>  usage() {
>  	echo "Usage: $0 [OPTIONS] [CONFIG [...]]"



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/2] merge_config.sh: whitespace cleanup
  2011-11-21 19:42   ` [PATCH 2/2] merge_config.sh: whitespace cleanup Darren Hart
@ 2011-11-21 20:14     ` John Stultz
  0 siblings, 0 replies; 20+ messages in thread
From: John Stultz @ 2011-11-21 20:14 UTC (permalink / raw)
  To: Darren Hart
  Cc: Linux Kernel Mailing List, Andrew Morton, Sam Ravnborg, gthelen,
	Dmitry Fink, Eric B Munson, Bruce Ashfield, Michal Marek,
	linux-kbuild, Richard Tartler

On Mon, 2011-11-21 at 11:42 -0800, Darren Hart wrote:
> Fix whitespace usage in the clean_up routine.
> 
> Signed-off-by: Darren Hart <dvhart@linux.intel.com>

Acked-by: John Stultz <john.stultz@linaro.org>

> ---
>  scripts/kconfig/merge_config.sh |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/kconfig/merge_config.sh b/scripts/kconfig/merge_config.sh
> index b91015d..07bb886 100755
> --- a/scripts/kconfig/merge_config.sh
> +++ b/scripts/kconfig/merge_config.sh
> @@ -21,8 +21,8 @@
>  #  See the GNU General Public License for more details.
> 
>  clean_up() {
> -       rm -f $TMP_FILE
> -       exit
> +	rm -f $TMP_FILE
> +	exit
>  }
>  trap clean_up HUP INT TERM
> 



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] kconfig: Add merge_config.sh script
  2011-11-19  6:51 ` Arnaud Lacombe
  2011-11-21 18:32   ` Darren Hart
@ 2011-11-21 22:06   ` John Stultz
  2011-11-21 22:48   ` john stultz
  2 siblings, 0 replies; 20+ messages in thread
From: John Stultz @ 2011-11-21 22:06 UTC (permalink / raw)
  To: Arnaud Lacombe
  Cc: Sam Ravnborg, gthelen, tartler, Dmitry Fink, Darren Hart,
	Eric B Munson, Bruce Ashfield, Michal Marek, linux-kbuild

On Sat, 2011-11-19 at 01:51 -0500, Arnaud Lacombe wrote:
> Hi,
> 
> On Thu, Nov 17, 2011 at 4:58 PM, john stultz <johnstul@us.ibm.com> wrote:
> > [...]
> >
> > v2:
> >  * Reworked to use alldefconfig instead of the proposed
> > olddefconfig as suggested by Sam Ravnborg.
> >
> > v3:
> >  * Script improvements from Dmitri.
> >  * allnoconfig option from Darren
> >  * pre-make exit option from Darren
> >  * lots of other fixes/cleanups from Darren.
> >  * Fix final check to not compain about config values in comments
> >
> If Dmitri and Darren have direct contribution to the script, shouldn't their
> Signed-off-by tag be present ?

Dmitri didn't provide a Signed-off-by, and I had reworked things
sufficiently that his patch didn't apply, so I re-implemented many of
the suggested improvements myself. However, Dmitri deserves the credits
for pointing the issues out.

Similarly, with Darren, there were a number of non-signed-off-by'ed
changes, I had to merge in by hand due to my folding of my own work in.
But there were also a few proper patches from Darren that I did fold in
and should have preserved his Signed-off-by. That was sloppy, so my
apologies. Its now fixed in my tree.

> You'll find below some more nits
> 
> 1) bail out early on error.
> 
> This fixes handling of non-existant file:
> 
> Before:
>     % sh scripts/kconfig/merge_config.sh non existant files
>     Merging non
>     sed: can't read non: No such file or directory
>     cat: non: No such file or directory
>     Merging existant
>     sed: can't read existant: No such file or directory
>     cat: existant: No such file or directory
>     Merging files
>     sed: can't read files: No such file or directory
>     cat: files: No such file or directory
>     scripts/kconfig/conf --alldefconfig Kconfig
>     #
>     # configuration written to .config
>     #
> 
> After:
> 
>     % sh scripts/kconfig/merge_config.sh non existant files
>     Merging non
>     sed: can't read non: No such file or directory
> 
> 2) re-implement argument parsing using sh(1) getopts builtin
> 
> 3) verify that the script was given enough argument to proceed. There isn't
> much point running the script with less than 2 arguments.
> 
> CC: Sam Ravnborg <sam@ravnborg.org>
> CC: gthelen@google.com
> CC: tartler@cs.fau.de
> CC: Dmitry Fink <Dmitry.Fink@palm.com>
> CC: Darren Hart <dvhart@linux.intel.com>
> CC: Eric B Munson <ebmunson@us.ibm.com>
> CC: Bruce Ashfield <Bruce.Ashfield@windriver.com>
> CC: Michal Marek <mmarek@suse.cz>
> CC: linux-kbuild@vger.kernel.org
> Signed-off-by: Arnaud Lacombe <lacombar@gmail.com>

Thanks for these great cleanups and feedback!

Acked-by: John Stultz <john.stultz@linaro.org>

I've queued these (along with your and Darren's other changes).

thanks
-john

> ---
>  scripts/kconfig/merge_config.sh |   24 ++++++++++++------------
>  1 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/scripts/kconfig/merge_config.sh b/scripts/kconfig/merge_config.sh
> index 890276b..abfd8b2 100755
> --- a/scripts/kconfig/merge_config.sh
> +++ b/scripts/kconfig/merge_config.sh
> @@ -20,6 +20,8 @@
>  #  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
>  #  See the GNU General Public License for more details.
> 
> +set -e
> +
>  clean_up() {
>         rm -f $TMP_FILE
>         exit
> @@ -36,29 +38,27 @@ usage() {
>  MAKE=true
>  ALLTARGET=alldefconfig
> 
> -while true; do
> -	case $1 in
> -	"-n")
> +while getopts "nmh" opt; do
> +	case ${opt} in
> +	n)
>  		ALLTARGET=allnoconfig
> -		shift
> -		continue
>  		;;
> -	"-m")
> +	m)
>  		MAKE=false
> -		shift
> -		continue
>  		;;
> -	"-h")
> +	h)
>  		usage
>  		exit
>  		;;
> -	*)
> -		break
> -		;;
>  	esac
>  done
> 
> +shift $(expr $OPTIND - 1)
> 
> +if [ $# -lt 2 ]; then
> +	usage
> +	exit 1
> +fi
> 
>  MERGE_LIST=$*
>  SED_CONFIG_EXP="s/^\(# \)\{0,1\}\(CONFIG_[a-zA-Z0-9_]*\)[= ].*/\2/p"



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] kconfig: Add merge_config.sh script
  2011-11-19  6:51 ` Arnaud Lacombe
  2011-11-21 18:32   ` Darren Hart
  2011-11-21 22:06   ` John Stultz
@ 2011-11-21 22:48   ` john stultz
  2011-11-21 22:55     ` john stultz
  2011-11-22  6:25     ` Arnaud Lacombe
  2 siblings, 2 replies; 20+ messages in thread
From: john stultz @ 2011-11-21 22:48 UTC (permalink / raw)
  To: Arnaud Lacombe
  Cc: Sam Ravnborg, gthelen, tartler, Dmitry Fink, Darren Hart,
	Eric B Munson, Bruce Ashfield, Michal Marek, linux-kbuild

On Sat, 2011-11-19 at 01:51 -0500, Arnaud Lacombe wrote:
> You'll find below some more nits
> 
> 1) bail out early on error.

Actually, I spoke a little too soon.

The set -e option causes us to bomb out if grep doesn't find a config
option (grep returns 1 if nothing is found). So I'll have to drop that
change for now.

thanks
-john



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] kconfig: Add merge_config.sh script
  2011-11-21 22:48   ` john stultz
@ 2011-11-21 22:55     ` john stultz
  2011-11-22  6:25     ` Arnaud Lacombe
  1 sibling, 0 replies; 20+ messages in thread
From: john stultz @ 2011-11-21 22:55 UTC (permalink / raw)
  To: Arnaud Lacombe
  Cc: Sam Ravnborg, gthelen, tartler, Dmitry Fink, Darren Hart,
	Eric B Munson, Bruce Ashfield, Michal Marek, linux-kbuild

On Mon, 2011-11-21 at 14:48 -0800, john stultz wrote:
> On Sat, 2011-11-19 at 01:51 -0500, Arnaud Lacombe wrote:
> > You'll find below some more nits
> > 
> > 1) bail out early on error.
> 
> Actually, I spoke a little too soon.
> 
> The set -e option causes us to bomb out if grep doesn't find a config
> option (grep returns 1 if nothing is found). So I'll have to drop that
> change for now.

Catching the error explicitly is easy enough, so folded in the following
fix (copied here from a terminal, so its whitespace damaged, I know) to
your patch:

@@ -20,8 +20,6 @@
 #  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
 #  See the GNU General Public License for more details.

-set -e
-
 clean_up() {
        rm -f $TMP_FILE
        exit
@@ -68,6 +66,9 @@ TMP_FILE=$(mktemp ./.tmp.config.XXXXXXXXXX)
 for MERGE_FILE in $MERGE_LIST ; do
        echo "Merging $MERGE_FILE"
        CFG_LIST=$(sed -n "$SED_CONFIG_EXP" $MERGE_FILE)
+       if [ $? != 0 ] ; then
+               exit 1
+       fi

        for CFG in $CFG_LIST ; do
                grep -q -w $CFG $TMP_FILE


thanks
-john


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] kconfig: Add merge_config.sh script
  2011-11-19  6:24     ` Arnaud Lacombe
  2011-11-21 18:22       ` Darren Hart
@ 2011-11-21 23:29       ` john stultz
  2011-11-21 23:41       ` john stultz
  2 siblings, 0 replies; 20+ messages in thread
From: john stultz @ 2011-11-21 23:29 UTC (permalink / raw)
  To: Arnaud Lacombe
  Cc: Andrew Morton, Sam Ravnborg, gthelen, tartler, Dmitry Fink,
	Darren Hart, Eric B Munson, Bruce Ashfield, Michal Marek,
	linux-kbuild

On Sat, 2011-11-19 at 01:24 -0500, Arnaud Lacombe wrote:
> Hi,
> 
> On Thu, Nov 17, 2011 at 5:54 PM, john stultz <johnstul@us.ibm.com> wrote:
> > On Thu, 2011-11-17 at 17:44 -0500, Arnaud Lacombe wrote:
> >> Hi,
> >>
> >> On Thu, Nov 17, 2011 at 4:58 PM, john stultz <johnstul@us.ibm.com> wrote:
> >> > Hey Andrew,
> >> >        I've tried sending this via Michal a few times, but haven't heard much
> >> > back. So I wanted to check if you would consider merging it via your
> >> > tree, or if you had any suggestions of who would be better to
> >> > review/merge this.
> >> >
> >> One of the worry I would have is that the script is merging config
> >> blindly, ie. there is no dependency checking done. I have some some
> >> work-in-progress to help resolving this, but still lots of thought to
> >> be implemented.
> >
> > So the script actually does warn you if a specified option is dropped
> > due to missing dependencies or if the option is removed.  So, I guess
> > could you clarify your concern a bit more?
> >
> well, assuming the following Kconfig's snippet:
> 
> choice
>         bool "choice"
> config A
>         bool "A"
> config B
>         bool "B"
> endchoice
> 
> and trying to merge:
> 
>  - `config1':
> 
> CONFIG_A=y
> 
>  - `config2':
> 
> Result in:
> 
>  % sh scripts/kconfig/merge_config.sh config1 config2
> Merging config1
> Merging config2
> scripts/kconfig/conf --alldefconfig Kconfig
> ./.tmp.config.uMY8Z97l9T:2:warning: override: B changes choice state
> #
> # configuration written to .config
> #
> 
> % cat .config
> #
> # Automatically generated file; DO NOT EDIT.
> # Linux Kernel Configuration
> #
> # CONFIG_A is not set
> CONFIG_B=y
> 
> so we still get the warning from the incantation of `alldefconfig',
> but the one in the script is defeated.
> 
> Moreover, there might still be performance optimization to realize,
> considering a base 'defconfig' amended by disabling
> CONFIG_DECOMPRESS_GZIP, we get:
> 
> % time sh scripts/kconfig/merge_config.sh base amend
> Merging base
> Merging amend
> Value of CONFIG_DECOMPRESS_GZIP is redefined by fragment amend:
> Previous value: CONFIG_DECOMPRESS_GZIP=y
> New value: # CONFIG_DECOMPRESS_GZIP is not set
> 
> scripts/kconfig/conf --alldefconfig Kconfig
> #
> # configuration written to .config
> #
> sh scripts/kconfig/merge_config.sh base amend  158.20s user 30.19s
> system 100% cpu 3:07.86 total

Yep. So you've caught a bug! Thanks for pointing this out!

> Just by getting rid of the two `| grep ...' in the last step:
> 
> --- a/scripts/kconfig/merge_config.sh
> +++ b/scripts/kconfig/merge_config.sh
> @@ -104,8 +104,8 @@ make KCONFIG_ALLCONFIG=$TMP_FILE $ALLTARGET
>  # Check all specified config values took (might have missed-dependency issues)
>  for CFG in $(sed -n "$SED_CONFIG_EXP" $TMP_FILE); do
> 
> -       REQUESTED_VAL=$(sed -n "$SED_CONFIG_EXP" $TMP_FILE | grep -w -e "$CFG")
> -       ACTUAL_VAL=$(sed -n "$SED_CONFIG_EXP" .config | grep -w -e "$CFG")
> +       REQUESTED_VAL=$(sed -n "/\<$CFG\>/!d; $SED_CONFIG_EXP" $TMP_FILE)
> +       ACTUAL_VAL=$(sed -n "/\<$CFG\>/!d; $SED_CONFIG_EXP" .config)
>         if [ "x$REQUESTED_VAL" != "x$ACTUAL_VAL" ] ; then
>                 echo "Value requested for $CFG not in final .config"
>                 echo "Requested value:  $REQUESTED_VAL"

So, its not the greps in my mind that are the issue, its the
SED_CONF_EXP lines. Those shouldn't be applied to the REQUESTED and
ACTUAL values, since they would strip out any difference between the
two, causing the comparison to be moot.

I was trying to fix the script from complaining like:
	Value requested for CONFIG_EXPERT not in final .config
	Requested value:  # CONFIG_EXPERT=y
	Actual value:     # CONFIG_EXPERT is not set

But clearly the change was wrong.

Anyway, reverting back to something like the following should fix it:

diff --git a/scripts/kconfig/merge_config.sh b/scripts/kconfig/merge_config.sh
index b146b76..adac068 100755
--- a/scripts/kconfig/merge_config.sh
+++ b/scripts/kconfig/merge_config.sh
@@ -105,8 +105,8 @@ make KCONFIG_ALLCONFIG=$TMP_FILE $ALLTARGET
 # Check all specified config values took (might have missed-dependency issues)
 for CFG in $(sed -n "$SED_CONFIG_EXP" $TMP_FILE); do
 
-	REQUESTED_VAL=$(sed -n "$SED_CONFIG_EXP" $TMP_FILE | grep -w -e "$CFG")
-	ACTUAL_VAL=$(sed -n "$SED_CONFIG_EXP" .config | grep -w -e "$CFG")
+	REQUESTED_VAL=$(grep -w -e "$CFG" $TMP_FILE)
+	ACTUAL_VAL=$(grep -w -e "$CFG" .config)
 	if [ "x$REQUESTED_VAL" != "x$ACTUAL_VAL" ] ; then
 		echo "Value requested for $CFG not in final .config"
 		echo "Requested value:  $REQUESTED_VAL"


Andrew: Would you prefer small fixes ontop of the patch you took, or
just a new patch that includes this along with Darren's and Arnaud's
fixes? 

thanks
-john


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH] kconfig: Add merge_config.sh script
  2011-11-19  6:24     ` Arnaud Lacombe
  2011-11-21 18:22       ` Darren Hart
  2011-11-21 23:29       ` john stultz
@ 2011-11-21 23:41       ` john stultz
  2 siblings, 0 replies; 20+ messages in thread
From: john stultz @ 2011-11-21 23:41 UTC (permalink / raw)
  To: Arnaud Lacombe
  Cc: Andrew Morton, Sam Ravnborg, gthelen, tartler, Dmitry Fink,
	Darren Hart, Eric B Munson, Bruce Ashfield, Michal Marek,
	linux-kbuild

On Sat, 2011-11-19 at 01:24 -0500, Arnaud Lacombe wrote:
> Hi,
> 
> On Thu, Nov 17, 2011 at 5:54 PM, john stultz <johnstul@us.ibm.com> wrote:
> > On Thu, 2011-11-17 at 17:44 -0500, Arnaud Lacombe wrote:
> >> Hi,
> >>
> >> On Thu, Nov 17, 2011 at 4:58 PM, john stultz <johnstul@us.ibm.com> wrote:
> >> > Hey Andrew,
> >> >        I've tried sending this via Michal a few times, but haven't heard much
> >> > back. So I wanted to check if you would consider merging it via your
> >> > tree, or if you had any suggestions of who would be better to
> >> > review/merge this.
> >> >
> >> One of the worry I would have is that the script is merging config
> >> blindly, ie. there is no dependency checking done. I have some some
> >> work-in-progress to help resolving this, but still lots of thought to
> >> be implemented.
> >
> > So the script actually does warn you if a specified option is dropped
> > due to missing dependencies or if the option is removed.  So, I guess
> > could you clarify your concern a bit more?
> >
> well, assuming the following Kconfig's snippet:
> 
> choice
>         bool "choice"
> config A
>         bool "A"
> config B
>         bool "B"
> endchoice
> 
> and trying to merge:
> 
>  - `config1':
> 
> CONFIG_A=y
> 
>  - `config2':
> 
> Result in:
> 
>  % sh scripts/kconfig/merge_config.sh config1 config2
> Merging config1
> Merging config2
> scripts/kconfig/conf --alldefconfig Kconfig
> ./.tmp.config.uMY8Z97l9T:2:warning: override: B changes choice state
> #
> # configuration written to .config
> #
> 
> % cat .config
> #
> # Automatically generated file; DO NOT EDIT.
> # Linux Kernel Configuration
> #
> # CONFIG_A is not set
> CONFIG_B=y
> 
> so we still get the warning from the incantation of `alldefconfig',
> but the one in the script is defeated.

You can find the fix for this along with other recent changes from
Darren and Arnaud in my git tree here:
git://git.linaro.org/people/jstultz/linux.git dev/config-fragments

Browse-able here:
http://git.linaro.org/gitweb?p=people/jstultz/linux.git;a=shortlog;h=refs/heads/dev/config-fragments

Let me know if you have any other thoughts or feedback!

thanks
-john



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] kconfig: Add merge_config.sh script
  2011-11-21 22:48   ` john stultz
  2011-11-21 22:55     ` john stultz
@ 2011-11-22  6:25     ` Arnaud Lacombe
  1 sibling, 0 replies; 20+ messages in thread
From: Arnaud Lacombe @ 2011-11-22  6:25 UTC (permalink / raw)
  To: john stultz
  Cc: Sam Ravnborg, gthelen, tartler, Dmitry Fink, Darren Hart,
	Eric B Munson, Bruce Ashfield, Michal Marek, linux-kbuild

Hi,

On Mon, Nov 21, 2011 at 5:48 PM, john stultz <johnstul@us.ibm.com> wrote:
> On Sat, 2011-11-19 at 01:51 -0500, Arnaud Lacombe wrote:
>> You'll find below some more nits
>>
>> 1) bail out early on error.
>
> Actually, I spoke a little too soon.
>
> The set -e option causes us to bomb out if grep doesn't find a config
> option (grep returns 1 if nothing is found). So I'll have to drop that
> change for now.
>
because the way the return value of grep(1) is checked is bogus. It
should not be:

    grep -q -w $CFG $TMP_FILE
    if [ $? -eq 0 ] ; then
        ...
    fi

but:

    if grep -q -w $CFG $TMP_FILE ; then
        ...
    fi

that way, you can still keep the `set -e'. IMHO, it should be set by
default for any shell script, just as a safety net, as `set -u'.

 - Arnaud

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2011-11-22  6:25 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-11-17 21:58 [PATCH] kconfig: Add merge_config.sh script john stultz
2011-11-17 22:44 ` Arnaud Lacombe
2011-11-17 22:54   ` john stultz
2011-11-19  6:24     ` Arnaud Lacombe
2011-11-21 18:22       ` Darren Hart
2011-11-21 23:29       ` john stultz
2011-11-21 23:41       ` john stultz
2011-11-19  6:51 ` Arnaud Lacombe
2011-11-21 18:32   ` Darren Hart
2011-11-21 22:06   ` John Stultz
2011-11-21 22:48   ` john stultz
2011-11-21 22:55     ` john stultz
2011-11-22  6:25     ` Arnaud Lacombe
2011-11-20 12:56 ` Michal Marek
2011-11-20 18:05   ` Arnaud Lacombe
2011-11-21 18:34     ` Darren Hart
2011-11-21 19:42 ` [PATCH 1/2] merge_config.sh: use signal names compatible with dash and bash Darren Hart
2011-11-21 19:42   ` [PATCH 2/2] merge_config.sh: whitespace cleanup Darren Hart
2011-11-21 20:14     ` John Stultz
2011-11-21 20:05   ` [PATCH 1/2] merge_config.sh: use signal names compatible with dash and bash John Stultz

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.