[PATCH] Check configuration files

[Miguel A. Flores Silverio]

Duble check generated configuration files in order to avoid
discrepancies like this one:

https://bugzilla.redhat.com/show_bug.cgi?id=1347454

Signed-off-by: Miguel Flores Silverio 
---
* 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
+    if [ -s .mismatches ]
+    then
+	echo "Error: Mismatches found in configuration files"
+	cat .mismatches
+	exit 1
+    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
--
2.7.4

[Paul Bolle]

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?)
> Signed-off-by: Miguel Flores Silverio
> 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.
> +    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

[Laura Abbott]

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
> >
> > 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

[Paul Bolle]

On Thu, 2016-08-25 at 09:16 -0700, Laura Abbott wrote:
> On 08/25/2016 03:19 AM, Paul Bolle wrote:
> 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.

Yes, I read that report.
> 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.

How does this patch catch these problems? The patch description doesn't
tell us that. (Imagine reading the patch description while
bugzilla.redhat.com is unreachable. How much does one then know what
this patch is actually all about?)

Including an example of the output of "cat .mismatches", say for the
issue that this bugzilla link references, might do wonders for people
reading this patch description.
> > 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.

No, we don't. We try to finish the build if that's possible. We might
emit warnings, but we try to keep the build going.

I'm pretty sure these configuration mismatches only happen for rather
obscure corner cases. Serious configuration problems will never hit the
tree in the first place.

Imagine trying to rebuild the kernel package, for whatever reason,
locally and hitting this build error for something entirely irrelevant.
(Like an issue with CONFIG_PWM that is apparently not interesting for
your build machine.) Why on earth should we try to make people unhappy
by breaking their build for them?
> There should be an option to ignore it though if people don't want.
> Miguel, can you add an option similar to listnewconfig_fail?

Thanks,

Paul Bolle

[Laura Abbott]

On 08/25/2016 11:35 AM, Paul Bolle wrote:
> On Thu, 2016-08-25 at 09:16 -0700, Laura Abbott wrote:
> > On 08/25/2016 03:19 AM, Paul Bolle wrote:
> > 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.
>
> Yes, I read that report.
> > 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.
>
> How does this patch catch these problems? The patch description doesn't
> tell us that. (Imagine reading the patch description while
> bugzilla.redhat.com is unreachable. How much does one then know what
> this patch is actually all about?)
>
> Including an example of the output of "cat .mismatches", say for the
> issue that this bugzilla link references, might do wonders for people
> reading this patch description.

That's a good suggestion.
> > > 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.
>
> No, we don't. We try to finish the build if that's possible. We might
> emit warnings, but we try to keep the build going.

We terminate the build for newoptions that aren't specified. I consider
this in the same category.
> I'm pretty sure these configuration mismatches only happen for rather
> obscure corner cases. Serious configuration problems will never hit the
> tree in the first place.

I consider a case where we have an option specified as =y or =m in our
configs and it gets generated as not enabled serious configuration
problems. What do you consider a serious configuration issue?
> Imagine trying to rebuild the kernel package, for whatever reason,
> locally and hitting this build error for something entirely irrelevant.
> (Like an issue with CONFIG_PWM that is apparently not interesting for
> your build machine.) Why on earth should we try to make people unhappy
> by breaking their build for them?

It's not the goal to break a build for anyone. I don't plan on merging
or turning this on until existing issues are fixed up. Ultimately
people who are making their own builds though have always been, well
on their own. I can request people test with their custom configurations
to see if there is anything we are missing. There will be an option
similar to listnewconfig to skip the error out as well. Do you have
other suggestions about minimizing the impact?
> > There should be an option to ignore it though if people don't want.
> > Miguel, can you add an option similar to listnewconfig_fail?
>
> Thanks,
>
> Paul Bolle

Thanks,
Laura

[Paul Bolle]

Hi Laura,

[It seems I just sent a reply to /dev/null. Let's try again.]

On Thu, 2016-08-25 at 12:08 -0700, Laura Abbott wrote:
> On 08/25/2016 11:35 AM, Paul Bolle wrote:
> > No, we don't. We try to finish the build if that's possible. We
> > might
> > emit warnings, but we try to keep the build going.
>
> We terminate the build for newoptions that aren't specified. I
> consider
> this in the same category.

What is a _new_ option? For the upstream build system there are only
valid or invalid options.

Remember that upstream changes kconfig symbols as it sees fit (dropping
symbols, adding symbols, changing symbols from 'y' to 'm', etc).
> > I'm pretty sure these configuration mismatches only happen for
> > rather
> > obscure corner cases. Serious configuration problems will never hit
> > the
> > tree in the first place.
>
> I consider a case where we have an option specified as =y or =m in
> our
> configs and it gets generated as not enabled serious configuration
> problems. What do you consider a serious configuration issue?

The upstream kconfig tools are extremely liberal. You can feed them an
outdated .config file and they'll always emit something. What they'll
emit might not be very usefull, but they'll _never_ error out. (If they
do error out that's a bug.)
> > Imagine trying to rebuild the kernel package, for whatever reason,
> > locally and hitting this build error for something entirely
> > irrelevant.
> > (Like an issue with CONFIG_PWM that is apparently not interesting
> > for
> > your build machine.) Why on earth should we try to make people
> > unhappy
> > by breaking their build for them?
>
> It's not the goal to break a build for anyone. I don't plan on
> merging
> or turning this on until existing issues are fixed up.

The kconfig symbols are not stable and never will be stable, so issues
will pop up again and again.
> Ultimately
> people who are making their own builds though have always been, well
> on their own. I can request people test with their custom
> configurations
> to see if there is anything we are missing. There will be an option
> similar to listnewconfig to skip the error out as well. Do you have
> other suggestions about minimizing the impact?

No. I'll just repeat that configuration issues should be non-fatal.
(Please note that I still don't know what this patch exactly makes
fatal.)

Thanks,

Paul Bolle

[Laura Abbott]

On 08/25/2016 12:53 PM, Paul Bolle wrote:
> Hi Laura,
>
> [It seems I just sent a reply to /dev/null. Let's try again.]
>
> On Thu, 2016-08-25 at 12:08 -0700, Laura Abbott wrote:
> > On 08/25/2016 11:35 AM, Paul Bolle wrote:
> > > No, we don't. We try to finish the build if that's possible. We
> > > might
> > > emit warnings, but we try to keep the build going.
> >
> > We terminate the build for newoptions that aren't specified. I
> > consider
> > this in the same category.
>
> What is a _new_ option? For the upstream build system there are only
> valid or invalid options.
>
> Remember that upstream changes kconfig symbols as it sees fit (dropping
> symbols, adding symbols, changing symbols from 'y' to 'm', etc).

I'm referring to us running oldconfig over all the config files
http://pkgs.fedoraproject.org/cgit/rpms/kernel.git/tree/kernel.spec#n1247
listnewconfig_fail is on by default so it forces us to specify new options.
> > > I'm pretty sure these configuration mismatches only happen for
> > > rather
> > > obscure corner cases. Serious configuration problems will never hit
> > > the
> > > tree in the first place.
> >
> > I consider a case where we have an option specified as =y or =m in
> > our
> > configs and it gets generated as not enabled serious configuration
> > problems. What do you consider a serious configuration issue?
>
> The upstream kconfig tools are extremely liberal. You can feed them an
> outdated .config file and they'll always emit something. What they'll
> emit might not be very usefull, but they'll _never_ error out. (If they
> do error out that's a bug.)

This is true. Fedora is being more restrictive here. It's more useful
for maintenance though and leads to fewer surprises.
> > > Imagine trying to rebuild the kernel package, for whatever reason,
> > > locally and hitting this build error for something entirely
> > > irrelevant.
> > > (Like an issue with CONFIG_PWM that is apparently not interesting
> > > for
> > > your build machine.) Why on earth should we try to make people
> > > unhappy
> > > by breaking their build for them?
> >
> > It's not the goal to break a build for anyone. I don't plan on
> > merging
> > or turning this on until existing issues are fixed up.
>
> The kconfig symbols are not stable and never will be stable, so issues
> will pop up again and again.

Yes, and we will need to keep them in sync. This is part of the work
of kernel maintenance.
> > Ultimately
> > people who are making their own builds though have always been, well
> > on their own. I can request people test with their custom
> > configurations
> > to see if there is anything we are missing. There will be an option
> > similar to listnewconfig to skip the error out as well. Do you have
> > other suggestions about minimizing the impact?
>
> No. I'll just repeat that configuration issues should be non-fatal.
> (Please note that I still don't know what this patch exactly makes
> fatal.)

I think this is where we are disagreeing. For an experienced individual,
accepting all configurations is the right thing to do. Mismatched
configurations have a much bigger impact for maintainers though
because a mismatched configuration is going to go out to many more users.
It's much easier just to reject bad configurations. The same is true
for newer users, if there's no error, it's much harder to go back and
debug a configuration issue versus just rejecting it outright.
> Thanks,
>
> Paul Bolle

Thanks,
Laura

[Paul Bolle]

On Fri, 2016-08-26 at 09:37 -0700, Laura Abbott wrote:
> I think this is where we are disagreeing. For an experienced individual,
> accepting all configurations is the right thing to do. Mismatched
> configurations have a much bigger impact for maintainers though
> because a mismatched configuration is going to go out to many more users.

It's hard for me to evaluate this since I still do not know what
"mismatched configurations" actually are.
> It's much easier just to reject bad configurations. The same is true
> for newer users, if there's no error, it's much harder to go back and
> debug a configuration issue versus just rejecting it outright.

Let's take listnewconfig.

I _think_ I understand why the people taking care of kernel.spec like
to see (what might or might not be relevant in) its output. But for
people buidling locally it's much less interesting. (I've been happy to
ignore it for years. I just rebase on Fedora's latest and greatest
.config every now and then and be done with it.)

To put it another way: it seems this is something that is fatal by
default and is mostly relevant for those maintaining kernel.spec but
less relevant for people building kernel rpms locally. Building the
kernel rpms locally is hard enough already. Do we need to make it a bit
harder by default?

Paul Bolle

[Miguel A. Flores Silverio]

On Thu, Aug 25, 2016 at 12:08 PM, Laura Abbott  wrote:
> On 08/25/2016 11:35 AM, Paul Bolle wrote:
> > On Thu, 2016-08-25 at 09:16 -0700, Laura Abbott wrote:
> > > On 08/25/2016 03:19 AM, Paul Bolle wrote:
> > > 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.
> >
> > Yes, I read that report.
> >
> > 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.
> >
> > How does this patch catch these problems? The patch description doesn't
> > tell us that. (Imagine reading the patch description while
> > bugzilla.redhat.com is unreachable. How much does one then know what
> > this patch is actually all about?)
> >
> > Including an example of the output of "cat .mismatches", say for the
> > issue that this bugzilla link references, might do wonders for people
> > reading this patch description.
>
> That's a good suggestion.

I am guilty of thinking everyone involved in the kernel was aware of this
issue, so I figured a short commit message
was enough. I will provide more details in future patches.

Thanks Paul!
> 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.
> >
> > No, we don't. We try to finish the build if that's possible. We might
> > emit warnings, but we try to keep the build going.
>
> We terminate the build for newoptions that aren't specified. I consider
> this in the same category.
>
> I'm pretty sure these configuration mismatches only happen for rather
> > obscure corner cases. Serious configuration problems will never hit the
> > tree in the first place.
>
> I consider a case where we have an option specified as =y or =m in our
> configs and it gets generated as not enabled serious configuration
> problems. What do you consider a serious configuration issue?
>
> Imagine trying to rebuild the kernel package, for whatever reason,
> > locally and hitting this build error for something entirely irrelevant.
> > (Like an issue with CONFIG_PWM that is apparently not interesting for
> > your build machine.) Why on earth should we try to make people unhappy
> > by breaking their build for them?
>
> It's not the goal to break a build for anyone. I don't plan on merging
> or turning this on until existing issues are fixed up. Ultimately
> people who are making their own builds though have always been, well
> on their own. I can request people test with their custom configurations
> to see if there is anything we are missing. There will be an option
> similar to listnewconfig to skip the error out as well. Do you have
> other suggestions about minimizing the impact?
>
> There should be an option to ignore it though if people don't want.
> > > Miguel, can you add an option similar to listnewconfig_fail?

Yes! It should be as simple as adding another macro similar to
listnewconfig_fail.
> Thanks,
> > Paul Bolle
>
> Thanks,
> Laura

[Paul Bolle]

On Sun, 2016-08-28 at 23:07 -0700, Miguel A. Flores Silverio wrote:
> On Thu, Aug 25, 2016 at 12:08 PM, Laura Abbott 
> wrote:
> > On 08/25/2016 11:35 AM, Paul Bolle wrote:
> > > On Thu, 2016-08-25 at 09:16 -0700, Laura Abbott wrote:
> > > > There should be an option to ignore it though if people don't
> > > > want.
> > > > Miguel, can you add an option similar to listnewconfig_fail?
>
> Yes! It should be as simple as adding another macro similar to
> listnewconfig_fail.

This made me look into listnewconfig_fail. (I build with that variable
set to zero - because I build vanilla kernel rpms so nopathces is set.)

It turns out we run "make listnewconfig" for no obvious reason if
listnewconfg_fail is set to zero. Ie, the section where "make
listnewconfig" is called should actually read:
    %if %{listnewconfig_fail}
      make ARCH=$Arch listnewconfig | grep -E '^CONFIG_' >.newoptions || true
      if [ -s .newoptions ]; then
        cat .newoptions
        exit 1
      fi
      rm -f .newoptions
    %endif

Perhaps, Miguel, you can look into that too while you're at it.

Thanks,

Paul Bolle