From 42330a10cf245e4749370a74dabf98c98b2c19a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A1rton=20Csord=C3=A1s?= Date: Thu, 2 Aug 2018 14:43:30 +0200 Subject: [PATCH] Refactoring instance manager --- libcodechecker/analyze/log_parser.py | 30 +++++---- libcodechecker/server/instance_manager.py | 75 ++++++++++++----------- libcodechecker/util.py | 21 ++++--- tests/unit/test_buildcmd_escaping.py | 8 +-- tests/unit/test_log_parser.py | 46 +++++++------- 5 files changed, 92 insertions(+), 88 deletions(-) diff --git a/libcodechecker/analyze/log_parser.py b/libcodechecker/analyze/log_parser.py index 338267bfe3..68719a5130 100644 --- a/libcodechecker/analyze/log_parser.py +++ b/libcodechecker/analyze/log_parser.py @@ -224,9 +224,9 @@ def remove_file_if_exists(filename): os.remove(filename) -def parse_compile_commands_json(logfile, parseLogOptions): +def parse_compile_commands_json(log_data, parseLogOptions): """ - logfile: is a compile command json + log_data: content of a compile command json. """ output_path = parseLogOptions.output_path @@ -239,13 +239,11 @@ def parse_compile_commands_json(logfile, parseLogOptions): actions = [] filtered_build_actions = {} - data = load_json_or_empty(logfile, {}) - compiler_includes = {} compiler_target = {} counter = 0 - for entry in data: + for entry in log_data: sourcefile = entry['file'] if not os.path.isabs(sourcefile): @@ -364,17 +362,17 @@ def parse_log(logfilepath, parseLogOptions): """ LOG.debug('Parsing log file: ' + logfilepath) - with open(logfilepath) as logfile: - try: - actions = parse_compile_commands_json(logfile, parseLogOptions) - except (ValueError, KeyError, TypeError) as ex: - if os.stat(logfilepath).st_size == 0: - LOG.error('The compile database is empty.') - else: - LOG.error('The compile database is not valid.') - LOG.debug(traceback.format_exc()) - LOG.debug(ex) - sys.exit(1) + try: + data = load_json_or_empty(logfilepath, {}) + actions = parse_compile_commands_json(data, parseLogOptions) + except (ValueError, KeyError, TypeError) as ex: + if os.stat(logfilepath).st_size == 0: + LOG.error('The compile database is empty.') + else: + LOG.error('The compile database is not valid.') + LOG.debug(traceback.format_exc()) + LOG.debug(ex) + sys.exit(1) LOG.debug('Parsing log file done.') diff --git a/libcodechecker/server/instance_manager.py b/libcodechecker/server/instance_manager.py index cfaa774701..353fad06ff 100644 --- a/libcodechecker/server/instance_manager.py +++ b/libcodechecker/server/instance_manager.py @@ -14,30 +14,31 @@ import getpass import json import os -import portalocker import psutil import socket import stat +import portalocker + from libcodechecker.util import load_json_or_empty -def __getInstanceDescriptorPath(folder=None): +def __get_instance_descriptor_path(folder=None): if not folder: folder = os.path.expanduser("~") return os.path.join(folder, ".codechecker.instances.json") -def __makeInstanceDescriptorFile(folder=None): - descriptor = __getInstanceDescriptorPath(folder) +def __make_instance_descriptor_file(folder=None): + descriptor = __get_instance_descriptor_path(folder) if not os.path.exists(descriptor): with open(descriptor, 'w') as f: json.dump([], f) os.chmod(descriptor, stat.S_IRUSR | stat.S_IWUSR) -def __checkInstance(hostname, pid): +def __check_instance(hostname, pid): """Check if the given process on the system is a valid, running CodeChecker for the current user.""" @@ -57,32 +58,34 @@ def __checkInstance(hostname, pid): return False -def __rewriteInstanceFile(append, remove, folder=None): - """This helper method reads the user's instance descriptor and manages it - eliminating dead records, appending new ones and reserialising the file.""" +def __rewrite_instance_file(append, remove, folder=None): + """ + This helper method reads the user's instance descriptor and manages it + eliminating dead records, appending new ones and re-serialising the file. + """ + __make_instance_descriptor_file(folder) + + append_pids = [i['pid'] for i in append] - __makeInstanceDescriptorFile(folder) - with open(__getInstanceDescriptorPath(folder), 'r+') as f: - portalocker.lock(f, portalocker.LOCK_EX) + # After reading, check every instance if they are still valid and + # make sure PID does not collide accidentally with the + # to-be-registered instances, if any exists in the append list as it + # would cause duplication. + # + # Also, we remove the records to the given PIDs, if any exists. + instances = [i for i in get_instances(folder) + if i['pid'] not in append_pids and + (i['hostname'] + ":" + str(i['pid'])) not in remove] - # After reading, check every instance if they are still valid and - # make sure PID does not collide accidentally with the - # to-be-registered instances, if any exists in the append list as it - # would cause duplication. - # - # Also, we remove the records to the given PIDs, if any exists. - append_pids = [i['pid'] for i in append] - instances = [i for i in json.load(f) - if i['pid'] not in append_pids and - (i['hostname'] + ":" + str(i['pid'])) not in remove and - __checkInstance(i['hostname'], i['pid'])] + with open(__get_instance_descriptor_path(folder), 'w') as instance_file: + portalocker.lock(instance_file, portalocker.LOCK_EX) instances = instances + append - f.seek(0) - f.truncate() - json.dump(instances, f, indent=2) - portalocker.unlock(f) + instance_file.seek(0) + instance_file.truncate() + json.dump(instances, instance_file, indent=2) + portalocker.unlock(instance_file) def register(pid, workspace, port, folder=None): @@ -91,12 +94,12 @@ def register(pid, workspace, port, folder=None): descriptor. """ - __rewriteInstanceFile([{"pid": pid, - "hostname": socket.gethostname(), - "workspace": workspace, - "port": port}], - [], - folder) + __rewrite_instance_file([{"pid": pid, + "hostname": socket.gethostname(), + "workspace": workspace, + "port": port}], + [], + folder) def unregister(pid, folder=None): @@ -105,7 +108,9 @@ def unregister(pid, folder=None): descriptor. """ - __rewriteInstanceFile([], [socket.gethostname() + ":" + str(pid)], folder) + __rewrite_instance_file([], + [socket.gethostname() + ":" + str(pid)], + folder) def get_instances(folder=None): @@ -113,7 +118,7 @@ def get_instances(folder=None): # This method does NOT write the descriptor file. - descriptor = __getInstanceDescriptorPath(folder) + descriptor = __get_instance_descriptor_path(folder) instances = load_json_or_empty(descriptor, {}, lock=True) - return [i for i in instances if __checkInstance(i['hostname'], i['pid'])] + return [i for i in instances if __check_instance(i['hostname'], i['pid'])] diff --git a/libcodechecker/util.py b/libcodechecker/util.py index a5211b8bc4..7de0dce9ce 100644 --- a/libcodechecker/util.py +++ b/libcodechecker/util.py @@ -13,21 +13,21 @@ import argparse import datetime import hashlib +import io import json import os -import portalocker import re import shutil import signal import socket import stat import subprocess -import sys import tempfile import uuid from threading import Timer +import portalocker import psutil from libcodechecker.logger import get_logger @@ -570,7 +570,7 @@ def load_json_or_empty(path, default=None, kind=None, lock=False): ret = default try: - with open(path, 'r') as handle: + with io.open(path, 'r') as handle: if lock: portalocker.lock(handle, portalocker.LOCK_SH) @@ -578,13 +578,18 @@ def load_json_or_empty(path, default=None, kind=None, lock=False): if lock: portalocker.unlock(handle) - except IOError as ex: - LOG.warning("Failed to open {0} file: {1}" - .format(kind if kind else 'json', path)) + except OSError as ex: + LOG.warning("Failed to open %s file: %s", + kind if kind else 'json', + path) LOG.warning(ex) except ValueError as ex: - LOG.warning("'{1}' is not a valid {0} file." - .format(kind if kind else 'json', path)) + LOG.warning("'%s' is not a valid %s file.", + kind if kind else 'json', + path) + LOG.warning(ex) + except TypeError as ex: + LOG.warning('Failed to process json file: %s', path) LOG.warning(ex) return ret diff --git a/tests/unit/test_buildcmd_escaping.py b/tests/unit/test_buildcmd_escaping.py index 2e0a957e9f..1fc549f778 100644 --- a/tests/unit/test_buildcmd_escaping.py +++ b/tests/unit/test_buildcmd_escaping.py @@ -74,8 +74,7 @@ def __get_cmp_json(self, buildcmd): "command": buildcmd + " -c " + self.src_file_path, "file": self.src_file_path} - compile_cmds = [compile_cmd] - return json.dumps(compile_cmds) + return [compile_cmd] def __get_comp_actions(self, compile_cmd): """ @@ -83,9 +82,8 @@ def __get_comp_actions(self, compile_cmd): to return the compilation actions. """ comp_cmd_json = self.__get_cmp_json(compile_cmd) - with closing(StringIO(comp_cmd_json)) as text: - return log_parser.parse_compile_commands_json(text, - ParseLogOptions()) + return log_parser.parse_compile_commands_json(comp_cmd_json, + ParseLogOptions()) def test_buildmgr(self): """ diff --git a/tests/unit/test_log_parser.py b/tests/unit/test_log_parser.py index e7b6394118..2a19ea9dba 100644 --- a/tests/unit/test_log_parser.py +++ b/tests/unit/test_log_parser.py @@ -150,26 +150,26 @@ def test_omit_preproc(self): """ Compiler preprocessor actions should be omitted. """ - preprocessor_actions = StringIO('''[ + preprocessor_actions = [ {"directory": "/tmp", - "command": "g++ /tmp/a.cpp -c /tmp/a.cpp", - "file": "/tmp/a.cpp" }, + "command": "g++ /tmp/a.cpp -c /tmp/a.cpp", + "file": "/tmp/a.cpp"}, {"directory": "/tmp", - "command": "g++ /tmp/a.cpp -E /tmp/a.cpp", - "file": "/tmp/a.cpp" }, + "command": "g++ /tmp/a.cpp -E /tmp/a.cpp", + "file": "/tmp/a.cpp"}, {"directory": "/tmp", - "command": "g++ /tmp/a.cpp -MT /tmp/a.cpp", - "file": "/tmp/a.cpp" }, + "command": "g++ /tmp/a.cpp -MT /tmp/a.cpp", + "file": "/tmp/a.cpp"}, {"directory": "/tmp", - "command": "g++ /tmp/a.cpp -MM /tmp/a.cpp", - "file": "/tmp/a.cpp" }, + "command": "g++ /tmp/a.cpp -MM /tmp/a.cpp", + "file": "/tmp/a.cpp"}, {"directory": "/tmp", - "command": "g++ /tmp/a.cpp -MF /tmp/a.cpp", - "file": "/tmp/a.cpp" }, + "command": "g++ /tmp/a.cpp -MF /tmp/a.cpp", + "file": "/tmp/a.cpp"}, {"directory": "/tmp", - "command": "g++ /tmp/a.cpp -M /tmp/a.cpp", - "file": "/tmp/a.cpp" }] - ''') + "command": "g++ /tmp/a.cpp -M /tmp/a.cpp", + "file": "/tmp/a.cpp"}] + build_actions = \ log_parser.parse_compile_commands_json(preprocessor_actions, ParseLogOptions()) @@ -182,11 +182,10 @@ def test_keep_compile_and_dep(self): """ Keep the compile command if -MD is set. Dependency generation is done as a side effect of the compilation. """ - preprocessor_actions = StringIO('''[ + preprocessor_actions = [ {"directory": "/tmp", - "command": "g++ /tmp/a.cpp -MD /tmp/a.cpp", - "file": "/tmp/a.cpp" }] - ''') + "command": "g++ /tmp/a.cpp -MD /tmp/a.cpp", + "file": "/tmp/a.cpp"}] build_actions = \ log_parser.parse_compile_commands_json(preprocessor_actions, @@ -197,14 +196,13 @@ def test_keep_compile_and_dep(self): def test_omit_dep_with_e(self): """ Skip the compile command if -MD is set together with -E. """ - preprocessor_actions = StringIO('''[ + preprocessor_actions = [ {"directory": "/tmp", - "command": "g++ /tmp/a.cpp -MD -E /tmp/a.cpp", - "file": "/tmp/a.cpp" }, + "command": "g++ /tmp/a.cpp -MD -E /tmp/a.cpp", + "file": "/tmp/a.cpp"}, {"directory": "/tmp", - "command": "g++ /tmp/a.cpp -E -MD /tmp/a.cpp", - "file": "/tmp/a.cpp" } ] - ''') + "command": "g++ /tmp/a.cpp -E -MD /tmp/a.cpp", + "file": "/tmp/a.cpp"}] build_actions = \ log_parser.parse_compile_commands_json(preprocessor_actions,