981292 - Move the CXXFLAGS variable in layout/build/Makefile.in to moz.build

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

[(no longer active)]

Attachment: Move the CXXFLAGS variable in layout/build/Makefile.in to moz.build

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Alternative approach

Comment 3

[Michael Shal [:mshal]]

Comment on attachment 8388118 [details] [diff] [review]
Alternative approach

Although this definitely improves the moz.build readability for this case, there is still the more general problem of needing -I flags on external source directories, correct? For example, we might also need a DIRECTX_INCLUDES for flags like this:

./gfx/angle/src/libEGL/Makefile.in:CXXFLAGS += -I'$(MOZ_DIRECTX_SDK_PATH)/include'

Does it make sense to introduce *_INCLUDES moz.build variables for each type of external include directory we'd need? (I haven't done an exhaustive search, so maybe those are the only two...)

Comment 4

[(no longer active)]

(In reply to comment #3)
> Comment on attachment 8388118 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=8388118
> Alternative approach
> 
> Although this definitely improves the moz.build readability for this case,
> there is still the more general problem of needing -I flags on external source
> directories, correct? For example, we might also need a DIRECTX_INCLUDES for
> flags like this:
> 
> ./gfx/angle/src/libEGL/Makefile.in:CXXFLAGS +=
> -I'$(MOZ_DIRECTX_SDK_PATH)/include'
> 
> Does it make sense to introduce *_INCLUDES moz.build variables for each type of
> external include directory we'd need? (I haven't done an exhaustive search, so
> maybe those are the only two...)

Why is this stuff needed at all?  Is it just that we treat anything with a leading slash as a path name relative to the topsrcdir?  Can't we just stat() to see if those directories exist in topsrcdir, and if not, fall back to not assuming that?

Comment 5

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c86d5a518965

Comment 6

[Ryan VanderMeulen [:RyanVM]]

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

Comment 7

[Michael Shal [:mshal]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #4)
> Why is this stuff needed at all?  Is it just that we treat anything with a
> leading slash as a path name relative to the topsrcdir?  Can't we just
> stat() to see if those directories exist in topsrcdir, and if not, fall back
> to not assuming that?

Good point. I thought we did more with LOCAL_INCLUDES in mozbuild, but it's really just checking for a leading slash to make it relative to either topsrcdir or srcdir. Adding a stat() here doesn't add any discernible overhead, either. I guess the only downside is the word "LOCAL_" would lose its meaning :)

Comment 8

[Mike Hommey [:glandium]]

There is a graceful way to handle this:
https://code.google.com/p/chromium/wiki/GNLanguage#Naming_things

Comment 9

[(no longer active)]

(In reply to comment #8)
> There is a graceful way to handle this:
> https://code.google.com/p/chromium/wiki/GNLanguage#Naming_things

WFM!  It's actually a good idea I think.  File a bug please?