922566 - Kill dom-config.mk

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

dom-config.mk can be converted to moz.build foo these days.

AFAICT dom-config.mk simply accumulates the set of active dom/ directories and adds them to LOCAL_INCLUDES. The intermediate variable (DOM_SRCDIRS) doesn't appear to be used anywhere else.

bz: must we add so many paths to LOCAL_INCLUDES? Can't the code under dom/ reference files in a saner manner? I'd love to work with you to do this in a saner manner.

Comment 1

[Gregory Szorc [:gps]]

See initial comment. (Sorry, forgot needinfo at file time.)

Comment 2

[Boris Zbarsky [:bzbarsky]]

I don't think I've dealt with dom-config.mk before...

The basic issue I think we've had in the past is that we had two choices for headers: add to LOCAL_INCLUDES or export and have them be visible to non-Gecko code.

Recently we've basically been doing the latter and not worrying too much about it, but it's mostly been for WebIDL things.  There's tons of private-ish source files we don't want to export but also need to include cross-dir.

So what are the options for doing that in a saner manner?

Comment 3

[Michael Shal [:mshal]]

One other option would be to automatically add -I$(srcdir) to all compilation commands. Then we could do:

#include "dom/quota/Client.h"

So we wouldn't need to add dom/quota to LOCAL_INCLUDES, or EXPORT Client.h.

Comment 4

[Gregory Szorc [:gps]]

We could certainly add -I$(srcdir) to all compile commands. But as it with the case with dom-config.mk and a number of other directories, they end up searching for headers in *multiple* source directories and I /think/ those source directories often don't correspond with the C++ namespace. e.g. many C++ namespaces have "mozilla" in them.

What if we exposed a method for installing header files into a central-but-not-packaged directory. e.g.

LOCAL_HEADERS.mozilla.dom += ['dom.h']
LOCAL_HEADERS.mozilla.dom.promise += ['Promise.h']

The build system would then establish a directory somewhere, let's call it <objdir>/local_includes:

  /
    dom/
      dom.h
      promise/
        Promise.h

And all compiler invocations would have -I<objdir>/local_includes.

Aside from eliminating the multiple -I madness, this also has the benefit of enforcing C++ namespacing of includes. And, it gives moz.build more info about more files, potentially allowing more intelligent decisions in the future. (A think a worthy goal is to have moz.build know about every file in the tree so it can alert about orphaned/untracked files, other wackiness.)

I'm not sure if this makes sense given the state of our C++.

Ehsan?

Comment 5

[Trevor Saunders (:tbsaunde)]

my guess would be that it won't be too bad to just stop including dom-config.mk in makefiles and adding back actually needed LOCAL_INCLUDES, other than the fact that a lot of the stuff that includes it is only built on b2g.

(In reply to Gregory Szorc [:gps] from comment #4)
> We could certainly add -I$(srcdir) to all compile commands. But as it with
> the case with dom-config.mk and a number of other directories, they end up
> searching for headers in *multiple* source directories and I /think/ those
> source directories often don't correspond with the C++ namespace. e.g. many
> C++ namespaces have "mozilla" in them.

reading mshal's comment I believe he meant to write -I$(TOPSRCDIR) not -I$(SRCDIR).  Including things relative to top srcdir doesn't sound totally crazy, and has nice advantages like it being obvious where the file lives in the source tree.

> What if we exposed a method for installing header files into a
> central-but-not-packaged directory. e.g.
> 
> LOCAL_HEADERS.mozilla.dom += ['dom.h']
> LOCAL_HEADERS.mozilla.dom.promise += ['Promise.h']
> 
> The build system would then establish a directory somewhere, let's call it
> <objdir>/local_includes:
> 
>   /
>     dom/
>       dom.h
>       promise/
>         Promise.h
> 
> And all compiler invocations would have -I<objdir>/local_includes.

One issue with this is that often we'd prefer other directories didn't include the non exported headers either, or that only some closely related directories include them.

> I'm not sure if this makes sense given the state of our C++.

I'd expect not too great especially for older stuff that isn't in any namespace.

Comment 6

[Michael Shal [:mshal]]

(In reply to Trevor Saunders (:tbsaunde) from comment #5)
> my guess would be that it won't be too bad to just stop including
> dom-config.mk in makefiles and adding back actually needed LOCAL_INCLUDES,
> other than the fact that a lot of the stuff that includes it is only built
> on b2g.
> 
> (In reply to Gregory Szorc [:gps] from comment #4)
> > We could certainly add -I$(srcdir) to all compile commands. But as it with
> > the case with dom-config.mk and a number of other directories, they end up
> > searching for headers in *multiple* source directories and I /think/ those
> > source directories often don't correspond with the C++ namespace. e.g. many
> > C++ namespaces have "mozilla" in them.
> 
> reading mshal's comment I believe he meant to write -I$(TOPSRCDIR) not
> -I$(SRCDIR).  Including things relative to top srcdir doesn't sound totally
> crazy, and has nice advantages like it being obvious where the file lives in
> the source tree.

Oops, you're right! I meant -I$(TOPSRCDIR). -I$(SRCDIR) is kinda already implied since #include "foo.h" will search for foo.h in the same directory as the source file. Hope I didn't confuse anyone :)

Comment 7

[Gregory Szorc [:gps]]

My fallback implementation for "kill dom-config.mk" is to create a .mozbuild file with the same logic and include() it from every moz.build currently including dom-config.mk. Hacky, but it works.

I'd prefer a better solution and want to work with the C++ gurus to facilitate that. Not sure how much work that is. It's conceivable we'll land the hacky dom-config.mozbuild as a stop-gap until the C++ situation can become sane. I don't want to create too much work for others, especially if it's busy work.

Comment 8

[(no longer active)]

The -I$(TOPSRCDIR) will definitely be better than what we currently have, and we wouldn't need to export headers and such any more, but it has the downside of requiring us to change our #include statements (because the exported paths and the srcdir paths are not always the same.)  I'm not sure if that's what Greg wants to do in this bug.  :-)

Other than that, is there anything else that I can help answer?

Comment 9

[(not currently active) Ted Mielczarek]

+1 to -I$(TOPSRCDIR). That seems like the sanest way to include non-exported headers. We will have to change some source to get rid of LOCAL_INCLUDES, but it makes the most sense to me.

Comment 10

[Trevor Saunders (:tbsaunde)]

SO, in news that will shock everyone I'm sure several of the directories that use dom-config.mk don't actually need it or only need a couple of directories to be searched, so pending making -I$(topsrcdir) work we can probably kill the shared file easily enough.

Comment 11

[(no longer active)]

(In reply to comment #9)
> +1 to -I$(TOPSRCDIR). That seems like the sanest way to include non-exported
> headers. We will have to change some source to get rid of LOCAL_INCLUDES, but
> it makes the most sense to me.

Well, ship it then!  :-)

Comment 12

[Trevor Saunders (:tbsaunde)]

Attachment: bug 922566 - kill dom-config.mk

Comment 13

[Gregory Szorc [:gps]]

Comment on attachment 815928 [details] [diff] [review]
bug 922566 - kill dom-config.mk

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

Beautiful!

The build changes all look fine. However, I'm not qualified to review the C++ bits. Can you get additional review on these, please?

::: dom/base/moz.build
@@ +132,5 @@
> +    '../media',
> +    '../network/src',
> +    '../src/geolocation',
> +    '../src/storage',
> +    '../time',

These could be written as /dom/battery, etc if you so desired.

Comment 14

[(no longer active)]

Hmm, Trevor, did you forget to request review for the C++ bits of this patch before landing it?

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d0732dc3f30e