Re: [PATCH] kconfig: Add merge_config.sh script

[john stultz <johnstul@us.ibm.com>]

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