Multiroot #3815

bruntib · 2023-01-12T13:50:07Z

[cmd] Multiroot directory analysis support

Currently the input of analysis is a compilation database JSON file. The
purpose of this PR is to support the following analysis invocations:

# Analyze one source file.
CodeChecker analyze main.c -o reports

#analyze all source files under a directory.
CodeChecker analyze my_project -o reports

This way the analysisa project with multiple root directories (i.e.
separate compilation databases for submodules in subdirectories)
is supported.

bruntib · 2023-01-12T13:52:47Z

This shouldn't be merged until #3807 is merged first.

Szelethus · 2023-01-13T11:09:35Z

I see you have proper descriptions in the commit messages, as well as a proper title -- could you copy paste them here?

vodorok

Please see my remarks

vodorok · 2023-01-23T10:24:06Z

analyzer/codechecker_analyzer/arg.py

+    otherwise raises an argparse.ArgumentTypeError which constitutes in a
+    graceful error message automatically by argparse module.
+    """
+    path = os.path.abspath(path)


Shouldn't we use pathlib? That lib has good multiplatform support.

We are using os.path module for handling paths. pathlib is an alternative module. I can see the advantages of its usage, but I think we shouldn't mix them in the code base. For example this existing_abspath() function is returning a path. Should it be a string or a pathlib.Path object? Should this be adapted at the caller side transitively?

vodorok · 2023-01-23T10:39:05Z

analyzer/codechecker_analyzer/analyzers/clangsa/config_handler.py

@@ -28,7 +28,6 @@ def __init__(self, environ):
        super(ClangSAConfigHandler, self).__init__()
        self.ctu_dir = ''
        self.ctu_on_demand = False
-        self.log_file = ''


Was this never used?

This member was stored in the construct_config_handler() function, but it is read nowhere. Since the store operation has been removed, this variable became completely unused.

vodorok · 2023-01-23T10:47:07Z

analyzer/codechecker_analyzer/cmd/analyze.py

-                             "files which were created during the build. "
-                             "The analyzers will check only the files "
-                             "registered in these build databases.")
+    parser.add_argument('input',


Shouldn't this change replicated in the check command? There is a logfile argument there too.

CodeChecker check command has a slightly different interface, for example it doesn't have this positional argument. It can be given the compilation database through -l flag, or a build command can be given by -b flag.

vodorok · 2023-01-23T10:51:19Z

analyzer/codechecker_analyzer/compilation_database.py

+    Traverse the parent directories of the given path and find the closest
+    compile_commands.json. If no JSON file exists with this name up to the root
+    then None returns.
+    The path of the first compilation database is returned even is it doesn't


... even if it doesn't ..

vodorok · 2023-01-23T10:53:05Z

analyzer/codechecker_analyzer/compilation_database.py

+
+    while True:
+        path = os.path.dirname(path)
+        compile_commands_json = os.path.join(path, COMPILATION_DATABASE)


I am wondering if should we hard code "compile_commands.json" as the used compilation database name.

In another language I'd say, this is a constant variable. The intention here was to express that "compile_commands.json" is a conventionally used name by other tools (e.g. CMake exports it with this name, Clangd and vim plugins are looking for this automatically, etc.). I just wanted to prevent potential typos at the places of usage. This variable is used at several places.

vodorok · 2023-01-23T11:36:00Z

analyzer/codechecker_analyzer/compilation_database.py

+        os.path.splitext(source_file_path)[1] in C_CPP_OBJC_OBJCPP_EXTS
+
+
+def build_actions_for_file(file_path: str) -> List[Dict]:


find_build_actions_for_source_file?

vodorok · 2023-01-23T11:41:29Z

analyzer/codechecker_analyzer/compilation_database.py

+
+    elif os.path.isdir(analysis_input):
+        compilation_database_files = \
+            find_all_compilation_databases(analysis_input)


I am not entirely sure why the per file and the directory based method has different compDB discovery implementations.
Isn/t the single source file case just a specialized (restricted in a sense) case of the directiry based?

This was the original implementation, that you described. The problem was its extreme bad performance. I couldn't wait for the compilation database collection for analyzing LLVM source code. That's why the directory-based analysis is initiated from the compilation databases, not the source files.

vodorok · 2023-01-23T11:47:45Z

analyzer/tests/unit/test_compilation_database.py

+            "file": "inner.c"
+        }]
+
+        with open(cls.comp_db_outer, "w",


Maybe an inner function the create the compdb-s could be created.

vodorok · 2023-01-23T11:51:30Z

analyzer/tests/unit/test_compilation_database.py

+        Test if the correct set of build actions return to each test source
+        file.
+
+        WARNING! compilation_database.gather_compilation_database() function is


Interesting consideration. I am thinking maybe we could somehow indicate in the assertion messages the path of the used compilation database? This way we could at least detect if something is wrong with the test environment.

vodorok · 2023-01-23T12:08:01Z

docs/analyzer/user_guide.md

-                        databases.
+  input                 The input of the analysis can be either a compilation
+                        database JSON file, a path to a source file or a path
+                        to a directory containing source files.


Collected some more places where analyze input is mentioned:
It is at your discretion how much you edit.

README.md

71, 116

analyzer/user_guide.md
-859, 910

usage.md
-125
The numbers are line numbers where the new mention of the new option is missing.

Currently the input of analysis is a compilation database JSON file. The purpose of this PR is to support the following analysis invocations: ``` # Analyze one source file. CodeChecker analyze main.c -o reports #analyze all source files under a directory. CodeChecker analyze my_project -o reports ``` This way the analysisa project with multiple root directories (i.e. separate compilation databases for submodules in subdirectories) is supported.

vodorok · 2023-02-03T13:26:36Z

LGTM

bruntib added enhancement 🌟 WIP 💣 Work In Progress CLI 💻 Related to the command-line interface, such as the cmd, store, etc. commands analyzer 📈 Related to the analyze commands (analysis driver) labels Jan 12, 2023

bruntib added this to the release 6.22.0 milestone Jan 12, 2023

bruntib requested review from dkrupp, martong and vodorok as code owners January 12, 2023 13:50

bruntib force-pushed the multiroot branch from cefd7b4 to 18d6044 Compare January 12, 2023 13:53

Szelethus self-requested a review January 13, 2023 11:09

whisperity marked this pull request as draft January 17, 2023 15:00

bruntib force-pushed the multiroot branch from 18d6044 to 08cb1db Compare January 19, 2023 14:21

bruntib mentioned this pull request Jan 19, 2023

Custom AST Provision #3800

Closed

bruntib force-pushed the multiroot branch from 08cb1db to 9f3982b Compare January 20, 2023 15:16

bruntib removed the WIP 💣 Work In Progress label Jan 20, 2023

bruntib marked this pull request as ready for review January 23, 2023 10:13

vodorok requested changes Jan 23, 2023

View reviewed changes

bruntib force-pushed the multiroot branch from 9f3982b to ea143bb Compare January 24, 2023 16:49

bruntib requested a review from vodorok January 24, 2023 16:50

bruntib force-pushed the multiroot branch from ea143bb to 871ef7e Compare January 25, 2023 08:43

vodorok approved these changes Feb 3, 2023

View reviewed changes

vodorok merged commit 5cbe27b into Ericsson:master Feb 6, 2023

bruntib deleted the multiroot branch February 6, 2023 13:39

bruntib mentioned this pull request Feb 6, 2023

CodeChecker to support clang-tools like compilation database lookup (multi-root workspace) #3708

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multiroot #3815

Multiroot #3815

bruntib commented Jan 12, 2023 •

edited

Loading

bruntib commented Jan 12, 2023

Szelethus commented Jan 13, 2023

vodorok left a comment

vodorok Jan 23, 2023

bruntib Jan 24, 2023

vodorok Jan 23, 2023

bruntib Jan 24, 2023

vodorok Jan 23, 2023

bruntib Jan 24, 2023

vodorok Jan 23, 2023

vodorok Jan 23, 2023

bruntib Jan 24, 2023

vodorok Jan 23, 2023

vodorok Jan 23, 2023

bruntib Jan 24, 2023

vodorok Jan 23, 2023

vodorok Jan 23, 2023

vodorok Jan 23, 2023

vodorok commented Feb 3, 2023

		os.path.splitext(source_file_path)[1] in C_CPP_OBJC_OBJCPP_EXTS


		def build_actions_for_file(file_path: str) -> List[Dict]:

Multiroot #3815

Multiroot #3815

Conversation

bruntib commented Jan 12, 2023 • edited Loading

bruntib commented Jan 12, 2023

Szelethus commented Jan 13, 2023

vodorok 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

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

vodorok commented Feb 3, 2023

bruntib commented Jan 12, 2023 •

edited

Loading