904572 - Mach command to output clang compilation database

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Joshua Cranmer [:jcranmer]]

Attachment: First cut

The clang compilation database is how clang's tooling knows how to process source code. As moz.build learns more about the build system, we can use it to process the compilation database properly.

I'm marking dependencies on the variables which need to be migrated to moz.build for this to be useful.

Comment 1

[Joshua Cranmer [:jcranmer]]

Attachment: First cut

It would help if I used hg qref first. :-)

Comment 2

[Mike Hommey [:glandium]]

Do commands in the compilation database have to be "clang" commands? If not, until we have enough data in moz.build, it could use make -C $(dirname $@) $(basename $@). That would be slower, but that would also be using the right flags.
Also note bug 892973 has a mach compileflags command. I need to refresh that patch.

Comment 3

[Joshua Cranmer [:jcranmer]]

Apparently, compilation databases don't need to refer to clang. I set one up with c++/gcc as the compiler, and got it to at least parse a single file (with a lot of errors because I don't have MOZILLA_INTERNAL_API passed down yet).

Comment 4

[Mike Hommey [:glandium]]

(In reply to Joshua Cranmer [:jcranmer] from comment #3)
> Apparently, compilation databases don't need to refer to clang. I set one up
> with c++/gcc as the compiler, and got it to at least parse a single file
> (with a lot of errors because I don't have MOZILLA_INTERNAL_API passed down
> yet).

what about make -C dir file ?

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 789545 [details] [diff] [review]
First cut

Review of attachment 789545 [details] [diff] [review]:
-----------------------------------------------------------------

Mike Hommey showed me an early version of a patch he is working on that does things similar to this (it essentially collects all the compiler invocation commands). He was talking about landing that soonish. If that happens, I think it would make sense to rebase this patch on top of that.

Comment 6

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #5)
> (it essentially collects all the compiler invocation
> commands)

It actually doesn't, but the patch in bug 892973 kind of does.

Comment 7

[(no longer active)]

Are there any plans on how to move forward with this?

Comment 8

[Gregory Szorc [:gps]]

Not that I'm aware of. With concurrent tier execution, it probably wouldn't be too difficult to hack together a make target and Python script for writing invoked compiler commands to individual files and then recombining things into a compilation database. This is similar to the approach I took in http://hg.gregoryszorc.com/gecko-collab/rev/d6ea4c36aa65

Comment 9

[(no longer active)]

Hmm, but wrapping the compiler like that doesn't really require build system support, right?  I was mostly hoping for something that can generate the db from moz.build's directly.  Is this something that we can do now?  If yes, can you please point me to some examples/docs?  Thanks!

Comment 10

[Joshua Cranmer [:jcranmer]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #7)
> Are there any plans on how to move forward with this?

I've been planning on making this happen. To this end, I have a patch to move the -DMOZILLA_INTERNAL_API logic into moz.build that's in the works. I also have ideas on using glandium's templating proposal to move config.mk logic to moz.build logic, specifically to fix this bug.

Comment 11

[(no longer active)]

(In reply to Joshua Cranmer [:jcranmer] (high latency until August 11) from comment #10)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #7)
> > Are there any plans on how to move forward with this?
> 
> I've been planning on making this happen. To this end, I have a patch to
> move the -DMOZILLA_INTERNAL_API logic into moz.build that's in the works. I
> also have ideas on using glandium's templating proposal to move config.mk
> logic to moz.build logic, specifically to fix this bug.

Ah that's great!  /me holds breath.  :-)

(FWIW I'm planning to use this for some rewriting projects badly in need of being done.)

Comment 12

[Mike Hommey [:glandium]]

Yeah, we're not there yet, but we're getting close.

Comment 13

[Gregory Szorc [:gps]]

I don't think I can add any new information.

FTR, I would support a hacked up tier iteration mode that executed rules.mk with dummied-out rules so we could capture exact compiler commands. The proper solution involving converting config.mk logic to Python doesn't need to block forward momentum elsewhere.

Comment 14

[(no longer active)]

(In reply to Gregory Szorc [:gps] from comment #13)
> FTR, I would support a hacked up tier iteration mode that executed rules.mk
> with dummied-out rules so we could capture exact compiler commands. The
> proper solution involving converting config.mk logic to Python doesn't need
> to block forward momentum elsewhere.

So should I go ahead and implement this?

Comment 15

[Gregory Szorc [:gps]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #14)
> (In reply to Gregory Szorc [:gps] from comment #13)
> > FTR, I would support a hacked up tier iteration mode that executed rules.mk
> > with dummied-out rules so we could capture exact compiler commands. The
> > proper solution involving converting config.mk logic to Python doesn't need
> > to block forward momentum elsewhere.
> 
> So should I go ahead and implement this?

Run it by glandium first. I just want to make sure it won't conflict with other work he's planned for the quarter.

Comment 16

[Joshua Cranmer [:jcranmer]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #14)
> So should I go ahead and implement this?

If you go ahead and fix this bug, I could probably scrounge up some scripts to run this on try and build compilation databases for all of our bajillion build combos for some truly serious automated rewriting fun.

Comment 17

[(no longer active)]

Mike?

Comment 18

[Mike Hommey [:glandium]]

Feel free to go ahead.

Comment 19

[(no longer active)]

Attachment: Add support for generating clang compilation database

This patch generates a compilation database which seems to work well for the entire code base, honoring things such as host source files, unified sources, etc.

Comment 20

[:garvan]

Ehsan, this is great! Xcode project generation is trivial with this patch.

I tested the build commands, and compilation mostly succeeds.
Exceptions:
- It is missing some per-file flags in media/libvpx/vp9. 
- xpcom/build/Unified_cpp_xpcom_build2.cpp fails with the output below. If I add -include unistd.h, it builds (without this flag, when I dump the preprocessed output, unistd.h _is_ being included so I assume the problem is the include order)
Error output:
mozilla-central/xpcom/build/PoisonIOInterposerMac.cpp:187:15: error: use of undeclared identifier 'lseek'; did you mean 'fseek'?
  off_t pos = lseek(aFd, 0, SEEK_CUR);
              ^~~~~
              fseek
In file included from obj-ff-dbg/xpcom/build/Unified_cpp_xpcom_build2.cpp:2:
In file included from mozilla-central/xpcom/build/LateWriteChecks.cpp:7:
In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/algorithm:628:
In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/memory:604:
In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/iterator:335:
In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/iosfwd:90:
In file included from /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk/usr/include/wchar.h:90:
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk/usr/include/stdio.h:251:6: note: 'fseek' declared here
int      fseek(FILE *, long, int);
         ^

Comment 21

[(no longer active)]

(In reply to Garvan Keeley [:garvank] from comment #20)
> Ehsan, this is great! Xcode project generation is trivial with this patch.

Happy to hear it helps. :-)

> I tested the build commands, and compilation mostly succeeds.
> Exceptions:
> - It is missing some per-file flags in media/libvpx/vp9. 

Yeah.  I'm waiting on glandium to review this patch before I handle those...

> - xpcom/build/Unified_cpp_xpcom_build2.cpp fails with the output below. If I
> add -include unistd.h, it builds (without this flag, when I dump the
> preprocessed output, unistd.h _is_ being included so I assume the problem is
> the include order)
> Error output:
> mozilla-central/xpcom/build/PoisonIOInterposerMac.cpp:187:15: error: use of
> undeclared identifier 'lseek'; did you mean 'fseek'?
>   off_t pos = lseek(aFd, 0, SEEK_CUR);
>               ^~~~~
>               fseek
> In file included from obj-ff-dbg/xpcom/build/Unified_cpp_xpcom_build2.cpp:2:
> In file included from mozilla-central/xpcom/build/LateWriteChecks.cpp:7:
> In file included from
> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.
> xctoolchain/usr/bin/../include/c++/v1/algorithm:628:
> In file included from
> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.
> xctoolchain/usr/bin/../include/c++/v1/memory:604:
> In file included from
> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.
> xctoolchain/usr/bin/../include/c++/v1/iterator:335:
> In file included from
> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.
> xctoolchain/usr/bin/../include/c++/v1/iosfwd:90:
> In file included from
> /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/
> Developer/SDKs/MacOSX10.10.sdk/usr/include/wchar.h:90:
> /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/
> Developer/SDKs/MacOSX10.10.sdk/usr/include/stdio.h:251:6: note: 'fseek'
> declared here
> int      fseek(FILE *, long, int);
>          ^

Hmm, this shouldn't happen!  Can you dump out the exact command line that your code constructs?  Including the current working directory?

Comment 22

[:garvan]

The command is copied verbatim from compile_commands.json (run from anywhere, the output is the same, this uses full paths):

$grep Unified_cpp_xpcom_build2 compile_commands.json | grep command
"command": "/usr/local/bin/ccache clang++ -o /dev/null -c -fvisibility=hidden -DOS_POSIX=1 -DOS_MACOSX=1 -D_IMPL_NS_STRINGAPI -DOMNIJAR_NAME='omni.ja' -DSTATIC_EXPORTABLE_JS_API -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -DAB_CD=en-US -DNO_NSPR_10_SUPPORT -I/Users/gkeeley/c/mozilla-central/xpcom/build -I/Users/gkeeley/c/obj-ff-dbg/xpcom/build -I/Users/gkeeley/c/obj-ff-dbg/ipc/ipdl/_ipdlheaders -I/Users/gkeeley/c/obj-ff-dbg/xpcom -I/Users/gkeeley/c/mozilla-central/ipc/chromium/src -I/Users/gkeeley/c/mozilla-central/ipc/glue -I/Users/gkeeley/c/mozilla-central/xpcom/base -I/Users/gkeeley/c/mozilla-central/xpcom/components -I/Users/gkeeley/c/mozilla-central/xpcom/ds -I/Users/gkeeley/c/mozilla-central/xpcom/glue -I/Users/gkeeley/c/mozilla-central/xpcom/io -I/Users/gkeeley/c/mozilla-central/xpcom/reflect/xptinfo -I/Users/gkeeley/c/mozilla-central/xpcom/threads -I/Users/gkeeley/c/mozilla-central/chrome -I/Users/gkeeley/c/mozilla-central/docshell/base -I/Users/gkeeley/c/mozilla-central/media/libvpx -I/Users/gkeeley/c/obj-ff-dbg/dist/include -I/Users/gkeeley/c/obj-ff-dbg/dist/include/nspr -I/Users/gkeeley/c/obj-ff-dbg/dist/include/nss -fPIC -Qunused-arguments -DMOZILLA_CLIENT -include /Users/gkeeley/c/obj-ff-dbg/mozilla-config.h -MD -MP -MF /Users/gkeeley/c/obj-ff-dbg/xpcom/build/.deps/showbuild.pp -Qunused-arguments -Qunused-arguments -Wall -Wempty-body -Woverloaded-virtual -Wsign-compare -Wwrite-strings -Wno-invalid-offsetof -Wno-inline-new-delete -Wno-c++0x-extensions -Wno-extended-offsetof -Wno-unknown-warning-option -Wno-return-type-c-linkage -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -std=gnu++0x -pthread -DNO_X11 -pipe -DDEBUG -DTRACING -g -fno-omit-frame-pointer -DNO_X11 /Users/gkeeley/c/obj-ff-dbg/xpcom/build/Unified_cpp_xpcom_build2.cpp",

Comment 23

[:garvan]

Here is the terminal session of first running make on Unified_cpp_xpcom_build2, then running the command from comment 22:
https://pastebin.mozilla.org/8834081

The working and non-working commands are functionally identical, here is the graphical diff, there are 7 differences, the working command is on the left:
https://www.dropbox.com/s/h6eac2c92kgrckp/Screenshot%202015-05-20%2011.36.33.png?dl=0

Comment 24

[:garvan]

Questions:
1) I want to CompilationDatabase().compile_db(), then parse the json output.
I try to import CompilationDatabase in my backend and get:

mach.base.MachError: Cannot register a command to an undefined category: compilation-database -> devenv

Any quick tip on how to import this? 

2) Can you add `def get_output_file()` that returns the full path to the output?

Comment 25

[(no longer active)]

(In reply to Garvan Keeley [:garvank] from comment #24)
> Questions:
> 1) I want to CompilationDatabase().compile_db(), then parse the json output.
> I try to import CompilationDatabase in my backend and get:
> 
> mach.base.MachError: Cannot register a command to an undefined category:
> compilation-database -> devenv
> 
> Any quick tip on how to import this? 

I have no idea.  I don't really know Python...  :/  But you probably shouldn't parse the json, you should instead use the util.py functions to do what you want.

> 2) Can you add `def get_output_file()` that returns the full path to the
> output?

Yeah, we should be able to hack something up for that, but that shouldn't be needed for the purpose of code completion in Xcode.  Please file a follow-up though, I'd like to wait for the review before making changes to this code, in case the patch doesn't get accepted.

Comment 26

[:garvan]

What is the status of this bug? Glandium ni'd you because the patch is waiting for your review.

This patch is needed for xcode project generation (bug 1063329). This is the only method I have seen of getting the _exact_ build commands per file (with the order of arguments correct), which is required for xcode generation.

Comment 27

[Gregory Szorc [:gps]]

Comment on attachment 8597744 [details] [diff] [review]
Add support for generating clang compilation database

Review of attachment 8597744 [details] [diff] [review]:
-----------------------------------------------------------------

Here's a quick pass until glandium finds time to review.

You may want to pre-emptively rebase this on top of glandium SOURCES refactorings: it should make this code simpler.

::: build/mach_bootstrap.py
@@ +84,5 @@
>      'python/mozboot/mozboot/mach_commands.py',
>      'python/mozbuild/mozbuild/mach_commands.py',
>      'python/mozbuild/mozbuild/backend/mach_commands.py',
>      'python/mozbuild/mozbuild/compilation/codecomplete.py',
> +    'python/mozbuild/mozbuild/compilation/database.py',

Hmm. Not sure who introduced a mach command handler in codecomplete.py, but we intentionally try to isolate frontend mach command code in mach_commands.py files away from generic backend code.

::: python/mozbuild/mozbuild/compilation/database.py
@@ +15,5 @@
> +from mozbuild.base import MachCommandBase
> +from mozbuild.compilation import util
> +
> +@CommandProvider
> +class CompilationDatabase(MachCommandBase):

Please move the mach command bits out of this file and refactor this to be a standalone Python module. This entails moving all the imports to the top of the file.

@@ +29,5 @@
> +            HostSources,
> +            UnifiedSources,
> +            GeneratedSources,
> +        )
> +        import json

This can go at module level.

@@ +53,5 @@
> +                    flags = self._get_dir_flags(obj.objdir)
> +                    self._build_db_line(obj, self.config_environment, f[0],
> +                                        flags, False)
> +            elif isinstance(obj, Sources) or isinstance(obj, HostSources) or \
> +                 isinstance(obj, GeneratedSources):

glandium bitrots you in bug 991983.

@@ +92,5 @@
> +
> +    def _build_db_line(self, obj, cenv, filename, flags, ishost):
> +        # Distinguish between host and target files.
> +        prefix = 'HOST_' if ishost else ''
> +        if filename.endswith('.c') or filename.endswith('.m'):

Should we lowercase filename in case UPPERCASE extensions are used? Do we have any instances of that in the tree?

@@ +99,5 @@
> +            # Add the Objective-C flags if needed.
> +            if filename.endswith('.m'):
> +                cflags += ' ' + flags['COMPILE_CMFLAGS']
> +        elif filename.endswith('.cpp') or filename.endswith('.cc') or \
> +             filename.endswith('.cxx') or filename.endswith('.mm'):

elif filename.endswith(('.cpp', '.cc', '.cxx', '.mm')):

@@ +115,5 @@
> +            if not os.path.isfile(name):
> +                name = obj.objdir + '/' + filename
> +            if not os.path.isfile(name):
> +                raise Exception('Cannot find ' + filename)
> +            filename = name

This whole block becomes much easier after glandium's refactoring of SOURCES.

@@ +122,5 @@
> +          compiler +
> +          ' -o /dev/null -c ' + # The output file doesn't matter, so just make something up.
> +          cflags + ' ' +
> +          filename
> +        )

cmd = ' '.join([
    compiler,
    '-o /dev/null -c',
    cflags,
    filename,
])

Comment 28

[(no longer active)]

(In reply to Gregory Szorc [:gps] from comment #27)
> You may want to pre-emptively rebase this on top of glandium SOURCES
> refactorings: it should make this code simpler.

Which bug is that?

> ::: build/mach_bootstrap.py
> @@ +84,5 @@
> >      'python/mozboot/mozboot/mach_commands.py',
> >      'python/mozbuild/mozbuild/mach_commands.py',
> >      'python/mozbuild/mozbuild/backend/mach_commands.py',
> >      'python/mozbuild/mozbuild/compilation/codecomplete.py',
> > +    'python/mozbuild/mozbuild/compilation/database.py',
> 
> Hmm. Not sure who introduced a mach command handler in codecomplete.py, but

I did in bug 892973, at your request.  ;-)

> we intentionally try to isolate frontend mach command code in
> mach_commands.py files away from generic backend code.

Err, isn't that contradicting with bug 892973 comment 22.

> ::: python/mozbuild/mozbuild/compilation/database.py
> @@ +15,5 @@
> > +from mozbuild.base import MachCommandBase
> > +from mozbuild.compilation import util
> > +
> > +@CommandProvider
> > +class CompilationDatabase(MachCommandBase):
> 
> Please move the mach command bits out of this file and refactor this to be a
> standalone Python module. This entails moving all the imports to the top of
> the file.

Where do you want me to move it?

> @@ +53,5 @@
> > +                    flags = self._get_dir_flags(obj.objdir)
> > +                    self._build_db_line(obj, self.config_environment, f[0],
> > +                                        flags, False)
> > +            elif isinstance(obj, Sources) or isinstance(obj, HostSources) or \
> > +                 isinstance(obj, GeneratedSources):
> 
> glandium bitrots you in bug 991983.

Sigh :(

> @@ +92,5 @@
> > +
> > +    def _build_db_line(self, obj, cenv, filename, flags, ishost):
> > +        # Distinguish between host and target files.
> > +        prefix = 'HOST_' if ishost else ''
> > +        if filename.endswith('.c') or filename.endswith('.m'):
> 
> Should we lowercase filename in case UPPERCASE extensions are used? Do we
> have any instances of that in the tree?

Hmm, I don't think so.  This is how we process UNIFIED_SOURCES, for example.  (And IIRC SOURCES[foo].flags too.)

> @@ +115,5 @@
> > +            if not os.path.isfile(name):
> > +                name = obj.objdir + '/' + filename
> > +            if not os.path.isfile(name):
> > +                raise Exception('Cannot find ' + filename)
> > +            filename = name
> 
> This whole block becomes much easier after glandium's refactoring of SOURCES.

Can you please tell me what that refactoring does, and how I should change my code on top of it?

Comment 29

[Mike Hommey [:glandium]]

Comment on attachment 8597744 [details] [diff] [review]
Add support for generating clang compilation database

Review of attachment 8597744 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/mach_bootstrap.py
@@ +41,5 @@
>      'config',
>      'dom/bindings',
>      'dom/bindings/parser',
>      'layout/tools/reftest',
> +    'media/webrtc/trunk/tools/gyp/pylib',

This shouldn't be required.

@@ +84,5 @@
>      'python/mozboot/mozboot/mach_commands.py',
>      'python/mozbuild/mozbuild/mach_commands.py',
>      'python/mozbuild/mozbuild/backend/mach_commands.py',
>      'python/mozbuild/mozbuild/compilation/codecomplete.py',
> +    'python/mozbuild/mozbuild/compilation/database.py',

> Hmm. Not sure who introduced a mach command handler in codecomplete.py

Ehsan did, and you reviewed it ;) (1bfcd43acd3c)

::: python/mozbuild/mozbuild/compilation/database.py
@@ +43,5 @@
> +
> +        # Iterate through moz.build, dumping all the lines into the db
> +        reader = BuildReader(self.config_environment)
> +        emitter = TreeMetadataEmitter(self.config_environment)
> +        for obj in emitter.emit(reader.read_topsrcdir()):

It feels to me this should be implemented as a build backend instead of doing its own iteration. Then the mach command becomes an alias for mach build-backend -b backend_name (and then I wonder if there needs to be a separate command for this)

@@ +92,5 @@
> +
> +    def _build_db_line(self, obj, cenv, filename, flags, ishost):
> +        # Distinguish between host and target files.
> +        prefix = 'HOST_' if ishost else ''
> +        if filename.endswith('.c') or filename.endswith('.m'):

> Should we lowercase filename in case UPPERCASE extensions are used? Do we have any instances of that in the tree?

TreeMetadataEmitter._process_sources doesn't do that.

@@ +99,5 @@
> +            # Add the Objective-C flags if needed.
> +            if filename.endswith('.m'):
> +                cflags += ' ' + flags['COMPILE_CMFLAGS']
> +        elif filename.endswith('.cpp') or filename.endswith('.cc') or \
> +             filename.endswith('.cxx') or filename.endswith('.mm'):

The *Sources objects have a canonical_suffix attribute that gives you '.cpp' for '.cpp', '.cc', and '.cxx' files. Please use that instead of manually checking the file name suffix (you can use it for all extensions, not only for C++ files). If we ever end up with new supported extensions, this code wouldn't need changes, then.

@@ +119,5 @@
> +            filename = name
> +
> +        cmd = (
> +          compiler +
> +          ' -o /dev/null -c ' + # The output file doesn't matter, so just make something up.

Is /dev/null ok with clang-cl?

Comment 30

[Joshua Cranmer [:jcranmer]]

(In reply to Mike Hommey [:glandium] from comment #29)
> It feels to me this should be implemented as a build backend instead of
> doing its own iteration. Then the mach command becomes an alias for mach
> build-backend -b backend_name (and then I wonder if there needs to be a
> separate command for this)

One thing I want to explore is using try to generate a super compilation database; having it not be a build backend would help tremendously.

Comment 31

[Mike Hommey [:glandium]]

(In reply to Joshua Cranmer [:jcranmer] from comment #30)
> One thing I want to explore is using try to generate a super compilation
> database; having it not be a build backend would help tremendously.

I fail to see how that makes a difference.

Comment 32

[Gregory Szorc [:gps]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #28)
> (In reply to Gregory Szorc [:gps] from comment #27)
> 
> > ::: build/mach_bootstrap.py
> > @@ +84,5 @@
> > >      'python/mozboot/mozboot/mach_commands.py',
> > >      'python/mozbuild/mozbuild/mach_commands.py',
> > >      'python/mozbuild/mozbuild/backend/mach_commands.py',
> > >      'python/mozbuild/mozbuild/compilation/codecomplete.py',
> > > +    'python/mozbuild/mozbuild/compilation/database.py',
> > 
> > Hmm. Not sure who introduced a mach command handler in codecomplete.py, but
> 
> I did in bug 892973, at your request.  ;-)

I must have been in a "meh" mood that day :)

> 
> > we intentionally try to isolate frontend mach command code in
> > mach_commands.py files away from generic backend code.
> 
> Err, isn't that contradicting with bug 892973 comment 22.

No. One reason for the separation is described at https://gecko.readthedocs.org/en/latest/python/mach/commands.html#minimizing-code-in-commands. Another reason is mach_commands.py files can't be imported. So the code can't easily be reused. That's why we prefer code to live in loadable Python modules.

> 
> > ::: python/mozbuild/mozbuild/compilation/database.py
> > @@ +15,5 @@
> > > +from mozbuild.base import MachCommandBase
> > > +from mozbuild.compilation import util
> > > +
> > > +@CommandProvider
> > > +class CompilationDatabase(MachCommandBase):
> > 
> > Please move the mach command bits out of this file and refactor this to be a
> > standalone Python module. This entails moving all the imports to the top of
> > the file.
> 
> Where do you want me to move it?

Create a mach_commands.py file for the mach bits. database.py should not have imports inside functions unless you are avoiding an import cycle (which is a good sign you should create a new .py file to hold the shared code).

Comment 33

[(no longer active)]

I don't think this needs to be implemented as a build backend.  We still don't have all of the required information in build backends to be able to form a complete command line to build a source file.  And I thought you already agreed around comment 18.

What is the concrete benefit in doing this as a build backend?  This patch is very useful because it will allow us to run clang based tools for automated code transformations, but for that to happen you pretty much have to run make showbuild, which requires the make backend to be present.

Comment 34

[Mike Hommey [:glandium]]

You can still use essentially the same implementation, but making it a build backend. That will make it future proof, more testable, and avoids adding yet another mach command (we really have too many of those nowadays). I was not suggesting you try to rely solely on the data available from the emitter.

Comment 35

[(no longer active)]

I'm probably going to maintain this as an out of tree patch, I guess.  I really don't have the bandwidth to learn how to implement build backends and other stuff requested here.  :(

Comment 36

[Gregory Szorc [:gps]]

Having a Clang compilation database is important because a lot of tools understand it and tools are good. Corner glandium or me in Whistler and hopefully one of us can bang this out.

Comment 37

[(no longer active)]

(In reply to Gregory Szorc [:gps] from comment #36)
> Having a Clang compilation database is important because a lot of tools
> understand it and tools are good. Corner glandium or me in Whistler and
> hopefully one of us can bang this out.

Sorry but with all due respect, I have lost the energy to fight over this.  Maybe there is a way to get the missing information that I talked about in the first paragraph of comment 33.  Comment 34 seems to have either ignored that part of my comment or answered it in a way that I don't understand.

If someone else wants to pick this up, please go ahead.  When I get some time, I'll publish a branch with my rebased patch on top of trunk that people can use in the mean time.  (And I am very frustrated at the outcome here.)

Comment 38

[Markus Stange [:mstange]]

(In reply to Mike Hommey [:glandium] from comment #29)
> It feels to me this should be implemented as a build backend instead of
> doing its own iteration. Then the mach command becomes an alias for mach
> build-backend -b backend_name (and then I wonder if there needs to be a
> separate command for this)

Mike, does this mean you'd still take this patch, assuming the other review comments are fixed?

Ehsan tells me that the reason we need to use the make backend when generating this information is that we need accurate -D, -I and --include flags, which are influenced by configure.in and config.mk. Do you have suggestions on how to get this information with a separate backend?

Comment 39

[Gregory Szorc [:gps]]

Is this feasible:

1) Create a make target that dumps compile invocations to machine readable files in a central directory
2) Create a make target that causes dumping in all registered directories
3) Aggregate results of dumping into a Clang database

I suspect this will take only a second or two to run, which means we could potentially integrate it into the existing recursive make backend. It also feels slightly less hacky than implementing so much post-processing in Python: rules.mk already knows what the commands will be - let's just make it easier to dump them from make.

Comment 40

[Mike Hommey [:glandium]]

Attachment: diff

Here you are: these are the minimal changes to turn your patch into a build backend. Run with mach build-backend --backend=CompileDB.
There are probably things to polish, and, obviously, still the review comments to address, but I'll leave that to you.

Comment 41

[(no longer active)]

Does this assume that the make backend exist?  (I think it does.)

Is that OK?  I thought the idea behind the build backends was that they consume the moz.build input, not the output of other build backends...

Comment 42

[(no longer active)]

Also, this doesn't remove the need for the post-processing of flags in Python that gps was talking about in comment 39.  Basically, if I understand this patch correctly, the only change that it makes is the command line used to invoke the tool (from ./mach compilation-database to ./mach build-backend --backend=CompileDB).

Comment 43

[Mike Hommey [:glandium]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #42)
> Basically, if I understand this patch correctly, the only change that it makes is the command line used
> to invoke the tool (from ./mach compilation-database to ./mach build-backend --backend=CompileDB).

Exactly, and that's what I was asking.

I think comment 39 was gps trying to bring a solution that you would implement since you were giving up. The post-processing is also not exactly new, since the completion thing kind of does the same. And it will vanish in the long term.

Comment 44

[(no longer active)]

Attachment: Add support for generating clang compilation database

Attaching a new version of the patch since folks are using it.

Comment 45

[Mike Hommey [:glandium]]

Attachment: Add support for generating clang compilation database

This merges both existing patches, applies review comments from both gps and myself, and is rebased on top of bug 1207882 and bug 1207893.

Comment 46

[(no longer active)]

Thanks, Mike!

Comment 47

[Gregory Szorc [:gps]]

Comment on attachment 8665727 [details] [diff] [review]
Add support for generating clang compilation database

Review of attachment 8665727 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. The performance comment isn't critical before landing. But I suspect it will have a significant impact. I trust you to add the cache in flight.

::: python/mozbuild/mozbuild/compilation/database.py
@@ +40,5 @@
> +        if isinstance(obj, UnifiedSources):
> +            # For unified sources, only include the unified source file.
> +            # Note that unified sources are never used for host sources.
> +            for f in obj.unified_source_mapping:
> +                flags = self._get_dir_flags(obj.objdir)

Wouldn't it be significantly faster to call self._get_dir_flags() once per directory. The result is idempotent, right?

Comment 48

[Mike Hommey [:glandium]]

_get_dir_flags itself has a cache anyways.

Comment 49

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c9db53f98d5

Comment 50

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6c9db53f98d5