From da84e3215e057f4bdbc4468beaafeb2cda00387e Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Tue, 19 Feb 2019 16:59:33 +0000 Subject: [PATCH 01/22] Extend abi-checking to different repos --- scripts/abi_check.py | 79 +++++++++++++++++++++++++++++++++++--------- 1 file changed, 63 insertions(+), 16 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index 7926bc8cd..d14236ead 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -28,11 +28,14 @@ import tempfile class AbiChecker(object): """API and ABI checker.""" - def __init__(self, report_dir, old_rev, new_rev, keep_all_reports): + def __init__(self, report_dir, old_repo, old_rev, new_repo, new_rev, + keep_all_reports): """Instantiate the API/ABI checker. report_dir: directory for output files + old_repo: repository for git revision to compare against old_rev: reference git revision to compare against + new_repo: repository for git revision to check new_rev: git revision to check keep_all_reports: if false, delete old reports """ @@ -42,7 +45,9 @@ class AbiChecker(object): self.report_dir = os.path.abspath(report_dir) self.keep_all_reports = keep_all_reports self.should_keep_report_dir = os.path.isdir(self.report_dir) + self.old_repo = old_repo self.old_rev = old_rev + self.new_repo = new_repo self.new_rev = new_rev self.mbedtls_modules = ["libmbedcrypto", "libmbedtls", "libmbedx509"] self.old_dumps = {} @@ -68,15 +73,35 @@ class AbiChecker(object): if not shutil.which(command): raise Exception("{} not installed, aborting".format(command)) - def get_clean_worktree_for_git_revision(self, git_rev): + def get_clean_worktree_for_git_revision(self, remote_repo, git_rev): """Make a separate worktree with git_rev checked out. Do not modify the current worktree.""" - self.log.info( - "Checking out git worktree for revision {}".format(git_rev) - ) git_worktree_path = tempfile.mkdtemp() + if remote_repo: + self.log.info( + "Checking out git worktree for revision {} from {}".format( + git_rev, remote_repo + ) + ) + fetch_process = subprocess.Popen( + [self.git_command, "fetch", remote_repo, git_rev], + cwd=self.repo_path, + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT + ) + fetch_output, _ = fetch_process.communicate() + self.log.info(fetch_output.decode("utf-8")) + if fetch_process.returncode != 0: + raise Exception("Fetching revision failed, aborting") + worktree_rev = "FETCH_HEAD" + else: + self.log.info( + "Checking out git worktree for revision {}".format(git_rev) + ) + worktree_rev = git_rev worktree_process = subprocess.Popen( - [self.git_command, "worktree", "add", "--detach", git_worktree_path, git_rev], + [self.git_command, "worktree", "add", "--detach", + git_worktree_path, worktree_rev], cwd=self.repo_path, stdout=subprocess.PIPE, stderr=subprocess.STDOUT @@ -158,9 +183,11 @@ class AbiChecker(object): if worktree_process.returncode != 0: raise Exception("Worktree cleanup failed, aborting") - def get_abi_dump_for_ref(self, git_rev): + def get_abi_dump_for_ref(self, remote_repo, git_rev): """Generate the ABI dumps for the specified git revision.""" - git_worktree_path = self.get_clean_worktree_for_git_revision(git_rev) + git_worktree_path = self.get_clean_worktree_for_git_revision( + remote_repo, git_rev + ) self.update_git_submodules(git_worktree_path) self.build_shared_libraries(git_worktree_path) abi_dumps = self.get_abi_dumps_from_shared_libraries( @@ -226,8 +253,8 @@ class AbiChecker(object): between self.old_rev and self.new_rev.""" self.check_repo_path() self.check_abi_tools_are_installed() - self.old_dumps = self.get_abi_dump_for_ref(self.old_rev) - self.new_dumps = self.get_abi_dump_for_ref(self.new_rev) + self.old_dumps = self.get_abi_dump_for_ref(self.old_repo, self.old_rev) + self.new_dumps = self.get_abi_dump_for_ref(self.new_repo, self.new_rev) return self.get_abi_compatibility_report() @@ -254,17 +281,37 @@ def run_main(): help="keep all reports, even if there are no compatibility issues", ) parser.add_argument( - "-o", "--old-rev", type=str, help="revision for old version", - required=True + "-o", "--old-rev", type=str, + help=("revision for old version." + "Can include repository before revision"), + required=True, nargs="+" ) parser.add_argument( - "-n", "--new-rev", type=str, help="revision for new version", - required=True + "-n", "--new-rev", type=str, + help=("revision for new version" + "Can include repository before revision"), + required=True, nargs="+" ) abi_args = parser.parse_args() + if len(abi_args.old_rev) == 1: + old_repo = None + old_rev = abi_args.old_rev[0] + elif len(abi_args.old_rev) == 2: + old_repo = abi_args.old_rev[0] + old_rev = abi_args.old_rev[1] + else: + raise Exception("Too many arguments passed for old version") + if len(abi_args.new_rev) == 1: + new_repo = None + new_rev = abi_args.new_rev[0] + elif len(abi_args.new_rev) == 2: + new_repo = abi_args.new_rev[0] + new_rev = abi_args.new_rev[1] + else: + raise Exception("Too many arguments passed for new version") abi_check = AbiChecker( - abi_args.report_dir, abi_args.old_rev, - abi_args.new_rev, abi_args.keep_all_reports + abi_args.report_dir, old_repo, old_rev, + new_repo, new_rev, abi_args.keep_all_reports ) return_code = abi_check.check_for_abi_changes() sys.exit(return_code) From c2883a29bc836dae2b001471339f33a0d632fe7d Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Wed, 20 Feb 2019 15:01:56 +0000 Subject: [PATCH 02/22] Add option to skip identifiers in ABI checks By default abi-compliance-checker will check the entire ABI/API. There are internal identifiers that we do not promise compatibility for, so we want the ability to skip them when checking the ABI/API. --- scripts/abi_check.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index d14236ead..559501dee 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -29,7 +29,7 @@ class AbiChecker(object): """API and ABI checker.""" def __init__(self, report_dir, old_repo, old_rev, new_repo, new_rev, - keep_all_reports): + keep_all_reports, skip_file=None): """Instantiate the API/ABI checker. report_dir: directory for output files @@ -38,6 +38,7 @@ class AbiChecker(object): new_repo: repository for git revision to check new_rev: git revision to check keep_all_reports: if false, delete old reports + skip_file: path to file containing symbols and types to skip """ self.repo_path = "." self.log = None @@ -49,6 +50,7 @@ class AbiChecker(object): self.old_rev = old_rev self.new_repo = new_repo self.new_rev = new_rev + self.skip_file = skip_file self.mbedtls_modules = ["libmbedcrypto", "libmbedtls", "libmbedx509"] self.old_dumps = {} self.new_dumps = {} @@ -216,6 +218,9 @@ class AbiChecker(object): "-strict", "-report-path", output_path ] + if self.skip_file: + abi_compliance_command += ["-skip-symbols", self.skip_file, + "-skip-types", self.skip_file] abi_compliance_process = subprocess.Popen( abi_compliance_command, stdout=subprocess.PIPE, @@ -292,6 +297,10 @@ def run_main(): "Can include repository before revision"), required=True, nargs="+" ) + parser.add_argument( + "-s", "--skip-file", type=str, + help="path to file containing symbols and types to skip" + ) abi_args = parser.parse_args() if len(abi_args.old_rev) == 1: old_repo = None @@ -311,7 +320,8 @@ def run_main(): raise Exception("Too many arguments passed for new version") abi_check = AbiChecker( abi_args.report_dir, old_repo, old_rev, - new_repo, new_rev, abi_args.keep_all_reports + new_repo, new_rev, abi_args.keep_all_reports, + abi_args.skip_file ) return_code = abi_check.check_for_abi_changes() sys.exit(return_code) From e62f9bbbf19e7a99e6c8ab6e0a760c5fc2964d30 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Thu, 21 Feb 2019 13:09:26 +0000 Subject: [PATCH 03/22] Add option for a brief report of problems only --- scripts/abi_check.py | 71 +++++++++++++++++++++++++++++++++----------- 1 file changed, 54 insertions(+), 17 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index 559501dee..afc2afe1c 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -9,10 +9,10 @@ Purpose This script is a small wrapper around the abi-compliance-checker and abi-dumper tools, applying them to compare the ABI and API of the library files from two different Git revisions within an Mbed TLS repository. -The results of the comparison are formatted as HTML and stored at -a configurable location. Returns 0 on success, 1 on ABI/API non-compliance, -and 2 if there is an error while running the script. -Note: must be run from Mbed TLS root. +The results of the comparison are either formatted as HTML and stored at +a configurable location, or a brief list of problems found is output. +Returns 0 on success, 1 on ABI/API non-compliance, and 2 if there is an error +while running the script. Note: must be run from Mbed TLS root. """ import os @@ -24,12 +24,14 @@ import argparse import logging import tempfile +import xml.etree.ElementTree as ET + class AbiChecker(object): """API and ABI checker.""" def __init__(self, report_dir, old_repo, old_rev, new_repo, new_rev, - keep_all_reports, skip_file=None): + keep_all_reports, brief, skip_file=None): """Instantiate the API/ABI checker. report_dir: directory for output files @@ -38,6 +40,7 @@ class AbiChecker(object): new_repo: repository for git revision to check new_rev: git revision to check keep_all_reports: if false, delete old reports + brief: if true, output shorter report to stdout skip_file: path to file containing symbols and types to skip """ self.repo_path = "." @@ -51,6 +54,7 @@ class AbiChecker(object): self.new_repo = new_repo self.new_rev = new_rev self.skip_file = skip_file + self.brief = brief self.mbedtls_modules = ["libmbedcrypto", "libmbedtls", "libmbedx509"] self.old_dumps = {} self.new_dumps = {} @@ -198,6 +202,24 @@ class AbiChecker(object): self.cleanup_worktree(git_worktree_path) return abi_dumps + def remove_children_with_tag(self, parent, tag): + children = parent.getchildren() + for child in children: + if child.tag == tag: + parent.remove(child) + else: + self.remove_children_with_tag(child, tag) + + def remove_extra_detail_from_report(self, report_root): + for tag in ['test_info', 'test_results', 'problem_summary', + 'added_symbols', 'removed_symbols', 'affected']: + self.remove_children_with_tag(report_root, tag) + + for report in report_root: + for problems in report.getchildren()[:]: + if not problems.getchildren(): + report.remove(problems) + def get_abi_compatibility_report(self): """Generate a report of the differences between the reference ABI and the new ABI. ABI dumps from self.old_rev and self.new_rev must @@ -216,31 +238,41 @@ class AbiChecker(object): "-old", self.old_dumps[mbed_module], "-new", self.new_dumps[mbed_module], "-strict", - "-report-path", output_path + "-report-path", output_path, ] if self.skip_file: abi_compliance_command += ["-skip-symbols", self.skip_file, "-skip-types", self.skip_file] + if self.brief: + abi_compliance_command += ["-report-format", "xml", + "-stdout"] abi_compliance_process = subprocess.Popen( abi_compliance_command, stdout=subprocess.PIPE, stderr=subprocess.STDOUT ) abi_compliance_output, _ = abi_compliance_process.communicate() - self.log.info(abi_compliance_output.decode("utf-8")) if abi_compliance_process.returncode == 0: compatibility_report += ( "No compatibility issues for {}\n".format(mbed_module) ) - if not self.keep_all_reports: + if not (self.keep_all_reports or self.brief): os.remove(output_path) elif abi_compliance_process.returncode == 1: - compliance_return_code = 1 - self.should_keep_report_dir = True - compatibility_report += ( - "Compatibility issues found for {}, " - "for details see {}\n".format(mbed_module, output_path) - ) + if self.brief: + self.log.info( + "Compatibility issues found for {}".format(mbed_module) + ) + report_root = ET.fromstring(abi_compliance_output.decode("utf-8")) + self.remove_extra_detail_from_report(report_root) + self.log.info(ET.tostring(report_root).decode("utf-8")) + else: + compliance_return_code = 1 + self.can_remove_report_dir = False + compatibility_report += ( + "Compatibility issues found for {}, " + "for details see {}\n".format(mbed_module, output_path) + ) else: raise Exception( "abi-compliance-checker failed with a return code of {}," @@ -271,8 +303,9 @@ def run_main(): abi-compliance-checker and abi-dumper tools, applying them to compare the ABI and API of the library files from two different Git revisions within an Mbed TLS repository. - The results of the comparison are formatted as HTML and stored - at a configurable location. Returns 0 on success, 1 on ABI/API + The results of the comparison are either formatted as HTML and + stored at a configurable location, or a brief list of problems + found is output. Returns 0 on success, 1 on ABI/API non-compliance, and 2 if there is an error while running the script. Note: must be run from Mbed TLS root.""" ) @@ -301,6 +334,10 @@ def run_main(): "-s", "--skip-file", type=str, help="path to file containing symbols and types to skip" ) + parser.add_argument( + "-b", "--brief", action="store_true", + help="output only the list of issues to stdout, instead of a full report", + ) abi_args = parser.parse_args() if len(abi_args.old_rev) == 1: old_repo = None @@ -321,7 +358,7 @@ def run_main(): abi_check = AbiChecker( abi_args.report_dir, old_repo, old_rev, new_repo, new_rev, abi_args.keep_all_reports, - abi_args.skip_file + abi_args.brief, abi_args.skip_file ) return_code = abi_check.check_for_abi_changes() sys.exit(return_code) From 3d3d552579d23d78e974cb7822d1d046fd4865fd Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Mon, 25 Feb 2019 17:01:55 +0000 Subject: [PATCH 04/22] Simplify logic for checking if report folder can be removed --- scripts/abi_check.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index afc2afe1c..30f38a9bb 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -48,7 +48,8 @@ class AbiChecker(object): self.setup_logger() self.report_dir = os.path.abspath(report_dir) self.keep_all_reports = keep_all_reports - self.should_keep_report_dir = os.path.isdir(self.report_dir) + self.can_remove_report_dir = not (os.path.isdir(self.report_dir) or + keep_all_reports) self.old_repo = old_repo self.old_rev = old_rev self.new_repo = new_repo @@ -280,7 +281,7 @@ class AbiChecker(object): ) os.remove(self.old_dumps[mbed_module]) os.remove(self.new_dumps[mbed_module]) - if not self.should_keep_report_dir and not self.keep_all_reports: + if self.can_remove_report_dir: os.rmdir(self.report_dir) self.log.info(compatibility_report) return compliance_return_code From 9f357d65d4c7ecd6c009471ba87cce19870e39e2 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Mon, 25 Feb 2019 11:35:05 +0000 Subject: [PATCH 05/22] Extend functionality to allow setting crypto submodule version As going forward we will have Crypto in a submodule, we will need to be able to check ABI compatibility between versions using different submodule versions. For TLS versions that support the submodule, we will always build using the submodule. If the Crypto submodule is used, libmbedcrypto.so is not in the main library folder, but in crypto/library instead. Given this, the script searches for *.so files and notes their path, in order to create the dumps correctly. --- scripts/abi_check.py | 63 +++++++++++++++++++++++++++++++++----------- 1 file changed, 48 insertions(+), 15 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index 30f38a9bb..6475a7e64 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -23,6 +23,7 @@ import subprocess import argparse import logging import tempfile +import fnmatch import xml.etree.ElementTree as ET @@ -30,15 +31,18 @@ import xml.etree.ElementTree as ET class AbiChecker(object): """API and ABI checker.""" - def __init__(self, report_dir, old_repo, old_rev, new_repo, new_rev, - keep_all_reports, brief, skip_file=None): + def __init__(self, report_dir, old_repo, old_rev, old_crypto_rev, + new_repo, new_rev, new_crypto_rev, keep_all_reports, brief, + skip_file=None): """Instantiate the API/ABI checker. report_dir: directory for output files old_repo: repository for git revision to compare against old_rev: reference git revision to compare against + old_crypto_rev: reference git revision for old crypto submodule new_repo: repository for git revision to check new_rev: git revision to check + new_crypto_rev: reference git revision for new crypto submodule keep_all_reports: if false, delete old reports brief: if true, output shorter report to stdout skip_file: path to file containing symbols and types to skip @@ -52,11 +56,13 @@ class AbiChecker(object): keep_all_reports) self.old_repo = old_repo self.old_rev = old_rev + self.old_crypto_rev = old_crypto_rev self.new_repo = new_repo self.new_rev = new_rev + self.new_crypto_rev = new_crypto_rev self.skip_file = skip_file self.brief = brief - self.mbedtls_modules = ["libmbedcrypto", "libmbedtls", "libmbedx509"] + self.mbedtls_modules = {} self.old_dumps = {} self.new_dumps = {} self.git_command = "git" @@ -119,7 +125,7 @@ class AbiChecker(object): raise Exception("Checking out worktree failed, aborting") return git_worktree_path - def update_git_submodules(self, git_worktree_path): + def update_git_submodules(self, git_worktree_path, crypto_rev): process = subprocess.Popen( [self.git_command, "submodule", "update", "--init", '--recursive'], cwd=git_worktree_path, @@ -130,12 +136,25 @@ class AbiChecker(object): self.log.info(output.decode("utf-8")) if process.returncode != 0: raise Exception("git submodule update failed, aborting") + if (os.path.exists(os.path.join(git_worktree_path, "crypto")) + and crypto_rev): + checkout_process = subprocess.Popen( + [self.git_command, "checkout", crypto_rev], + cwd=os.path.join(git_worktree_path, "crypto"), + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT + ) + checkout_output, _ = checkout_process.communicate() + self.log.info(checkout_output.decode("utf-8")) + if checkout_process.returncode != 0: + raise Exception("git checkout failed, aborting") def build_shared_libraries(self, git_worktree_path): """Build the shared libraries in the specified worktree.""" my_environment = os.environ.copy() my_environment["CFLAGS"] = "-g -Og" my_environment["SHARED"] = "1" + my_environment["USE_CRYPTO_SUBMODULE"] = "1" make_process = subprocess.Popen( self.make_command, env=my_environment, @@ -145,6 +164,11 @@ class AbiChecker(object): ) make_output, _ = make_process.communicate() self.log.info(make_output.decode("utf-8")) + for root, dirs, files in os.walk(git_worktree_path): + for file in fnmatch.filter(files, "*.so"): + self.mbedtls_modules[os.path.splitext(file)[0]] = os.path.join( + root, file + ) if make_process.returncode != 0: raise Exception("make failed, aborting") @@ -153,14 +177,13 @@ class AbiChecker(object): It must be checked out in git_worktree_path and the shared libraries must have been built.""" abi_dumps = {} - for mbed_module in self.mbedtls_modules: + for mbed_module, module_path in self.mbedtls_modules.items(): output_path = os.path.join( self.report_dir, "{}-{}.dump".format(mbed_module, git_ref) ) abi_dump_command = [ "abi-dumper", - os.path.join( - git_worktree_path, "library", mbed_module + ".so"), + module_path, "-o", output_path, "-lver", git_ref ] @@ -190,12 +213,12 @@ class AbiChecker(object): if worktree_process.returncode != 0: raise Exception("Worktree cleanup failed, aborting") - def get_abi_dump_for_ref(self, remote_repo, git_rev): + def get_abi_dump_for_ref(self, remote_repo, git_rev, crypto_rev): """Generate the ABI dumps for the specified git revision.""" git_worktree_path = self.get_clean_worktree_for_git_revision( remote_repo, git_rev ) - self.update_git_submodules(git_worktree_path) + self.update_git_submodules(git_worktree_path, crypto_rev) self.build_shared_libraries(git_worktree_path) abi_dumps = self.get_abi_dumps_from_shared_libraries( git_rev, git_worktree_path @@ -227,7 +250,7 @@ class AbiChecker(object): be available.""" compatibility_report = "" compliance_return_code = 0 - for mbed_module in self.mbedtls_modules: + for mbed_module, module_path in self.mbedtls_modules.items(): output_path = os.path.join( self.report_dir, "{}-{}-{}.html".format( mbed_module, self.old_rev, self.new_rev @@ -291,8 +314,10 @@ class AbiChecker(object): between self.old_rev and self.new_rev.""" self.check_repo_path() self.check_abi_tools_are_installed() - self.old_dumps = self.get_abi_dump_for_ref(self.old_repo, self.old_rev) - self.new_dumps = self.get_abi_dump_for_ref(self.new_repo, self.new_rev) + self.old_dumps = self.get_abi_dump_for_ref(self.old_repo, self.old_rev, + self.old_crypto_rev) + self.new_dumps = self.get_abi_dump_for_ref(self.new_repo, self.new_rev, + self.new_crypto_rev) return self.get_abi_compatibility_report() @@ -325,12 +350,20 @@ def run_main(): "Can include repository before revision"), required=True, nargs="+" ) + parser.add_argument( + "-oc", "--old-crypto-rev", type=str, + help="revision for old crypto version", + ) parser.add_argument( "-n", "--new-rev", type=str, help=("revision for new version" "Can include repository before revision"), required=True, nargs="+" ) + parser.add_argument( + "-nc", "--new-crypto-rev", type=str, + help="revision for new crypto version", + ) parser.add_argument( "-s", "--skip-file", type=str, help="path to file containing symbols and types to skip" @@ -357,9 +390,9 @@ def run_main(): else: raise Exception("Too many arguments passed for new version") abi_check = AbiChecker( - abi_args.report_dir, old_repo, old_rev, - new_repo, new_rev, abi_args.keep_all_reports, - abi_args.brief, abi_args.skip_file + abi_args.report_dir, old_repo, old_rev, abi_args.old_crypto_rev, + new_repo, new_rev, abi_args.new_crypto_rev, + abi_args.keep_all_reports, abi_args.brief, abi_args.skip_file ) return_code = abi_check.check_for_abi_changes() sys.exit(return_code) From 3e7a980d625050feea6da7ec41c1b6aec26ca586 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Wed, 27 Feb 2019 16:53:40 +0000 Subject: [PATCH 06/22] Add handling for cases when not all .so files are present We may wish to compare ABI/API between Mbed TLS and Mbed Crypto, which will cause issues as not all .so files are shared. Only compare .so files which both libraries have. --- scripts/abi_check.py | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index 6475a7e64..3f697fd47 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -62,7 +62,7 @@ class AbiChecker(object): self.new_crypto_rev = new_crypto_rev self.skip_file = skip_file self.brief = brief - self.mbedtls_modules = {} + self.mbedtls_modules = {"old": {}, "new": {}} self.old_dumps = {} self.new_dumps = {} self.git_command = "git" @@ -149,7 +149,7 @@ class AbiChecker(object): if checkout_process.returncode != 0: raise Exception("git checkout failed, aborting") - def build_shared_libraries(self, git_worktree_path): + def build_shared_libraries(self, git_worktree_path, version): """Build the shared libraries in the specified worktree.""" my_environment = os.environ.copy() my_environment["CFLAGS"] = "-g -Og" @@ -166,20 +166,23 @@ class AbiChecker(object): self.log.info(make_output.decode("utf-8")) for root, dirs, files in os.walk(git_worktree_path): for file in fnmatch.filter(files, "*.so"): - self.mbedtls_modules[os.path.splitext(file)[0]] = os.path.join( - root, file + self.mbedtls_modules[version][os.path.splitext(file)[0]] = ( + os.path.join(root, file) ) if make_process.returncode != 0: raise Exception("make failed, aborting") - def get_abi_dumps_from_shared_libraries(self, git_ref, git_worktree_path): + def get_abi_dumps_from_shared_libraries(self, git_ref, git_worktree_path, + version): """Generate the ABI dumps for the specified git revision. It must be checked out in git_worktree_path and the shared libraries must have been built.""" abi_dumps = {} - for mbed_module, module_path in self.mbedtls_modules.items(): + for mbed_module, module_path in self.mbedtls_modules[version].items(): output_path = os.path.join( - self.report_dir, "{}-{}.dump".format(mbed_module, git_ref) + self.report_dir, version, "{}-{}.dump".format( + mbed_module, git_ref + ) ) abi_dump_command = [ "abi-dumper", @@ -213,15 +216,15 @@ class AbiChecker(object): if worktree_process.returncode != 0: raise Exception("Worktree cleanup failed, aborting") - def get_abi_dump_for_ref(self, remote_repo, git_rev, crypto_rev): + def get_abi_dump_for_ref(self, remote_repo, git_rev, crypto_rev, version): """Generate the ABI dumps for the specified git revision.""" git_worktree_path = self.get_clean_worktree_for_git_revision( remote_repo, git_rev ) self.update_git_submodules(git_worktree_path, crypto_rev) - self.build_shared_libraries(git_worktree_path) + self.build_shared_libraries(git_worktree_path, version) abi_dumps = self.get_abi_dumps_from_shared_libraries( - git_rev, git_worktree_path + git_rev, git_worktree_path, version ) self.cleanup_worktree(git_worktree_path) return abi_dumps @@ -250,7 +253,9 @@ class AbiChecker(object): be available.""" compatibility_report = "" compliance_return_code = 0 - for mbed_module, module_path in self.mbedtls_modules.items(): + shared_modules = list(set(self.mbedtls_modules["old"].keys()) & + set(self.mbedtls_modules["new"].keys())) + for mbed_module in shared_modules: output_path = os.path.join( self.report_dir, "{}-{}-{}.html".format( mbed_module, self.old_rev, self.new_rev @@ -315,9 +320,9 @@ class AbiChecker(object): self.check_repo_path() self.check_abi_tools_are_installed() self.old_dumps = self.get_abi_dump_for_ref(self.old_repo, self.old_rev, - self.old_crypto_rev) + self.old_crypto_rev, "old") self.new_dumps = self.get_abi_dump_for_ref(self.new_repo, self.new_rev, - self.new_crypto_rev) + self.new_crypto_rev, "new") return self.get_abi_compatibility_report() From 4831145cdd143aaea64402e10207210a7b167ca1 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Wed, 27 Feb 2019 17:33:31 +0000 Subject: [PATCH 07/22] Add ability to compare submodules from different repositories As before with wanting to compare revisions across different repositories, the ability to select the crypto submodule from a different repository is useful. --- scripts/abi_check.py | 86 +++++++++++++++++++++++++++++++++----------- 1 file changed, 65 insertions(+), 21 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index 3f697fd47..30baadcdb 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -32,17 +32,19 @@ class AbiChecker(object): """API and ABI checker.""" def __init__(self, report_dir, old_repo, old_rev, old_crypto_rev, - new_repo, new_rev, new_crypto_rev, keep_all_reports, brief, - skip_file=None): + old_crypto_repo, new_repo, new_rev, new_crypto_rev, + new_crypto_repo, keep_all_reports, brief, skip_file=None): """Instantiate the API/ABI checker. report_dir: directory for output files old_repo: repository for git revision to compare against old_rev: reference git revision to compare against old_crypto_rev: reference git revision for old crypto submodule + old_crypto_repo: repository for git revision for old crypto submodule new_repo: repository for git revision to check new_rev: git revision to check new_crypto_rev: reference git revision for new crypto submodule + new_crypto_repo: repository for git revision for new crypto submodule keep_all_reports: if false, delete old reports brief: if true, output shorter report to stdout skip_file: path to file containing symbols and types to skip @@ -57,9 +59,11 @@ class AbiChecker(object): self.old_repo = old_repo self.old_rev = old_rev self.old_crypto_rev = old_crypto_rev + self.old_crypto_repo = old_crypto_repo self.new_repo = new_repo self.new_rev = new_rev self.new_crypto_rev = new_crypto_rev + self.new_crypto_repo = new_crypto_repo self.skip_file = skip_file self.brief = brief self.mbedtls_modules = {"old": {}, "new": {}} @@ -125,7 +129,8 @@ class AbiChecker(object): raise Exception("Checking out worktree failed, aborting") return git_worktree_path - def update_git_submodules(self, git_worktree_path, crypto_rev): + def update_git_submodules(self, git_worktree_path, crypto_repo, + crypto_rev): process = subprocess.Popen( [self.git_command, "submodule", "update", "--init", '--recursive'], cwd=git_worktree_path, @@ -138,16 +143,30 @@ class AbiChecker(object): raise Exception("git submodule update failed, aborting") if (os.path.exists(os.path.join(git_worktree_path, "crypto")) and crypto_rev): - checkout_process = subprocess.Popen( - [self.git_command, "checkout", crypto_rev], - cwd=os.path.join(git_worktree_path, "crypto"), - stdout=subprocess.PIPE, - stderr=subprocess.STDOUT - ) - checkout_output, _ = checkout_process.communicate() - self.log.info(checkout_output.decode("utf-8")) - if checkout_process.returncode != 0: - raise Exception("git checkout failed, aborting") + if crypto_repo: + shutil.rmtree(os.path.join(git_worktree_path, "crypto")) + clone_process = subprocess.Popen( + [self.git_command, "clone", crypto_repo, + "--branch", crypto_rev, "crypto"], + cwd=git_worktree_path, + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT + ) + clone_output, _ = clone_process.communicate() + self.log.info(clone_output.decode("utf-8")) + if clone_process.returncode != 0: + raise Exception("git clone failed, aborting") + else: + checkout_process = subprocess.Popen( + [self.git_command, "checkout", crypto_rev], + cwd=os.path.join(git_worktree_path, "crypto"), + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT + ) + checkout_output, _ = checkout_process.communicate() + self.log.info(checkout_output.decode("utf-8")) + if checkout_process.returncode != 0: + raise Exception("git checkout failed, aborting") def build_shared_libraries(self, git_worktree_path, version): """Build the shared libraries in the specified worktree.""" @@ -216,12 +235,13 @@ class AbiChecker(object): if worktree_process.returncode != 0: raise Exception("Worktree cleanup failed, aborting") - def get_abi_dump_for_ref(self, remote_repo, git_rev, crypto_rev, version): + def get_abi_dump_for_ref(self, remote_repo, git_rev, crypto_repo, + crypto_rev, version): """Generate the ABI dumps for the specified git revision.""" git_worktree_path = self.get_clean_worktree_for_git_revision( remote_repo, git_rev ) - self.update_git_submodules(git_worktree_path, crypto_rev) + self.update_git_submodules(git_worktree_path, crypto_repo, crypto_rev) self.build_shared_libraries(git_worktree_path, version) abi_dumps = self.get_abi_dumps_from_shared_libraries( git_rev, git_worktree_path, version @@ -320,8 +340,10 @@ class AbiChecker(object): self.check_repo_path() self.check_abi_tools_are_installed() self.old_dumps = self.get_abi_dump_for_ref(self.old_repo, self.old_rev, + self.old_crypto_repo, self.old_crypto_rev, "old") self.new_dumps = self.get_abi_dump_for_ref(self.new_repo, self.new_rev, + self.new_crypto_repo, self.new_crypto_rev, "new") return self.get_abi_compatibility_report() @@ -356,8 +378,9 @@ def run_main(): required=True, nargs="+" ) parser.add_argument( - "-oc", "--old-crypto-rev", type=str, - help="revision for old crypto version", + "-oc", "--old-crypto-rev", type=str, nargs="+", + help=("revision for old crypto version." + "Can include repository before revision"), ) parser.add_argument( "-n", "--new-rev", type=str, @@ -366,8 +389,9 @@ def run_main(): required=True, nargs="+" ) parser.add_argument( - "-nc", "--new-crypto-rev", type=str, - help="revision for new crypto version", + "-nc", "--new-crypto-rev", type=str, nargs="+", + help=("revision for new crypto version" + "Can include repository before revision"), ) parser.add_argument( "-s", "--skip-file", type=str, @@ -394,9 +418,29 @@ def run_main(): new_rev = abi_args.new_rev[1] else: raise Exception("Too many arguments passed for new version") + old_crypto_repo = None + old_crypto_rev = None + if abi_args.old_crypto_rev: + if len(abi_args.old_crypto_rev) == 1: + old_crypto_rev = abi_args.old_crypto_rev[0] + elif len(abi_args.old_crypto_rev) == 2: + old_crypto_repo = abi_args.old_crypto_rev[0] + old_crypto_rev = abi_args.old_crypto_rev[1] + else: + raise Exception("Too many arguments passed for old crypto version") + new_crypto_repo = None + new_crypto_rev = None + if abi_args.new_crypto_rev: + if len(abi_args.new_crypto_rev) == 1: + new_crypto_rev = abi_args.new_crypto_rev[0] + elif len(abi_args.new_crypto_rev) == 2: + new_crypto_repo = abi_args.new_crypto_rev[0] + new_crypto_rev = abi_args.new_crypto_rev[1] + else: + raise Exception("Too many arguments passed for new crypto version") abi_check = AbiChecker( - abi_args.report_dir, old_repo, old_rev, abi_args.old_crypto_rev, - new_repo, new_rev, abi_args.new_crypto_rev, + abi_args.report_dir, old_repo, old_rev, old_crypto_rev, + old_crypto_repo, new_repo, new_rev, new_crypto_rev, new_crypto_repo, abi_args.keep_all_reports, abi_args.brief, abi_args.skip_file ) return_code = abi_check.check_for_abi_changes() From ddf25a6095cf0dd021284db6064dd5f4e9d0a0c6 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Thu, 28 Feb 2019 11:52:39 +0000 Subject: [PATCH 08/22] Only build the library We only need the .so files, so only build the library --- scripts/abi_check.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index 30baadcdb..6c8be0d68 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -175,7 +175,7 @@ class AbiChecker(object): my_environment["SHARED"] = "1" my_environment["USE_CRYPTO_SUBMODULE"] = "1" make_process = subprocess.Popen( - self.make_command, + [self.make_command, "lib"], env=my_environment, cwd=git_worktree_path, stdout=subprocess.PIPE, From c5132ffc41632a34301621ae945b5d48d73b2af3 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Fri, 1 Mar 2019 09:54:44 +0000 Subject: [PATCH 09/22] Use optional arguments for setting repositories --- scripts/abi_check.py | 80 +++++++++++++++----------------------------- 1 file changed, 27 insertions(+), 53 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index 6c8be0d68..12ee44ed2 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -372,26 +372,34 @@ def run_main(): help="keep all reports, even if there are no compatibility issues", ) parser.add_argument( - "-o", "--old-rev", type=str, - help=("revision for old version." - "Can include repository before revision"), - required=True, nargs="+" + "-o", "--old-rev", type=str, help="revision for old version.", + required=True, ) parser.add_argument( - "-oc", "--old-crypto-rev", type=str, nargs="+", - help=("revision for old crypto version." - "Can include repository before revision"), + "-or", "--old-repo", type=str, help="repository for old version." ) parser.add_argument( - "-n", "--new-rev", type=str, - help=("revision for new version" - "Can include repository before revision"), - required=True, nargs="+" + "-oc", "--old-crypto-rev", type=str, + help="revision for old crypto submodule." ) parser.add_argument( - "-nc", "--new-crypto-rev", type=str, nargs="+", - help=("revision for new crypto version" - "Can include repository before revision"), + "-ocr", "--old-crypto-repo", type=str, + help="repository for old crypto submodule." + ) + parser.add_argument( + "-n", "--new-rev", type=str, help="revision for new version", + required=True, + ) + parser.add_argument( + "-nr", "--new-repo", type=str, help="repository for new version." + ) + parser.add_argument( + "-nc", "--new-crypto-rev", type=str, + help="revision for new crypto version" + ) + parser.add_argument( + "-ncr", "--new-crypto-repo", type=str, + help="repository for new crypto submodule." ) parser.add_argument( "-s", "--skip-file", type=str, @@ -402,46 +410,12 @@ def run_main(): help="output only the list of issues to stdout, instead of a full report", ) abi_args = parser.parse_args() - if len(abi_args.old_rev) == 1: - old_repo = None - old_rev = abi_args.old_rev[0] - elif len(abi_args.old_rev) == 2: - old_repo = abi_args.old_rev[0] - old_rev = abi_args.old_rev[1] - else: - raise Exception("Too many arguments passed for old version") - if len(abi_args.new_rev) == 1: - new_repo = None - new_rev = abi_args.new_rev[0] - elif len(abi_args.new_rev) == 2: - new_repo = abi_args.new_rev[0] - new_rev = abi_args.new_rev[1] - else: - raise Exception("Too many arguments passed for new version") - old_crypto_repo = None - old_crypto_rev = None - if abi_args.old_crypto_rev: - if len(abi_args.old_crypto_rev) == 1: - old_crypto_rev = abi_args.old_crypto_rev[0] - elif len(abi_args.old_crypto_rev) == 2: - old_crypto_repo = abi_args.old_crypto_rev[0] - old_crypto_rev = abi_args.old_crypto_rev[1] - else: - raise Exception("Too many arguments passed for old crypto version") - new_crypto_repo = None - new_crypto_rev = None - if abi_args.new_crypto_rev: - if len(abi_args.new_crypto_rev) == 1: - new_crypto_rev = abi_args.new_crypto_rev[0] - elif len(abi_args.new_crypto_rev) == 2: - new_crypto_repo = abi_args.new_crypto_rev[0] - new_crypto_rev = abi_args.new_crypto_rev[1] - else: - raise Exception("Too many arguments passed for new crypto version") abi_check = AbiChecker( - abi_args.report_dir, old_repo, old_rev, old_crypto_rev, - old_crypto_repo, new_repo, new_rev, new_crypto_rev, new_crypto_repo, - abi_args.keep_all_reports, abi_args.brief, abi_args.skip_file + abi_args.report_dir, abi_args.old_repo, abi_args.old_rev, + abi_args.old_crypto_rev, abi_args.old_crypto_repo, + abi_args.new_repo, abi_args.new_rev, abi_args.new_crypto_rev, + abi_args.new_crypto_repo, abi_args.keep_all_reports, + abi_args.brief, abi_args.skip_file ) return_code = abi_check.check_for_abi_changes() sys.exit(return_code) From 4cde8a05133f86d31b8155a54a10ddb7fbfaa32b Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Tue, 5 Mar 2019 15:21:32 +0000 Subject: [PATCH 10/22] Improve documentation --- scripts/abi_check.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index 12ee44ed2..7e6d7fcfd 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -10,7 +10,7 @@ This script is a small wrapper around the abi-compliance-checker and abi-dumper tools, applying them to compare the ABI and API of the library files from two different Git revisions within an Mbed TLS repository. The results of the comparison are either formatted as HTML and stored at -a configurable location, or a brief list of problems found is output. +a configurable location, or are given as a brief list of problems. Returns 0 on success, 1 on ABI/API non-compliance, and 2 if there is an error while running the script. Note: must be run from Mbed TLS root. """ @@ -357,10 +357,10 @@ def run_main(): to compare the ABI and API of the library files from two different Git revisions within an Mbed TLS repository. The results of the comparison are either formatted as HTML and - stored at a configurable location, or a brief list of problems - found is output. Returns 0 on success, 1 on ABI/API - non-compliance, and 2 if there is an error while running the - script. Note: must be run from Mbed TLS root.""" + stored at a configurable location, or are given as a brief list + of problems. Returns 0 on success, 1 on ABI/API non-compliance, + and 2 if there is an error while running the script. + Note: must be run from Mbed TLS root.""" ) ) parser.add_argument( From e29ce70ca5761beb72e7a1c2d7d1f52927db2205 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Tue, 5 Mar 2019 15:23:25 +0000 Subject: [PATCH 11/22] Reduce indentation levels --- scripts/abi_check.py | 52 +++++++++++++++++++++++--------------------- 1 file changed, 27 insertions(+), 25 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index 7e6d7fcfd..bab2fcdac 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -141,32 +141,34 @@ class AbiChecker(object): self.log.info(output.decode("utf-8")) if process.returncode != 0: raise Exception("git submodule update failed, aborting") - if (os.path.exists(os.path.join(git_worktree_path, "crypto")) + if not (os.path.exists(os.path.join(git_worktree_path, "crypto")) and crypto_rev): - if crypto_repo: - shutil.rmtree(os.path.join(git_worktree_path, "crypto")) - clone_process = subprocess.Popen( - [self.git_command, "clone", crypto_repo, - "--branch", crypto_rev, "crypto"], - cwd=git_worktree_path, - stdout=subprocess.PIPE, - stderr=subprocess.STDOUT - ) - clone_output, _ = clone_process.communicate() - self.log.info(clone_output.decode("utf-8")) - if clone_process.returncode != 0: - raise Exception("git clone failed, aborting") - else: - checkout_process = subprocess.Popen( - [self.git_command, "checkout", crypto_rev], - cwd=os.path.join(git_worktree_path, "crypto"), - stdout=subprocess.PIPE, - stderr=subprocess.STDOUT - ) - checkout_output, _ = checkout_process.communicate() - self.log.info(checkout_output.decode("utf-8")) - if checkout_process.returncode != 0: - raise Exception("git checkout failed, aborting") + return + + if crypto_repo: + shutil.rmtree(os.path.join(git_worktree_path, "crypto")) + clone_process = subprocess.Popen( + [self.git_command, "clone", crypto_repo, + "--branch", crypto_rev, "crypto"], + cwd=git_worktree_path, + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT + ) + clone_output, _ = clone_process.communicate() + self.log.info(clone_output.decode("utf-8")) + if clone_process.returncode != 0: + raise Exception("git clone failed, aborting") + else: + checkout_process = subprocess.Popen( + [self.git_command, "checkout", crypto_rev], + cwd=os.path.join(git_worktree_path, "crypto"), + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT + ) + checkout_output, _ = checkout_process.communicate() + self.log.info(checkout_output.decode("utf-8")) + if checkout_process.returncode != 0: + raise Exception("git checkout failed, aborting") def build_shared_libraries(self, git_worktree_path, version): """Build the shared libraries in the specified worktree.""" From 7c1a73370bd6ffb091dbc7cb811ee447f6e176aa Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Tue, 5 Mar 2019 16:25:38 +0000 Subject: [PATCH 12/22] Add RepoVersion class to make handling of many arguments easier There are a number of arguments being passed around, nearly all of which are duplicated between the old and new versions. Moving these into a separate class should hopefully make it simpler to follow what is being done. --- scripts/abi_check.py | 145 +++++++++++++++++++++---------------------- 1 file changed, 70 insertions(+), 75 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index bab2fcdac..cc8667899 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -28,23 +28,37 @@ import fnmatch import xml.etree.ElementTree as ET +class RepoVersion(object): + + def __init__(self, version, repository, revision, + crypto_repository, crypto_revision): + """Class containing details for a particular revision. + + version: either 'old' or 'new' + repository: repository for git revision + revision: git revision for comparison + crypto_repository: repository for git revision of crypto submodule + crypto_revision: git revision of crypto submodule + """ + self.version = version + self.repository = repository + self.revision = revision + self.crypto_repository = crypto_repository + self.crypto_revision = crypto_revision + self.abi_dumps = {} + self.modules = {} + + class AbiChecker(object): """API and ABI checker.""" - def __init__(self, report_dir, old_repo, old_rev, old_crypto_rev, - old_crypto_repo, new_repo, new_rev, new_crypto_rev, - new_crypto_repo, keep_all_reports, brief, skip_file=None): + def __init__(self, old_version, new_version, report_dir, + keep_all_reports, brief, skip_file=None): """Instantiate the API/ABI checker. + old_version: RepoVersion containing details to compare against + new_version: RepoVersion containing details to check report_dir: directory for output files - old_repo: repository for git revision to compare against - old_rev: reference git revision to compare against - old_crypto_rev: reference git revision for old crypto submodule - old_crypto_repo: repository for git revision for old crypto submodule - new_repo: repository for git revision to check - new_rev: git revision to check - new_crypto_rev: reference git revision for new crypto submodule - new_crypto_repo: repository for git revision for new crypto submodule keep_all_reports: if false, delete old reports brief: if true, output shorter report to stdout skip_file: path to file containing symbols and types to skip @@ -56,19 +70,10 @@ class AbiChecker(object): self.keep_all_reports = keep_all_reports self.can_remove_report_dir = not (os.path.isdir(self.report_dir) or keep_all_reports) - self.old_repo = old_repo - self.old_rev = old_rev - self.old_crypto_rev = old_crypto_rev - self.old_crypto_repo = old_crypto_repo - self.new_repo = new_repo - self.new_rev = new_rev - self.new_crypto_rev = new_crypto_rev - self.new_crypto_repo = new_crypto_repo + self.old_version = old_version + self.new_version = new_version self.skip_file = skip_file self.brief = brief - self.mbedtls_modules = {"old": {}, "new": {}} - self.old_dumps = {} - self.new_dumps = {} self.git_command = "git" self.make_command = "make" @@ -90,18 +95,19 @@ class AbiChecker(object): if not shutil.which(command): raise Exception("{} not installed, aborting".format(command)) - def get_clean_worktree_for_git_revision(self, remote_repo, git_rev): - """Make a separate worktree with git_rev checked out. + def get_clean_worktree_for_git_revision(self, version): + """Make a separate worktree with version.revision checked out. Do not modify the current worktree.""" git_worktree_path = tempfile.mkdtemp() - if remote_repo: + if version.repository: self.log.info( "Checking out git worktree for revision {} from {}".format( - git_rev, remote_repo + version.revision, version.repository ) ) fetch_process = subprocess.Popen( - [self.git_command, "fetch", remote_repo, git_rev], + [self.git_command, "fetch", + version.repository, version.revision], cwd=self.repo_path, stdout=subprocess.PIPE, stderr=subprocess.STDOUT @@ -112,10 +118,10 @@ class AbiChecker(object): raise Exception("Fetching revision failed, aborting") worktree_rev = "FETCH_HEAD" else: - self.log.info( - "Checking out git worktree for revision {}".format(git_rev) - ) - worktree_rev = git_rev + self.log.info("Checking out git worktree for revision {}".format( + version.revision + )) + worktree_rev = version.revision worktree_process = subprocess.Popen( [self.git_command, "worktree", "add", "--detach", git_worktree_path, worktree_rev], @@ -129,8 +135,7 @@ class AbiChecker(object): raise Exception("Checking out worktree failed, aborting") return git_worktree_path - def update_git_submodules(self, git_worktree_path, crypto_repo, - crypto_rev): + def update_git_submodules(self, git_worktree_path, version): process = subprocess.Popen( [self.git_command, "submodule", "update", "--init", '--recursive'], cwd=git_worktree_path, @@ -142,14 +147,14 @@ class AbiChecker(object): if process.returncode != 0: raise Exception("git submodule update failed, aborting") if not (os.path.exists(os.path.join(git_worktree_path, "crypto")) - and crypto_rev): + and version.crypto_revision): return - if crypto_repo: + if version.crypto_repository: shutil.rmtree(os.path.join(git_worktree_path, "crypto")) clone_process = subprocess.Popen( - [self.git_command, "clone", crypto_repo, - "--branch", crypto_rev, "crypto"], + [self.git_command, "clone", version.crypto_repository, + "--branch", version.crypto_revision, "crypto"], cwd=git_worktree_path, stdout=subprocess.PIPE, stderr=subprocess.STDOUT @@ -160,7 +165,7 @@ class AbiChecker(object): raise Exception("git clone failed, aborting") else: checkout_process = subprocess.Popen( - [self.git_command, "checkout", crypto_rev], + [self.git_command, "checkout", version.crypto_revision], cwd=os.path.join(git_worktree_path, "crypto"), stdout=subprocess.PIPE, stderr=subprocess.STDOUT @@ -187,29 +192,28 @@ class AbiChecker(object): self.log.info(make_output.decode("utf-8")) for root, dirs, files in os.walk(git_worktree_path): for file in fnmatch.filter(files, "*.so"): - self.mbedtls_modules[version][os.path.splitext(file)[0]] = ( + version.modules[os.path.splitext(file)[0]] = ( os.path.join(root, file) ) if make_process.returncode != 0: raise Exception("make failed, aborting") - def get_abi_dumps_from_shared_libraries(self, git_ref, git_worktree_path, + def get_abi_dumps_from_shared_libraries(self, git_worktree_path, version): """Generate the ABI dumps for the specified git revision. It must be checked out in git_worktree_path and the shared libraries must have been built.""" - abi_dumps = {} - for mbed_module, module_path in self.mbedtls_modules[version].items(): + for mbed_module, module_path in version.modules.items(): output_path = os.path.join( - self.report_dir, version, "{}-{}.dump".format( - mbed_module, git_ref + self.report_dir, version.version, "{}-{}.dump".format( + mbed_module, version.revision ) ) abi_dump_command = [ "abi-dumper", module_path, "-o", output_path, - "-lver", git_ref + "-lver", version.revision ] abi_dump_process = subprocess.Popen( abi_dump_command, @@ -220,8 +224,7 @@ class AbiChecker(object): self.log.info(abi_dump_output.decode("utf-8")) if abi_dump_process.returncode != 0: raise Exception("abi-dumper failed, aborting") - abi_dumps[mbed_module] = output_path - return abi_dumps + version.abi_dumps[mbed_module] = output_path def cleanup_worktree(self, git_worktree_path): """Remove the specified git worktree.""" @@ -237,19 +240,13 @@ class AbiChecker(object): if worktree_process.returncode != 0: raise Exception("Worktree cleanup failed, aborting") - def get_abi_dump_for_ref(self, remote_repo, git_rev, crypto_repo, - crypto_rev, version): + def get_abi_dump_for_ref(self, version): """Generate the ABI dumps for the specified git revision.""" - git_worktree_path = self.get_clean_worktree_for_git_revision( - remote_repo, git_rev - ) - self.update_git_submodules(git_worktree_path, crypto_repo, crypto_rev) + git_worktree_path = self.get_clean_worktree_for_git_revision(version) + self.update_git_submodules(git_worktree_path, version) self.build_shared_libraries(git_worktree_path, version) - abi_dumps = self.get_abi_dumps_from_shared_libraries( - git_rev, git_worktree_path, version - ) + self.get_abi_dumps_from_shared_libraries(git_worktree_path, version) self.cleanup_worktree(git_worktree_path) - return abi_dumps def remove_children_with_tag(self, parent, tag): children = parent.getchildren() @@ -275,19 +272,20 @@ class AbiChecker(object): be available.""" compatibility_report = "" compliance_return_code = 0 - shared_modules = list(set(self.mbedtls_modules["old"].keys()) & - set(self.mbedtls_modules["new"].keys())) + shared_modules = list(set(self.old_version.modules.keys()) & + set(self.new_version.modules.keys())) for mbed_module in shared_modules: output_path = os.path.join( self.report_dir, "{}-{}-{}.html".format( - mbed_module, self.old_rev, self.new_rev + mbed_module, self.old_version.revision, + self.new_version.revision ) ) abi_compliance_command = [ "abi-compliance-checker", "-l", mbed_module, - "-old", self.old_dumps[mbed_module], - "-new", self.new_dumps[mbed_module], + "-old", self.old_version.abi_dumps[mbed_module], + "-new", self.new_version.abi_dumps[mbed_module], "-strict", "-report-path", output_path, ] @@ -329,8 +327,8 @@ class AbiChecker(object): "abi-compliance-checker failed with a return code of {}," " aborting".format(abi_compliance_process.returncode) ) - os.remove(self.old_dumps[mbed_module]) - os.remove(self.new_dumps[mbed_module]) + os.remove(self.old_version.abi_dumps[mbed_module]) + os.remove(self.new_version.abi_dumps[mbed_module]) if self.can_remove_report_dir: os.rmdir(self.report_dir) self.log.info(compatibility_report) @@ -341,12 +339,8 @@ class AbiChecker(object): between self.old_rev and self.new_rev.""" self.check_repo_path() self.check_abi_tools_are_installed() - self.old_dumps = self.get_abi_dump_for_ref(self.old_repo, self.old_rev, - self.old_crypto_repo, - self.old_crypto_rev, "old") - self.new_dumps = self.get_abi_dump_for_ref(self.new_repo, self.new_rev, - self.new_crypto_repo, - self.new_crypto_rev, "new") + self.get_abi_dump_for_ref(self.old_version) + self.get_abi_dump_for_ref(self.new_version) return self.get_abi_compatibility_report() @@ -412,12 +406,13 @@ def run_main(): help="output only the list of issues to stdout, instead of a full report", ) abi_args = parser.parse_args() + old_version = RepoVersion("old", abi_args.old_repo, abi_args.old_rev, + abi_args.old_crypto_repo, abi_args.old_crypto_rev) + new_version = RepoVersion("new", abi_args.new_repo, abi_args.new_rev, + abi_args.new_crypto_repo, abi_args.new_crypto_rev) abi_check = AbiChecker( - abi_args.report_dir, abi_args.old_repo, abi_args.old_rev, - abi_args.old_crypto_rev, abi_args.old_crypto_repo, - abi_args.new_repo, abi_args.new_rev, abi_args.new_crypto_rev, - abi_args.new_crypto_repo, abi_args.keep_all_reports, - abi_args.brief, abi_args.skip_file + old_version, new_version, abi_args.report_dir, + abi_args.keep_all_reports, abi_args.brief, abi_args.skip_file ) return_code = abi_check.check_for_abi_changes() sys.exit(return_code) From 3a5f6c83bc25f7b96b8338319d0ca373934b7a81 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Tue, 5 Mar 2019 16:30:39 +0000 Subject: [PATCH 13/22] Prefix internal functions with underscore --- scripts/abi_check.py | 42 +++++++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index cc8667899..193169154 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -65,7 +65,7 @@ class AbiChecker(object): """ self.repo_path = "." self.log = None - self.setup_logger() + self._setup_logger() self.report_dir = os.path.abspath(report_dir) self.keep_all_reports = keep_all_reports self.can_remove_report_dir = not (os.path.isdir(self.report_dir) or @@ -84,7 +84,7 @@ class AbiChecker(object): if current_dir != root_dir: raise Exception("Must be run from Mbed TLS root") - def setup_logger(self): + def _setup_logger(self): self.log = logging.getLogger() self.log.setLevel(logging.INFO) self.log.addHandler(logging.StreamHandler()) @@ -95,7 +95,7 @@ class AbiChecker(object): if not shutil.which(command): raise Exception("{} not installed, aborting".format(command)) - def get_clean_worktree_for_git_revision(self, version): + def _get_clean_worktree_for_git_revision(self, version): """Make a separate worktree with version.revision checked out. Do not modify the current worktree.""" git_worktree_path = tempfile.mkdtemp() @@ -135,7 +135,7 @@ class AbiChecker(object): raise Exception("Checking out worktree failed, aborting") return git_worktree_path - def update_git_submodules(self, git_worktree_path, version): + def _update_git_submodules(self, git_worktree_path, version): process = subprocess.Popen( [self.git_command, "submodule", "update", "--init", '--recursive'], cwd=git_worktree_path, @@ -175,7 +175,7 @@ class AbiChecker(object): if checkout_process.returncode != 0: raise Exception("git checkout failed, aborting") - def build_shared_libraries(self, git_worktree_path, version): + def _build_shared_libraries(self, git_worktree_path, version): """Build the shared libraries in the specified worktree.""" my_environment = os.environ.copy() my_environment["CFLAGS"] = "-g -Og" @@ -198,8 +198,8 @@ class AbiChecker(object): if make_process.returncode != 0: raise Exception("make failed, aborting") - def get_abi_dumps_from_shared_libraries(self, git_worktree_path, - version): + def _get_abi_dumps_from_shared_libraries(self, git_worktree_path, + version): """Generate the ABI dumps for the specified git revision. It must be checked out in git_worktree_path and the shared libraries must have been built.""" @@ -226,7 +226,7 @@ class AbiChecker(object): raise Exception("abi-dumper failed, aborting") version.abi_dumps[mbed_module] = output_path - def cleanup_worktree(self, git_worktree_path): + def _cleanup_worktree(self, git_worktree_path): """Remove the specified git worktree.""" shutil.rmtree(git_worktree_path) worktree_process = subprocess.Popen( @@ -240,26 +240,26 @@ class AbiChecker(object): if worktree_process.returncode != 0: raise Exception("Worktree cleanup failed, aborting") - def get_abi_dump_for_ref(self, version): + def _get_abi_dump_for_ref(self, version): """Generate the ABI dumps for the specified git revision.""" - git_worktree_path = self.get_clean_worktree_for_git_revision(version) - self.update_git_submodules(git_worktree_path, version) - self.build_shared_libraries(git_worktree_path, version) - self.get_abi_dumps_from_shared_libraries(git_worktree_path, version) - self.cleanup_worktree(git_worktree_path) + git_worktree_path = self._get_clean_worktree_for_git_revision(version) + self._update_git_submodules(git_worktree_path, version) + self._build_shared_libraries(git_worktree_path, version) + self._get_abi_dumps_from_shared_libraries(git_worktree_path, version) + self._cleanup_worktree(git_worktree_path) - def remove_children_with_tag(self, parent, tag): + def _remove_children_with_tag(self, parent, tag): children = parent.getchildren() for child in children: if child.tag == tag: parent.remove(child) else: - self.remove_children_with_tag(child, tag) + self._remove_children_with_tag(child, tag) - def remove_extra_detail_from_report(self, report_root): + def _remove_extra_detail_from_report(self, report_root): for tag in ['test_info', 'test_results', 'problem_summary', 'added_symbols', 'removed_symbols', 'affected']: - self.remove_children_with_tag(report_root, tag) + self._remove_children_with_tag(report_root, tag) for report in report_root: for problems in report.getchildren()[:]: @@ -313,7 +313,7 @@ class AbiChecker(object): "Compatibility issues found for {}".format(mbed_module) ) report_root = ET.fromstring(abi_compliance_output.decode("utf-8")) - self.remove_extra_detail_from_report(report_root) + self._remove_extra_detail_from_report(report_root) self.log.info(ET.tostring(report_root).decode("utf-8")) else: compliance_return_code = 1 @@ -339,8 +339,8 @@ class AbiChecker(object): between self.old_rev and self.new_rev.""" self.check_repo_path() self.check_abi_tools_are_installed() - self.get_abi_dump_for_ref(self.old_version) - self.get_abi_dump_for_ref(self.new_version) + self._get_abi_dump_for_ref(self.old_version) + self._get_abi_dump_for_ref(self.new_version) return self.get_abi_compatibility_report() From 1d95c539e981df548648f91b4818d5993e47f4b1 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Fri, 8 Mar 2019 11:12:19 +0000 Subject: [PATCH 14/22] Fetch the remote crypto branch, rather than cloning it --- scripts/abi_check.py | 40 +++++++++++++++++++++------------------- 1 file changed, 21 insertions(+), 19 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index 193169154..d23b038ea 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -151,29 +151,31 @@ class AbiChecker(object): return if version.crypto_repository: - shutil.rmtree(os.path.join(git_worktree_path, "crypto")) - clone_process = subprocess.Popen( - [self.git_command, "clone", version.crypto_repository, - "--branch", version.crypto_revision, "crypto"], - cwd=git_worktree_path, - stdout=subprocess.PIPE, - stderr=subprocess.STDOUT - ) - clone_output, _ = clone_process.communicate() - self.log.info(clone_output.decode("utf-8")) - if clone_process.returncode != 0: - raise Exception("git clone failed, aborting") - else: - checkout_process = subprocess.Popen( - [self.git_command, "checkout", version.crypto_revision], + fetch_process = subprocess.Popen( + [self.git_command, "fetch", version.crypto_repository, + version.crypto_revision], cwd=os.path.join(git_worktree_path, "crypto"), stdout=subprocess.PIPE, stderr=subprocess.STDOUT ) - checkout_output, _ = checkout_process.communicate() - self.log.info(checkout_output.decode("utf-8")) - if checkout_process.returncode != 0: - raise Exception("git checkout failed, aborting") + fetch_output, _ = fetch_process.communicate() + self.log.info(fetch_output.decode("utf-8")) + if fetch_process.returncode != 0: + raise Exception("git fetch failed, aborting") + crypto_rev = "FETCH_HEAD" + else: + crypto_rev = version.crypto_revision + + checkout_process = subprocess.Popen( + [self.git_command, "checkout", crypto_rev], + cwd=os.path.join(git_worktree_path, "crypto"), + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT + ) + checkout_output, _ = checkout_process.communicate() + self.log.info(checkout_output.decode("utf-8")) + if checkout_process.returncode != 0: + raise Exception("git checkout failed, aborting") def _build_shared_libraries(self, git_worktree_path, version): """Build the shared libraries in the specified worktree.""" From 3c3da790d21c88645ca80800eb8e57532cf78011 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Fri, 8 Mar 2019 11:30:04 +0000 Subject: [PATCH 15/22] Add verbose switch to silence all output except the final report --- scripts/abi_check.py | 34 +++++++++++++++++++++------------- 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index d23b038ea..a25c85fc1 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -52,7 +52,7 @@ class RepoVersion(object): class AbiChecker(object): """API and ABI checker.""" - def __init__(self, old_version, new_version, report_dir, + def __init__(self, verbose, old_version, new_version, report_dir, keep_all_reports, brief, skip_file=None): """Instantiate the API/ABI checker. @@ -65,6 +65,7 @@ class AbiChecker(object): """ self.repo_path = "." self.log = None + self.verbose = verbose self._setup_logger() self.report_dir = os.path.abspath(report_dir) self.keep_all_reports = keep_all_reports @@ -86,7 +87,10 @@ class AbiChecker(object): def _setup_logger(self): self.log = logging.getLogger() - self.log.setLevel(logging.INFO) + if self.verbose: + self.log.setLevel(logging.DEBUG) + else: + self.log.setLevel(logging.INFO) self.log.addHandler(logging.StreamHandler()) @staticmethod @@ -100,7 +104,7 @@ class AbiChecker(object): Do not modify the current worktree.""" git_worktree_path = tempfile.mkdtemp() if version.repository: - self.log.info( + self.log.debug( "Checking out git worktree for revision {} from {}".format( version.revision, version.repository ) @@ -113,12 +117,12 @@ class AbiChecker(object): stderr=subprocess.STDOUT ) fetch_output, _ = fetch_process.communicate() - self.log.info(fetch_output.decode("utf-8")) + self.log.debug(fetch_output.decode("utf-8")) if fetch_process.returncode != 0: raise Exception("Fetching revision failed, aborting") worktree_rev = "FETCH_HEAD" else: - self.log.info("Checking out git worktree for revision {}".format( + self.log.debug("Checking out git worktree for revision {}".format( version.revision )) worktree_rev = version.revision @@ -130,7 +134,7 @@ class AbiChecker(object): stderr=subprocess.STDOUT ) worktree_output, _ = worktree_process.communicate() - self.log.info(worktree_output.decode("utf-8")) + self.log.debug(worktree_output.decode("utf-8")) if worktree_process.returncode != 0: raise Exception("Checking out worktree failed, aborting") return git_worktree_path @@ -143,7 +147,7 @@ class AbiChecker(object): stderr=subprocess.STDOUT ) output, _ = process.communicate() - self.log.info(output.decode("utf-8")) + self.log.debug(output.decode("utf-8")) if process.returncode != 0: raise Exception("git submodule update failed, aborting") if not (os.path.exists(os.path.join(git_worktree_path, "crypto")) @@ -159,7 +163,7 @@ class AbiChecker(object): stderr=subprocess.STDOUT ) fetch_output, _ = fetch_process.communicate() - self.log.info(fetch_output.decode("utf-8")) + self.log.debug(fetch_output.decode("utf-8")) if fetch_process.returncode != 0: raise Exception("git fetch failed, aborting") crypto_rev = "FETCH_HEAD" @@ -173,7 +177,7 @@ class AbiChecker(object): stderr=subprocess.STDOUT ) checkout_output, _ = checkout_process.communicate() - self.log.info(checkout_output.decode("utf-8")) + self.log.debug(checkout_output.decode("utf-8")) if checkout_process.returncode != 0: raise Exception("git checkout failed, aborting") @@ -191,7 +195,7 @@ class AbiChecker(object): stderr=subprocess.STDOUT ) make_output, _ = make_process.communicate() - self.log.info(make_output.decode("utf-8")) + self.log.debug(make_output.decode("utf-8")) for root, dirs, files in os.walk(git_worktree_path): for file in fnmatch.filter(files, "*.so"): version.modules[os.path.splitext(file)[0]] = ( @@ -223,7 +227,7 @@ class AbiChecker(object): stderr=subprocess.STDOUT ) abi_dump_output, _ = abi_dump_process.communicate() - self.log.info(abi_dump_output.decode("utf-8")) + self.log.debug(abi_dump_output.decode("utf-8")) if abi_dump_process.returncode != 0: raise Exception("abi-dumper failed, aborting") version.abi_dumps[mbed_module] = output_path @@ -238,7 +242,7 @@ class AbiChecker(object): stderr=subprocess.STDOUT ) worktree_output, _ = worktree_process.communicate() - self.log.info(worktree_output.decode("utf-8")) + self.log.debug(worktree_output.decode("utf-8")) if worktree_process.returncode != 0: raise Exception("Worktree cleanup failed, aborting") @@ -361,6 +365,10 @@ def run_main(): Note: must be run from Mbed TLS root.""" ) ) + parser.add_argument( + "-v", "--verbose", action="store_true", + help="set verbosity level", + ) parser.add_argument( "-r", "--report-dir", type=str, default="reports", help="directory where reports are stored, default is reports", @@ -413,7 +421,7 @@ def run_main(): new_version = RepoVersion("new", abi_args.new_repo, abi_args.new_rev, abi_args.new_crypto_repo, abi_args.new_crypto_rev) abi_check = AbiChecker( - old_version, new_version, abi_args.report_dir, + abi_args.verbose, old_version, new_version, abi_args.report_dir, abi_args.keep_all_reports, abi_args.brief, abi_args.skip_file ) return_code = abi_check.check_for_abi_changes() From fe9a67510e0f2ab97e7050c496ef77d97e57c0d8 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Thu, 4 Apr 2019 14:39:33 +0100 Subject: [PATCH 16/22] Don't put abi dumps in subfolders --- scripts/abi_check.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index a25c85fc1..a5ef7e648 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -211,8 +211,8 @@ class AbiChecker(object): must have been built.""" for mbed_module, module_path in version.modules.items(): output_path = os.path.join( - self.report_dir, version.version, "{}-{}.dump".format( - mbed_module, version.revision + self.report_dir, "{}-{}-{}.dump".format( + mbed_module, version.revision, version.version ) ) abi_dump_command = [ From 8184df5de98200c88cefabaf395603020128dc27 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Fri, 5 Apr 2019 17:06:17 +0100 Subject: [PATCH 17/22] Fix pylint issues --- scripts/abi_check.py | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index a5ef7e648..9f99309af 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -140,6 +140,9 @@ class AbiChecker(object): return git_worktree_path def _update_git_submodules(self, git_worktree_path, version): + """If the crypto submodule is present, initialize it. + if version.crypto_revision exists, update it to that revision, + otherwise update it to the default revision""" process = subprocess.Popen( [self.git_command, "submodule", "update", "--init", '--recursive'], cwd=git_worktree_path, @@ -196,7 +199,7 @@ class AbiChecker(object): ) make_output, _ = make_process.communicate() self.log.debug(make_output.decode("utf-8")) - for root, dirs, files in os.walk(git_worktree_path): + for root, dirs, files in os.walk(git_worktree_path): # pylint: disable=unused-variable for file in fnmatch.filter(files, "*.so"): version.modules[os.path.splitext(file)[0]] = ( os.path.join(root, file) @@ -204,11 +207,10 @@ class AbiChecker(object): if make_process.returncode != 0: raise Exception("make failed, aborting") - def _get_abi_dumps_from_shared_libraries(self, git_worktree_path, - version): + def _get_abi_dumps_from_shared_libraries(self, version): """Generate the ABI dumps for the specified git revision. - It must be checked out in git_worktree_path and the shared libraries - must have been built.""" + The shared libraries must have been built and the module paths + present in version.modules.""" for mbed_module, module_path in version.modules.items(): output_path = os.path.join( self.report_dir, "{}-{}-{}.dump".format( @@ -251,7 +253,7 @@ class AbiChecker(object): git_worktree_path = self._get_clean_worktree_for_git_revision(version) self._update_git_submodules(git_worktree_path, version) self._build_shared_libraries(git_worktree_path, version) - self._get_abi_dumps_from_shared_libraries(git_worktree_path, version) + self._get_abi_dumps_from_shared_libraries(version) self._cleanup_worktree(git_worktree_path) def _remove_children_with_tag(self, parent, tag): @@ -264,7 +266,7 @@ class AbiChecker(object): def _remove_extra_detail_from_report(self, report_root): for tag in ['test_info', 'test_results', 'problem_summary', - 'added_symbols', 'removed_symbols', 'affected']: + 'added_symbols', 'removed_symbols', 'affected']: self._remove_children_with_tag(report_root, tag) for report in report_root: @@ -274,8 +276,8 @@ class AbiChecker(object): def get_abi_compatibility_report(self): """Generate a report of the differences between the reference ABI - and the new ABI. ABI dumps from self.old_rev and self.new_rev must - be available.""" + and the new ABI. ABI dumps from self.old_version and self.new_version + must be available.""" compatibility_report = "" compliance_return_code = 0 shared_modules = list(set(self.old_version.modules.keys()) & @@ -416,10 +418,14 @@ def run_main(): help="output only the list of issues to stdout, instead of a full report", ) abi_args = parser.parse_args() - old_version = RepoVersion("old", abi_args.old_repo, abi_args.old_rev, - abi_args.old_crypto_repo, abi_args.old_crypto_rev) - new_version = RepoVersion("new", abi_args.new_repo, abi_args.new_rev, - abi_args.new_crypto_repo, abi_args.new_crypto_rev) + old_version = RepoVersion( + "old", abi_args.old_repo, abi_args.old_rev, + abi_args.old_crypto_repo, abi_args.old_crypto_rev + ) + new_version = RepoVersion( + "new", abi_args.new_repo, abi_args.new_rev, + abi_args.new_crypto_repo, abi_args.new_crypto_rev + ) abi_check = AbiChecker( abi_args.verbose, old_version, new_version, abi_args.report_dir, abi_args.keep_all_reports, abi_args.brief, abi_args.skip_file From 0d1ca5110796e22ab22ebf7c7e23112c376d9940 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Tue, 9 Apr 2019 09:14:17 +0100 Subject: [PATCH 18/22] Use namespaces instead of full classes --- scripts/abi_check.py | 69 ++++++++++++++++++++------------------------ 1 file changed, 31 insertions(+), 38 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index 9f99309af..5a92a7d40 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -24,36 +24,15 @@ import argparse import logging import tempfile import fnmatch +from types import SimpleNamespace import xml.etree.ElementTree as ET -class RepoVersion(object): - - def __init__(self, version, repository, revision, - crypto_repository, crypto_revision): - """Class containing details for a particular revision. - - version: either 'old' or 'new' - repository: repository for git revision - revision: git revision for comparison - crypto_repository: repository for git revision of crypto submodule - crypto_revision: git revision of crypto submodule - """ - self.version = version - self.repository = repository - self.revision = revision - self.crypto_repository = crypto_repository - self.crypto_revision = crypto_revision - self.abi_dumps = {} - self.modules = {} - - class AbiChecker(object): """API and ABI checker.""" - def __init__(self, verbose, old_version, new_version, report_dir, - keep_all_reports, brief, skip_file=None): + def __init__(self, old_version, new_version, configuration): """Instantiate the API/ABI checker. old_version: RepoVersion containing details to compare against @@ -65,16 +44,16 @@ class AbiChecker(object): """ self.repo_path = "." self.log = None - self.verbose = verbose + self.verbose = configuration.verbose self._setup_logger() - self.report_dir = os.path.abspath(report_dir) - self.keep_all_reports = keep_all_reports + self.report_dir = os.path.abspath(configuration.report_dir) + self.keep_all_reports = configuration.keep_all_reports self.can_remove_report_dir = not (os.path.isdir(self.report_dir) or - keep_all_reports) + self.keep_all_reports) self.old_version = old_version self.new_version = new_version - self.skip_file = skip_file - self.brief = brief + self.skip_file = configuration.skip_file + self.brief = configuration.brief self.git_command = "git" self.make_command = "make" @@ -418,18 +397,32 @@ def run_main(): help="output only the list of issues to stdout, instead of a full report", ) abi_args = parser.parse_args() - old_version = RepoVersion( - "old", abi_args.old_repo, abi_args.old_rev, - abi_args.old_crypto_repo, abi_args.old_crypto_rev + old_version = SimpleNamespace( + version="old", + repository=abi_args.old_repo, + revision=abi_args.old_rev, + crypto_repository=abi_args.old_crypto_repo, + crypto_revision=abi_args.old_crypto_rev, + abi_dumps={}, + modules={} ) - new_version = RepoVersion( - "new", abi_args.new_repo, abi_args.new_rev, - abi_args.new_crypto_repo, abi_args.new_crypto_rev + new_version = SimpleNamespace( + version="new", + repository=abi_args.new_repo, + revision=abi_args.new_rev, + crypto_repository=abi_args.new_crypto_repo, + crypto_revision=abi_args.new_crypto_rev, + abi_dumps={}, + modules={} ) - abi_check = AbiChecker( - abi_args.verbose, old_version, new_version, abi_args.report_dir, - abi_args.keep_all_reports, abi_args.brief, abi_args.skip_file + configuration = SimpleNamespace( + verbose=abi_args.verbose, + report_dir=abi_args.report_dir, + keep_all_reports=abi_args.keep_all_reports, + brief=abi_args.brief, + skip_file=abi_args.skip_file ) + abi_check = AbiChecker(old_version, new_version, configuration) return_code = abi_check.check_for_abi_changes() sys.exit(return_code) except Exception: # pylint: disable=broad-except From 492bc402a3d47012216e0fa5cf51564cb693c30e Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Thu, 11 Apr 2019 15:50:41 +0100 Subject: [PATCH 19/22] Check that the report directory is a directory --- scripts/abi_check.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index 5a92a7d40..e6253e99b 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -48,7 +48,7 @@ class AbiChecker(object): self._setup_logger() self.report_dir = os.path.abspath(configuration.report_dir) self.keep_all_reports = configuration.keep_all_reports - self.can_remove_report_dir = not (os.path.isdir(self.report_dir) or + self.can_remove_report_dir = not (os.path.exists(self.report_dir) or self.keep_all_reports) self.old_version = old_version self.new_version = new_version @@ -397,6 +397,9 @@ def run_main(): help="output only the list of issues to stdout, instead of a full report", ) abi_args = parser.parse_args() + if os.path.isfile(abi_args.report_dir): + print("Error: {} is not a directory".format(abi_args.report_dir)) + parser.exit() old_version = SimpleNamespace( version="old", repository=abi_args.old_repo, From f67e3498631582d00e6ecf07c27281045ff926a7 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Fri, 12 Apr 2019 15:17:02 +0100 Subject: [PATCH 20/22] Correct documentation --- scripts/abi_check.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index e6253e99b..0bbb9f872 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -37,10 +37,10 @@ class AbiChecker(object): old_version: RepoVersion containing details to compare against new_version: RepoVersion containing details to check - report_dir: directory for output files - keep_all_reports: if false, delete old reports - brief: if true, output shorter report to stdout - skip_file: path to file containing symbols and types to skip + configuration.report_dir: directory for output files + configuration.keep_all_reports: if false, delete old reports + configuration.brief: if true, output shorter report to stdout + configuration.skip_file: path to file containing symbols and types to skip """ self.repo_path = "." self.log = None From f025d5395ed3cdd76cac7e6ac0a27b9742783a27 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Fri, 12 Apr 2019 15:18:02 +0100 Subject: [PATCH 21/22] Start unused variable with underscore --- scripts/abi_check.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index 0bbb9f872..af293cd2a 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -178,7 +178,7 @@ class AbiChecker(object): ) make_output, _ = make_process.communicate() self.log.debug(make_output.decode("utf-8")) - for root, dirs, files in os.walk(git_worktree_path): # pylint: disable=unused-variable + for root, _dirs, files in os.walk(git_worktree_path): for file in fnmatch.filter(files, "*.so"): version.modules[os.path.splitext(file)[0]] = ( os.path.join(root, file) From b2ee0b87826aa197da14e796f3305a81643f1b14 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Fri, 12 Apr 2019 16:24:25 +0100 Subject: [PATCH 22/22] Use check_output instead of Popen --- scripts/abi_check.py | 101 ++++++++++++++----------------------------- 1 file changed, 33 insertions(+), 68 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index af293cd2a..f837f7a79 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -88,80 +88,60 @@ class AbiChecker(object): version.revision, version.repository ) ) - fetch_process = subprocess.Popen( + fetch_output = subprocess.check_output( [self.git_command, "fetch", version.repository, version.revision], cwd=self.repo_path, - stdout=subprocess.PIPE, stderr=subprocess.STDOUT ) - fetch_output, _ = fetch_process.communicate() self.log.debug(fetch_output.decode("utf-8")) - if fetch_process.returncode != 0: - raise Exception("Fetching revision failed, aborting") worktree_rev = "FETCH_HEAD" else: self.log.debug("Checking out git worktree for revision {}".format( version.revision )) worktree_rev = version.revision - worktree_process = subprocess.Popen( + worktree_output = subprocess.check_output( [self.git_command, "worktree", "add", "--detach", git_worktree_path, worktree_rev], cwd=self.repo_path, - stdout=subprocess.PIPE, stderr=subprocess.STDOUT ) - worktree_output, _ = worktree_process.communicate() self.log.debug(worktree_output.decode("utf-8")) - if worktree_process.returncode != 0: - raise Exception("Checking out worktree failed, aborting") return git_worktree_path def _update_git_submodules(self, git_worktree_path, version): """If the crypto submodule is present, initialize it. if version.crypto_revision exists, update it to that revision, otherwise update it to the default revision""" - process = subprocess.Popen( + update_output = subprocess.check_output( [self.git_command, "submodule", "update", "--init", '--recursive'], cwd=git_worktree_path, - stdout=subprocess.PIPE, stderr=subprocess.STDOUT ) - output, _ = process.communicate() - self.log.debug(output.decode("utf-8")) - if process.returncode != 0: - raise Exception("git submodule update failed, aborting") + self.log.debug(update_output.decode("utf-8")) if not (os.path.exists(os.path.join(git_worktree_path, "crypto")) and version.crypto_revision): return if version.crypto_repository: - fetch_process = subprocess.Popen( + fetch_output = subprocess.check_output( [self.git_command, "fetch", version.crypto_repository, version.crypto_revision], cwd=os.path.join(git_worktree_path, "crypto"), - stdout=subprocess.PIPE, stderr=subprocess.STDOUT ) - fetch_output, _ = fetch_process.communicate() self.log.debug(fetch_output.decode("utf-8")) - if fetch_process.returncode != 0: - raise Exception("git fetch failed, aborting") crypto_rev = "FETCH_HEAD" else: crypto_rev = version.crypto_revision - checkout_process = subprocess.Popen( + checkout_output = subprocess.check_output( [self.git_command, "checkout", crypto_rev], cwd=os.path.join(git_worktree_path, "crypto"), - stdout=subprocess.PIPE, stderr=subprocess.STDOUT ) - checkout_output, _ = checkout_process.communicate() self.log.debug(checkout_output.decode("utf-8")) - if checkout_process.returncode != 0: - raise Exception("git checkout failed, aborting") def _build_shared_libraries(self, git_worktree_path, version): """Build the shared libraries in the specified worktree.""" @@ -169,22 +149,18 @@ class AbiChecker(object): my_environment["CFLAGS"] = "-g -Og" my_environment["SHARED"] = "1" my_environment["USE_CRYPTO_SUBMODULE"] = "1" - make_process = subprocess.Popen( + make_output = subprocess.check_output( [self.make_command, "lib"], env=my_environment, cwd=git_worktree_path, - stdout=subprocess.PIPE, stderr=subprocess.STDOUT ) - make_output, _ = make_process.communicate() self.log.debug(make_output.decode("utf-8")) for root, _dirs, files in os.walk(git_worktree_path): for file in fnmatch.filter(files, "*.so"): version.modules[os.path.splitext(file)[0]] = ( os.path.join(root, file) ) - if make_process.returncode != 0: - raise Exception("make failed, aborting") def _get_abi_dumps_from_shared_libraries(self, version): """Generate the ABI dumps for the specified git revision. @@ -202,30 +178,22 @@ class AbiChecker(object): "-o", output_path, "-lver", version.revision ] - abi_dump_process = subprocess.Popen( + abi_dump_output = subprocess.check_output( abi_dump_command, - stdout=subprocess.PIPE, stderr=subprocess.STDOUT ) - abi_dump_output, _ = abi_dump_process.communicate() self.log.debug(abi_dump_output.decode("utf-8")) - if abi_dump_process.returncode != 0: - raise Exception("abi-dumper failed, aborting") version.abi_dumps[mbed_module] = output_path def _cleanup_worktree(self, git_worktree_path): """Remove the specified git worktree.""" shutil.rmtree(git_worktree_path) - worktree_process = subprocess.Popen( + worktree_output = subprocess.check_output( [self.git_command, "worktree", "prune"], cwd=self.repo_path, - stdout=subprocess.PIPE, stderr=subprocess.STDOUT ) - worktree_output, _ = worktree_process.communicate() self.log.debug(worktree_output.decode("utf-8")) - if worktree_process.returncode != 0: - raise Exception("Worktree cleanup failed, aborting") def _get_abi_dump_for_ref(self, version): """Generate the ABI dumps for the specified git revision.""" @@ -282,38 +250,35 @@ class AbiChecker(object): if self.brief: abi_compliance_command += ["-report-format", "xml", "-stdout"] - abi_compliance_process = subprocess.Popen( - abi_compliance_command, - stdout=subprocess.PIPE, - stderr=subprocess.STDOUT - ) - abi_compliance_output, _ = abi_compliance_process.communicate() - if abi_compliance_process.returncode == 0: + try: + subprocess.check_output( + abi_compliance_command, + stderr=subprocess.STDOUT + ) + except subprocess.CalledProcessError as err: + if err.returncode == 1: + compliance_return_code = 1 + if self.brief: + self.log.info( + "Compatibility issues found for {}".format(mbed_module) + ) + report_root = ET.fromstring(err.output.decode("utf-8")) + self._remove_extra_detail_from_report(report_root) + self.log.info(ET.tostring(report_root).decode("utf-8")) + else: + self.can_remove_report_dir = False + compatibility_report += ( + "Compatibility issues found for {}, " + "for details see {}\n".format(mbed_module, output_path) + ) + else: + raise err + else: compatibility_report += ( "No compatibility issues for {}\n".format(mbed_module) ) if not (self.keep_all_reports or self.brief): os.remove(output_path) - elif abi_compliance_process.returncode == 1: - if self.brief: - self.log.info( - "Compatibility issues found for {}".format(mbed_module) - ) - report_root = ET.fromstring(abi_compliance_output.decode("utf-8")) - self._remove_extra_detail_from_report(report_root) - self.log.info(ET.tostring(report_root).decode("utf-8")) - else: - compliance_return_code = 1 - self.can_remove_report_dir = False - compatibility_report += ( - "Compatibility issues found for {}, " - "for details see {}\n".format(mbed_module, output_path) - ) - else: - raise Exception( - "abi-compliance-checker failed with a return code of {}," - " aborting".format(abi_compliance_process.returncode) - ) os.remove(self.old_version.abi_dumps[mbed_module]) os.remove(self.new_version.abi_dumps[mbed_module]) if self.can_remove_report_dir: