diff options
| -rw-r--r-- | Makefile | 5 | ||||
| -rwxr-xr-x | extras/scripts/crcchecker.py | 203 | ||||
| -rwxr-xr-x | extras/scripts/tests/test_crcchecker.sh | 140 | ||||
| -rwxr-xr-x | src/tools/vppapigen/vppapigen.py | 74 | ||||
| -rw-r--r-- | src/tools/vppapigen/vppapigen_crc.py | 16 |
5 files changed, 415 insertions, 23 deletions
@@ -227,6 +227,7 @@ help: @echo " checkstyle - check coding style" @echo " checkstyle-commit - check commit message format" @echo " checkstyle-test - check test framework coding style" + @echo " checkstyle-api - check api for incompatible changes" @echo " fixstyle - fix coding style" @echo " doxygen - (re)generate documentation" @echo " bootstrap-doxygen - setup Doxygen dependencies" @@ -677,6 +678,10 @@ checkstyle-all: checkstyle-commit checkstyle checkstyle-test fixstyle: @build-root/scripts/checkstyle.sh --fix +.PHONY: checkstyle-api +checkstyle-api: + @extras/scripts/crcchecker.py --check-patch + # necessary because Bug 1696324 - Update to python3.6 breaks PyYAML dependencies # Status: CLOSED CANTFIX # https://bugzilla.redhat.com/show_bug.cgi?id=1696324 diff --git a/extras/scripts/crcchecker.py b/extras/scripts/crcchecker.py new file mode 100755 index 00000000000..7f83d2e6d85 --- /dev/null +++ b/extras/scripts/crcchecker.py @@ -0,0 +1,203 @@ +#!/usr/bin/env python3 + +import sys +import os +import json +import argparse +from subprocess import run, PIPE, check_output, CalledProcessError + +rootdir = os.path.dirname(os.path.realpath(__file__)) + '/../..' + +def crc_from_apigen(revision, 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 ' + 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) + 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) + sys.exit(-2) + + return json.loads(rv.stdout) + + +def dict_compare(d1, d2): + d1_keys = set(d1.keys()) + d2_keys = set(d2.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]) + return added, removed, modified, same + + +def filelist_from_git_ls(): + filelist = [] + git_ls = 'git ls-files *.api' + rv = run(git_ls.split(), stdout=PIPE, stderr=PIPE) + if rv.returncode != 0: + sys.exit(rv.returncode) + + for l in rv.stdout.decode('ascii').split('\n'): + if len(l): + filelist.append(l) + return filelist + + +def is_uncommitted_changes(): + git_status = 'git status --porcelain -uno' + rv = run(git_status.split(), stdout=PIPE, stderr=PIPE) + if rv.returncode != 0: + sys.exit(rv.returncode) + + if len(rv.stdout): + return True + return False + + +def filelist_from_git_grep(filename): + filelist = [] + try: + rv = check_output(f'git grep -e "import .*{filename}" -- *.api', shell=True) + except CalledProcessError as err: + return [] + print('RV', err.returncode) + for l in rv.decode('ascii').split('\n'): + if l: + f, p = l.split(':') + filelist.append(f) + return filelist + + +def filelist_from_patchset(): + 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) + + # Check for dependencies (imports) + imported_files = [] + for f in filelist: + imported_files.extend(filelist_from_git_grep(os.path.basename(f))) + + filelist.extend(imported_files) + return set(filelist) + +def is_deprecated(d, k): + if 'options' in d[k] and 'deprecated' in d[k]['options']: + return True + return False + +def is_in_progress(d, k): + try: + if d[k]['options']['status'] == 'in_progress': + return True + except: + return False + +def report(old, new, added, removed, modified, same): + backwards_incompatible = 0 + 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): + backwards_incompatible += 1 + print(f'removed: ** {k}') + else: + print(f'removed: {k}') + 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): + backwards_incompatible += 1 + print(f'modified: ** {k}') + else: + print(f'modified: {k}') + return backwards_incompatible + + +def main(): + 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') + parser.add_argument('files', nargs='*') + parser.add_argument('--diff', help='Files to compare (on filesystem)', nargs=2) + + args = parser.parse_args() + + if args.diff and args.files: + parser.print_help() + sys.exit(-1) + + # Diff two files + if args.diff: + oldcrcs = crc_from_apigen(None, args.diff[0]) + newcrcs = crc_from_apigen(None, args.diff[1]) + added, removed, modified, same = dict_compare(newcrcs, oldcrcs) + backwards_incompatible = report(oldcrcs, newcrcs, added, removed, modified, same) + sys.exit(0) + + # Dump CRC for messages in given files / revision + 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}') + sys.exit(0) + + # Find changes between current patchset and given revision (previous) + if args.check_patchset: + if args.git_revision: + 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) + 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() + + 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)) + + added, removed, modified, same = dict_compare(newcrcs, oldcrcs) + backwards_incompatible = report(oldcrcs, newcrcs, added, removed, modified, same) + + if args.check_patchset: + 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) + +if __name__ == '__main__': + main() diff --git a/extras/scripts/tests/test_crcchecker.sh b/extras/scripts/tests/test_crcchecker.sh new file mode 100755 index 00000000000..7cfda086483 --- /dev/null +++ b/extras/scripts/tests/test_crcchecker.sh @@ -0,0 +1,140 @@ +#!/bin/sh +set -eux + +TMPDIR=$(mktemp -d /tmp/vpp-crccheck-test-XXXXX) + +CURR_ROOT=$(git rev-parse --show-toplevel) +CURR_DIR=$(pwd) + +verify_check_patchset_fails() { + if (extras/scripts/crcchecker.py --check-patchset); then + echo "ERROR - check succeeded, it should have failed!" + exit 1; + fi +} + +finish() { + if [ -e "$TMPDIR" ]; then + echo "Temporary directory is: $TMPDIR" + fi +} +trap finish EXIT + + +# make a copy of the current repo that we can play with +cd ${TMPDIR} +mkdir misc-files +git clone ${CURR_ROOT} vpp-uut +cd vpp-uut + +# maybe grab the CRC checker +# git fetch "https://gerrit.fd.io/r/vpp" refs/changes/81/26881/14 && git cherry-pick FETCH_HEAD || echo "Already there" + + +echo "TEST 1: Check the current patchset..." +extras/scripts/crcchecker.py --check-patchset + +echo "TEST 2: Dumping the current manifest..." +extras/scripts/crcchecker.py --dump-manifest >${TMPDIR}/misc-files/manifest.txt + +echo "TEST 3: Checking the 20.01 version of acl.api...." +extras/scripts/crcchecker.py --git-revision v20.01 src/plugins/acl/acl.api + +echo "TEST 4: Add a new field into a message in acl.api, and check patchset - must fail..." +sed -i -e 's#vpe_pid;#vpe_pid; u32 sneaky_new_field;#' src/plugins/acl/acl.api +verify_check_patchset_fails + +echo "TEST 5: Rename the changed acl.api file and not add it to git... must fail (due to deletion of the APIs)..." +mv src/plugins/acl/acl.api src/plugins/acl/acl_new.api +verify_check_patchset_fails + +echo "TEST 6: Add the renamed file to git commit... must fail (due to addition of the fields)..." +git add src/plugins/acl/acl_new.api +git commit -m "added acl_new.api" +verify_check_patchset_fails + +echo "TEST 7: Verify we can delete deprecated message" +git commit -a -m "reset" +cat >crccheck.api <<EOL +option version="1.0.0"; +autoreply define crccheck +{ + option deprecated="v20.11"; + bool foo; +}; +EOL +git add crccheck.api +git commit -m "deprecated api"; +# delete API +cat >crccheck.api <<EOL +option version="1.0.0"; +autoreply define crccheck_2 +{ + bool foo; +}; +EOL +git add crccheck.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 +git commit -m "renamed api"; +verify_check_patchset_fails +# fix it. +sed -i -e 's/crccheck_3/crccheck_2/g' 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 +autoreply define crc_new_check_in_progress +{ + option status="in_progress"; + bool foobar; +}; +EOL +verify_check_patchset_fails + +echo "TEST10: Verify that the in-progress message can be added" +git add 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 +git commit -m "renamed 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 +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 +option version="0.0.1"; +autoreply define spot_the_error +{ + option status="in_progress" + bool something_important; +}; +EOL +git add crccheck2.api +git commit -m "a new message with a syntax error"; +verify_check_patchset_fails + +# get rid of the "erroneous" commit in the previous test +git reset --hard HEAD~1 + +echo "TEST: All tests got the expected result, cleaning up." + +# done with all the tests - clean up +cd ${CURR_DIR} + +# beware of empty variables, careful with deletion +rm -rf ${TMPDIR}/vpp-uut +rm -rf ${TMPDIR}/misc-files +rmdir ${TMPDIR} diff --git a/src/tools/vppapigen/vppapigen.py b/src/tools/vppapigen/vppapigen.py index b17ad6d15c9..28712e4d949 100755 --- a/src/tools/vppapigen/vppapigen.py +++ b/src/tools/vppapigen/vppapigen.py @@ -9,6 +9,7 @@ import logging import binascii import os import sys +from subprocess import Popen, PIPE log = logging.getLogger('vppapigen') @@ -275,14 +276,17 @@ class Define(): elif f == 'autoreply': self.autoreply = True + remove = [] for b in block: if isinstance(b, Option): if b[1] == 'singular' and b[2] == 'true': self.singular = True else: self.options[b.option] = b.value - block.remove(b) + remove.append(b) + block = [x for x in block if not x in remove] + self.block = block self.vla = vla_is_last_check(name, block) self.crc = str(block).encode() @@ -322,25 +326,20 @@ class Import(): return seen_imports[args[0]] - def __init__(self, filename): + def __init__(self, filename, revision): if self._initialized: return else: self.filename = filename # Deal with imports - parser = VPPAPI(filename=filename) + parser = VPPAPI(filename=filename, revision=revision) dirlist = dirlist_get() f = filename for dir in dirlist: f = os.path.join(dir, filename) if os.path.exists(f): break - if sys.version[0] == '2': - with open(f) as fd: - self.result = parser.parse_file(fd, None) - else: - with open(f, encoding='utf-8') as fd: - self.result = parser.parse_file(fd, None) + self.result = parser.parse_filename(f, None) self._initialized = True def __repr__(self): @@ -430,10 +429,11 @@ class ParseError(Exception): class VPPAPIParser(object): tokens = VPPAPILexer.tokens - def __init__(self, filename, logger): + def __init__(self, filename, logger, revision=None): self.filename = filename self.logger = logger self.fields = [] + self.revision = revision def _parse_error(self, msg, coord): raise ParseError("%s: %s" % (coord, msg)) @@ -478,7 +478,7 @@ class VPPAPIParser(object): def p_import(self, p): '''import : IMPORT STRING_LITERAL ';' ''' - p[0] = Import(p[2]) + p[0] = Import(p[2], revision=self.revision) def p_service(self, p): '''service : SERVICE '{' service_statements '}' ';' ''' @@ -731,23 +731,42 @@ class VPPAPIParser(object): class VPPAPI(object): - def __init__(self, debug=False, filename='', logger=None): + def __init__(self, debug=False, filename='', logger=None, revision=None): self.lexer = lex.lex(module=VPPAPILexer(filename), debug=debug) - self.parser = yacc.yacc(module=VPPAPIParser(filename, logger), + self.parser = yacc.yacc(module=VPPAPIParser(filename, logger, + revision=revision), write_tables=False, debug=debug) self.logger = logger + self.revision = revision + self.filename = filename def parse_string(self, code, debug=0, lineno=1): self.lexer.lineno = lineno return self.parser.parse(code, lexer=self.lexer, debug=debug) - def parse_file(self, fd, debug=0): + def parse_fd(self, fd, debug=0): data = fd.read() return self.parse_string(data, debug=debug) - def autoreply_block(self, name): + def parse_filename(self, filename, debug=0): + if self.revision: + git_show = f'git show {self.revision}:{filename}' + with Popen(git_show.split(), stdout=PIPE, encoding='utf-8') as git: + return self.parse_fd(git.stdout, None) + else: + try: + with open(filename, encoding='utf-8') as fd: + return self.parse_fd(fd, None) + except FileNotFoundError: + print(f'File not found: {filename}', file=sys.stderr) + sys.exit(2) + + def autoreply_block(self, name, parent): block = [Field('u32', 'context'), Field('i32', 'retval')] + # inherhit the parent's options + for k,v in parent.options.items(): + block.append(Option(k, v)) return Define(name + '_reply', [], block) def process(self, objs): @@ -767,7 +786,7 @@ class VPPAPI(object): if isinstance(o, Define): s[tname].append(o) if o.autoreply: - s[tname].append(self.autoreply_block(o.name)) + s[tname].append(self.autoreply_block(o.name, o)) elif isinstance(o, Option): s[tname][o.option] = o.value elif type(o) is list: @@ -925,9 +944,7 @@ def main(): cliparser.add_argument('--pluginpath', default=""), cliparser.add_argument('--includedir', action='append'), cliparser.add_argument('--outputdir', action='store'), - cliparser.add_argument('--input', - type=argparse.FileType('r', encoding='UTF-8'), - default=sys.stdin) + cliparser.add_argument('--input') cliparser.add_argument('--output', nargs='?', type=argparse.FileType('w', encoding='UTF-8'), default=sys.stdout) @@ -935,6 +952,8 @@ def main(): cliparser.add_argument('output_module', nargs='?', default='C') cliparser.add_argument('--debug', action='store_true') cliparser.add_argument('--show-name', nargs=1) + cliparser.add_argument('--git-revision', + help="Git revision to use for opening files") args = cliparser.parse_args() dirlist_add(args.includedir) @@ -944,8 +963,8 @@ def main(): # Filename if args.show_name: filename = args.show_name[0] - elif args.input != sys.stdin: - filename = args.input.name + elif args.input: + filename = args.input else: filename = '' @@ -954,8 +973,17 @@ def main(): else: logging.basicConfig() - parser = VPPAPI(debug=args.debug, filename=filename, logger=log) - parsed_objects = parser.parse_file(args.input, log) + parser = VPPAPI(debug=args.debug, filename=filename, logger=log, + revision=args.git_revision) + + try: + if not args.input: + parsed_objects = parser.parse_fd(sys.stdin, log) + else: + parsed_objects = parser.parse_filename(args.input, log) + except ParseError as e: + print('Parse error: ', e, file=sys.stderr) + sys.exit(1) # Build a list of objects. Hash of lists. result = [] diff --git a/src/tools/vppapigen/vppapigen_crc.py b/src/tools/vppapigen/vppapigen_crc.py new file mode 100644 index 00000000000..b3cb5855c9d --- /dev/null +++ b/src/tools/vppapigen/vppapigen_crc.py @@ -0,0 +1,16 @@ +# CRC generation +import json + +# +# Plugin entry point +# +def run(args, input_filename, s): + j = {} + major = 0 + if 'version' in s['Option']: + v = s['Option']['version'] + (major, minor, patch) = v.split('.') + for t in s['Define']: + j[t.name] = {'crc': f'{t.crc:#08x}', 'version': major, + 'options': t.options} + return json.dumps(j, indent=4, separators=(',', ': ')) |