diff mbox series

[ovs-dev,v5] checkpatch: Add new check-authors-file option to checkpatch.py.

Message ID 2ee996363955a1df77923723e029a03b3046e247.1727870503.git.echaudro@redhat.com
State Accepted
Commit a6ccd111552dad29a26d029aaa2af07848526621
Delegated to: aaron conole
Headers show
Series [ovs-dev,v5] checkpatch: Add new check-authors-file option to checkpatch.py. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed

Commit Message

Eelco Chaudron Oct. 2, 2024, 12:01 p.m. UTC
This patch adds a new option, --check-authors-file, to the checkpatch
tool to help OVS maintainers check for missing authors in the
AUTHORS.rst file.

Co-authored-by: Aaron Conole <aconole@redhat.com>
Signed-off-by: Aaron Conole <aconole@redhat.com>
Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
---
v3: - Also include co-authors in the check.
    - Only report the end, when all patches are checked.
    - Fixed spelling mistake.
    - Determine git root directory for AUTHORS.rst location.
v4: - Added unit test.
v5: - Incorporated Aaron's addition to check for updated
      AUTHORS.rst in the patches.
---
 tests/checkpatch.at     | 21 ++++++++++
 utilities/checkpatch.py | 87 +++++++++++++++++++++++++++++++++++++++--
 2 files changed, 105 insertions(+), 3 deletions(-)

Comments

Aaron Conole Oct. 9, 2024, 8:27 p.m. UTC | #1
Eelco Chaudron <echaudro@redhat.com> writes:

> This patch adds a new option, --check-authors-file, to the checkpatch
> tool to help OVS maintainers check for missing authors in the
> AUTHORS.rst file.
>
> Co-authored-by: Aaron Conole <aconole@redhat.com>
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
> ---

Thanks Eelco - applied.
Ilya Maximets Oct. 10, 2024, 10:33 a.m. UTC | #2
On 10/9/24 22:27, Aaron Conole wrote:
> Eelco Chaudron <echaudro@redhat.com> writes:
> 
>> This patch adds a new option, --check-authors-file, to the checkpatch
>> tool to help OVS maintainers check for missing authors in the
>> AUTHORS.rst file.
>>
>> Co-authored-by: Aaron Conole <aconole@redhat.com>
>> Signed-off-by: Aaron Conole <aconole@redhat.com>
>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
>> ---
> 
> Thanks Eelco - applied.

Hi, Aaron and Eelco.

This change broke CirrusCI:
  https://cirrus-ci.com/task/4643991453433856?logs=check#L3142

24. checkpatch.at:638: testing checkpatch - AUTHORS.rst existence ...
./checkpatch.at:36: $PYTHON3 $top_srcdir/utilities/checkpatch.py $3 -q test.patch
./checkpatch.at:32: $PYTHON3 $top_srcdir/utilities/checkpatch.py $3 -q test.patch
--- /dev/null	2024-10-09 20:10:11.586373000 +0000
+++ /tmp/cirrus-ci-build/tests/testsuite.dir/at-groups/24/stderr	2024-10-09 20:10:11.585078000 +0000
@@ -0,0 +1,2 @@
+/bin/sh: git: not found
+/bin/sh: git: not found
stdout:
ERROR: Could not open AUTHORS.rst in './'!
Lines checked: 4, Warnings: 0, Errors: 1
ovsdb-server.log:
sed: ovsdb-server.log: No such file or directory
ovs-vswitchd.log:
sed: ovs-vswitchd.log: No such file or directory
24. checkpatch.at:638: 24. checkpatch - AUTHORS.rst existence (checkpatch.at:638): FAILED (checkpatch.at:32)


Could you please check?
In general, we should not require git to run unit tests.

Best regards, Ilya Maximets.
Eelco Chaudron Oct. 10, 2024, 10:54 a.m. UTC | #3
On 10 Oct 2024, at 12:33, Ilya Maximets wrote:

> On 10/9/24 22:27, Aaron Conole wrote:
>> Eelco Chaudron <echaudro@redhat.com> writes:
>>
>>> This patch adds a new option, --check-authors-file, to the checkpatch
>>> tool to help OVS maintainers check for missing authors in the
>>> AUTHORS.rst file.
>>>
>>> Co-authored-by: Aaron Conole <aconole@redhat.com>
>>> Signed-off-by: Aaron Conole <aconole@redhat.com>
>>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
>>> ---
>>
>> Thanks Eelco - applied.
>
> Hi, Aaron and Eelco.
>
> This change broke CirrusCI:
>  https://cirrus-ci.com/task/4643991453433856?logs=check#L3142
>
> 24. checkpatch.at:638: testing checkpatch - AUTHORS.rst existence ...
> ./checkpatch.at:36: $PYTHON3 $top_srcdir/utilities/checkpatch.py $3 -q test.patch
> ./checkpatch.at:32: $PYTHON3 $top_srcdir/utilities/checkpatch.py $3 -q test.patch
> --- /dev/null	2024-10-09 20:10:11.586373000 +0000
> +++ /tmp/cirrus-ci-build/tests/testsuite.dir/at-groups/24/stderr	2024-10-09 20:10:11.585078000 +0000
> @@ -0,0 +1,2 @@
> +/bin/sh: git: not found
> +/bin/sh: git: not found
> stdout:
> ERROR: Could not open AUTHORS.rst in './'!
> Lines checked: 4, Warnings: 0, Errors: 1
> ovsdb-server.log:
> sed: ovsdb-server.log: No such file or directory
> ovs-vswitchd.log:
> sed: ovs-vswitchd.log: No such file or directory
> 24. checkpatch.at:638: 24. checkpatch - AUTHORS.rst existence (checkpatch.at:638): FAILED (checkpatch.at:32)
>
>
> Could you please check?
> In general, we should not require git to run unit tests.

Will take a look. Thought I ran it through CirrusCI :(

//Eelco
Eelco Chaudron Oct. 10, 2024, 1:41 p.m. UTC | #4
On 10 Oct 2024, at 12:54, Eelco Chaudron wrote:

> On 10 Oct 2024, at 12:33, Ilya Maximets wrote:
>
>> On 10/9/24 22:27, Aaron Conole wrote:
>>> Eelco Chaudron <echaudro@redhat.com> writes:
>>>
>>>> This patch adds a new option, --check-authors-file, to the checkpatch
>>>> tool to help OVS maintainers check for missing authors in the
>>>> AUTHORS.rst file.
>>>>
>>>> Co-authored-by: Aaron Conole <aconole@redhat.com>
>>>> Signed-off-by: Aaron Conole <aconole@redhat.com>
>>>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
>>>> ---
>>>
>>> Thanks Eelco - applied.
>>
>> Hi, Aaron and Eelco.
>>
>> This change broke CirrusCI:
>> https://cirrus-ci.com/task/4643991453433856?logs=check#L3142
>>
>> 24. checkpatch.at:638: testing checkpatch - AUTHORS.rst existence ...
>> ./checkpatch.at:36: $PYTHON3 $top_srcdir/utilities/checkpatch.py $3 -q test.patch
>> ./checkpatch.at:32: $PYTHON3 $top_srcdir/utilities/checkpatch.py $3 -q test.patch
>> --- /dev/null	2024-10-09 20:10:11.586373000 +0000
>> +++ /tmp/cirrus-ci-build/tests/testsuite.dir/at-groups/24/stderr	2024-10-09 20:10:11.585078000 +0000
>> @@ -0,0 +1,2 @@
>> +/bin/sh: git: not found
>> +/bin/sh: git: not found
>> stdout:
>> ERROR: Could not open AUTHORS.rst in './'!
>> Lines checked: 4, Warnings: 0, Errors: 1
>> ovsdb-server.log:
>> sed: ovsdb-server.log: No such file or directory
>> ovs-vswitchd.log:
>> sed: ovs-vswitchd.log: No such file or directory
>> 24. checkpatch.at:638: 24. checkpatch - AUTHORS.rst existence (checkpatch.at:638): FAILED (checkpatch.at:32)
>>
>>
>> Could you please check?
>> In general, we should not require git to run unit tests.
>
> Will take a look. Thought I ran it through CirrusCI :(

Sent out a new patch, please take a look.

//Eelco
diff mbox series

Patch

diff --git a/tests/checkpatch.at b/tests/checkpatch.at
index 34971c514..fa179c707 100755
--- a/tests/checkpatch.at
+++ b/tests/checkpatch.at
@@ -634,3 +634,24 @@  try_checkpatch \
     "--skip-committer-signoff"
 
 AT_CLEANUP
+
+AT_SETUP([checkpatch - AUTHORS.rst existence])
+
+try_checkpatch \
+   "Author: A <a@a.com>
+    Commit: A <a@a.com>
+    Subject: netdev: Subject.
+
+    Signed-off-by: A <a@a.com>" \
+    ""
+
+try_checkpatch \
+   "Author: A <a@a.com>
+    Commit: A <a@a.com>
+    Subject: netdev: Subject.
+
+    Signed-off-by: A <a@a.com>" \
+    "WARNING: Author 'A <a@a.com>' is not in the AUTHORS.rst file!" \
+    "-a"
+
+AT_CLEANUP
diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index 742a0bc47..53b13bcf2 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -28,12 +28,14 @@  __errors = 0
 __warnings = 0
 empty_return_check_state = 0
 print_file_name = None
+check_authors_file = False
 checking_file = False
 total_line = 0
 colors = False
 spellcheck = False
 quiet = False
 spell_check_dict = None
+missing_authors = []
 
 
 def open_spell_check_dict():
@@ -666,6 +668,10 @@  checks = [
      'check': lambda x: has_efgrep(x),
      'print':
      lambda: print_error("grep -E/-F should be used instead of egrep/fgrep")},
+
+    {'regex': 'AUTHORS.rst$', 'match_name': None,
+     'check': lambda x: update_missing_authors(x),
+     'print': None},
 ]
 
 
@@ -860,9 +866,56 @@  def run_subject_checks(subject, spellcheck=False):
     return warnings
 
 
+def get_top_directory():
+    with os.popen('git rev-parse --show-toplevel') as pipe:
+        path = pipe.read()
+
+    if path:
+        return path.strip()
+
+    return "."
+
+
+def update_missing_authors(diffed_line):
+    global missing_authors
+    for author in missing_authors:
+        m = re.search(r'<(.*?)>', author)
+        if not m:
+            continue
+        pattern = r'\b' + re.escape(m.group(1)) + r'\b'
+        if re.search(pattern, diffed_line) is None:
+            continue
+        else:
+            missing_authors.remove(author)
+
+    return False
+
+
+def do_authors_exist(authors):
+    authors = list(set(authors))
+    missing_authors = []
+
+    try:
+        with open(get_top_directory() + "/AUTHORS.rst", "r") as file:
+            file_content = file.read()
+            for author in authors:
+                m = re.search(r'<(.*?)>', author)
+                if not m:
+                    continue
+                pattern = r'\b' + re.escape(m.group(1)) + r'\b'
+                if re.search(pattern, file_content) is None:
+                    missing_authors.append(author)
+
+    except FileNotFoundError:
+        print_error("Could not open AUTHORS.rst in '%s/'!" %
+                    get_top_directory())
+
+    return missing_authors
+
+
 def ovs_checkpatch_parse(text, filename, author=None, committer=None):
     global print_file_name, total_line, checking_file, \
-        empty_return_check_state
+        empty_return_check_state, missing_authors
 
     PARSE_STATE_HEADING = 0
     PARSE_STATE_DIFF_HEADER = 1
@@ -977,6 +1030,11 @@  def ovs_checkpatch_parse(text, filename, author=None, committer=None):
                                       "who are not authors or co-authors or "
                                       "committers: %s"
                                       % ", ".join(extra_sigs))
+
+                if check_authors_file:
+                    missing_authors = do_authors_exist(missing_authors +
+                                                       co_authors + [author])
+
             elif is_committer.match(line):
                 committer = is_committer.match(line).group(2)
             elif is_author.match(line):
@@ -1067,6 +1125,8 @@  Input options:
 
 Check options:
 -h|--help                      This help message
+-a|--check-authors-file        Check AUTHORS file for existence of the authors.
+                               Should be used by commiters only!
 -b|--skip-block-whitespace     Skips the if/while/for whitespace tests
 -l|--skip-leading-whitespace   Skips the leading whitespace test
 -q|--quiet                     Only print error and warning information
@@ -1089,6 +1149,19 @@  def ovs_checkpatch_print_result():
         print("Lines checked: %d, no obvious problems found\n" % (total_line))
 
 
+def ovs_checkpatch_print_missing_authors():
+    if missing_authors:
+        if len(missing_authors) == 1:
+            print_warning("Author '%s' is not in the AUTHORS.rst file!"
+                          % missing_authors[0])
+        else:
+            print_warning("Authors '%s' are not in the AUTHORS.rst file!"
+                          % ', '.join(missing_authors))
+        return True
+
+    return False
+
+
 def ovs_checkpatch_file(filename):
     try:
         mail = email.message_from_file(open(filename, 'r', encoding='utf8'))
@@ -1116,6 +1189,8 @@  def ovs_checkpatch_file(filename):
         result = True
 
     ovs_checkpatch_print_result()
+    if ovs_checkpatch_print_missing_authors():
+        result = True
     return result
 
 
@@ -1138,9 +1213,10 @@  if __name__ == '__main__':
                                           sys.argv[1:])
         n_patches = int(numeric_options[-1][1:]) if numeric_options else 0
 
-        optlist, args = getopt.getopt(args, 'bhlstfSq',
+        optlist, args = getopt.getopt(args, 'abhlstfSq',
                                       ["check-file",
                                        "help",
+                                       "check-authors-file",
                                        "skip-block-whitespace",
                                        "skip-leading-whitespace",
                                        "skip-signoff-lines",
@@ -1157,6 +1233,8 @@  if __name__ == '__main__':
         if o in ("-h", "--help"):
             usage()
             sys.exit(0)
+        elif o in ("-a", "--check-authors-file"):
+            check_authors_file = True
         elif o in ("-b", "--skip-block-whitespace"):
             skip_block_whitespace_check = True
         elif o in ("-l", "--skip-leading-whitespace"):
@@ -1207,9 +1285,10 @@  Subject: %s
             if not quiet:
                 print('== Checking %s ("%s") ==' % (revision[0:12], name))
             result = ovs_checkpatch_parse(patch, revision)
-            ovs_checkpatch_print_result()
             if result:
                 status = EXIT_FAILURE
+        if ovs_checkpatch_print_missing_authors():
+            status = EXIT_FAILURE
         sys.exit(status)
 
     if not args:
@@ -1218,6 +1297,8 @@  Subject: %s
             sys.exit(EXIT_FAILURE)
         result = ovs_checkpatch_parse(sys.stdin.read(), '-')
         ovs_checkpatch_print_result()
+        if ovs_checkpatch_print_missing_authors():
+            result = EXIT_FAILURE
         sys.exit(result)
 
     status = 0