New subcommand infrastructure + splitting out log command #610

whisperity · 2017-03-10T16:00:41Z

This PR introduces a new command infrastructure to help in further compartmentalising CodeChecker which begun in #584. Closes #593.

Overview

New entrypoints

New CodeChecker entrypoints are introduced, following the "usual scheme" set out by the giants ahead of us. In line with, e.g. how Git works (git log and git-log are both commands), we will now have each "part"/"subcommand"/"feature" of CodeChecker as a direct entry point, such as codechecker-log. This, in the future, will help us create more dynamic helps, such as man pages for a Debian package, or a TAB autocompletion support (#576).

In reality, codechecker-log and CodeChecker log has the same behaviour, codechecker-log is just a directly callable wrapper script that is also put in the PATH.

Putting the argument list and the executed function together

AKA: The decline of arg_handler.py. 😉

libcodechecker/arg_handler.py is a voloptous patchwork, which imports everything from all parts of CodeChecker. While the executed code are in this file, their help and argument definitions are in bin/CodeChecker.py...

From now on, subcommands of CodeChecker will be located in their own file, libcodechecker/log.py, which has an easy interface.

get_argparser_ctor_args returns a dict which specifies how the subcommand argument parser should be constructed
add_arguments_to_parser(parser) is the function which puts the arguments into the constructed argument parser.
main(args) executes the method of the subcommand. (Here args is a parsed argparse.Namespace.)

Dynamic package building and loading

The CodeChecker.py script will now dynamically load the available subcommands' help and argument list. This dynamic list is created at package build via the install script. However, if the user supplies a valid subcommand (either calling CodeChecker log or codechecker-log, the two are equal), the helpers, and executed actions (and thus, every library module that executed function needs) for all the other commands will not be loaded.

While CodeChecker is already really fast on most machines, I hope this will help to increase speed even more, and this will definitely help to further compartmentalise CodeChecker.

All the above, as of this PR, only applies to CodeChecker log, the remaining commands are untouched, for now.

Helpers

Just like how the test framework is given a "bootstrap script" to create new test cases, I'm introducing scripts/create_new_subcommand.py which, when called with a single subcommand-name argument, will create, in the development directory, the neccessary skeleton for a new command, CodeChecker subcommand-name (or codechecker-subcommand-name).

Also, as a minor change, I've updated the CodeChecker log's help message a bit.

whisperity · 2017-03-13T10:02:16Z

Due to the log command already being refurbished and thus we nevertheless would need to rebase this, I am introducing the fix for #580 here. Once again, thank you @rizsotto for the prompt reply at rizsotto/scan-build#79.

gyorb · 2017-03-14T08:54:25Z

bin/CodeChecker.py

-    main()
+    # Load the available CodeChecker subcommands.
+    # This list is generated dynamically by scripts/build_package.py
+    version_cfg = os.path.join(os.environ['CC_PACKAGE_ROOT'],


We should not depend on environment variables. I try to remove them. I think we should hardcode the path here or create a better solution for it. (move it into a python module and import it?)

gyorb · 2017-03-14T09:05:01Z

libcodechecker/log/host_check.py

+    try:
+        res = subprocess.call(intercept_cmd,
+                              env=env,
+                              stdout=subprocess.PIPE,


There is a note not to use subprocess.PIPE in the call because it can deadlock. popen and communicate is the recommended solution.

We don't even need to properly communicate with intercept-build here, we only need to see if the command returns properly.

gyorb · 2017-03-14T09:08:39Z

scripts/build_package.py

@@ -639,9 +639,35 @@ def build_package(repository_root, build_package_config, env=None):
    time_now = time.strftime("%Y-%m-%dT%H:%M")
    version_json_data['package_build_date'] = time_now

+    version_json_data['available_commands'] = []


Why do we put the subcommands into the version config file?

The version file is the one which contains the most build-time created information, such at git describe and the like.

I felt it is appropriate to put the information here as the list of available commands is a property of a particular build taken, ie. users could arbitrarily add or remove commands before building their package.

rizsotto · 2017-03-14T10:08:13Z

libcodechecker/analyze/log_parser.py

+        elif 'arguments' in entry:
+            # Newest versions of intercept-build create an argument vector
+            # instead of a command string.
+            command = ' '.join(entry['arguments'])


When the argument list is ["one", "two three"] then you concatenate it as you do it here, you won't be able to get the original list back with split. You need to annotate the list items better. (Or if you won't do the split by yourself, the called shell will do it. So you need shell escaping. How portable is that?) The main driver to change this in intercept-build was to get rid of these concatenation and splitting errors. The current compilation database parser in Clang understands the arguments filed... So the question, why don't you pass the command around as a list instead of a string?

The changes in this patch are specifically around creating a new architecture for the commands. There are already at least one issue about argument list escaping, #505 comes to mind. The moment we start to refurbish the check command, we'll look into these issues.

Right now, ld-logger puts custom arguments from

CodeChecker check -n run -b "g++ -DVAR='va lu e' main.cpp -o a.out"

to the build.json as: \"-DVAR=va lu e\" but these values – and I've checked it just now – do not reach the analyzer command.

In case of intercept-build, however, -DVAR=va reaches the analyzer. I will now make sure the same escape sequence is applied to the command here, and we will discuss proper argument forwarding later.

Not sure that i get the question... the point i was trying to rise was that when you read the compilation database, it's better to keep arguments as list (don't concatenate it) and if it was a command make it as a list (with shlex.split or something clever)... So either way, you get a list of arguments... So, you can call subprocess.call or subprocess.check_call without shell=True. (Which will make your code more portable and get rid of the problem i've mentioned before.)

For the record --- I've already removed the question because I found the solution meanwhile, the question was:

How would you pass an argument list to clang, as right now the build.json never directly reaches the analyzer?

The build database is parsed and filtered, and from that do we create the analyzer invocation.

I've investigated the issue and came up with the solution that works both for intercept-build and ld-logger, while also esentially fixing the aforementioned issue #505. Please see #631.

Xazax-hun

I have one question, otherwise looks good.

Xazax-hun · 2017-03-16T14:47:26Z

bin/codechecker-log

+CC = os.path.join(THIS_PATH, "CodeChecker")
+
+# Load CodeChecker from the current folder (the wrapper script (without .py))
+CodeChecker = imp.load_source('CodeChecker', CC)


What is the reason for using imp here? Can't we load it with the regular import syntax?

No. The folder where the environment bootstrapper resides is not in the PYTHONPATH. Also it's not a real Python package either, and importing from relative paths outside a package is an error, and inside a package is still discouraged.

See, for example, this question on SO. We couldn't use from ..bin because we can't make assumptions on where the CC scripts that go into the users' PATH are installed.

gyorb · 2017-03-23T11:17:31Z

Could we squash some of the commits?

some text formating only (could be part of another)
the main part of his pull request the log command changes... (maybe squash some of them to be more in one group)
intercept-build changes (I think this should be at the end/beginning of the commits and not mixed together with the log command changes)

whisperity · 2017-03-23T11:30:29Z

Sure, done. 😉

…e paths

whisperity mentioned this pull request Mar 10, 2017

Cleanup command line arguments #415

Closed

3 tasks

gyorb suggested changes Mar 14, 2017

View reviewed changes

rizsotto reviewed Mar 14, 2017

View reviewed changes

whisperity force-pushed the new_log_command branch 2 times, most recently from 7497433 to b90acea Compare March 16, 2017 14:02

Xazax-hun approved these changes Mar 16, 2017

View reviewed changes

whisperity force-pushed the new_log_command branch from 3b50fbc to 23b495d Compare March 23, 2017 11:30

whisperity force-pushed the new_log_command branch from 23b495d to 8930d8c Compare March 23, 2017 13:33

whisperity added 4 commits March 23, 2017 14:36

Create new dynamically loaded subcommand system

4cb96fc

Helper scripts for bootstrapping a new subcommand

485d609

codechecker-log: the first subcommand moved to new system

6cc0131

Fix intercept-build invocation and add support for argvector, relativ…

4137434

…e paths

whisperity force-pushed the new_log_command branch from 8930d8c to 4137434 Compare March 23, 2017 13:37

gyorb approved these changes Mar 23, 2017

View reviewed changes

gyorb merged commit 063a0f6 into Ericsson:master Mar 23, 2017

whisperity deleted the new_log_command branch March 23, 2017 16:55

whisperity mentioned this pull request Apr 3, 2017

Update log command in the user guide #624

Merged

This was referenced Apr 13, 2017

Fix escaping multiword string literals, once again #631

Merged

intercept build error handling #580

Open

Refurbished checker listing and detail view #634

Merged

whisperity mentioned this pull request Apr 25, 2017

New user commands for analysis #639

Merged

whisperity mentioned this pull request May 10, 2017

Finish subcommand revamping #638

Closed

17 tasks

whisperity mentioned this pull request Jul 6, 2017

update compilation command handling #704

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New subcommand infrastructure + splitting out log command #610

New subcommand infrastructure + splitting out log command #610

whisperity commented Mar 10, 2017

whisperity commented Mar 13, 2017

gyorb Mar 14, 2017

gyorb Mar 14, 2017

whisperity Mar 14, 2017

gyorb Mar 14, 2017

whisperity Mar 14, 2017

rizsotto Mar 14, 2017 •

edited

Loading

whisperity Mar 14, 2017

rizsotto Apr 13, 2017

whisperity Apr 13, 2017 •

edited

Loading

Xazax-hun left a comment

Xazax-hun Mar 16, 2017

whisperity Mar 16, 2017

gyorb commented Mar 23, 2017

whisperity commented Mar 23, 2017

New subcommand infrastructure + splitting out log command #610

New subcommand infrastructure + splitting out log command #610

Conversation

whisperity commented Mar 10, 2017

Overview

New entrypoints

Putting the argument list and the executed function together

Dynamic package building and loading

Helpers

whisperity commented Mar 13, 2017

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

rizsotto Mar 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whisperity Apr 13, 2017 • edited Loading

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

gyorb commented Mar 23, 2017

whisperity commented Mar 23, 2017

rizsotto Mar 14, 2017 •

edited

Loading

whisperity Apr 13, 2017 •

edited

Loading