From 344745605665e2c0d7b187fba3a815b6cd372998 Mon Sep 17 00:00:00 2001 From: Jeongik Cha Date: Wed, 29 Jul 2020 19:57:55 +0900 Subject: [PATCH 1/2] Use inclusive language in diff tool Also, apply recommendation from the lint Bug: 162284096 Test: m compare_images Change-Id: I43a031a56aadd4b51ee309500df58348d23072aa --- vndk/tools/image-diff-tool/diff.py | 206 ++++++++++++++--------------- 1 file changed, 98 insertions(+), 108 deletions(-) diff --git a/vndk/tools/image-diff-tool/diff.py b/vndk/tools/image-diff-tool/diff.py index 234e92a2c..d12609f94 100644 --- a/vndk/tools/image-diff-tool/diff.py +++ b/vndk/tools/image-diff-tool/diff.py @@ -12,41 +12,38 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. - -import os -import subprocess -import sys -from collections import defaultdict -from pathlib import Path -import hashlib import argparse -import zipfile +import collections import fnmatch +import hashlib +import os +import pathlib +import subprocess import tempfile - +import zipfile def silent_call(cmd): - return subprocess.call(cmd, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) == 0 - + return subprocess.call( + cmd, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) == 0 def sha1sum(f): - with open(f, 'rb') as fin: + with open(f, "rb") as fin: return hashlib.sha1(fin.read()).hexdigest() - def sha1sum_without_signing_key(filepath): apk = zipfile.ZipFile(filepath) l = [] for f in sorted(apk.namelist()): - if f.startswith('META-INF/'): + if f.startswith("META-INF/"): continue l.append(hashlib.sha1(apk.read(f)).hexdigest()) l.append(f) return hashlib.sha1(",".join(l).encode()).hexdigest() - def strip_and_sha1sum(filepath): - # TODO: save striped file in tmp directory to support readonly directory. - tmp_filepath = filepath + '.tmp.no-build-id' - strip_all_and_remove_build_id = lambda: silent_call( - ["llvm-strip", "--strip-all", "--keep-section=.ARM.attributes", - "--remove-section=.note.gnu.build-id", filepath, "-o", tmp_filepath]) + """Strip informations of elf file and calculate sha1 hash.""" + tmp_filepath = filepath + ".tmp.no-build-id" + llvm_strip = [ + "llvm-strip", "--strip-all", "--keep-section=.ARM.attributes", + "--remove-section=.note.gnu.build-id", filepath, "-o", tmp_filepath + ] + strip_all_and_remove_build_id = lambda: silent_call(llvm_strip) try: if strip_all_and_remove_build_id(): return sha1sum(tmp_filepath) @@ -55,65 +52,51 @@ def strip_and_sha1sum(filepath): finally: if os.path.exists(tmp_filepath): os.remove(tmp_filepath) - return sha1sum(filepath) - - -def make_filter_from_whitelists(whitelists, all_targets): - """Creates a callable filter from a list of whitelist files. - - Whitelist can contain pathname patterns or ignored lines. Pathnames are case +def make_filter_from_allowlists(allowlists, all_targets): + """Creates a callable filter from a list of allowlist files. + Allowlist can contain pathname patterns or skipped lines. Pathnames are case insensitive. - - Whitelist can contain single-line comments. Comment lines begin with # - + Allowlist can contain single-line comments. Comment lines begin with # For example, this ignores the file "system/build.prop": SYSTEM/build.prop - This ignores txt files: *.txt - This ignores files in directory "system/dontcare/" SYSTEM/dontcare/* - This ignores lines prefixed with pat1 or pat2 in file "system/build.prop": SYSTEM/build.prop=pat1 pat2 - Args: - whitelists: A list of whitelist filenames. + allowlists: A list of allowlist filenames. all_targets: A list of targets to compare. - Returns: A callable object that accepts a file pathname and returns True if the file - is ignored by the whitelists and False when it is not. + is skipped by the allowlists and False when it is not. """ - ignored_patterns = set() - ignored_lines = defaultdict(list) - for whitelist in whitelists: - if not os.path.isfile(whitelist): + skipped_patterns = set() + skipped_lines = collections.defaultdict(list) + for allowlist in allowlists: + if not os.path.isfile(allowlist): continue - with open(whitelist, 'rb') as f: + with open(allowlist, "rb") as f: for line in f: pat = line.strip().decode() - if pat.startswith('#'): + if pat.startswith("#"): continue - if pat and pat[-1] == '\\': - pat = pat.rstrip('\\') - if '=' in pat: - filename, prefixes = pat.split('=', 1) + if pat and pat[-1] == "\\": + pat = pat.rstrip("\\") + if "=" in pat: + filename, prefixes = pat.split("=", 1) prefixes = prefixes.split() if prefixes: - ignored_lines[filename.lower()].extend(prefixes) + skipped_lines[filename.lower()].extend(prefixes) elif pat: - ignored_patterns.add(pat.lower()) - - def diff_with_ignored_lines(filename, prefixes): + skipped_patterns.add(pat.lower()) + def diff_with_skipped_lines(filename, prefixes): """Compares sha1 digest of file while ignoring lines. - Args: filename: File to compare among each target. - prefixes: A list of prefixes. Lines that start with prefix are ignored. - + prefixes: A list of prefixes. Lines that start with prefix are skipped. Returns: True if file is identical among each target. """ @@ -123,7 +106,7 @@ def make_filter_from_whitelists(whitelists, all_targets): if not os.path.isfile(pathname): return False sha1 = hashlib.sha1() - with open(pathname, 'rb') as f: + with open(pathname, "rb") as f: for line in f: line_text = line.decode() if not any(line_text.startswith(prefix) for prefix in prefixes): @@ -131,109 +114,111 @@ def make_filter_from_whitelists(whitelists, all_targets): file_digest_respect_ignore.append(sha1.hexdigest()) return (len(file_digest_respect_ignore) == len(all_targets) and len(set(file_digest_respect_ignore)) == 1) - - def whitelist_filter(filename): + def allowlist_filter(filename): norm_filename = filename.lower() - for pattern in ignored_patterns: + for pattern in skipped_patterns: if fnmatch.fnmatch(norm_filename, pattern): return True - if norm_filename in ignored_lines: - ignored_prefixes = ignored_lines[norm_filename] - return diff_with_ignored_lines(filename, ignored_prefixes) + if norm_filename in skipped_lines: + skipped_prefixes = skipped_lines[norm_filename] + return diff_with_skipped_lines(filename, skipped_prefixes) return False - - return whitelist_filter - - -def main(all_targets, search_paths, whitelists, ignore_signing_key=False, list_only=False): + return allowlist_filter +def main(all_targets, + search_paths, + allowlists, + ignore_signing_key=False, + list_only=False): def run(path): - is_native_component = silent_call(["llvm-objdump", "-a", path]) - is_apk = path.endswith('.apk') - if is_native_component: + is_executable_component = silent_call(["llvm-objdump", "-a", path]) + is_apk = path.endswith(".apk") + if is_executable_component: return strip_and_sha1sum(path) elif is_apk and ignore_signing_key: return sha1sum_without_signing_key(path) else: return sha1sum(path) - # artifact_sha1_target_map[filename][sha1] = list of targets - artifact_sha1_target_map = defaultdict(lambda: defaultdict(list)) + artifact_sha1_target_map = collections.defaultdict( + lambda: collections.defaultdict(list)) for target in all_targets: paths = [] for search_path in search_paths: - for path in Path(target, search_path).glob('**/*'): + for path in pathlib.Path(target, search_path).glob("**/*"): if path.exists() and not path.is_dir(): paths.append((str(path), str(path.relative_to(target)))) - target_basename = os.path.basename(os.path.normpath(target)) for path, filename in paths: sha1 = 0 if not list_only: sha1 = run(path) artifact_sha1_target_map[filename][sha1].append(target_basename) - def pretty_print(sha1, filename, targets, exclude_sha1): if exclude_sha1: - return '{}, {}\n'.format(filename, ';'.join(targets)) - - return '{}, {}, {}\n'.format(filename, sha1[:10], ';'.join(targets)) - + return "{}, {}\n".format(filename, ";".join(targets)) + return "{}, {}, {}\n".format(filename, sha1[:10], ";".join(targets)) def is_common(sha1_target_map): - for sha1, targets in sha1_target_map.items(): + for _, targets in sha1_target_map.items(): return len(sha1_target_map) == 1 and len(targets) == len(all_targets) return False - - whitelist_filter = make_filter_from_whitelists(whitelists, all_targets) - + allowlist_filter = make_filter_from_allowlists(allowlists, all_targets) common = [] diff = [] - whitelisted_diff = [] + allowlisted_diff = [] for filename, sha1_target_map in artifact_sha1_target_map.items(): if is_common(sha1_target_map): for sha1, targets in sha1_target_map.items(): common.append(pretty_print(sha1, filename, targets, list_only)) else: - if whitelist_filter(filename): + if allowlist_filter(filename): for sha1, targets in sha1_target_map.items(): - whitelisted_diff.append(pretty_print(sha1, filename, targets, list_only)) + allowlisted_diff.append( + pretty_print(sha1, filename, targets, list_only)) else: for sha1, targets in sha1_target_map.items(): diff.append(pretty_print(sha1, filename, targets, list_only)) - common = sorted(common) diff = sorted(diff) - whitelisted_diff = sorted(whitelisted_diff) - + allowlisted_diff = sorted(allowlisted_diff) header = "filename, sha1sum, targets\n" if list_only: header = "filename, targets\n" - - with open("common.csv", 'w') as fout: + with open("common.csv", "w") as fout: fout.write(header) fout.writelines(common) - with open("diff.csv", 'w') as fout: + with open("diff.csv", "w") as fout: fout.write(header) fout.writelines(diff) - with open("whitelisted_diff.csv", 'w') as fout: + with open("allowlisted_diff.csv", "w") as fout: fout.write(header) - fout.writelines(whitelisted_diff) - -def main_with_zip(extracted_paths, args): - for origin_path, tmp_path in zip(args.target, extracted_paths): + fout.writelines(allowlisted_diff) +def main_with_zip(extracted_paths, main_args): + for origin_path, tmp_path in zip(main_args.target, extracted_paths): unzip_cmd = ["unzip", "-qd", tmp_path, os.path.join(origin_path, "*.zip")] - unzip_cmd.extend([os.path.join(s, "*") for s in args.search_path]) + unzip_cmd.extend([os.path.join(s, "*") for s in main_args.search_path]) subprocess.call(unzip_cmd) - main(extracted_paths, args.search_path, args.whitelist, args.ignore_signing_key, list_only=args.list_only) - + main( + extracted_paths, + main_args.search_path, + main_args.allowlist, + main_args.ignore_signing_key, + list_only=main_args.list_only) if __name__ == "__main__": - parser = argparse.ArgumentParser(prog="compare_images", usage="compare_images -t model1 model2 [model...] -s dir1 [dir...] [-i] [-u] [-p] [-w whitelist1] [-w whitelist2]") - parser.add_argument("-t", "--target", nargs='+', required=True) - parser.add_argument("-s", "--search_path", nargs='+', required=True) - parser.add_argument("-i", "--ignore_signing_key", action='store_true') - parser.add_argument("-l", "--list_only", action='store_true', help='Compare file list only and ignore SHA-1 diff') - parser.add_argument("-u", "--unzip", action='store_true') - parser.add_argument("-p", "--preserve_extracted_files", action='store_true') - parser.add_argument("-w", "--whitelist", action="append", default=[]) + parser = argparse.ArgumentParser( + prog="compare_images", + usage="compare_images -t model1 model2 [model...] -s dir1 [dir...] [-i] [-u] [-p] [-w allowlist1] [-w allowlist2]" + ) + parser.add_argument("-t", "--target", nargs="+", required=True) + parser.add_argument("-s", "--search_path", nargs="+", required=True) + parser.add_argument("-i", "--ignore_signing_key", action="store_true") + parser.add_argument( + "-l", + "--list_only", + action="store_true", + help="Compare file list only and ignore SHA-1 diff") + parser.add_argument("-u", "--unzip", action="store_true") + parser.add_argument("-p", "--preserve_extracted_files", action="store_true") + parser.add_argument("-w", "--allowlist", action="append", default=[]) args = parser.parse_args() if len(args.target) < 2: parser.error("The number of targets has to be at least two.") @@ -247,4 +232,9 @@ if __name__ == "__main__": os.makedirs(p) main_with_zip(target_in_tmp, args) else: - main(args.target, args.search_path, args.whitelist, args.ignore_signing_key, list_only=args.list_only) + main( + args.target, + args.search_path, + args.allowlist, + args.ignore_signing_key, + list_only=args.list_only) From c5d47e8b93a6c96c7678153aa2c325ce60eb76cf Mon Sep 17 00:00:00 2001 From: Jeongik Cha Date: Wed, 29 Jul 2020 20:18:17 +0900 Subject: [PATCH 2/2] Use inclusive language in diff script Test: run compare_images_and_print.sh Bug: 162284096 Change-Id: I1f5cebbc61606a16ddfae8f64dfc57e01cbcbd3d --- .../{whitelist.txt => allowlist.txt} | 0 .../image-diff-tool/compare_images_and_print.sh | 16 ++++++++-------- 2 files changed, 8 insertions(+), 8 deletions(-) rename vndk/tools/image-diff-tool/{whitelist.txt => allowlist.txt} (100%) diff --git a/vndk/tools/image-diff-tool/whitelist.txt b/vndk/tools/image-diff-tool/allowlist.txt similarity index 100% rename from vndk/tools/image-diff-tool/whitelist.txt rename to vndk/tools/image-diff-tool/allowlist.txt diff --git a/vndk/tools/image-diff-tool/compare_images_and_print.sh b/vndk/tools/image-diff-tool/compare_images_and_print.sh index 23b45f7ee..8c94cd396 100755 --- a/vndk/tools/image-diff-tool/compare_images_and_print.sh +++ b/vndk/tools/image-diff-tool/compare_images_and_print.sh @@ -41,26 +41,26 @@ readonly CHECK_DIFF_RESULT . build/envsetup.sh lunch aosp_arm64 build/soong/soong_ui.bash --make-mode compare_images -COMMON_WHITELIST=development/vndk/tools/image-diff-tool/whitelist.txt -out/host/linux-x86/bin/compare_images $1 -u -w "${COMMON_WHITELIST}" +COMMON_ALLOWLIST=development/vndk/tools/image-diff-tool/allowlist.txt +out/host/linux-x86/bin/compare_images $1 -u -w "${COMMON_ALLOWLIST}" if [ -v DIST_DIR ]; then - cp common.csv diff.csv whitelisted_diff.csv "${DIST_DIR}" + cp common.csv diff.csv allowlisted_diff.csv "${DIST_DIR}" fi echo >&2 -echo >&2 " - Different parts (that are whitelisted)" -cat >&2 whitelisted_diff.csv +echo >&2 " - Different parts (that are allowlisted)" +cat >&2 allowlisted_diff.csv echo >&2 -echo >&2 " - Different parts (that are not whitelisted)" +echo >&2 " - Different parts (that are not allowlisted)" cat >&2 diff.csv if [ "$(wc -l diff.csv | cut -d' ' -f1)" -gt "1" ]; then cat >&2 <