[PATCH] D9600: Add scan-build python implementation

[Devin Coughlin via cfe-commits]

dcoughlin added a comment.

Thanks Laszlo!

Is there a more descriptive name than "intercept-build" (I realize scan-build is pretty general too). It seems to me the point of the intercept-build tool is to generate the compilation database. I think it would be helpful if the tool name indicated that rather than the *method* used to to generate the database. I don't have any great suggestions: "compilation-database-build", "log-build", "log-compilation-build", ...

A couple more comments inline.


================
Comment at: tools/scan-build-py/MANIFEST.in:1
@@ +1,2 @@
+include README.md
+include *.txt
----------------
rizsotto.mailinglist wrote:
> dcoughlin wrote:
> > How about this one? Is it needed in clang trunk?
> in order to make this code a standalone python tool tool, we need this file. (see llvm/utils/lit directory for example.)
Can you explain why this is needed? We didn't need one for the perl scan-build nor for SATestBuild.py/SATestAdd.py?

A difference between lit and scan-build is that scan-build is not a standalone tool. Are you envisioning users installing scan-build without installing clang?

================
Comment at: tools/scan-build-py/libear/__init__.py:1
@@ +1,2 @@
+# -*- coding: utf-8 -*-
+#                     The LLVM Compiler Infrastructure
----------------
jroelofs wrote:
> rizsotto.mailinglist wrote:
> > dcoughlin wrote:
> > > How does this file fit into the overall build picture? Will this file go away once scan-build-py is built with the common clang cmake?
> > this is quiet confusing me. previously you were asking make it work without installation. this file makes it possible to compile the `ear` library compiled before the build runs to use as preloaded library. the thing which is not needed is the CMakefile actually.
> I think the best way forward would be to teach the CMake build how to run `setup.py`.  Then this would work both with and without installation, and it'd use the same code paths for both.
> previously you were asking make it work without installation.

Yes. It is very important to support that workflow since many users have multiple versions of clang on their systems at the same time.

> this file makes it possible to compile the ear library compiled before the build runs to use as preloaded library. 

Shouldn't this be done as part of the build process and not as part of installation? 

Can the interception library (eventually) be built as part of the global CMake build? It seems quite fragile to have custom build logic in a python file.

================
Comment at: tools/scan-build-py/libear/__init__.py:1
@@ +1,2 @@
+# -*- coding: utf-8 -*-
+#                     The LLVM Compiler Infrastructure
----------------
dcoughlin wrote:
> jroelofs wrote:
> > rizsotto.mailinglist wrote:
> > > dcoughlin wrote:
> > > > How does this file fit into the overall build picture? Will this file go away once scan-build-py is built with the common clang cmake?
> > > this is quiet confusing me. previously you were asking make it work without installation. this file makes it possible to compile the `ear` library compiled before the build runs to use as preloaded library. the thing which is not needed is the CMakefile actually.
> > I think the best way forward would be to teach the CMake build how to run `setup.py`.  Then this would work both with and without installation, and it'd use the same code paths for both.
> > previously you were asking make it work without installation.
> 
> Yes. It is very important to support that workflow since many users have multiple versions of clang on their systems at the same time.
> 
> > this file makes it possible to compile the ear library compiled before the build runs to use as preloaded library. 
> 
> Shouldn't this be done as part of the build process and not as part of installation? 
> 
> Can the interception library (eventually) be built as part of the global CMake build? It seems quite fragile to have custom build logic in a python file.
jroelofs wrote:

> I think the best way forward would be to teach the CMake build how to run setup.py

What is the benefit of running `setup.py` and treating scan-build as a python package? It seems to me that the fact that scan-build is written in python should be an implementation detail. Most importantly, installing scan-build shouldn't muck up the user's python environment.

In my opinion, it would be more user-friendly to install into the new scan-build install hierarchy that jroelofs recently added. This way all the scan-build stuff is under one directory that the user can blow away rather than hidden deep in the bowels of some python environment. This makes it much easier to ensure that scan-build and clang are in sync and would prevent the user from accidentally installing scan-build into a virtualenv (which doesn't make sense because clang doesn't live there).

================
Comment at: tools/scan-build-py/libscanbuild/driver.py:1
@@ +1,2 @@
+# -*- coding: utf-8 -*-
+#                     The LLVM Compiler Infrastructure
----------------
rizsotto.mailinglist wrote:
> dcoughlin wrote:
> > Why is this file called "driver"?
> any recommendation? it was the only entry point before the interposition was introduced. so it was the driver of the libscanbuild library.
How about naming the files containing entry points after the name of the command-line tool that that invokes the entry point? Or moving the entry-point code to the command-line tool so only shared code is in modules? As it stands, it is hard to tell what code is called by what tool.

================
Comment at: tools/scan-build-py/libscanbuild/intercept.py:98
@@ +97,3 @@
+
+    if args.override_compiler or not ear_library_path:
+        environment.update({
----------------
rizsotto.mailinglist wrote:
> dcoughlin wrote:
> > What is the purpose of setting CC/CXX environmental variables here? Doesn't libear intercept calls to the compiler? Why is intercept-cc/cxx needed?
> documentation added. this branch is used when preload does not work. (eg.: windows builds or failed to compile `libear` for unknown reasons.)
Is there an indication to the user that preload didn't work? Do you think there should be one? (Or is this an implementation detail that users do not need to be aware of?)

================
Comment at: tools/scan-build-py/libscanbuild/runner.py:113
@@ +112,3 @@
+ at require(['clang', 'analyze', 'directory', 'output'])
+def run_analyzer(opts, continuation=report_failure):
+    """ It assembles the analysis command line and executes it. Capture the
----------------
rizsotto.mailinglist wrote:
> dcoughlin wrote:
> > What is the point of "continuation"?
> this module use continuation to pass the current state and the method to call. i find this abstraction much better than create class methods (or a single method as it in the Perl implementation). Specially better if you want to test these methods!
It would be better to decompose larger tasks into individually testable subtasks and test the output of each of these individually. There is no need for continuations here. Please remove them.

================
Comment at: tools/scan-build-py/libscanbuild/shell.py:1
@@ +1,2 @@
+# -*- coding: utf-8 -*-
+#                     The LLVM Compiler Infrastructure
----------------
rizsotto.mailinglist wrote:
> dcoughlin wrote:
> > I'm surprised there is not a library to do this.
> > 
> > Also, maybe a better name for this module  is "shell_escaping".
> the system library package called `shlex` and it has only the decode method. and failed on many of the real life tests. (in previous version i used that.)
> 
> i don't find `shell_escaping` a _better_ name. specially when i think how the method names would be: `shell_escaping.encode`. i think `shell.encode` is more descriptive.
I found it confusing that the shell module didn't call the shell. You module comment says: "This module implements basic shell escaping/unescaping methods." Please use a name for the file that communicates that intent. It doesn't haven to be "shell_escaping", but "shell" is not descriptive enough. This is important so that someone who is unfamiliar with the code base can quickly tell whether the file is relevant to the task at hand.


http://reviews.llvm.org/D9600