diff options
| -rw-r--r-- | MAINTAINERS | 1 | ||||
| -rw-r--r-- | Makefile | 2 | ||||
| -rwxr-xr-x | extras/scripts/crcchecker.py | 218 | ||||
| -rwxr-xr-x | extras/scripts/tests/test_crcchecker.sh | 46 | ||||
| -rw-r--r-- | src/tools/vppapigen/vppapigen_crc.py | 3 | ||||
| -rw-r--r-- | src/vat2/test/vat2_test.api | 1 |
6 files changed, 177 insertions, 94 deletions
diff --git a/MAINTAINERS b/MAINTAINERS index c2fa416ca64..6dd2532d4fe 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -580,6 +580,7 @@ Binary API Compiler for Python I: vppapigen M: Ole Troan <otroan@employees.org> F: src/tools/vppapigen/ +F: extras/scripts/crcchecker.py API trace tool I: vppapitrace @@ -631,7 +631,7 @@ fixstyle: .PHONY: checkstyle-api checkstyle-api: - @extras/scripts/crcchecker.py --check-patch + @extras/scripts/crcchecker.py --check-patchset # necessary because Bug 1696324 - Update to python3.6 breaks PyYAML dependencies # Status: CLOSED CANTFIX diff --git a/extras/scripts/crcchecker.py b/extras/scripts/crcchecker.py index 2b026338129..5060d9f2e0e 100755 --- a/extras/scripts/crcchecker.py +++ b/extras/scripts/crcchecker.py @@ -1,134 +1,176 @@ #!/usr/bin/env python3 +''' +crcchecker is a tool to used to enforce that .api messages do not change. +API files with a semantic version < 1.0.0 are ignored. +''' + import sys import os import json import argparse +import re from subprocess import run, PIPE, check_output, CalledProcessError -rootdir = os.path.dirname(os.path.realpath(__file__)) + '/../..' +# pylint: disable=subprocess-run-check + +ROOTDIR = os.path.dirname(os.path.realpath(__file__)) + '/../..' +APIGENBIN = f'{ROOTDIR}/src/tools/vppapigen/vppapigen.py' + def crc_from_apigen(revision, filename): + '''Runs vppapigen with crc plugin returning a JSON object with CRCs for + all APIs in filename''' if not revision and not os.path.isfile(filename): print(f'skipping: {filename}', file=sys.stderr) return {} - apigen_bin = f'{rootdir}/src/tools/vppapigen/vppapigen.py' + if revision: - apigen = (f'{apigen_bin} --git-revision {revision} --includedir src ' + apigen = (f'{APIGENBIN} --git-revision {revision} --includedir src ' f'--input {filename} CRC') else: - apigen = (f'{apigen_bin} --includedir src --input {filename} CRC') - rv = run(apigen.split(), stdout=PIPE, stderr=PIPE) - if rv.returncode == 2: # No such file - print(f'skipping: {revision}:{filename} {rv}', file=sys.stderr) + apigen = (f'{APIGENBIN} --includedir src --input {filename} CRC') + returncode = run(apigen.split(), stdout=PIPE, stderr=PIPE) + if returncode.returncode == 2: # No such file + print(f'skipping: {revision}:{filename} {returncode}', file=sys.stderr) return {} - if rv.returncode != 0: - print(f'vppapigen failed for {revision}:{filename} with command\n {apigen}\n error: {rv}', - rv.stderr.decode('ascii'), file=sys.stderr) + if returncode.returncode != 0: + print(f'vppapigen failed for {revision}:{filename} with ' + 'command\n {apigen}\n error: {rv}', + returncode.stderr.decode('ascii'), file=sys.stderr) sys.exit(-2) - return json.loads(rv.stdout) + return json.loads(returncode.stdout) -def dict_compare(d1, d2): - d1_keys = set(d1.keys()) - d2_keys = set(d2.keys()) +def dict_compare(dict1, dict2): + '''Compare two dictionaries returning added, removed, modified + and equal entries''' + d1_keys = set(dict1.keys()) + d2_keys = set(dict2.keys()) intersect_keys = d1_keys.intersection(d2_keys) added = d1_keys - d2_keys removed = d2_keys - d1_keys - modified = {o: (d1[o], d2[o]) for o in intersect_keys if d1[o]['crc'] != d2[o]['crc']} - same = set(o for o in intersect_keys if d1[o] == d2[o]) + modified = {o: (dict1[o], dict2[o]) for o in intersect_keys + if dict1[o]['crc'] != dict2[o]['crc']} + same = set(o for o in intersect_keys if dict1[o] == dict2[o]) return added, removed, modified, same def filelist_from_git_ls(): + '''Returns a list of all api files in the git repository''' filelist = [] git_ls = 'git ls-files *.api' - rv = run(git_ls.split(), stdout=PIPE, stderr=PIPE) - if rv.returncode != 0: - sys.exit(rv.returncode) + returncode = run(git_ls.split(), stdout=PIPE, stderr=PIPE) + if returncode.returncode != 0: + sys.exit(returncode.returncode) - for l in rv.stdout.decode('ascii').split('\n'): - if len(l): - filelist.append(l) + for line in returncode.stdout.decode('ascii').split('\n'): + if line: + filelist.append(line) return filelist def is_uncommitted_changes(): + '''Returns true if there are uncommitted changes in the repo''' git_status = 'git status --porcelain -uno' - rv = run(git_status.split(), stdout=PIPE, stderr=PIPE) - if rv.returncode != 0: - sys.exit(rv.returncode) + returncode = run(git_status.split(), stdout=PIPE, stderr=PIPE) + if returncode.returncode != 0: + sys.exit(returncode.returncode) - if len(rv.stdout): + if returncode.stdout: return True return False def filelist_from_git_grep(filename): + '''Returns a list of api files that this <filename> api files imports.''' filelist = [] try: - rv = check_output(f'git grep -e "import .*{filename}" -- *.api', shell=True) - except CalledProcessError as err: + returncode = check_output(f'git grep -e "import .*{filename}"' + ' -- *.api', + shell=True) + except CalledProcessError: return [] - print('RV', err.returncode) - for l in rv.decode('ascii').split('\n'): - if l: - f, p = l.split(':') - filelist.append(f) + for line in returncode.decode('ascii').split('\n'): + if line: + filename, _ = line.split(':') + filelist.append(filename) return filelist -def filelist_from_patchset(): +def filelist_from_patchset(pattern): + '''Returns list of api files in changeset and the list of api + files they import.''' filelist = [] - git_cmd = '((git diff HEAD~1.. --name-only;git ls-files -m) | sort -u)' - rv = check_output(git_cmd, shell=True) - for l in rv.decode('ascii').split('\n'): - if len(l) and os.path.splitext(l)[1] == '.api': - filelist.append(l) + git_cmd = ('((git diff HEAD~1.. --name-only;git ls-files -m) | ' + 'sort -u | grep "\\.api$")') + returncode = check_output(git_cmd, shell=True) # Check for dependencies (imports) imported_files = [] - for f in filelist: - imported_files.extend(filelist_from_git_grep(os.path.basename(f))) + for line in returncode.decode('ascii').split('\n'): + if not line: + continue + if not re.search(pattern, line): + continue + filelist.append(line) + imported_files.extend(filelist_from_git_grep(os.path.basename(line))) filelist.extend(imported_files) return set(filelist) -def is_deprecated(d, k): - if 'options' in d[k]: - if 'deprecated' in d[k]['options']: + +def is_deprecated(message): + '''Given a message, return True if message is deprecated''' + if 'options' in message: + if 'deprecated' in message['options']: return True # recognize the deprecated format - if 'status' in d[k]['options'] and d[k]['options']['status'] == 'deprecated': + if 'status' in message['options'] and \ + message['options']['status'] == 'deprecated': print("WARNING: please use 'option deprecated;'") return True return False -def is_in_progress(d, k): - if 'options' in d[k]: - if 'in_progress' in d[k]['options']: + +def is_in_progress(message): + '''Given a message, return True if message is marked as in_progress''' + if 'options' in message: + if 'in_progress' in message['options']: return True # recognize the deprecated format - if 'status' in d[k]['options'] and d[k]['options']['status'] == 'in_progress': + if 'status' in message['options'] and \ + message['options']['status'] == 'in_progress': print("WARNING: please use 'option in_progress;'") return True return False + def report(new, old): - added, removed, modified, same = dict_compare(new, old) + '''Given a dictionary of new crcs and old crcs, print all the + added, removed, modified, in-progress, deprecated messages. + Return the number of backwards incompatible changes made.''' + + # pylint: disable=too-many-branches + + new.pop('_version', None) + old.pop('_version', None) + added, removed, modified, _ = dict_compare(new, old) backwards_incompatible = 0 + # print the full list of in-progress messages # they should eventually either disappear of become supported for k in new.keys(): newversion = int(new[k]['version']) - if newversion == 0 or is_in_progress(new, k): + if newversion == 0 or is_in_progress(new[k]): print(f'in-progress: {k}') for k in added: print(f'added: {k}') for k in removed: oldversion = int(old[k]['version']) - if oldversion > 0 and not is_deprecated(old, k) and not is_in_progress(old, k): + if oldversion > 0 and not is_deprecated(old[k]) and not \ + is_in_progress(old[k]): backwards_incompatible += 1 print(f'removed: ** {k}') else: @@ -136,7 +178,7 @@ def report(new, old): for k in modified.keys(): oldversion = int(old[k]['version']) newversion = int(new[k]['version']) - if oldversion > 0 and not is_in_progress(old, k): + if oldversion > 0 and not is_in_progress(old[k]): backwards_incompatible += 1 print(f'modified: ** {k}') else: @@ -145,9 +187,9 @@ def report(new, old): # check which messages are still there but were marked for deprecation for k in new.keys(): newversion = int(new[k]['version']) - if newversion > 0 and is_deprecated(new, k): + if newversion > 0 and is_deprecated(new[k]): if k in old: - if not is_deprecated(old, k): + if not is_deprecated(old[k]): print(f'deprecated: {k}') else: print(f'added+deprecated: {k}') @@ -155,16 +197,49 @@ def report(new, old): return backwards_incompatible +def check_patchset(): + '''Compare the changes to API messages in this changeset. + Ignores API files with version < 1.0.0. + Only considers API files located under the src directory in the repo. + ''' + files = filelist_from_patchset('^src/') + revision = 'HEAD~1' + + oldcrcs = {} + newcrcs = {} + for filename in files: + # Ignore files that have version < 1.0.0 + _ = crc_from_apigen(None, filename) + if _['_version']['major'] == '0': + continue + + newcrcs.update(_) + oldcrcs.update(crc_from_apigen(revision, filename)) + + backwards_incompatible = report(newcrcs, oldcrcs) + if backwards_incompatible: + # alert on changing production API + print("crcchecker: Changing production APIs in an incompatible way", + file=sys.stderr) + sys.exit(-1) + else: + print('*' * 67) + print('* VPP CHECKAPI SUCCESSFULLY COMPLETED') + print('*' * 67) + + def main(): + '''Main entry point.''' parser = argparse.ArgumentParser(description='VPP CRC checker.') parser.add_argument('--git-revision', help='Git revision to compare against') parser.add_argument('--dump-manifest', action='store_true', help='Dump CRC for all messages') parser.add_argument('--check-patchset', action='store_true', - help='Dump CRC for all messages') + help='Check patchset for backwards incompatbile changes') parser.add_argument('files', nargs='*') - parser.add_argument('--diff', help='Files to compare (on filesystem)', nargs=2) + parser.add_argument('--diff', help='Files to compare (on filesystem)', + nargs=2) args = parser.parse_args() @@ -183,10 +258,10 @@ def main(): if args.dump_manifest: files = args.files if args.files else filelist_from_git_ls() crcs = {} - for f in files: - crcs.update(crc_from_apigen(args.git_revision, f)) - for k, v in crcs.items(): - print(f'{k}: {v}') + for filename in files: + crcs.update(crc_from_apigen(args.git_revision, filename)) + for k, value in crcs.items(): + print(f'{k}: {value}') sys.exit(0) # Find changes between current patchset and given revision (previous) @@ -195,21 +270,23 @@ def main(): print('Argument git-revision ignored', file=sys.stderr) # Check there are no uncomitted changes if is_uncommitted_changes(): - print('Please stash or commit changes in workspace', file=sys.stderr) + print('Please stash or commit changes in workspace', + file=sys.stderr) sys.exit(-1) - files = filelist_from_patchset() - else: - # Find changes between current workspace and revision - # Find changes between a given file and a revision - files = args.files if args.files else filelist_from_git_ls() + check_patchset() + sys.exit(0) + + # Find changes between current workspace and revision + # Find changes between a given file and a revision + files = args.files if args.files else filelist_from_git_ls() revision = args.git_revision if args.git_revision else 'HEAD~1' oldcrcs = {} newcrcs = {} - for f in files: - newcrcs.update(crc_from_apigen(None, f)) - oldcrcs.update(crc_from_apigen(revision, f)) + for file in files: + newcrcs.update(crc_from_apigen(None, file)) + oldcrcs.update(crc_from_apigen(revision, file)) backwards_incompatible = report(newcrcs, oldcrcs) @@ -223,5 +300,6 @@ def main(): print('* VPP CHECKAPI SUCCESSFULLY COMPLETED') print('*' * 67) + if __name__ == '__main__': main() diff --git a/extras/scripts/tests/test_crcchecker.sh b/extras/scripts/tests/test_crcchecker.sh index 9cfc66ae523..4caffe28e0d 100755 --- a/extras/scripts/tests/test_crcchecker.sh +++ b/extras/scripts/tests/test_crcchecker.sh @@ -55,7 +55,7 @@ verify_check_patchset_fails echo "TEST 7: Verify we can delete deprecated message" git commit -a -m "reset" -cat >crccheck.api <<EOL +cat >src/crccheck.api <<EOL option version="1.0.0"; autoreply define crccheck { @@ -63,22 +63,22 @@ autoreply define crccheck bool foo; }; EOL -git add crccheck.api +git add src/crccheck.api git commit -m "deprecated api"; # delete API -cat >crccheck.api <<EOL +cat >src/crccheck.api <<EOL option version="1.0.0"; autoreply define crccheck_2 { bool foo; }; EOL -git add crccheck.api +git add src/crccheck.api git commit -m "deprecated api"; extras/scripts/crcchecker.py --check-patchset echo "TEST 7.1: Verify we can delete deprecated message (old/confused style)" -cat >crccheck_dep.api <<EOL +cat >src/crccheck_dep.api <<EOL option version="1.0.0"; autoreply define crccheck { @@ -86,31 +86,31 @@ autoreply define crccheck bool foo; }; EOL -git add crccheck_dep.api +git add src/crccheck_dep.api git commit -m "deprecated api"; # delete API -cat >crccheck_dep.api <<EOL +cat >src/crccheck_dep.api <<EOL option version="1.0.0"; autoreply define crccheck_2 { bool foo; }; EOL -git add crccheck_dep.api +git add src/crccheck_dep.api git commit -m "deprecated api"; extras/scripts/crcchecker.py --check-patchset echo "TEST 8: Verify that we can not rename a non-deprecated message" -sed -i -e 's/crccheck_2/crccheck_3/g' crccheck.api -git add crccheck.api +sed -i -e 's/crccheck_2/crccheck_3/g' src/crccheck.api +git add src/crccheck.api git commit -m "renamed api"; verify_check_patchset_fails # fix it. -sed -i -e 's/crccheck_3/crccheck_2/g' crccheck.api +sed -i -e 's/crccheck_3/crccheck_2/g' src/crccheck.api git commit -a --amend -m "empty commit after we renamed api back" --allow-empty echo "TEST 9: Verify that the check fails if the changes are not committed" -cat >>crccheck.api <<EOL +cat >>src/crccheck.api <<EOL autoreply define crc_new_check_in_progress { option status="in_progress"; @@ -120,31 +120,31 @@ EOL verify_check_patchset_fails echo "TEST10: Verify that the in-progress message can be added" -git add crccheck.api +git add src/crccheck.api git commit -m "added a new in-progress api"; extras/scripts/crcchecker.py --check-patchset echo "TEST11: Verify we can rename an in-progress API" -sed -i -e 's/crc_new_check_in_progress/crc_new_check_in_progress_2/g' crccheck.api -git add crccheck.api +sed -i -e 's/crc_new_check_in_progress/crc_new_check_in_progress_2/g' src/crccheck.api +git add src/crccheck.api git commit -m "renamed in-progress api"; extras/scripts/crcchecker.py --check-patchset echo "TEST11.1: Switch to new designation of in-progress API" -sed -i -e 's/status="in_progress"/in_progress/g' crccheck.api -git add crccheck.api +sed -i -e 's/status="in_progress"/in_progress/g' src/crccheck.api +git add src/crccheck.api git commit -m "new designation of in-progress api"; extras/scripts/crcchecker.py --check-patchset echo "TEST12: Verify we can add a field to an in-progress API" -sed -i -e 's/foobar;/foobar; bool new_baz;/g' crccheck.api -git add crccheck.api +sed -i -e 's/foobar;/foobar; bool new_baz;/g' src/crccheck.api +git add src/crccheck.api git commit -m "new field added in in-progress api"; extras/scripts/crcchecker.py --check-patchset echo "TEST13: Verify we fail the check if the file can not be compiled" -cat >crccheck2.api <<EOL +cat >src/crccheck2.api <<EOL option version="0.0.1"; autoreply define spot_the_error { @@ -152,7 +152,7 @@ autoreply define spot_the_error bool something_important; }; EOL -git add crccheck2.api +git add src/crccheck2.api git commit -m "a new message with a syntax error"; verify_check_patchset_fails @@ -160,13 +160,13 @@ verify_check_patchset_fails git reset --hard HEAD~1 echo "TEST14: Verify we handle new .api file" -cat >crccheck3.api <<EOL +cat >src/crccheck3.api <<EOL autoreply define foo { bool bar; }; EOL -git add crccheck3.api +git add src/crccheck3.api git commit -m "a new message in new file"; extras/scripts/crcchecker.py --check-patchset diff --git a/src/tools/vppapigen/vppapigen_crc.py b/src/tools/vppapigen/vppapigen_crc.py index 6947f12d467..791e347292e 100644 --- a/src/tools/vppapigen/vppapigen_crc.py +++ b/src/tools/vppapigen/vppapigen_crc.py @@ -10,9 +10,12 @@ process_imports = True def run(args, input_filename, s): j = {} major = 0 + minor = 0 + patch = 0 if 'version' in s['Option']: v = s['Option']['version'] (major, minor, patch) = v.split('.') + j['_version'] = {'major': major, 'minor': minor, 'patch': patch} for t in s['Define']: j[t.name] = {'crc': f'{t.crc:#08x}', 'version': major, 'options': t.options} diff --git a/src/vat2/test/vat2_test.api b/src/vat2/test/vat2_test.api index 6a2c94d182e..20b483aa1db 100644 --- a/src/vat2/test/vat2_test.api +++ b/src/vat2/test/vat2_test.api @@ -13,6 +13,7 @@ * limitations under the License. */ +option version="0.0.0"; import "vnet/ip/ip_types.api"; autoreply define test_prefix { |