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

[PATCH] 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.

Reported-by: Guillaume Tucker <guillaume.tucker@collabora.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 scripts/kconfig/merge_config.sh | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/scripts/kconfig/merge_config.sh b/scripts/kconfig/merge_config.sh
index d924c51d28b7..96e960dce968 100755
--- a/scripts/kconfig/merge_config.sh
+++ b/scripts/kconfig/merge_config.sh
@@ -14,9 +14,10 @@
 #  Copyright 2011 Linaro
 
 clean_up() {
+	RET=$1
 	rm -f $TMP_FILE
 	rm -f $MERGE_FILE
-	exit
+	exit ${RET}
 }
 trap clean_up HUP INT TERM
 
@@ -171,6 +172,10 @@ fi
 # 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 $OUTPUT_ARG $ALLTARGET
+RET=$?
+if [ "$RET" != "0" ]; then
+	clean_up $RET
+fi
 
 
 # Check all specified config values took (might have missed-dependency issues)
-- 
2.20.1

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

[Masahiro Yamada]

On Fri, Aug 9, 2019 at 7:27 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.
>
> Reported-by: Guillaume Tucker <guillaume.tucker@collabora.com>
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---

I am not a big fan of this way of fixing.



[1] 'set -e' is useful to catch any error in this script.

[2] I think trapping only EXIT is smarter.
    The clean() help will be invoked when this script exits
    for whatever reason; the temporary files will be cleaned up
    when the script is interrupted, errors-out, or finishes
    successfully.


I would change like follows:


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





--
Best Regards
Masahiro Yamada

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

[Mark Brown]

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

On Sat, Aug 10, 2019 at 06:11:10PM +0900, Masahiro Yamada wrote:
> On Fri, Aug 9, 2019 at 7:27 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

> I am not a big fan of this way of fixing.

> [1] 'set -e' is useful to catch any error in this script.

Right, that was actually my first thought but since there was a handler
and it was already being called directly this must be a deliberate style
so I did things this way in order to fit in with the existing style :/

> [2] I think trapping only EXIT is smarter.
>     The clean() help will be invoked when this script exits
>     for whatever reason; the temporary files will be cleaned up
>     when the script is interrupted, errors-out, or finishes
>     successfully.

> I would change like follows:

That works for me, 

Reviewed-by: Mark Brown <broonie@kernel.org>
Tested-by: Mark Brown <broonie@kernel.org>

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