api: crchcecker ignore version < 1.0.0 and outside of src directory

This reverts commit 510aaa8911843206f7b9ff48b41e3c7b8c4a99fe.

Reason for revert: failed in case of no api file in changeset.

Change-Id: I2c6f01b25a35128df870418eef0008766bb590df
Type: fix
Signed-off-by: Ole Troan <ot@cisco.com>

2 files changed, 93 insertions, 171 deletions

diff --git a/extras/scripts/crcchecker.py b/extras/scripts/crcchecker.py
index 5060d9f2e0e..2b026338129 100755
--- a/extras/scripts/crcchecker.py
+++ b/extras/scripts/crcchecker.py
@@ -1,176 +1,134 @@
 #!/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
 
-# pylint: disable=subprocess-run-check
-
-ROOTDIR = os.path.dirname(os.path.realpath(__file__)) + '/../..'
-APIGENBIN = f'{ROOTDIR}/src/tools/vppapigen/vppapigen.py'
-
+rootdir = os.path.dirname(os.path.realpath(__file__)) + '/../..'
 
 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'{APIGENBIN} --git-revision {revision} --includedir src '
+        apigen = (f'{apigen_bin} --git-revision {revision} --includedir src '
                   f'--input {filename} CRC')
     else:
-        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)
+        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 returncode.returncode != 0:
-        print(f'vppapigen failed for {revision}:{filename} with '
-              'command\n {apigen}\n error: {rv}',
-              returncode.stderr.decode('ascii'), file=sys.stderr)
+    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(returncode.stdout)
+    return json.loads(rv.stdout)
 
 
-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())
+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: (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])
+    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():
-    '''Returns a list of all api files in the git repository'''
     filelist = []
     git_ls = 'git ls-files *.api'
-    returncode = run(git_ls.split(), stdout=PIPE, stderr=PIPE)
-    if returncode.returncode != 0:
-        sys.exit(returncode.returncode)
+    rv = run(git_ls.split(), stdout=PIPE, stderr=PIPE)
+    if rv.returncode != 0:
+        sys.exit(rv.returncode)
 
-    for line in returncode.stdout.decode('ascii').split('\n'):
-        if line:
-            filelist.append(line)
+    for l in rv.stdout.decode('ascii').split('\n'):
+        if len(l):
+            filelist.append(l)
     return filelist
 
 
 def is_uncommitted_changes():
-    '''Returns true if there are uncommitted changes in the repo'''
     git_status = 'git status --porcelain -uno'
-    returncode = run(git_status.split(), stdout=PIPE, stderr=PIPE)
-    if returncode.returncode != 0:
-        sys.exit(returncode.returncode)
+    rv = run(git_status.split(), stdout=PIPE, stderr=PIPE)
+    if rv.returncode != 0:
+        sys.exit(rv.returncode)
 
-    if returncode.stdout:
+    if len(rv.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:
-        returncode = check_output(f'git grep -e "import .*{filename}"'
-                                  ' -- *.api',
-                                  shell=True)
-    except CalledProcessError:
+        rv = check_output(f'git grep -e "import .*{filename}" -- *.api', shell=True)
+    except CalledProcessError as err:
         return []
-    for line in returncode.decode('ascii').split('\n'):
-        if line:
-            filename, _ = line.split(':')
-            filelist.append(filename)
+        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(pattern):
-    '''Returns list of api files in changeset and the list of api
-    files they import.'''
+def filelist_from_patchset():
     filelist = []
-    git_cmd = ('((git diff HEAD~1.. --name-only;git ls-files -m) | '
-               'sort -u | grep "\\.api$")')
-    returncode = check_output(git_cmd, shell=True)
+    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 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)))
+    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(message):
-    '''Given a message, return True if message is deprecated'''
-    if 'options' in message:
-        if 'deprecated' in message['options']:
+def is_deprecated(d, k):
+    if 'options' in d[k]:
+        if 'deprecated' in d[k]['options']:
             return True
         # recognize the deprecated format
-        if 'status' in message['options'] and \
-           message['options']['status'] == 'deprecated':
+        if 'status' in d[k]['options'] and d[k]['options']['status'] == 'deprecated':
             print("WARNING: please use 'option deprecated;'")
             return True
     return False
 
-
-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']:
+def is_in_progress(d, k):
+    if 'options' in d[k]:
+        if 'in_progress' in d[k]['options']:
             return True
         # recognize the deprecated format
-        if 'status' in message['options'] and \
-           message['options']['status'] == 'in_progress':
+        if  'status' in d[k]['options'] and d[k]['options']['status'] == 'in_progress':
             print("WARNING: please use 'option in_progress;'")
             return True
     return False
 
-
 def report(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)
+    added, removed, modified, same = 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:
@@ -178,7 +136,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:
@@ -187,9 +145,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}')
@@ -197,49 +155,16 @@ 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='Check patchset for backwards incompatbile changes')
+                        help='Dump CRC for all messages')
     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()
 
@@ -258,10 +183,10 @@ def main():
     if args.dump_manifest:
         files = args.files if args.files else filelist_from_git_ls()
         crcs = {}
-        for filename in files:
-            crcs.update(crc_from_apigen(args.git_revision, filename))
-        for k, value in crcs.items():
-            print(f'{k}: {value}')
+        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)
@@ -270,23 +195,21 @@ 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)
-        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()
+        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 file in files:
-        newcrcs.update(crc_from_apigen(None, file))
-        oldcrcs.update(crc_from_apigen(revision, file))
+    for f in files:
+        newcrcs.update(crc_from_apigen(None, f))
+        oldcrcs.update(crc_from_apigen(revision, f))
 
     backwards_incompatible = report(newcrcs, oldcrcs)
 
@@ -300,6 +223,5 @@ 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 4caffe28e0d..9cfc66ae523 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 >src/crccheck.api <<EOL
+cat >crccheck.api <<EOL
 option version="1.0.0";
 autoreply define crccheck
 {
@@ -63,22 +63,22 @@ autoreply define crccheck
   bool foo;
 };
 EOL
-git add src/crccheck.api
+git add crccheck.api
 git commit -m "deprecated api";
 # delete API
-cat >src/crccheck.api <<EOL
+cat >crccheck.api <<EOL
 option version="1.0.0";
 autoreply define crccheck_2
 {
   bool foo;
 };
 EOL
-git add src/crccheck.api
+git add 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 >src/crccheck_dep.api <<EOL
+cat >crccheck_dep.api <<EOL
 option version="1.0.0";
 autoreply define crccheck
 {
@@ -86,31 +86,31 @@ autoreply define crccheck
   bool foo;
 };
 EOL
-git add src/crccheck_dep.api
+git add crccheck_dep.api
 git commit -m "deprecated api";
 # delete API
-cat >src/crccheck_dep.api <<EOL
+cat >crccheck_dep.api <<EOL
 option version="1.0.0";
 autoreply define crccheck_2
 {
   bool foo;
 };
 EOL
-git add src/crccheck_dep.api
+git add 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' src/crccheck.api
-git add src/crccheck.api
+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' src/crccheck.api
+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 >>src/crccheck.api <<EOL
+cat >>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 src/crccheck.api
+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' src/crccheck.api
-git add src/crccheck.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 "TEST11.1: Switch to new designation of in-progress API"
-sed -i -e 's/status="in_progress"/in_progress/g' src/crccheck.api
-git add src/crccheck.api
+sed -i -e 's/status="in_progress"/in_progress/g' crccheck.api
+git add 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' src/crccheck.api
-git add src/crccheck.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 >src/crccheck2.api <<EOL
+cat >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 src/crccheck2.api
+git add 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 >src/crccheck3.api <<EOL
+cat >crccheck3.api <<EOL
 autoreply define foo
 {
   bool bar;
 };
 EOL
-git add src/crccheck3.api
+git add crccheck3.api
 git commit -m "a new message in new file";
 extras/scripts/crcchecker.py --check-patchset