Closed Bug 981292 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla30

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

Attachments

(2 files)

No description provided.
Assignee: nobody → ehsan
Attachment #8388077 - Flags: review?(mshal)
Attachment #8388077 - Flags: review?(mh+mozilla)
Attachment #8388077 - Flags: review?(gps)
Attachment #8388118 - Flags: review?(mshal)
Attachment #8388118 - Flags: review?(mh+mozilla)
Attachment #8388118 - Flags: review?(gps)
Attachment #8388077 - Flags: review?(mshal)
Attachment #8388077 - Flags: review?(mh+mozilla)
Attachment #8388077 - Flags: review?(gps)
Attachment #8388077 - Flags: review+
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...)
Attachment #8388118 - Flags: review?(mshal)
Attachment #8388118 - Flags: review?(mh+mozilla)
Attachment #8388118 - Flags: review?(gps)
Attachment #8388118 - Flags: feedback+
(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?
https://hg.mozilla.org/mozilla-central/rev/c86d5a518965
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
(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 :)
(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?
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.