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

From: Guillaume Tucker
Date: Mon Sep 02 2019 - 10:19:33 EST


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@xxxxxxxxxxxxx>
>> Reported-by: Guillaume Tucker <guillaume.tucker@xxxxxxxxxxxxx>
>> Signed-off-by: Mark Brown <broonie@xxxxxxxxxx>
>> ---
>
> 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