[meta-oe,RFC,v2] insane: Inappropriate patch reasoning

if a patch uses Upstream-Status: Inappropriate it should provide a machine
readable reasoning in square brackets.

According to latest wiki entry that would be

not author
native
licensing
configuration
enable feature
disable feature
bugfix .*
embedded specific
no upstream
other

a detailed reasoning could be provided as part of the commit message,
but format of the metadata line is fixed.

This patch adds a check to insane.bbclass and warns if there is a
non-compliant reasoning given, or none at all.

In a follow-up this should be turned into an error, as it was done
with missing Upstream-Status

Can be skipped with newly added patch-metadata key via INSANE_SKIP

Signed-off-by: Konrad Weihmann <kweihmann@outlook.com>
---
v2: add possibility to skip with patch-metadata in INSANE_SKIP

 meta/classes/insane.bbclass | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

On Thu, 2021-12-16 at 13:26 +0100, Konrad Weihmann wrote:
> if a patch uses Upstream-Status: Inappropriate it should provide a machine
> readable reasoning in square brackets.
> 
> According to latest wiki entry that would be
> 
> not author
> native
> licensing
> configuration
> enable feature
> disable feature
> bugfix .*
> embedded specific
> no upstream
> other
> 
> a detailed reasoning could be provided as part of the commit message,
> but format of the metadata line is fixed.
> 
> This patch adds a check to insane.bbclass and warns if there is a
> non-compliant reasoning given, or none at all.
> 
> In a follow-up this should be turned into an error, as it was done
> with missing Upstream-Status
> 
> Can be skipped with newly added patch-metadata key via INSANE_SKIP
> 
> Signed-off-by: Konrad Weihmann <kweihmann@outlook.com>
> ---
> v2: add possibility to skip with patch-metadata in INSANE_SKIP
> 
>  meta/classes/insane.bbclass | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/meta/classes/insane.bbclass b/meta/classes/insane.bbclass
> index 240f3aad62..eae8e0e549 100644
> --- a/meta/classes/insane.bbclass
> +++ b/meta/classes/insane.bbclass
> @@ -1124,6 +1124,8 @@ python do_qa_staging() {
>  python do_qa_patch() {
>      import subprocess
>  
> +    skip = (d.getVar('INSANE_SKIP') or "").split()
> +
>      ###########################################################################
>      # Check patch.log for fuzz warnings
>      #
> @@ -1191,6 +1193,29 @@ python do_qa_patch() {
>                 bb.error("Malformed Upstream-Status in patch\n%s\nPlease correct according to %s :\n%s" % (fullpath, guidelines, match_kinda.group(0)))
>             else:
>                 bb.error("Missing Upstream-Status in patch\n%s\nPlease add according to %s ." % (fullpath, guidelines))
> +       
> +       if 'patch-metadata' in skip:
> +           continue
> +       
> +       inappr_message_re = r'Inappropriate(\s+\[(?P<reason>.*)\])*'
> +       inappr_reasons = [
> +            'not author',
> +            'native',
> +            'licensing',
> +            'configuration',
> +            'enable feature',
> +            'disable feature',
> +            'bugfix .*',
> +            'embedded specific',
> +            'no upstream',
> +            'other',
> +       ]
> +       for match_inappr in re.finditer(inappr_message_re, content, re.IGNORECASE | re.MULTILINE):
> +
> +           if 'reason' not in match_inappr.groupdict():
> +               bb.warning("Missing Upstream-Status: Inappropriate reasoning in patch\n%s\nPlease add according to %s ." % (fullpath, guidelines))
> +           elif not any(re.match(x, match_inappr.groupdict().get('reason', '') or '') for x in inappr_reasons):
> +               bb.warning("Malformed Upstream-Status: Inappropriate in patch\n%s\nPlease correct according to %s :\n%s" % (fullpath, guidelines, match_inappr.group(0)))
>  }
>  
>  python do_qa_configure() {

I appreciate the intent here but I think there are details we need to look at
first.

Whilst this list of inappropriate reasons looks like a good start, I'm not sure
it is exactly the list of things we want to encourage people to mark patches
with. The wiki was written a long time ago and I think we want to make sure it
is right before we go through the work of classifying a large number of patches.
I'm very much in favour of an approach which looks at actions we could take
based upon a given classification.

I'm also worried we actually lose information with any "forced" transition like
this. There may be links to discussion in the current [] field and I'd much
rather keep those links that force people into changing them and them being
lost. Common sense says people would move them to the patch description but that
isn't any guarantee that it would happen.

Also, this isn't really how we go about making transitions like this. Usually
we'd make some decision about a direction and then we'd migrate to it over time.
This patch will start generating hundreds of warnings on the autobuilder to the
point that all warnings would become meaningless. Currently it operates mostly
warning free and where we do hit them, usually intermittently, we get them
fixed. Once things start to fail, they "rot" quickly as one warning turns into
many more unnoticed.

In some ways I'd prefer we add a new field, something other than Inappropriate
and then over time we could classify patches. We'd use the approach we're using
with Pending where we gradually encourage movement over time.

Cheers,

Richard

On 16.12.21 15:54, Richard Purdie wrote:
> On Thu, 2021-12-16 at 13:26 +0100, Konrad Weihmann wrote:
>> if a patch uses Upstream-Status: Inappropriate it should provide a machine
>> readable reasoning in square brackets.
>>
>> According to latest wiki entry that would be
>>
>> not author
>> native
>> licensing
>> configuration
>> enable feature
>> disable feature
>> bugfix .*
>> embedded specific
>> no upstream
>> other
>>
>> a detailed reasoning could be provided as part of the commit message,
>> but format of the metadata line is fixed.
>>
>> This patch adds a check to insane.bbclass and warns if there is a
>> non-compliant reasoning given, or none at all.
>>
>> In a follow-up this should be turned into an error, as it was done
>> with missing Upstream-Status
>>
>> Can be skipped with newly added patch-metadata key via INSANE_SKIP
>>
>> Signed-off-by: Konrad Weihmann <kweihmann@outlook.com>
>> ---
>> v2: add possibility to skip with patch-metadata in INSANE_SKIP
>>
>>   meta/classes/insane.bbclass | 25 +++++++++++++++++++++++++
>>   1 file changed, 25 insertions(+)
>>
>> diff --git a/meta/classes/insane.bbclass b/meta/classes/insane.bbclass
>> index 240f3aad62..eae8e0e549 100644
>> --- a/meta/classes/insane.bbclass
>> +++ b/meta/classes/insane.bbclass
>> @@ -1124,6 +1124,8 @@ python do_qa_staging() {
>>   python do_qa_patch() {
>>       import subprocess
>>   
>> +    skip = (d.getVar('INSANE_SKIP') or "").split()
>> +
>>       ###########################################################################
>>       # Check patch.log for fuzz warnings
>>       #
>> @@ -1191,6 +1193,29 @@ python do_qa_patch() {
>>                  bb.error("Malformed Upstream-Status in patch\n%s\nPlease correct according to %s :\n%s" % (fullpath, guidelines, match_kinda.group(0)))
>>              else:
>>                  bb.error("Missing Upstream-Status in patch\n%s\nPlease add according to %s ." % (fullpath, guidelines))
>> +
>> +       if 'patch-metadata' in skip:
>> +           continue
>> +
>> +       inappr_message_re = r'Inappropriate(\s+\[(?P<reason>.*)\])*'
>> +       inappr_reasons = [
>> +            'not author',
>> +            'native',
>> +            'licensing',
>> +            'configuration',
>> +            'enable feature',
>> +            'disable feature',
>> +            'bugfix .*',
>> +            'embedded specific',
>> +            'no upstream',
>> +            'other',
>> +       ]
>> +       for match_inappr in re.finditer(inappr_message_re, content, re.IGNORECASE | re.MULTILINE):
>> +
>> +           if 'reason' not in match_inappr.groupdict():
>> +               bb.warning("Missing Upstream-Status: Inappropriate reasoning in patch\n%s\nPlease add according to %s ." % (fullpath, guidelines))
>> +           elif not any(re.match(x, match_inappr.groupdict().get('reason', '') or '') for x in inappr_reasons):
>> +               bb.warning("Malformed Upstream-Status: Inappropriate in patch\n%s\nPlease correct according to %s :\n%s" % (fullpath, guidelines, match_inappr.group(0)))
>>   }
>>   
>>   python do_qa_configure() {
> 
> I appreciate the intent here but I think there are details we need to look at
> first.
> 
> Whilst this list of inappropriate reasons looks like a good start, I'm not sure
> it is exactly the list of things we want to encourage people to mark patches
> with. The wiki was written a long time ago and I think we want to make sure it
> is right before we go through the work of classifying a large number of patches.
> I'm very much in favour of an approach which looks at actions we could take
> based upon a given classification.
> 
> I'm also worried we actually lose information with any "forced" transition like
> this. There may be links to discussion in the current [] field and I'd much
> rather keep those links that force people into changing them and them being
> lost. Common sense says people would move them to the patch description but that
> isn't any guarantee that it would happen.
> 
> Also, this isn't really how we go about making transitions like this. Usually
> we'd make some decision about a direction and then we'd migrate to it over time.
> This patch will start generating hundreds of warnings on the autobuilder to the
> point that all warnings would become meaningless. Currently it operates mostly
> warning free and where we do hit them, usually intermittently, we get them
> fixed. Once things start to fail, they "rot" quickly as one warning turns into
> many more unnoticed.
> 
> In some ways I'd prefer we add a new field, something other than Inappropriate
> and then over time we could classify patches. We'd use the approach we're using
> with Pending where we gradually encourage movement over time.
> 
> Cheers,
> 
> Richard
> 
> 

Fine - I'm also in favor of a transition phase - I just wanted to 
highlight that Inappropriate lost almost all of its meaning as the grep 
output from an earlier mail showed.
One thing I could think about is to at least flag the patch that doesn't 
even provide a reasoning.

At this point we could do the very same for core as for the 
Upstream-Status and do a hard error when not even a one is given.

Then in a second round fix all the misspellings - and maybe rethink the 
way the classification is done -- any I'm fully with you -- find a way 
to enforce a textual justification besides the classification part.

BTW one thing that for instance bothers me is the wide use of 
"oe-specific" even though it was never documented.


Concluding from all that I'm rather not keen on inventing more and more 
metadata IDs - but would like to see movement into a direction of having 
better patch checks with better classification.

> 
>