Fix pylint warnings in art_apex_test
* Don't shadow global variables
* Remove unnecessary semicolons
* Do not use `len()` to determine if a sequence is empty
* Replace some if statements with test
* Remove unused variables
* Remove useless super methods
* Switch to enumerate where possible
* Make some class members public
* Restrict exceptions coming from zip
* Remove not checked boolean function returns
* Long lines were untouched
Test: art/build/apex/runtests.sh
Change-Id: I247ee2ba5903ae69f478dbfc5517a39c3f5e9e0e
diff --git a/build/apex/art_apex_test.py b/build/apex/art_apex_test.py
index 4fcc2cc..c25bd2d 100755
--- a/build/apex/art_apex_test.py
+++ b/build/apex/art_apex_test.py
@@ -24,15 +24,18 @@
logging.basicConfig(format='%(message)s')
+
class FSObject:
def __init__(self, name, is_dir, is_exec, is_symlink):
self.name = name
self.is_dir = is_dir
self.is_exec = is_exec
self.is_symlink = is_symlink
+
def __str__(self):
return '%s(dir=%r,exec=%r,symlink=%r)' % (self.name, self.is_dir, self.is_exec, self.is_symlink)
+
class TargetApexProvider:
def __init__(self, apex, tmpdir, debugfs):
self._tmpdir = tmpdir
@@ -40,8 +43,8 @@
self._folder_cache = {}
self._payload = os.path.join(self._tmpdir, 'apex_payload.img')
# Extract payload to tmpdir.
- zip = zipfile.ZipFile(apex)
- zip.extract('apex_payload.img', tmpdir)
+ apex_zip = zipfile.ZipFile(apex)
+ apex_zip.extract('apex_payload.img', tmpdir)
def __del__(self):
# Delete temps.
@@ -49,22 +52,22 @@
os.remove(self._payload)
def get(self, path):
- dir, name = os.path.split(path)
- if len(dir) == 0:
- dir = '.'
- map = self.read_dir(dir)
- return map[name] if name in map else None
+ apex_dir, name = os.path.split(path)
+ if not apex_dir:
+ apex_dir = '.'
+ apex_map = self.read_dir(apex_dir)
+ return apex_map[name] if name in apex_map else None
- def read_dir(self, dir):
- if dir in self._folder_cache:
- return self._folder_cache[dir]
+ def read_dir(self, apex_dir):
+ if apex_dir in self._folder_cache:
+ return self._folder_cache[apex_dir]
# Cannot use check_output as it will annoy with stderr.
- process = subprocess.Popen([self._debugfs, '-R', 'ls -l -p %s' % (dir), self._payload],
+ process = subprocess.Popen([self._debugfs, '-R', 'ls -l -p %s' % apex_dir, self._payload],
stdout=subprocess.PIPE, stderr=subprocess.PIPE,
universal_newlines=True)
- stdout, stderr = process.communicate()
+ stdout, _ = process.communicate()
res = str(stdout)
- map = {}
+ apex_map = {}
# Debugfs output looks like this:
# debugfs 1.44.4 (18-Aug-2018)
# /12/040755/0/2000/.//
@@ -88,30 +91,33 @@
continue
comps = line.split('/')
if len(comps) != 8:
- logging.warn('Could not break and parse line \'%s\'', line)
+ logging.warning('Could not break and parse line \'%s\'', line)
continue
bits = comps[2]
name = comps[5]
if len(bits) != 6:
- logging.warn('Dont understand bits \'%s\'', bits)
+ logging.warning('Dont understand bits \'%s\'', bits)
continue
- is_dir = True if bits[1] == '4' else False
+ is_dir = bits[1] == '4'
+
def is_exec_bit(ch):
- return True if int(ch) & 1 == 1 else False
+ return int(ch) & 1 == 1
+
is_exec = is_exec_bit(bits[3]) and is_exec_bit(bits[4]) and is_exec_bit(bits[5])
- is_symlink = True if bits[1] == '2' else False
- map[name] = FSObject(name, is_dir, is_exec, is_symlink)
- self._folder_cache[dir] = map
- return map
+ is_symlink = bits[1] == '2'
+ apex_map[name] = FSObject(name, is_dir, is_exec, is_symlink)
+ self._folder_cache[apex_dir] = apex_map
+ return apex_map
+
class HostApexProvider:
def __init__(self, apex, tmpdir):
self._tmpdir = tmpdir
- self._folder_cache = {}
+ self.folder_cache = {}
self._payload = os.path.join(self._tmpdir, 'apex_payload.zip')
# Extract payload to tmpdir.
- zip = zipfile.ZipFile(apex)
- zip.extract('apex_payload.zip', tmpdir)
+ apex_zip = zipfile.ZipFile(apex)
+ apex_zip.extract('apex_payload.zip', tmpdir)
def __del__(self):
# Delete temps.
@@ -119,24 +125,24 @@
os.remove(self._payload)
def get(self, path):
- dir, name = os.path.split(path)
- if len(dir) == 0:
- dir = ''
- map = self.read_dir(dir)
- return map[name] if name in map else None
+ apex_dir, name = os.path.split(path)
+ if not apex_dir:
+ apex_dir = ''
+ apex_map = self.read_dir(apex_dir)
+ return apex_map[name] if name in apex_map else None
- def read_dir(self, dir):
- if dir in self._folder_cache:
- return self._folder_cache[dir]
- if not self._folder_cache:
+ def read_dir(self, apex_dir):
+ if apex_dir in self.folder_cache:
+ return self.folder_cache[apex_dir]
+ if not self.folder_cache:
self.parse_zip()
- if dir in self._folder_cache:
- return self._folder_cache[dir]
+ if apex_dir in self.folder_cache:
+ return self.folder_cache[apex_dir]
return {}
def parse_zip(self):
- zip = zipfile.ZipFile(self._payload)
- infos = zip.infolist()
+ apex_zip = zipfile.ZipFile(self._payload)
+ infos = apex_zip.infolist()
for zipinfo in infos:
path = zipinfo.filename
@@ -144,20 +150,21 @@
assert path
def get_octal(val, index):
- return (val >> (index * 3)) & 0x7;
+ return (val >> (index * 3)) & 0x7
+
def bits_is_exec(val):
# TODO: Enforce group/other, too?
return get_octal(val, 2) & 1 == 1
is_zipinfo = True
while path:
- dir, base = os.path.split(path)
+ apex_dir, base = os.path.split(path)
# TODO: If directories are stored, base will be empty.
- if not dir in self._folder_cache:
- self._folder_cache[dir] = {}
- dir_map = self._folder_cache[dir]
- if not base in dir_map:
+ if apex_dir not in self.folder_cache:
+ self.folder_cache[apex_dir] = {}
+ dir_map = self.folder_cache[apex_dir]
+ if base not in dir_map:
if is_zipinfo:
bits = (zipinfo.external_attr >> 16) & 0xFFFF
is_dir = get_octal(bits, 4) == 4
@@ -169,7 +176,8 @@
is_dir = True
dir_map[base] = FSObject(base, is_dir, is_exec, is_symlink)
is_zipinfo = False
- path = dir
+ path = apex_dir
+
# DO NOT USE DIRECTLY! This is an "abstract" base class.
class Checker:
@@ -177,75 +185,68 @@
self._provider = provider
self._errors = 0
- def fail(self, msg, *args):
+ def fail(self, msg, *fail_args):
self._errors += 1
- logging.error(msg, args)
+ logging.error(msg, fail_args)
def error_count(self):
return self._errors
+
def reset_errors(self):
self._errors = 0
def is_file(self, file):
fs_object = self._provider.get(file)
if fs_object is None:
- return (False, 'Could not find %s')
+ return False, 'Could not find %s'
if fs_object.is_dir:
- return (False, '%s is a directory')
- return (True, '')
+ return False, '%s is a directory'
+ return True, ''
def check_file(self, file):
chk = self.is_file(file)
if not chk[0]:
self.fail(chk[1], file)
return chk[0]
+
def check_no_file(self, file):
chk = self.is_file(file)
if chk[0]:
self.fail('File %s does exist', file)
- return not chk[0]
def check_binary(self, file):
- path = 'bin/%s' % (file)
+ path = 'bin/%s' % file
if not self.check_file(path):
- return False
+ return
if not self._provider.get(path).is_exec:
self.fail('%s is not executable', path)
- return False
- return True
def check_binary_symlink(self, file):
- path = 'bin/%s' % (file)
+ path = 'bin/%s' % file
fs_object = self._provider.get(path)
if fs_object is None:
self.fail('Could not find %s', path)
- return False
+ return
if fs_object.is_dir:
self.fail('%s is a directory', path)
- return False
+ return
if not fs_object.is_symlink:
self.fail('%s is not a symlink', path)
- return False
- return True
def check_single_library(self, file):
- res1 = self.is_file('lib/%s' % (file))
- res2 = self.is_file('lib64/%s' % (file))
+ res1 = self.is_file('lib/%s' % file)
+ res2 = self.is_file('lib64/%s' % file)
if not res1[0] and not res2[0]:
self.fail('Library missing: %s', file)
- return False
- return True
def check_no_library(self, file):
- res1 = self.is_file('lib/%s' % (file))
- res2 = self.is_file('lib64/%s' % (file))
+ res1 = self.is_file('lib/%s' % file)
+ res2 = self.is_file('lib64/%s' % file)
if res1[0] or res2[0]:
self.fail('Library exists: %s', file)
- return False
- return True
def check_java_library(self, file):
- return self.check_file('javalib/%s' % (file))
+ return self.check_file('javalib/%s' % file)
# Just here for docs purposes, even if it isn't good Python style.
@@ -263,71 +264,67 @@
class Arch32Checker(Checker):
- def __init__(self, provider):
- super().__init__(provider)
-
def check_multilib_binary(self, file):
- return all([self.check_binary('%s32' % (file)),
- self.check_no_file('bin/%s64' % (file)),
- self.check_binary_symlink(file)])
+ self.check_binary('%s32' % file)
+ self.check_no_file('bin/%s64' % file)
+ self.check_binary_symlink(file)
def check_library(self, file):
# TODO: Use $TARGET_ARCH (e.g. check whether it is "arm" or "arm64") to improve
# the precision of this test?
- return all([self.check_file('lib/%s' % (file)), self.check_no_file('lib64/%s' % (file))])
+ self.check_file('lib/%s' % file)
+ self.check_no_file('lib64/%s' % file)
def check_first_library(self, file):
- return self.check_library(file)
+ self.check_library(file)
def check_prefer32_binary(self, file):
- return self.check_binary('%s32' % (file))
+ self.check_binary('%s32' % file)
class Arch64Checker(Checker):
- def __init__(self, provider):
- super().__init__(provider)
-
def check_multilib_binary(self, file):
- return all([self.check_no_file('bin/%s32' % (file)),
- self.check_binary('%s64' % (file)),
- self.check_binary_symlink(file)])
+ self.check_no_file('bin/%s32' % file)
+ self.check_binary('%s64' % file)
+ self.check_binary_symlink(file)
def check_library(self, file):
# TODO: Use $TARGET_ARCH (e.g. check whether it is "arm" or "arm64") to improve
# the precision of this test?
- return all([self.check_no_file('lib/%s' % (file)), self.check_file('lib64/%s' % (file))])
+ self.check_no_file('lib/%s' % file)
+ self.check_file('lib64/%s' % file)
def check_first_library(self, file):
- return self.check_library(file)
+ self.check_library(file)
def check_prefer32_binary(self, file):
- return self.check_binary('%s64' % (file))
+ self.check_binary('%s64' % file)
class MultilibChecker(Checker):
- def __init__(self, provider):
- super().__init__(provider)
-
def check_multilib_binary(self, file):
- return all([self.check_binary('%s32' % (file)),
- self.check_binary('%s64' % (file)),
- self.check_binary_symlink(file)])
+ self.check_binary('%s32' % file)
+ self.check_binary('%s64' % file)
+ self.check_binary_symlink(file)
def check_library(self, file):
# TODO: Use $TARGET_ARCH (e.g. check whether it is "arm" or "arm64") to improve
# the precision of this test?
- return all([self.check_file('lib/%s' % (file)), self.check_file('lib64/%s' % (file))])
+ self.check_file('lib/%s' % file)
+ self.check_file('lib64/%s' % file)
def check_first_library(self, file):
- return all([self.check_no_file('lib/%s' % (file)), self.check_file('lib64/%s' % (file))])
+ self.check_no_file('lib/%s' % file)
+ self.check_file('lib64/%s' % file)
def check_prefer32_binary(self, file):
- return self.check_binary('%s32' % (file))
+ self.check_binary('%s32' % file)
class ReleaseChecker:
def __init__(self, checker):
self._checker = checker
+
def __str__(self):
return 'Release Checker'
@@ -392,9 +389,11 @@
self._checker.check_java_library('bouncycastle.jar')
self._checker.check_java_library('apache-xml.jar')
+
class ReleaseTargetChecker:
def __init__(self, checker):
self._checker = checker
+
def __str__(self):
return 'Release (Target) Checker'
@@ -407,9 +406,11 @@
self._checker.check_library('libpac.so')
self._checker.check_library('libz.so')
+
class ReleaseHostChecker:
def __init__(self, checker):
- self._checker = checker;
+ self._checker = checker
+
def __str__(self):
return 'Release (Host) Checker'
@@ -421,9 +422,11 @@
self._checker.check_library('libicuuc-host.so')
self._checker.check_library('libz-host.so')
+
class DebugChecker:
def __init__(self, checker):
self._checker = checker
+
def __str__(self):
return 'Debug Checker'
@@ -452,9 +455,11 @@
# Check that the mounted image contains additional required debug libraries.
self._checker.check_library('libadbconnectiond.so')
+
class DebugTargetChecker:
def __init__(self, checker):
self._checker = checker
+
def __str__(self):
return 'Debug (Target) Checker'
@@ -463,124 +468,138 @@
self._checker.check_binary('oatdump')
self._checker.check_first_library('libart-disassembler.so')
-def print_list(provider):
- def print_list_impl(provider, path):
- map = provider.read_dir(path)
- if map is None:
- return
- map = dict(map)
- if '.' in map:
- del map['.']
- if '..' in map:
- del map['..']
- for (_, val) in sorted(map.items()):
- new_path = os.path.join(path, val.name)
- print(new_path)
- if val.is_dir:
- print_list_impl(provider, new_path)
- print_list_impl(provider, '')
-def print_tree(provider, title):
- def get_vertical(has_next_list):
- str = ''
- for v in has_next_list:
- str += '%s ' % ('│' if v else ' ')
- return str
- def get_last_vertical(last):
- return '└── ' if last else '├── ';
- def print_tree_impl(provider, path, has_next_list):
- map = provider.read_dir(path)
- if map is None:
- return
- map = dict(map)
- if '.' in map:
- del map['.']
- if '..' in map:
- del map['..']
- key_list = list(sorted(map.keys()))
- for i in range(0, len(key_list)):
- val = map[key_list[i]]
- prev = get_vertical(has_next_list)
- last = get_last_vertical(i == len(key_list) - 1)
- print('%s%s%s' % (prev, last, val.name))
- if val.is_dir:
- has_next_list.append(i < len(key_list) - 1)
- print_tree_impl(provider, os.path.join(path, val.name), has_next_list)
- has_next_list.pop()
- print('%s' % (title))
- print_tree_impl(provider, '', [])
+class List:
+ def __init__(self, provider):
+ self._provider = provider
+ self._path = ''
+
+ def print_list(self):
+ apex_map = self._provider.read_dir(self._path)
+ if apex_map is None:
+ return
+ apex_map = dict(apex_map)
+ if '.' in apex_map:
+ del apex_map['.']
+ if '..' in apex_map:
+ del apex_map['..']
+ for (_, val) in sorted(apex_map.items()):
+ self._path = os.path.join(self._path, val.name)
+ print(self._path)
+ if val.is_dir:
+ self.print_list()
+
+
+class Tree:
+ def __init__(self, provider, title):
+ print('%s' % title)
+ self._provider = provider
+ self._path = ''
+ self._has_next_list = []
+
+ @staticmethod
+ def get_vertical(has_next_list):
+ string = ''
+ for v in has_next_list:
+ string += '%s ' % ('│' if v else ' ')
+ return string
+
+ @staticmethod
+ def get_last_vertical(last):
+ return '└── ' if last else '├── '
+
+ def print_tree(self):
+ apex_map = self._provider.read_dir(self._path)
+ if apex_map is None:
+ return
+ apex_map = dict(apex_map)
+ if '.' in apex_map:
+ del apex_map['.']
+ if '..' in apex_map:
+ del apex_map['..']
+ key_list = list(sorted(apex_map.keys()))
+ for i, val in enumerate(key_list):
+ prev = self.get_vertical(self._has_next_list)
+ last = self.get_last_vertical(i == len(key_list) - 1)
+ print('%s%s%s' % (prev, last, val.name))
+ if val.is_dir:
+ self._has_next_list.append(i < len(key_list) - 1)
+ self._path = os.path.join(self._path, val.name)
+ self.print_tree()
+ self._has_next_list.pop()
+
# Note: do not sys.exit early, for __del__ cleanup.
-def artApexTestMain(args):
- if args.tree and args.debug:
+def art_apex_test_main(test_args):
+ if test_args.tree and test_args.debug:
logging.error("Both of --tree and --debug set")
return 1
- if args.list and args.debug:
+ if test_args.list and test_args.debug:
logging.error("Both of --list and --debug set")
return 1
- if args.list and args.tree:
+ if test_args.list and test_args.tree:
logging.error("Both of --list and --tree set")
return 1
- if not args.tmpdir:
+ if not test_args.tmpdir:
logging.error("Need a tmpdir.")
return 1
- if not args.host and not args.debugfs:
+ if not test_args.host and not test_args.debugfs:
logging.error("Need debugfs.")
return 1
- if args.bitness not in ['32', '64', 'multilib', 'auto']:
+ if test_args.bitness not in ['32', '64', 'multilib', 'auto']:
logging.error('--bitness needs to be one of 32|64|multilib|auto')
try:
- if args.host:
- apex_provider = HostApexProvider(args.apex, args.tmpdir)
+ if test_args.host:
+ apex_provider = HostApexProvider(test_args.apex, test_args.tmpdir)
else:
- apex_provider = TargetApexProvider(args.apex, args.tmpdir, args.debugfs)
- except Exception as e:
+ apex_provider = TargetApexProvider(test_args.apex, test_args.tmpdir, test_args.debugfs)
+ except (zipfile.BadZipFile, zipfile.LargeZipFile) as e:
logging.error('Failed to create provider: %s', e)
return 1
- if args.tree:
- print_tree(apex_provider, args.apex)
+ if test_args.tree:
+ Tree(apex_provider, test_args.apex).print_tree()
return 0
- if args.list:
- print_list(apex_provider)
+ if test_args.list:
+ List(apex_provider).print_list()
return 0
checkers = []
- if args.bitness == 'auto':
- logging.warn('--bitness=auto, trying to autodetect. This may be incorrect!')
+ if test_args.bitness == 'auto':
+ logging.warning('--bitness=auto, trying to autodetect. This may be incorrect!')
has_32 = apex_provider.get('lib') is not None
has_64 = apex_provider.get('lib64') is not None
if has_32 and has_64:
- logging.warn(' Detected multilib')
- args.bitness = 'multilib'
+ logging.warning(' Detected multilib')
+ test_args.bitness = 'multilib'
elif has_32:
- logging.warn(' Detected 32-only')
- args.bitness = '32'
+ logging.warning(' Detected 32-only')
+ test_args.bitness = '32'
elif has_64:
- logging.warn(' Detected 64-only')
- args.bitness = '64'
+ logging.warning(' Detected 64-only')
+ test_args.bitness = '64'
else:
logging.error(' Could not detect bitness, neither lib nor lib64 contained.')
- print('%s' % (apex_provider._folder_cache))
+ print('%s' % apex_provider.folder_cache)
return 1
- if args.bitness == '32':
+ if test_args.bitness == '32':
base_checker = Arch32Checker(apex_provider)
- elif args.bitness == '64':
+ elif test_args.bitness == '64':
base_checker = Arch64Checker(apex_provider)
else:
- assert args.bitness == 'multilib'
+ assert test_args.bitness == 'multilib'
base_checker = MultilibChecker(apex_provider)
checkers.append(ReleaseChecker(base_checker))
- if args.host:
+ if test_args.host:
checkers.append(ReleaseHostChecker(base_checker))
else:
checkers.append(ReleaseTargetChecker(base_checker))
- if args.debug:
+ if test_args.debug:
checkers.append(DebugChecker(base_checker))
- if args.debug and not args.host:
+ if test_args.debug and not test_args.host:
checkers.append(DebugTargetChecker(base_checker))
failed = False
@@ -596,31 +615,32 @@
return 1 if failed else 0
-def artApexTestDefault(parser):
- if not 'ANDROID_PRODUCT_OUT' in os.environ:
+
+def art_apex_test_default(test_parser):
+ if 'ANDROID_PRODUCT_OUT' not in os.environ:
logging.error('No-argument use requires ANDROID_PRODUCT_OUT')
sys.exit(1)
product_out = os.environ['ANDROID_PRODUCT_OUT']
- if not 'ANDROID_HOST_OUT' in os.environ:
+ if 'ANDROID_HOST_OUT' not in os.environ:
logging.error('No-argument use requires ANDROID_HOST_OUT')
sys.exit(1)
host_out = os.environ['ANDROID_HOST_OUT']
- args = parser.parse_args(['dummy']) # For consistency.
- args.debugfs = '%s/bin/debugfs' % (host_out)
- args.tmpdir = '.'
- args.tree = False
- args.list = False
- args.bitness = 'auto'
+ test_args = test_parser.parse_args(['dummy']) # For consistency.
+ test_args.debugfs = '%s/bin/debugfs' % host_out
+ test_args.tmpdir = '.'
+ test_args.tree = False
+ test_args.list = False
+ test_args.bitness = 'auto'
failed = False
- if not os.path.exists(args.debugfs):
+ if not os.path.exists(test_args.debugfs):
logging.error("Cannot find debugfs (default path %s). Please build it, e.g., m debugfs",
- args.debugfs)
+ test_args.debugfs)
sys.exit(1)
# TODO: Add host support
- configs= [
+ configs = [
{'name': 'com.android.runtime.release', 'debug': False, 'host': False},
{'name': 'com.android.runtime.debug', 'debug': True, 'host': False},
]
@@ -628,20 +648,19 @@
for config in configs:
logging.info(config['name'])
# TODO: Host will need different path.
- args.apex = '%s/system/apex/%s.apex' % (product_out, config['name'])
- if not os.path.exists(args.apex):
+ test_args.apex = '%s/system/apex/%s.apex' % (product_out, config['name'])
+ if not os.path.exists(test_args.apex):
failed = True
- logging.error("Cannot find APEX %s. Please build it first.", args.apex)
+ logging.error("Cannot find APEX %s. Please build it first.", test_args.apex)
continue
- args.debug = config['debug']
- args.host = config['host']
- exit_code = artApexTestMain(args)
- if exit_code != 0:
- failed = True
+ test_args.debug = config['debug']
+ test_args.host = config['host']
+ failed = art_apex_test_main(test_args) != 0
if failed:
sys.exit(1)
+
if __name__ == "__main__":
parser = argparse.ArgumentParser(description='Check integrity of a Runtime APEX.')
@@ -660,12 +679,12 @@
parser.add_argument('--bitness', help='Bitness to check, 32|64|multilib|auto', default='auto')
if len(sys.argv) == 1:
- artApexTestDefault(parser)
+ art_apex_test_default(parser)
else:
args = parser.parse_args()
if args is None:
sys.exit(1)
- exit_code = artApexTestMain(args)
+ exit_code = art_apex_test_main(args)
sys.exit(exit_code)