Comment on attachment 186914 [details] [review]
patch

This should be expanded to also work with other module types, and it needs some documentation.

Also, "/tmp/" shouldn't be hardcoded, even as a default value, and that temporary directory shouldn't be fixed as several modules could be building in parallel (in a buildbot configuration for example).

And I believe it will also need a "supports-scan-build" attribute tag, to make it possible for a moduleset to exclude a module from being analysed (similar to "supports-non-srcdir-builds").

Comment on attachment 186914 [details] [review]
patch

I think a new scan phase should be added, rather than adding it to the build phase.

Re: Other module types
I don't know that it will work for other modules as I only know of it supporting xcode, autotools, and make

Re: /tmp
I chose it because I know jhbuild is used on non-POSIX compliant OSes and am not certain if they will have $TMPDIR set.  I suppose I can use $TMPDIR ? $TMPDIR : /tmp

Re: collisions
Each module is placed in a subdirectory, so different modules can't collide, and scan-build puts results into datestamped, versioned directories, so subsequent runs will be placed in separate subdirectories.

Re: Module exclusion
That is already there for jhbuildrc as module_scan_build.  I can't imagine a case in which an autotools project would not support scan-build, and if there were, it would probably be a bug with scan-build rather than an issue with the project itself.

Re: phase
It would be nice if there were a phase to upload the results during an autobuild (we're working on rsyncing the results and picking the URL out of the build log), but the scanner runs during build, so I don't think that's very plausible.

Created attachment 186951 [details] [review]
patch v2

Updated patch to address some concerns above

Sorry I was not quick enough in answering, for the temporary directory you should use tempfile.gettempdir()

> I can't imagine a case in which an autotools project would not support scan-build, and if there
> were, it would probably be a bug with scan-build rather than an issue with the project itself.

Actually the autotools module type has been mistreated in many ways, and it's often used with projects that mimick in some ways autotools.

Created attachment 186999 [details] [review]
patch v3

Updated to use tempfile.gettempdir()
Added support for static-analyzer attribute which defaults to true

Can I please get more feedback about my latest patch?  If it is acceptable, can it please be pushed?

Thanks.

Created attachment 187602 [details] [review]
patch v4

Also prepend to make check

Cool feature: Just turn on the static-analyzer configuration and get analysis.

There is one problem with static_analyzer_outputdir. It has to be created if it does not exist. Otherwise, it falls back to /tmp/...
Something like:

diff --git i/jhbuild/modtypes/autotools.py w/jhbuild/modtypes/autotools.py
index 3db23a3..ae13821 100644
--- i/jhbuild/modtypes/autotools.py
+++ w/jhbuild/modtypes/autotools.py
@@ -223,6 +223,9 @@ class AutogenModule(Package, DownloadableModule):
         if self.supports_static_analyzer and buildscript.config.module_static_analyzer.get(self.name, buildscript.config.static_analyzer):
             template = buildscript.config.static_analyzer_template
 
+            if not os.path.exists(buildscript.config.static_analyzer_outputdir):
+                os.makedirs(buildscript.config.static_analyzer_outputdir)
+
             vars = {'outputdir': buildscript.config.static_analyzer_outputdir,
                     'module': self.name
                    }

Yeah, I noticed that, but I was of the opinion that that was a bug in clang (but I never got around to filing it).

That being said, it's probably a good idea to do that in jhbuild as well to sanity-check ... you might want to die if os.makedirs fails though.

Oh, right. It creates only the last directory.

Created attachment 190689 [details] [review]
patch v4 with conflict resolution

Created attachment 190690 [details] [review]
clang: Create the output root directory if necessary

Created attachment 197716 [details] [review]
rebased to current master and squashed the mkdir followup into it

Updated to current master.  ping.

Quick and hopefully last comments I'll have; sorry this is fell off my radar.

Has this been tested with jhbuild new default of running with make -jN ?

Also, the required trailing space in 'static_analyzer_template' will definitely cause problem, I'd suggest appending that space in the static_analyzer_pre_cmd method.

And could the xml attribute be named supports-static-analyzer? (dashes, not underscores).

Created attachment 197895 [details] [review]
0001-Support-running-scan-build-Clang-Static-Analyzer-wit.patch

Updated patch to address comments.  I use -j1 in my tinderbox, so it has been tested with -jN.  If you're curious about it working with parallel (ie N != 1) builds, I can test that for you, but it should work fine.

Pushed, thanks for your work. It would be really great if that could somehow be integratd with the buildbot support…

It would be great if this was described in the jhbuild manual.

Created attachment 260400 [details] [review]
autotools: Fix XML attribute name for ‘supports-static-analyzer’

The rest of the code references ‘supports-static-analyzer’, but the code
which actually reads the XML was using ‘static-analyzer’. Standardise on
the former.

Created attachment 260401 [details] [review]
doc: Add documentation for the static analysis feature

Document the configuration file variables, the moduleset attribue, and
general usage of the feature.

Reopening as per comment #19.

(Reviewed by fredp on IRC — thanks!)

Attachment 260400 [details] pushed as f5ac582 - autotools: Fix XML attribute name for ‘supports-static-analyzer’
Attachment 260401 [details] pushed as 6be8596 - doc: Add documentation for the static analysis feature