From 16dfdb3539ad5bae7d6f53874f7d1c66c0eca5e8 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 4 Jul 2019 18:59:36 +0200 Subject: [PATCH 1/4] Allow running /somewhere/else/path/to/abi_check.py Don't require abi_check.py to be the one in scripts/ under the current directory. --- scripts/abi_check.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index 502c7ae02..6edc54fde 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -59,9 +59,7 @@ class AbiChecker(object): @staticmethod def check_repo_path(): - current_dir = os.path.realpath('.') - root_dir = os.path.dirname(os.path.dirname(os.path.realpath(__file__))) - if current_dir != root_dir: + if not all(os.path.isdir(d) for d in ["include", "library", "tests"]): raise Exception("Must be run from Mbed TLS root") def _setup_logger(self): From cc02658d27a56f7c65d86e69a46b259f303283d8 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 4 Jul 2019 19:00:31 +0200 Subject: [PATCH 2/4] Document how to build the typical argument for -s --- scripts/abi_check.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index 6edc54fde..e8ab1cb79 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -355,7 +355,9 @@ def run_main(): ) parser.add_argument( "-s", "--skip-file", type=str, - help="path to file containing symbols and types to skip" + help=("path to file containing symbols and types to skip " + "(typically \"-s identifiers\" after running " + "\"tests/scripts/list-identifiers.sh --internal\")") ) parser.add_argument( "-b", "--brief", action="store_true", From 2b3f1346ef51eb99337584fdf9cd1dcd1b60de3c Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 4 Jul 2019 19:01:22 +0200 Subject: [PATCH 3/4] Record the commits that were compared Record the commit ID in addition to the symbolic name of the version being tested. This makes it easier to figure out what has been compared when reading logs that don't always indicate explicitly what things like HEAD are. This makes the title of HTML reports somewhat verbose, but I think that's a small price to pay. --- scripts/abi_check.py | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index e8ab1cb79..6aef9924f 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -106,6 +106,12 @@ class AbiChecker(object): stderr=subprocess.STDOUT ) self.log.debug(worktree_output.decode("utf-8")) + version.commit = subprocess.check_output( + [self.git_command, "rev-parse", worktree_rev], + cwd=git_worktree_path, + stderr=subprocess.STDOUT + ).decode("ascii").rstrip() + self.log.debug("Commit is {}".format(version.commit)) return git_worktree_path def _update_git_submodules(self, git_worktree_path, version): @@ -161,6 +167,13 @@ class AbiChecker(object): os.path.join(root, file) ) + @staticmethod + def _pretty_revision(version): + if version.revision == version.commit: + return version.revision + else: + return "{} ({})".format(version.revision, version.commit) + def _get_abi_dumps_from_shared_libraries(self, version): """Generate the ABI dumps for the specified git revision. The shared libraries must have been built and the module paths @@ -175,7 +188,7 @@ class AbiChecker(object): "abi-dumper", module_path, "-o", output_path, - "-lver", version.revision + "-lver", self._pretty_revision(version), ] abi_dump_output = subprocess.check_output( abi_dump_command, @@ -224,7 +237,10 @@ class AbiChecker(object): """Generate a report of the differences between the reference ABI and the new ABI. ABI dumps from self.old_version and self.new_version must be available.""" - compatibility_report = "" + compatibility_report = ("Checking evolution from {} to {}\n".format( + self._pretty_revision(self.old_version), + self._pretty_revision(self.new_version) + )) compliance_return_code = 0 shared_modules = list(set(self.old_version.modules.keys()) & set(self.new_version.modules.keys())) @@ -371,6 +387,7 @@ def run_main(): version="old", repository=abi_args.old_repo, revision=abi_args.old_rev, + commit=None, crypto_repository=abi_args.old_crypto_repo, crypto_revision=abi_args.old_crypto_rev, abi_dumps={}, @@ -380,6 +397,7 @@ def run_main(): version="new", repository=abi_args.new_repo, revision=abi_args.new_rev, + commit=None, crypto_repository=abi_args.new_crypto_repo, crypto_revision=abi_args.new_crypto_rev, abi_dumps={}, From b14b7305c6030d866c38b584832e6ad1b986dd52 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 4 Jul 2019 19:17:40 +0200 Subject: [PATCH 4/4] Split _abi_compliance_command into smaller functions This makes the code easier to read and pacifies pylint. --- scripts/abi_check.py | 115 ++++++++++++++++++++++++------------------- 1 file changed, 63 insertions(+), 52 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index 6aef9924f..533aaea3d 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -233,73 +233,84 @@ class AbiChecker(object): if not problems.getchildren(): report.remove(problems) + def _abi_compliance_command(self, mbed_module, output_path): + """Build the command to run to analyze the library mbed_module. + The report will be placed in output_path.""" + abi_compliance_command = [ + "abi-compliance-checker", + "-l", mbed_module, + "-old", self.old_version.abi_dumps[mbed_module], + "-new", self.new_version.abi_dumps[mbed_module], + "-strict", + "-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"] + return abi_compliance_command + + def _is_library_compatible(self, mbed_module, compatibility_report): + """Test if the library mbed_module has remained compatible. + Append a message regarding compatibility to compatibility_report.""" + output_path = os.path.join( + self.report_dir, "{}-{}-{}.html".format( + mbed_module, self.old_version.revision, + self.new_version.revision + ) + ) + try: + subprocess.check_output( + self._abi_compliance_command(mbed_module, output_path), + stderr=subprocess.STDOUT + ) + except subprocess.CalledProcessError as err: + if err.returncode != 1: + raise err + 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.append( + "Compatibility issues found for {}, " + "for details see {}".format(mbed_module, output_path) + ) + return False + compatibility_report.append( + "No compatibility issues for {}".format(mbed_module) + ) + if not (self.keep_all_reports or self.brief): + os.remove(output_path) + return True + 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_version and self.new_version must be available.""" - compatibility_report = ("Checking evolution from {} to {}\n".format( + compatibility_report = ["Checking evolution from {} to {}".format( self._pretty_revision(self.old_version), self._pretty_revision(self.new_version) - )) + )] compliance_return_code = 0 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_version.revision, - self.new_version.revision - ) - ) - abi_compliance_command = [ - "abi-compliance-checker", - "-l", mbed_module, - "-old", self.old_version.abi_dumps[mbed_module], - "-new", self.new_version.abi_dumps[mbed_module], - "-strict", - "-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"] - 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) + if not self._is_library_compatible(mbed_module, + compatibility_report): + compliance_return_code = 1 for version in [self.old_version, self.new_version]: for mbed_module, mbed_module_dump in version.abi_dumps.items(): os.remove(mbed_module_dump) if self.can_remove_report_dir: os.rmdir(self.report_dir) - self.log.info(compatibility_report) + self.log.info("\n".join(compatibility_report)) return compliance_return_code def check_for_abi_changes(self):