[PATCH] D9600: Add scan-build python implementation

Jonathan Roelofs via cfe-commits cfe-commits at lists.llvm.org
Sat Nov 21 15:32:45 PST 2015



On 11/21/15 9:50 AM, Devin Coughlin wrote:
> 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.

Oh, good point. This is also important if we're cross building the 
toolchain... this library should get built with the same compiler that's 
building clang.

>
> ================ 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
>
>
>

-- 
Jon Roelofs
jonathan at codesourcery.com
CodeSourcery / Mentor Embedded


More information about the cfe-commits mailing list