Re: [PATCH] Check configuration files
On 08/25/2016 03:19 AM, Paul Bolle wrote:
On Fri, 2016-08-19 at 13:24 -0700, Miguel Flores Silverio wrote:
> Duble check generated configuration files in order to avoid
> discrepancies like this one:
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1347454
What does this actually mean? Discrepancies (and mismatches, which you
use in the patch itself) are both rather generic terms.
(The main issue with having CONFIG_PWM_LPSS set to 'm' in one of the
.config fragments is that it is actually unneeded. See, the kconfig
tools will set it when other kconfig options select it. So why bother
including it in the .config fragments?
Is that the issue this patch addresses?)
The issue is that CONFIG_PWM_LPSS was set =m but it wasn't actually
set at all. See https://bugzilla.redhat.com/show_bug.cgi?id=1335196
as well. We're setting these options so we think they are enabled
but in reality they aren't. This is confusing for both us and
the users. The goal here is to catch these problems at build time
so they can be corrected sooner.
> Signed-off-by: Miguel Flores Silverio
<floresmigu3l(a)gmail.com>
> ---
> * PATCHv1
> - Check config files at compilation time
>
> * PATCHv2
> - Check config files before and after oldconfig is aplied
> - Save log of mismatches in logs directory in tree
>
> * PATCHv3
> - save mismatches in .mismatches and exit compilation if
> there any.
> - Display error message and config options in .mismatches
> - Fixed bug in awk statment
> - Only save mismatches of enabled config options
> - Delete temporary files outside function to avoid confusion
>
> kernel.spec | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/kernel.spec b/kernel.spec
> index 8d3587d..431a17c 100644
> --- a/kernel.spec
> +++ b/kernel.spec
> @@ -1231,9 +1231,21 @@ rm -f kernel-%{version}-*debug.config
>
> %define make make %{?cross_opts}
>
> +# Check configuration files for any discrepancies
> +CheckConfigs(){
> + awk 'NR==FNR{configs[$0]++;next;}
> + NR!=FNR && !($0 in configs)' $1 $2 | egrep '^CONFIG_' >
.mismatches
This awk one-liner looks like line noise to me. The comment
above CheckConfigs() doesn't tell me anything that the name of this
function already does.
> + if [ -s .mismatches ]
> + then
> + echo "Error: Mismatches found in configuration files"
Errors should go to stderr, please.
> + cat .mismatches
> + exit 1
This terminates the build, doesn't it? But we should only terminate the
build when we're unsure how to continue. Because otherwise we might
break people's build for configuration issues they personally couldn't
care less about. For instance, I'm running an x86 laptop with
CONFIG_PWM not set ever since I run Fedora 24. Why should I care? I
have no idea.
We terminate the build because something is misconfigured. If an option
is set =m in the config fragments but not in the generated config someone
has specified something wrong.
There should be an option to ignore it though if people don't want.
Miguel, can you add an option similar to listnewconfig_fail?
> + fi
> +}
> # now run oldconfig over all the config files
> for i in *.config
> do
> + cat $i > temp-$i
> mv $i .config
> Arch=`head -1 .config | cut -b 3-`
> make ARCH=$Arch listnewconfig | grep -E '^CONFIG_' >.newoptions ||
> true
> @@ -1247,6 +1259,8 @@ do
> make ARCH=$Arch oldnoconfig
> echo "# $Arch" > configs/$i
> cat .config >> configs/$i
> + CheckConfigs configs/$i temp-$i
> + rm temp-$i
> done
> # end of kernel config
> %endif
Thanks,
Paul Bolle