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 <kweihm...@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
-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#159791):
https://lists.openembedded.org/g/openembedded-core/message/159791
Mute This Topic: https://lists.openembedded.org/mt/87765780/21656
Group Owner: openembedded-core+ow...@lists.openembedded.org
Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
