[PATCH v2] merge_config.sh: Check error codes from make

[PATCH v2] merge_config.sh: Check error codes from make

[Mark Brown]

When we execute make after merging the configurations we ignore any
errors it produces causing whatever is running merge_config.sh to be
unaware of any failures.  This issue was noticed by Guillaume Tucker
while looking at problems with testing of clang only builds in KernelCI
which caused Kbuild to be unable to find a working host compiler.

This implementation was suggested by Yamada-san.

Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com>
Reported-by: Guillaume Tucker <guillaume.tucker@collabora.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 scripts/kconfig/merge_config.sh | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/scripts/kconfig/merge_config.sh b/scripts/kconfig/merge_config.sh
index d924c51d28b7..bec246719aea 100755
--- a/scripts/kconfig/merge_config.sh
+++ b/scripts/kconfig/merge_config.sh
@@ -13,12 +13,12 @@
 #  Copyright (c) 2009-2010 Wind River Systems, Inc.
 #  Copyright 2011 Linaro
 
+set -e
+
 clean_up() {
 	rm -f $TMP_FILE
 	rm -f $MERGE_FILE
-	exit
 }
-trap clean_up HUP INT TERM
 
 usage() {
 	echo "Usage: $0 [OPTIONS] [CONFIG [...]]"
@@ -110,6 +110,9 @@ TMP_FILE=$(mktemp ./.tmp.config.XXXXXXXXXX)
 MERGE_FILE=$(mktemp ./.merge_tmp.config.XXXXXXXXXX)
 
 echo "Using $INITFILE as base"
+
+trap clean_up EXIT
+
 cat $INITFILE > $TMP_FILE
 
 # Merge files, printing warnings on overridden values
@@ -155,7 +158,6 @@ if [ "$RUNMAKE" = "false" ]; then
 	echo "#"
 	echo "# merged configuration written to $KCONFIG_CONFIG (needs make)"
 	echo "#"
-	clean_up
 	exit
 fi
 
@@ -185,5 +187,3 @@ for CFG in $(sed -n -e "$SED_CONFIG_EXP1" -e "$SED_CONFIG_EXP2" $TMP_FILE); do
 		echo ""
 	fi
 done
-
-clean_up
-- 
2.20.1

Re: [PATCH v2] merge_config.sh: Check error codes from make

[Masahiro Yamada]

On Tue, Aug 20, 2019 at 5:07 AM Mark Brown <broonie@kernel.org> wrote:
>
> When we execute make after merging the configurations we ignore any
> errors it produces causing whatever is running merge_config.sh to be
> unaware of any failures.  This issue was noticed by Guillaume Tucker
> while looking at problems with testing of clang only builds in KernelCI
> which caused Kbuild to be unable to find a working host compiler.
>
> This implementation was suggested by Yamada-san.
>
> Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> Reported-by: Guillaume Tucker <guillaume.tucker@collabora.com>
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---

Applied to linux-kbuild.
Thanks.

>  scripts/kconfig/merge_config.sh | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/scripts/kconfig/merge_config.sh b/scripts/kconfig/merge_config.sh
> index d924c51d28b7..bec246719aea 100755
> --- a/scripts/kconfig/merge_config.sh
> +++ b/scripts/kconfig/merge_config.sh
> @@ -13,12 +13,12 @@
>  #  Copyright (c) 2009-2010 Wind River Systems, Inc.
>  #  Copyright 2011 Linaro
>
> +set -e
> +
>  clean_up() {
>         rm -f $TMP_FILE
>         rm -f $MERGE_FILE
> -       exit
>  }
> -trap clean_up HUP INT TERM
>
>  usage() {
>         echo "Usage: $0 [OPTIONS] [CONFIG [...]]"
> @@ -110,6 +110,9 @@ TMP_FILE=$(mktemp ./.tmp.config.XXXXXXXXXX)
>  MERGE_FILE=$(mktemp ./.merge_tmp.config.XXXXXXXXXX)
>
>  echo "Using $INITFILE as base"
> +
> +trap clean_up EXIT
> +
>  cat $INITFILE > $TMP_FILE
>
>  # Merge files, printing warnings on overridden values
> @@ -155,7 +158,6 @@ if [ "$RUNMAKE" = "false" ]; then
>         echo "#"
>         echo "# merged configuration written to $KCONFIG_CONFIG (needs make)"
>         echo "#"
> -       clean_up
>         exit
>  fi
>
> @@ -185,5 +187,3 @@ for CFG in $(sed -n -e "$SED_CONFIG_EXP1" -e "$SED_CONFIG_EXP2" $TMP_FILE); do
>                 echo ""
>         fi
>  done
> -
> -clean_up
> --
> 2.20.1
>


-- 
Best Regards
Masahiro Yamada

Re: [PATCH v2] merge_config.sh: Check error codes from make

[Jon Hunter]

On 19/08/2019 21:06, Mark Brown wrote:
> When we execute make after merging the configurations we ignore any
> errors it produces causing whatever is running merge_config.sh to be
> unaware of any failures.  This issue was noticed by Guillaume Tucker
> while looking at problems with testing of clang only builds in KernelCI
> which caused Kbuild to be unable to find a working host compiler.
> 
> This implementation was suggested by Yamada-san.
> 
> Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> Reported-by: Guillaume Tucker <guillaume.tucker@collabora.com>
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---

I have noticed some recent build failures on -next and the bisect is 
pointing to this commit. I have been looking at why this commit is 
making the builds fail and I see a few different things going on ...

1. By using 'set -e', if grep fails to find a kconfig option in the   
   resulting config file, then script exits silently without reporting 
   which option it failed to find. Hence, it is unclear what triggered 
   the failure. This may happen when options are being disabled.

2. If an option is disabled by the config fragment that happens to be a 
   parent of other kconfig options, then although the parent and 
   children are disabled correctly, the script may fail because it no 
   longer finds the children in the resulting config file. A specific 
   example, here is CONFIG_NFS_V4. We disable this option due to 
   issues with some host machines we use, and disabling this also 
   disables CONFIG_NFS_V4_1 and CONFIG_NFS_V4_2. Now if all 3 of these 
   options are enabled by default in the base config file, such as the 
   case in the ARM64 defconfig, then disabling CONFIG_NFS_V4 in the 
   config fragment causes merge_config.sh to fail because  
   CONFIG_NFS_V4_1 and CONFIG_NFS_V4_2 are not defined at all in 
   the resulting config. This causes grep to fail to find these and 
   hence causes the script to terminate. In the resulting config file we 
   just have '# CONFIG_NFS_V4 is not set'. I am not sure if there is an 
   easy way to determine if a missing config option is legitimate or 
   not. 

A simple way to test the above is ...

 $ export ARCH=arm64
 $ echo "CONFIG_NFS_V4=n" > kfrag                                                                                                                                                   
 $ ./scripts/kconfig/merge_config.sh arch/arm64/configs/defconfig kfrag 

If the intent is to catch errors returned by make, then one simple fix would be ...

diff --git a/scripts/kconfig/merge_config.sh b/scripts/kconfig/merge_config.sh
index bec246719aea..63c8565206a4 100755
--- a/scripts/kconfig/merge_config.sh
+++ b/scripts/kconfig/merge_config.sh
@@ -179,7 +179,7 @@ make KCONFIG_ALLCONFIG=$TMP_FILE $OUTPUT_ARG $ALLTARGET
 for CFG in $(sed -n -e "$SED_CONFIG_EXP1" -e "$SED_CONFIG_EXP2" $TMP_FILE); do
 
        REQUESTED_VAL=$(grep -w -e "$CFG" $TMP_FILE)
-       ACTUAL_VAL=$(grep -w -e "$CFG" "$KCONFIG_CONFIG")
+       ACTUAL_VAL=$(grep -w -e "$CFG" "$KCONFIG_CONFIG" || true)
        if [ "x$REQUESTED_VAL" != "x$ACTUAL_VAL" ] ; then
                echo "Value requested for $CFG not in final .config"
                echo "Requested value:  $REQUESTED_VAL"


I have been using merge_config.sh to enable and disable various options
we need for testing for sometime now and so would hope I am not doing
anything out of the ordinary here. 

Let me know your thoughts.

Cheers
Jon

-- 
nvpublic

Re: [PATCH v2] merge_config.sh: Check error codes from make

[Guillaume Tucker]

On 02/09/2019 15:06, Jon Hunter wrote:
> 
> On 19/08/2019 21:06, Mark Brown wrote:
>> When we execute make after merging the configurations we ignore any
>> errors it produces causing whatever is running merge_config.sh to be
>> unaware of any failures.  This issue was noticed by Guillaume Tucker
>> while looking at problems with testing of clang only builds in KernelCI
>> which caused Kbuild to be unable to find a working host compiler.
>>
>> This implementation was suggested by Yamada-san.
>>
>> Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>> Reported-by: Guillaume Tucker <guillaume.tucker@collabora.com>
>> Signed-off-by: Mark Brown <broonie@kernel.org>
>> ---
> 
> I have noticed some recent build failures on -next and the bisect is 
> pointing to this commit. I have been looking at why this commit is 
> making the builds fail and I see a few different things going on ...
> 
> 1. By using 'set -e', if grep fails to find a kconfig option in the   
>    resulting config file, then script exits silently without reporting 
>    which option it failed to find. Hence, it is unclear what triggered 
>    the failure. This may happen when options are being disabled.
> 
> 2. If an option is disabled by the config fragment that happens to be a 
>    parent of other kconfig options, then although the parent and 
>    children are disabled correctly, the script may fail because it no 
>    longer finds the children in the resulting config file. A specific 
>    example, here is CONFIG_NFS_V4. We disable this option due to 
>    issues with some host machines we use, and disabling this also 
>    disables CONFIG_NFS_V4_1 and CONFIG_NFS_V4_2. Now if all 3 of these 
>    options are enabled by default in the base config file, such as the 
>    case in the ARM64 defconfig, then disabling CONFIG_NFS_V4 in the 
>    config fragment causes merge_config.sh to fail because  
>    CONFIG_NFS_V4_1 and CONFIG_NFS_V4_2 are not defined at all in 
>    the resulting config. This causes grep to fail to find these and 
>    hence causes the script to terminate. In the resulting config file we 
>    just have '# CONFIG_NFS_V4 is not set'. I am not sure if there is an 
>    easy way to determine if a missing config option is legitimate or 
>    not. 
> 
> A simple way to test the above is ...
> 
>  $ export ARCH=arm64
>  $ echo "CONFIG_NFS_V4=n" > kfrag                                                                                                                                                   
>  $ ./scripts/kconfig/merge_config.sh arch/arm64/configs/defconfig kfrag 
> 
> If the intent is to catch errors returned by make, then one simple fix would be ...
> 
> diff --git a/scripts/kconfig/merge_config.sh b/scripts/kconfig/merge_config.sh
> index bec246719aea..63c8565206a4 100755
> --- a/scripts/kconfig/merge_config.sh
> +++ b/scripts/kconfig/merge_config.sh
> @@ -179,7 +179,7 @@ make KCONFIG_ALLCONFIG=$TMP_FILE $OUTPUT_ARG $ALLTARGET
>  for CFG in $(sed -n -e "$SED_CONFIG_EXP1" -e "$SED_CONFIG_EXP2" $TMP_FILE); do
>  
>         REQUESTED_VAL=$(grep -w -e "$CFG" $TMP_FILE)
> -       ACTUAL_VAL=$(grep -w -e "$CFG" "$KCONFIG_CONFIG")
> +       ACTUAL_VAL=$(grep -w -e "$CFG" "$KCONFIG_CONFIG" || true)
>         if [ "x$REQUESTED_VAL" != "x$ACTUAL_VAL" ] ; then
>                 echo "Value requested for $CFG not in final .config"
>                 echo "Requested value:  $REQUESTED_VAL"
> 
> 
> I have been using merge_config.sh to enable and disable various options
> we need for testing for sometime now and so would hope I am not doing
> anything out of the ordinary here. 
> 
> Let me know your thoughts.

I've added you to another thread with a fix I sent last week for
the same issue.

The way I addressed it with "echo" was to explicitly return an
empty line as that is essentially what is then being used to
compare the config values.  I guess "true" also works in
practice.

My understanding is that "set -e" was added primarily to catch
errors returned by the make command.  The config value checks
with grep have always been warnings that don't cause errors, so I
would assume that it should stay like this until there's a
conscious decision to change this behaviour.

Thanks,
Guillaume

Re: [PATCH v2] merge_config.sh: Check error codes from make

[Mark Brown]

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

On Mon, Sep 02, 2019 at 03:06:28PM +0100, Jon Hunter wrote:

> If the intent is to catch errors returned by make, then one simple fix would be ...

I proposed a simpler version originally that only caught errors from
make, this version was suggested by Yamada-san.  Like Guillaume says he
posted a proposed fix for this last week.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]