[PATCH] D24040: codechecker tool core

[Laszlo Nagy via cfe-commits]

rizsotto.mailinglist added a comment.

Gyorgy and the ericsson team, thanks for doing this. very good job! good targeted functionality. i don't want to underestimate the complexity it requires, but to me this is a giant code. i do miss the explanation of the overall functional description what a module does and how it relates to other modules. i found a gap between the high level overview and the code comments.
generally speaking i found this code under documented and a bit verbose. the comments in the code is not really paint the picture that i need in order to fix a bug, or implement a feature. and it might only be my personal taste, but found that creating types (or classes this case) in a dynamically typed script language, it makes the code very verbose while it does not implement much.
was commented only on two random modules, but found the same patterns in many other ones.
found that this code is not `pep8` conform. `pylint` found a lot of errors and warnings, and a couple of duplicate codes.
i hope we can fix these findings and that would make this code more solid and understood by others.


================
Comment at: tools/codechecker/bin/CodeChecker:35
@@ +34,3 @@
+
+    python = os.path.join('python')
+    common_lib = os.path.join(package_root,
----------------
wow, that looks woodoo. can you explain (in code comment) why is that necessary?

================
Comment at: tools/codechecker/bin/CodeChecker:67
@@ +66,3 @@
+
+    original_env = os.environ.copy()
+    checker_env = original_env
----------------
can you explain (in code comment) why are you playing with the environment? what problem does it solve and why do you need a backup?

================
Comment at: tools/codechecker/bin/CodeChecker:70
@@ +69,3 @@
+
+    tmp_dir = tempfile.mkdtemp()
+
----------------
can you backport `tempfile.TemporaryDirectory` (from python 3.2) in one of your modules and use it inside `with` for proper resource guarding?

================
Comment at: tools/codechecker/bin/CodeChecker:85
@@ +84,3 @@
+        pid = os.getpid()
+        for p in procPool:
+            os.kill(p, signal.SIGINT)
----------------
why not store the `subprocess.Popen` object and send signal via `send_signal`? (that would make it more portable too.)

================
Comment at: tools/codechecker/libcodechecker/build_manager.py:30
@@ +29,3 @@
+    """
+    proc = subprocess.Popen(command,
+                            bufsize=-1,
----------------
is there any reason not to use `subprocess.call` instead? if you want that to be in silent, you shall pass `{'stdout': subprocess.PIPE, 'stderr': subprocess.STDOUT}` to it, otherwise it prints the output without you doing the copy. also noticed the `shell=True` which makes it very non-portable. and the `command` parameter is a string instead of a list. is there any reason for doing like this?

================
Comment at: tools/codechecker/libcodechecker/build_manager.py:55
@@ +54,3 @@
+
+    try:
+        original_env_file = os.environ['CODECHECKER_ORIGINAL_BUILD_ENV']
----------------
save-to-file and restore-from-file the environment would deserve a separate module i would say. is there any reason why it's not having those utility methods?

================
Comment at: tools/codechecker/libcodechecker/build_manager.py:131
@@ +130,3 @@
+            return None
+    except AttributeError as ex:
+        # args.log_file was not set
----------------
why not have a command line parameter accessor method, that would either return the value if that was given or None? i found this pattern multiple files. that would get rid of the `try` block completely.

================
Comment at: tools/codechecker/libcodechecker/build_manager.py:150
@@ +149,3 @@
+
+            if intercept_build_executable == None:
+                if platform.system() == 'Linux':
----------------
`if intercept_build_executable is None:` would be more python.

================
Comment at: tools/codechecker/libcodechecker/build_manager.py:169
@@ +168,3 @@
+
+            open(log_file, 'a').close()  # same as linux's touch
+
----------------
the comment is not really helpful!
why does it need a touch? or you just create an empty one? what for?

================
Comment at: tools/codechecker/libcodechecker/build_manager.py:177
@@ +176,3 @@
+    except AttributeError as aerr:
+        LOG.error(aerr)
+        sys.exit(1)
----------------
`logger.exception` is logging the exception value implicitly at `ERROR` level too. but what i'm trying to say is, there is no error message in this line. you just throw the exception into the user face. can you add some explanation or instruction for the user?


https://reviews.llvm.org/D24040