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 |
Context | Check | Description |
---|---|---|
ovsrobot/apply-robot | success | apply and check: success |
ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
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.
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.
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
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 --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