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 <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
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.
-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#159794):
https://lists.openembedded.org/g/openembedded-core/message/159794
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]
-=-=-=-=-=-=-=-=-=-=-=-
