Add compiler info to result directory #1050

martong · 2017-10-17T16:42:21Z

Add the results of gcc -v like invocations as files to the result dir.
This is needed for later local investigation of any failed analysis.

Add the results of gcc -v like invocations as files to the reult dir. This is needed for later local investigation of any failed analysis.

Xazax-hun

I have some high level questions and found some nits otherwise looks good.

Xazax-hun · 2017-10-18T09:56:52Z

libcodechecker/analyze/log_parser.py

+    for line in lines.splitlines(True):
+        line = line.strip()
+        if line.startswith(end_mark):
+            do_append = False


Can there be multiple occurrences of start_mark? If no, could we just break here?

This is the original code for parsing, unfortunately diff shows this as completely new code.
Still, I think we could just break here.

Xazax-hun · 2017-10-18T09:58:25Z

libcodechecker/analyze/log_parser.py

-
-    return target
+    err = get_compiler_err(cmd)
+    dump_compiler_info(output_path, "compiler_target.json",


Does it make sense to both dump compiler info when getting the target and when getting the includes?

Xazax-hun · 2017-10-18T09:59:57Z

libcodechecker/analyze/log_parser.py

+    return target
+
+
+def dump_compiler_info(output_path, filename, data):


If the file already exists, we just append to it.
Is it ensured that multiple concurrent checks (to different result directories) won't interfere?
When will we clean up this file? If we don't do that, this file will just grow with subsequent checks to the same result directory.

Good point, as we discussed I modifed the analyze.py to remove these files when starting a new analysis.

Xazax-hun · 2017-10-18T10:01:03Z

libcodechecker/analyze/log_parser.py

@@ -208,13 +243,13 @@ def parse_compile_commands_json(logfile, add_compiler_defaults=False):
    return actions


-def parse_log(logfilepath, add_compiler_defaults=False):
+def parse_log(logfilepath, output_path, add_compiler_defaults=False):


I think output_path worth a documentation, what this refers to.

Xazax-hun

Generally looks good. I was wondering if it would be cleaner to have only one json file.

Xazax-hun · 2017-10-20T14:26:32Z

libcodechecker/analyze/log_parser.py

-    return target
+def remove_file_if_exists(filename):
+    try:
+        os.remove(filename)  # throws if file does not exsist


We start comments with capital letters and terminate them with periods.

martong · 2017-10-20T14:45:44Z

libcodechecker/analyze/log_parser.py

+    if os.path.exists(filename):
+        append_write = 'a'
+    with open(filename, append_write) as f:
+        f.write(json.dumps(data))


Probably this will not be a proper json in case of multiple compilers.
We have to

read the file first with

then do a json.loads on the content

then add the new entry

then write back

martong · 2017-10-20T15:45:15Z

@Xazax-hun, as we discussed two files make the json handling code more compact for our benefit.

Xazax-hun

LGTM! But could you squash this into two commit? One for the feature one for the test.

whisperity

(Also, commits are many.)

whisperity · 2017-10-26T10:52:48Z

libcodechecker/analyze/log_parser.py

@@ -10,6 +10,8 @@
 import traceback
 import subprocess
 import shlex
+import json


Import order has been violated here.

whisperity · 2017-10-26T10:53:06Z

libcodechecker/analyze/log_parser.py

-    """
+
+def get_compiler_err(cmd):
+    '''


''' ➡️ """

whisperity · 2017-10-26T10:53:34Z

libcodechecker/analyze/log_parser.py

-    Returns a list of default includes of the given compiler.
-    """
+
+def get_compiler_err(cmd):


Also, don't we have a method for arbitrary command invocation and output saving somewhere in util? Or am I mixing it up with the test library?

there is util.call_command but that does not handle the stderr. Also this code is refactored from https://github.com/Ericsson/codechecker/pull/1050/files?diff=split#diff-3edc7a6cc15c42194786d15e8ab5852dL95

whisperity · 2017-10-26T10:54:15Z

libcodechecker/analyze/log_parser.py

+            # where we need to strip the "(framework directory)" string.
+            # For instance:
+            # /System/Library/Frameworks (framework directory)
+            fpos = line.find("(framework directory)")


string::find(substr) == -1 can be replaced with substr not in string, which is more Pythonic.

Ok, I agree.
Still, this is again not new code despite it looks like that in the diff; merge conflict resolution brought this in :(

whisperity · 2017-10-26T10:56:14Z

libcodechecker/analyze/log_parser.py

-    return target
+def remove_file_if_exists(filename):
+    try:
+        os.remove(filename)  # Throws if file does not exsist.


These comments are unneccessary here. Also, I recall we use the following form almost everywhere in the code.

if os.path.isfile(a): # Or exists() os.remove(a)

whisperity · 2017-10-26T10:57:04Z

libcodechecker/analyze/log_parser.py

    import json
    # The add-compiler-defaults is a deprecated argument
    # and we always perform target and include auto-detection.
    add_compiler_defaults = True
    LOG.debug('parse_compile_commands_json: ' + str(add_compiler_defaults))

+    if output_path is not None:
+        remove_file_if_exists(os.path.join(output_path,


Is it always applicable for incremental analysis to remove these files? What if analysis configuration changes?

It is not a problem. If the analysis configuration changes the compile database still remains the same. Compiler includes and targets do not depend on analysis configuration.

whisperity · 2017-10-26T10:58:37Z

tests/unit/test_buildcmd_escaping.py

@@ -38,7 +38,7 @@ def setup_class(cls):
        cls.src_file_name = "main.cpp"
        cls.src_file_path = os.path.join(cls.tmp_dir,
                                         cls.src_file_name)
-        cls.compiler = "g++"
+        cls.compiler = "clang++"


Why is this change introduced?

I think this is a bug which is present since a while. cls.compiler is called with --target and gcc7 exits with an error (return value not equal with 0), however older gcc like gcc 4.8 just gives a warning and returns with 0.

whisperity · 2017-10-26T10:59:45Z

tests/functional/analyze/test_analyze.py

+
+        # Create a compilation database.
+        build_log = [{"directory": self.test_workspace,
+                      "command": "gcc -c " + source_file,


Due to gcc not existing on macOS systems, this way of creating a build log on the fly is prone to break macOS tests. See #824.

I thought gcc is a symbolic link to clang on macOS. https://stackoverflow.com/questions/19535422/os-x-10-9-gcc-links-to-clang
Has this changed in systems newer than El Capitan?

I've no macOS machine, so I'm not sure. There was definitely no such thing as gcc and g++ on the Travis macOS instances a few months ago.

I am not sure if this is really a problem since there are several other uses like this in other test cases:

tests/functional/analyze/test_analyze.py|102 col 35| "command": "gcc -c " + source_file, tests/functional/analyze/test_analyze.py|172 col 35| "command": "gcc -c " + source_file, tests/functional/analyze/test_analyze.py|205 col 35| "command": "gcc -c " + source_file, tests/functional/analyze/test_analyze.py|240 col 35| "gcc -c " + source_file) tests/functional/analyze/test_analyze.py|303 col 35| "command": "gcc -c " + source_file, tests/functional/ctu/test_files/buildlog.json|4 col 17| "command": "gcc -c lib.c -o lib.o", tests/functional/ctu/test_files/buildlog.json|9 col 17| "command": "gcc -c main.c -o main.o",

in this case compilation.json is not created on the fly, but wired in into the test case. So it will not cause problem on Mac.

dkrupp · 2017-11-06T12:58:50Z

libcodechecker/analyze/log_parser.py

+    if os.path.exists(filename):
+        with open(filename, 'r') as f:
+            all_data = json.load(f)
+    all_data.update(data)


be careful, this will not add a new member in the hash, but update an existing one. Thus you will always have one element in the dict such as {'target': 'arm', 'compiler': 'gcc-arm'}

Fixed in a new squashed commit

dkrupp

LGTM.

On the long term however we may want to perform the compiler detection as a part of log and not analyze. The advantage would be that one would not need the presence of the compilers in the analysis phase (maybe the compilers exist only temporarily in the env).

martong · 2017-11-06T17:15:10Z

We could not squash the merge commit 3df5ecd , so for simplicity we keep the 4 commit.
Next time I will rebase and will not create a separate merge commit.

Gabor Marton added 3 commits October 17, 2017 18:07

Add compiler info to result directory

e0922ca

Add the results of gcc -v like invocations as files to the reult dir. This is needed for later local investigation of any failed analysis.

Fix wrong local setup for package_layout.json

2524369

Merge remote-tracking branch 'central/master'

3df5ecd

martong requested review from dkrupp, whisperity, gyorb and rnkovacs October 17, 2017 16:42

Xazax-hun suggested changes Oct 18, 2017

View reviewed changes

Xazax-hun suggested changes Oct 20, 2017

View reviewed changes

martong commented Oct 20, 2017

View reviewed changes

Xazax-hun approved these changes Oct 25, 2017

View reviewed changes

whisperity added the analyzer 📈 Related to the analyze commands (analysis driver) label Oct 26, 2017

whisperity requested changes Oct 26, 2017

View reviewed changes

martong force-pushed the master branch 3 times, most recently from 5134a67 to 6bb7c51 Compare November 6, 2017 10:10

dkrupp reviewed Nov 6, 2017

View reviewed changes

Fix bugs in 'Add compiler info to result directory' feature

bb381e7

martong force-pushed the master branch from 6bb7c51 to bb381e7 Compare November 6, 2017 14:02

dkrupp approved these changes Nov 6, 2017

View reviewed changes

martong merged commit cb144db into Ericsson:master Nov 6, 2017

gyorb added this to the release 6.2 milestone Nov 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add compiler info to result directory #1050

Add compiler info to result directory #1050

martong commented Oct 17, 2017 •

edited

Loading

Xazax-hun left a comment

Xazax-hun Oct 18, 2017

martong Oct 19, 2017

Xazax-hun Oct 18, 2017

Xazax-hun Oct 18, 2017

martong Oct 19, 2017 •

edited

Loading

Xazax-hun Oct 18, 2017

Xazax-hun left a comment

Xazax-hun Oct 20, 2017

martong Oct 20, 2017

martong commented Oct 20, 2017

Xazax-hun left a comment

whisperity left a comment

whisperity Oct 26, 2017

whisperity Oct 26, 2017

whisperity Oct 26, 2017

martong Nov 3, 2017

whisperity Oct 26, 2017

martong Nov 3, 2017

whisperity Oct 26, 2017

whisperity Oct 26, 2017

martong Oct 26, 2017

whisperity Oct 26, 2017

martong Oct 26, 2017 •

edited

Loading

whisperity Oct 26, 2017

martong Oct 26, 2017

whisperity Oct 26, 2017

martong Nov 6, 2017

dkrupp Nov 6, 2017

dkrupp Nov 6, 2017

martong Nov 6, 2017

dkrupp left a comment

martong commented Nov 6, 2017

		return target


		def dump_compiler_info(output_path, filename, data):

Add compiler info to result directory #1050

Add compiler info to result directory #1050

Conversation

martong commented Oct 17, 2017 • edited Loading

Xazax-hun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martong Oct 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xazax-hun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martong commented Oct 20, 2017

Xazax-hun left a comment

Choose a reason for hiding this comment

whisperity left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martong Oct 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkrupp left a comment

Choose a reason for hiding this comment

martong commented Nov 6, 2017

martong commented Oct 17, 2017 •

edited

Loading

martong Oct 19, 2017 •

edited

Loading

martong Oct 26, 2017 •

edited

Loading