From 3da150422906edc64b68157d78aa05fbfb0e0083 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Thu, 1 Mar 2018 14:53:49 +0000 Subject: [PATCH 01/31] Add script for ABI compatibility checking --- scripts/abi_check.py | 233 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 233 insertions(+) create mode 100755 scripts/abi_check.py diff --git a/scripts/abi_check.py b/scripts/abi_check.py new file mode 100755 index 000000000..0f063a3f3 --- /dev/null +++ b/scripts/abi_check.py @@ -0,0 +1,233 @@ +#!/usr/bin/env python3 + +# 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. + +import os +import sys +import traceback +import shutil +import subprocess +import argparse +import logging +import tempfile + + +class AbiChecker(object): + + def __init__(self, report_dir, old_rev, new_rev, keep_all_reports): + self.repo_path = "." + self.log = None + 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.old_rev = old_rev + self.new_rev = new_rev + self.mbedtls_modules = ["libmbedcrypto", "libmbedtls", "libmbedx509"] + self.old_dumps = {} + self.new_dumps = {} + self.git_command = "git" + self.make_command = "make" + + def check_repo_path(self): + if not __file__ == os.path.join(".", "scripts", "abi_check.py"): + raise Exception("Must be run from Mbed TLS root") + + def setup_logger(self): + self.log = logging.getLogger() + self.log.setLevel(logging.INFO) + self.log.addHandler(logging.StreamHandler()) + + def check_abi_tools_are_installed(self): + for command in ["abi-dumper", "abi-compliance-checker"]: + if not shutil.which(command): + raise Exception("{} not installed, aborting".format(command)) + + def get_clean_worktree_for_git_revision(self, git_rev): + self.log.info( + "Checking out git worktree for revision {}".format(git_rev) + ) + git_worktree_path = tempfile.mkdtemp() + worktree_process = subprocess.Popen( + [self.git_command, "worktree", "add", git_worktree_path, git_rev], + cwd=self.repo_path, + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT + ) + worktree_output, _ = worktree_process.communicate() + self.log.info(worktree_output.decode("utf-8")) + if worktree_process.returncode != 0: + raise Exception("Checking out worktree failed, aborting") + return git_worktree_path + + def build_shared_libraries(self, git_worktree_path): + my_environment = os.environ.copy() + my_environment["CFLAGS"] = "-g -Og" + my_environment["SHARED"] = "1" + make_process = subprocess.Popen( + self.make_command, + env=my_environment, + cwd=git_worktree_path, + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT + ) + make_output, _ = make_process.communicate() + self.log.info(make_output.decode("utf-8")) + if make_process.returncode != 0: + raise Exception("make failed, aborting") + + def get_abi_dumps_from_shared_libraries(self, git_ref, git_worktree_path): + abi_dumps = {} + for mbed_module in self.mbedtls_modules: + 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"), + "-o", output_path, + "-lver", git_ref + ] + abi_dump_process = subprocess.Popen( + abi_dump_command, + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT + ) + abi_dump_output, _ = abi_dump_process.communicate() + 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 + + def cleanup_worktree(self, git_worktree_path): + shutil.rmtree(git_worktree_path) + worktree_process = subprocess.Popen( + [self.git_command, "worktree", "prune"], + cwd=self.repo_path, + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT + ) + worktree_output, _ = worktree_process.communicate() + self.log.info(worktree_output.decode("utf-8")) + if worktree_process.returncode != 0: + raise Exception("Worktree cleanup failed, aborting") + + def get_abi_dump_for_ref(self, git_rev): + git_worktree_path = self.get_clean_worktree_for_git_revision(git_rev) + self.build_shared_libraries(git_worktree_path) + abi_dumps = self.get_abi_dumps_from_shared_libraries( + git_rev, git_worktree_path + ) + self.cleanup_worktree(git_worktree_path) + return abi_dumps + + def get_abi_compatibility_report(self): + compatibility_report = "" + compliance_return_code = 0 + for mbed_module in self.mbedtls_modules: + output_path = os.path.join( + self.report_dir, "{}-{}-{}.html".format( + mbed_module, self.old_rev, self.new_rev + ) + ) + abi_compliance_command = [ + "abi-compliance-checker", + "-l", mbed_module, + "-old", self.old_dumps[mbed_module], + "-new", self.new_dumps[mbed_module], + "-strict", + "-report-path", output_path + ] + 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: + 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) + ) + else: + raise Exception( + "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]) + if not self.should_keep_report_dir and not self.keep_all_reports: + os.rmdir(self.report_dir) + self.log.info(compatibility_report) + return compliance_return_code + + def check_for_abi_changes(self): + 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) + return self.get_abi_compatibility_report() + + +def run_main(): + try: + parser = argparse.ArgumentParser( + description=( + "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." + ) + ) + parser.add_argument( + "-r", "--report_dir", type=str, default="reports", + help="directory where reports are stored, default is reports", + ) + parser.add_argument( + "-k", "--keep_all_reports", action="store_true", + 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 + ) + parser.add_argument( + "-n", "--new_rev", type=str, help="revision for new version", + required=True + ) + abi_args = parser.parse_args() + abi_check = AbiChecker( + abi_args.report_dir, abi_args.old_rev, + abi_args.new_rev, abi_args.keep_all_reports + ) + return_code = abi_check.check_for_abi_changes() + sys.exit(return_code) + except Exception as error: + traceback.print_exc(error) + sys.exit(2) + + +if __name__ == "__main__": + run_main() From e3e6b183519ecefaa246a417475c393381ef7df7 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Mon, 12 Mar 2018 15:44:31 +0000 Subject: [PATCH 02/31] Add copyright to abi_check script --- scripts/abi_check.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index 0f063a3f3..f9fb7f65d 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -1,5 +1,11 @@ #!/usr/bin/env python3 - +# +# This file is part of mbed TLS (https://tls.mbed.org) +# +# Copyright (c) 2018, Arm Limited, All Rights Reserved +# +# 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. From c47ac2651a07de76c1b85b2e6e6e08ebeaf0f082 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Thu, 15 Mar 2018 10:12:06 +0000 Subject: [PATCH 03/31] Fix current directory check --- scripts/abi_check.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index f9fb7f65d..98d8be422 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -1,6 +1,6 @@ #!/usr/bin/env python3 # -# This file is part of mbed TLS (https://tls.mbed.org) +# This file is part of Mbed TLS (https://tls.mbed.org) # # Copyright (c) 2018, Arm Limited, All Rights Reserved # @@ -42,7 +42,9 @@ class AbiChecker(object): self.make_command = "make" def check_repo_path(self): - if not __file__ == os.path.join(".", "scripts", "abi_check.py"): + current_dir = os.path.realpath('.') + root_dir = os.path.dirname(os.path.dirname(os.path.realpath(__file__))) + if current_dir != root_dir: raise Exception("Must be run from Mbed TLS root") def setup_logger(self): @@ -230,8 +232,8 @@ def run_main(): ) return_code = abi_check.check_for_abi_changes() sys.exit(return_code) - except Exception as error: - traceback.print_exc(error) + except Exception: + traceback.print_exc() sys.exit(2) From 4cd7a9b8ed73fb6838cd567f142912c69c83ea68 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Fri, 6 Apr 2018 11:23:22 +0100 Subject: [PATCH 04/31] Updated abi_check.py docstrings --- scripts/abi_check.py | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index 98d8be422..14250d2b9 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -1,18 +1,19 @@ #!/usr/bin/env python3 -# -# This file is part of Mbed TLS (https://tls.mbed.org) -# -# Copyright (c) 2018, Arm Limited, All Rights Reserved -# -# 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. +""" +This file is part of Mbed TLS (https://tls.mbed.org) + +Copyright (c) 2018, Arm Limited, All Rights Reserved + +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: requires Python 3, must be run from Mbed TLS root. +""" import os import sys @@ -205,8 +206,8 @@ def run_main(): " 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." + "while running the script. Note: requires Python 3, " + "must be run from Mbed TLS root." ) ) parser.add_argument( From 31321ca89389039126e042ff9a4b64d6abf3e247 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Mon, 16 Apr 2018 12:02:29 +0100 Subject: [PATCH 05/31] Fix minor issues with command line options --- scripts/abi_check.py | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index 14250d2b9..8f9cd0f43 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -12,7 +12,7 @@ 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: requires Python 3, must be run from Mbed TLS root. +Note: must be run from Mbed TLS root. """ import os @@ -199,31 +199,30 @@ def run_main(): try: parser = argparse.ArgumentParser( description=( - "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: requires Python 3, " - "must be run from Mbed TLS root." + """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.""" ) ) parser.add_argument( - "-r", "--report_dir", type=str, default="reports", + "-r", "--report-dir", type=str, default="reports", help="directory where reports are stored, default is reports", ) parser.add_argument( - "-k", "--keep_all_reports", action="store_true", + "-k", "--keep-all-reports", action="store_true", help="keep all reports, even if there are no compatibility issues", ) parser.add_argument( - "-o", "--old_rev", type=str, help="revision for old version", + "-o", "--old-rev", type=str, help="revision for old version", required=True ) parser.add_argument( - "-n", "--new_rev", type=str, help="revision for new version", + "-n", "--new-rev", type=str, help="revision for new version", required=True ) abi_args = parser.parse_args() From 5857c2f43ffb45e9358f128b6d9e976425a0ed78 Mon Sep 17 00:00:00 2001 From: Jaeden Amero Date: Fri, 2 Nov 2018 16:22:37 +0000 Subject: [PATCH 06/31] abi_check: Allow checking current checkout Without a "--detach" option, git worktree will refuse to checkout a branch that's already checked out. This makes the abi_check.py script not very useful for checking the currently checked out branch, as git will error that the branch is already checked out. Add the "--detach" option to check out the new temporary worktree in detached head mode. This is acceptable because we aren't planning on working on the branch and just want a checkout to do ABI checking from. --- 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 8f9cd0f43..056c1169a 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -64,7 +64,7 @@ class AbiChecker(object): ) git_worktree_path = tempfile.mkdtemp() worktree_process = subprocess.Popen( - [self.git_command, "worktree", "add", git_worktree_path, git_rev], + [self.git_command, "worktree", "add", "--detach", git_worktree_path, git_rev], cwd=self.repo_path, stdout=subprocess.PIPE, stderr=subprocess.STDOUT From 346f9595c956b98f714ca4231312c1a5ff98bd4c Mon Sep 17 00:00:00 2001 From: Jaeden Amero Date: Fri, 2 Nov 2018 16:35:09 +0000 Subject: [PATCH 07/31] abi_check: Update submodules When grabbing a fresh copy of a branch, it's required to also fetch the submodule. Add fetching the submodule to abi_check.py. --- scripts/abi_check.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index 056c1169a..fe5dd3f21 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -75,6 +75,18 @@ class AbiChecker(object): raise Exception("Checking out worktree failed, aborting") return git_worktree_path + def update_git_submodules(self, git_worktree_path): + process = subprocess.Popen( + [self.git_command, "submodule", "update", "--init", '--recursive'], + cwd=git_worktree_path, + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT + ) + output, _ = process.communicate() + self.log.info(output.decode("utf-8")) + if process.returncode != 0: + raise Exception("git submodule update failed, aborting") + def build_shared_libraries(self, git_worktree_path): my_environment = os.environ.copy() my_environment["CFLAGS"] = "-g -Og" @@ -131,6 +143,7 @@ class AbiChecker(object): def get_abi_dump_for_ref(self, git_rev): git_worktree_path = self.get_clean_worktree_for_git_revision(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( git_rev, git_worktree_path From fceb4ce767472c49c0c10bd8e755d27e4afa2702 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 25 Feb 2019 20:36:52 +0100 Subject: [PATCH 08/31] abi_check.py: Document more methods --- scripts/abi_check.py | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index fe5dd3f21..88435ef02 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -26,8 +26,16 @@ import tempfile class AbiChecker(object): + """API and ABI checker.""" def __init__(self, report_dir, old_rev, new_rev, keep_all_reports): + """Instantiate the API/ABI checker. + + report_dir: directory for output files + old_rev: reference git revision to compare against + new_rev: git revision to check + keep_all_reports: if false, delete old reports + """ self.repo_path = "." self.log = None self.setup_logger() @@ -42,7 +50,8 @@ class AbiChecker(object): self.git_command = "git" self.make_command = "make" - def check_repo_path(self): + @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: @@ -53,12 +62,15 @@ class AbiChecker(object): self.log.setLevel(logging.INFO) self.log.addHandler(logging.StreamHandler()) - def check_abi_tools_are_installed(self): + @staticmethod + def check_abi_tools_are_installed(): for command in ["abi-dumper", "abi-compliance-checker"]: if not shutil.which(command): raise Exception("{} not installed, aborting".format(command)) def get_clean_worktree_for_git_revision(self, 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) ) @@ -88,6 +100,7 @@ class AbiChecker(object): raise Exception("git submodule update 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" @@ -104,6 +117,9 @@ class AbiChecker(object): raise Exception("make failed, aborting") def get_abi_dumps_from_shared_libraries(self, git_ref, git_worktree_path): + """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 in self.mbedtls_modules: output_path = os.path.join( @@ -129,6 +145,7 @@ class AbiChecker(object): return abi_dumps def cleanup_worktree(self, git_worktree_path): + """Remove the specified git worktree.""" shutil.rmtree(git_worktree_path) worktree_process = subprocess.Popen( [self.git_command, "worktree", "prune"], @@ -142,6 +159,7 @@ class AbiChecker(object): raise Exception("Worktree cleanup failed, aborting") def get_abi_dump_for_ref(self, git_rev): + """Generate the ABI dumps for the specified git revision.""" git_worktree_path = self.get_clean_worktree_for_git_revision(git_rev) self.update_git_submodules(git_worktree_path) self.build_shared_libraries(git_worktree_path) @@ -152,6 +170,9 @@ class AbiChecker(object): return abi_dumps 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.""" compatibility_report = "" compliance_return_code = 0 for mbed_module in self.mbedtls_modules: @@ -201,6 +222,8 @@ class AbiChecker(object): return compliance_return_code def check_for_abi_changes(self): + """Generate a report of ABI differences + 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) From 834ebc415c9ea60a73c93d2ffc15177f99f13838 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Tue, 19 Feb 2019 16:59:33 +0000 Subject: [PATCH 09/31] 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 88435ef02..e45cd9ac2 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 d3cde6f2d39974fe66bcde0507014dc810bd4a42 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Wed, 20 Feb 2019 15:01:56 +0000 Subject: [PATCH 10/31] 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 e45cd9ac2..14e38fba8 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 32e7a50c8270503c32c91d0e16ced7057a5b6e0f Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Thu, 21 Feb 2019 13:09:26 +0000 Subject: [PATCH 11/31] 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 14e38fba8..fcd40b713 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 ab3893b815a83e0749674a56075d6c1af9b8b918 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Mon, 25 Feb 2019 17:01:55 +0000 Subject: [PATCH 12/31] 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 fcd40b713..a94185229 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 d9ad9ec81c7c96b3481a1a13caa84ab6fc143d1f Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Mon, 25 Feb 2019 11:35:05 +0000 Subject: [PATCH 13/31] 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 a94185229..088c07ad8 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 d98d8b50dc0c8bed3e0b5de48b70c0df9efdb7aa Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Wed, 27 Feb 2019 16:53:40 +0000 Subject: [PATCH 14/31] 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 088c07ad8..fbe31dd6c 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 87aedf6202dcd1281fb87dff00fd30ede8cc4310 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Wed, 27 Feb 2019 17:33:31 +0000 Subject: [PATCH 15/31] 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 fbe31dd6c..b4f1c3688 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 997c287ce93486568f9382fa90ab4504c6b6969c Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Thu, 28 Feb 2019 11:52:39 +0000 Subject: [PATCH 16/31] 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 b4f1c3688..7ddc443b3 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 826e5af85d47d39a86583facb5b99d96f55e229c Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Fri, 1 Mar 2019 09:54:44 +0000 Subject: [PATCH 17/31] 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 7ddc443b3..7784067c9 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 765d20d3d8b7b4731f7878753c7dfad85fce1188 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Tue, 5 Mar 2019 15:21:32 +0000 Subject: [PATCH 18/31] 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 7784067c9..78f979f1a 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 7be79c9e7778b3ccd8932c32ccc9e54574b67095 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Tue, 5 Mar 2019 15:23:25 +0000 Subject: [PATCH 19/31] 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 78f979f1a..6e259e3fb 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 02b6865dc76dd04bdb76403ba616bb3336285239 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Tue, 5 Mar 2019 16:25:38 +0000 Subject: [PATCH 20/31] 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 6e259e3fb..a263ecdfb 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 7bb9cb5ce3e24426e6be60f8cb35b993ffdef075 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Tue, 5 Mar 2019 16:30:39 +0000 Subject: [PATCH 21/31] 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 a263ecdfb..148a3aff5 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 b743309c007818e2ecafc16c7915aa19f8f96383 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Fri, 8 Mar 2019 11:12:19 +0000 Subject: [PATCH 22/31] 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 148a3aff5..93ab65b2b 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 f0f9f7fe7d0778e4bca6a9da9d1316076c0ca74b Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Fri, 8 Mar 2019 11:30:04 +0000 Subject: [PATCH 23/31] 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 93ab65b2b..a2a52042e 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 cf434259418de2ee31a8853173fc2d1085d23234 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Thu, 4 Apr 2019 14:39:33 +0100 Subject: [PATCH 24/31] 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 a2a52042e..b30b5693a 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 b7447e7d2ad6062f4086a9e48a5e4ceaa4eefc29 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Fri, 5 Apr 2019 17:06:17 +0100 Subject: [PATCH 25/31] 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 b30b5693a..6e2ed5b42 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 f1d272d0ca7d7571f2363a499f1347830b9e4b7d Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Tue, 9 Apr 2019 09:14:17 +0100 Subject: [PATCH 26/31] 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 6e2ed5b42..8951fcbcc 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: From 03625fe311b0ebab0a86c33014f347e019073b2b Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Thu, 11 Apr 2019 15:50:41 +0100 Subject: [PATCH 27/31] 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 8951fcbcc..60b8b1324 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 bbc6ccfa2f137d8c330c51e8cda49288e7da2524 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Fri, 12 Apr 2019 15:17:02 +0100 Subject: [PATCH 28/31] 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 60b8b1324..9f087c5b6 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 31a1e99874d349cecb9e14b89c33c527e338d06e Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Fri, 12 Apr 2019 15:18:02 +0100 Subject: [PATCH 29/31] 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 9f087c5b6..55679a214 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 4a483e4829e68c1e340bbd309e37b15c6fa014c1 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Fri, 12 Apr 2019 16:24:25 +0100 Subject: [PATCH 30/31] 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 55679a214..e5d5c99c3 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: From 62a18e32d0a329cbe065244185790c4d3550a14e Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Thu, 18 Apr 2019 14:59:51 +0100 Subject: [PATCH 31/31] Add documentation for why we're catching all exceptions We wish to distinguish between success, an abi break and a script failure, so catch all uncaught exceptions and exit explicitly with status 2 --- 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 e5d5c99c3..f837f7a79 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -393,7 +393,9 @@ def run_main(): abi_check = AbiChecker(old_version, new_version, configuration) return_code = abi_check.check_for_abi_changes() sys.exit(return_code) - except Exception: + except Exception: # pylint: disable=broad-except + # Print the backtrace and exit explicitly so as to exit with + # status 2, not 1. traceback.print_exc() sys.exit(2)