Move more LOCAL_INCLUDES to moz.build

RESOLVED FIXED in mozilla28

Status

()

Core
Build Config
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Ms2ger, Assigned: Ms2ger)

Tracking

(Blocks: 1 bug)

unspecified
mozilla28
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(7 attachments)

Comment hidden (empty)
(Assignee)

Comment 1

4 years ago
Created attachment 830345 [details] [diff] [review]
Part a: Move LOCAL_INCLUDES to moz.build in gfx/;
Attachment #830345 - Flags: review?(mshal)
(Assignee)

Comment 2

4 years ago
Created attachment 830347 [details] [diff] [review]
Part b: Move LOCAL_INCLUDES to moz.build in widget/;
Attachment #830347 - Flags: review?(mshal)
(Assignee)

Comment 3

4 years ago
Created attachment 830348 [details] [diff] [review]
Part c: Move LOCAL_INCLUDES to moz.build in view/;
Attachment #830348 - Flags: review?(mshal)
(Assignee)

Comment 4

4 years ago
Created attachment 830349 [details] [diff] [review]
Part d: Move LOCAL_INCLUDES to moz.build in image/;
Attachment #830349 - Flags: review?(mshal)
(Assignee)

Comment 5

4 years ago
Created attachment 830350 [details] [diff] [review]
Part e: Move LOCAL_INCLUDES to moz.build in layout/;
Attachment #830350 - Flags: review?(mshal)
(Assignee)

Comment 6

4 years ago
Created attachment 830351 [details] [diff] [review]
Part f: Move LOCAL_INCLUDES to moz.build in intl/;
Attachment #830351 - Flags: review?(mshal)
(Assignee)

Comment 7

4 years ago
Created attachment 830352 [details] [diff] [review]
Part g: Move LOCAL_INCLUDES to moz.build in netwerk/;
Attachment #830352 - Flags: review?(mshal)
Comment on attachment 830345 [details] [diff] [review]
Part a: Move LOCAL_INCLUDES to moz.build in gfx/;

>diff --git a/gfx/angle/moz.build b/gfx/angle/moz.build
>+LOCAL_INCLUDES += [
>+    'include',
>+    'include/KHR',
>+    'src',
>+]
>+
>+

Double newline :)

Everything else looks good!
Attachment #830345 - Flags: review?(mshal) → review+

Updated

4 years ago
Attachment #830347 - Flags: review?(mshal) → review+
Comment on attachment 830348 [details] [diff] [review]
Part c: Move LOCAL_INCLUDES to moz.build in view/;

>diff --git a/view/src/moz.build b/view/src/moz.build
>--- a/view/src/moz.build
>+++ b/view/src/moz.build
>@@ -14,8 +14,12 @@
> LIBRARY_NAME = 'gkview_s'
> 
> FAIL_ON_WARNINGS = True
> 
> LIBXUL_LIBRARY = True
> 
> MSVC_ENABLE_PGO = True
> 
>+LOCAL_INCLUDES += [
>+    '../../content/events/src/',
>+]

Maybe just '/content/events/src' since it's a top-level path?
Attachment #830348 - Flags: review?(mshal) → review+
Comment on attachment 830349 [details] [diff] [review]
Part d: Move LOCAL_INCLUDES to moz.build in image/;

>diff --git a/image/decoders/icon/gtk/Makefile.in b/image/decoders/icon/gtk/Makefile.in
>--- a/image/decoders/icon/gtk/Makefile.in
>+++ b/image/decoders/icon/gtk/Makefile.in
>@@ -1,10 +1,11 @@
> # This Source Code Form is subject to the terms of the Mozilla Public
> # License, v. 2.0. If a copy of the MPL was not distributed with this
> # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> 
>+include $(topsrcdir)/config/rules.mk

Why do we need to include config/rules.mk again? (and in image/decoders/icon/qt)
Attachment #830349 - Flags: review?(mshal) → review+
Comment on attachment 830350 [details] [diff] [review]
Part e: Move LOCAL_INCLUDES to moz.build in layout/;

>diff --git a/layout/build/moz.build b/layout/build/moz.build
>+if CONFIG['MOZ_B2G_RIL']:
>+    LOCAL_INCLUDES += [
>+        '/dom/system/gonk',
>+    ]

This block seems to line up with 'ifeq (gonk,$(MOZ_WIDGET_TOOLKIT))' in the Makefile. At least, I don't see where MOZ_B2G_RIL is coming from. Is that intentional?

>diff --git a/layout/generic/Makefile.in b/layout/generic/Makefile.in
>--- a/layout/generic/Makefile.in
>+++ b/layout/generic/Makefile.in
>@@ -1,37 +1,25 @@
> # This Source Code Form is subject to the terms of the Mozilla Public
> # License, v. 2.0. If a copy of the MPL was not distributed with this
> # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> 
>-RESOURCES_HTML = \
>+resources_html = \
> 		$(srcdir)/folder.png \
> 		$(NULL)

> libs::
>-	$(INSTALL) $(RESOURCES_HTML) $(DIST)/bin/res/html
>+	$(INSTALL) $(resources_html) $(DIST)/bin/res/html

I don't see what this does - it looks like it just unnecessarily bit-rots bug 772828.
Attachment #830350 - Flags: review?(mshal) → review+

Updated

4 years ago
Attachment #830351 - Flags: review?(mshal) → review+
Comment on attachment 830352 [details] [diff] [review]
Part g: Move LOCAL_INCLUDES to moz.build in netwerk/;

># HG changeset patch
># User Ms2ger <ms2ger@gmail.com>
># Date 1384199690 -3600
>#      Mon Nov 11 20:54:50 2013 +0100
># Node ID 6765fa87e6be8d5827d4d8260543b8aa83be9615
># Parent  716a4eae7921c3fdb33b8355b4c408f8d4cc445a
>Bug 937224 - Part g: Move LOCAL_INCLUDES to moz.build in netwerk/; r=?mshal
>
>diff --git a/netwerk/base/src/Makefile.in b/netwerk/base/src/Makefile.in
> ifdef MOZ_ENABLE_QTNETWORK
>-	LOCAL_INCLUDES += -I$(srcdir)/../../system/qt
>-	OS_INCLUDES += $(MOZ_QT_CFLAGS)
>+OS_INCLUDES += $(MOZ_QT_CFLAGS)

Hmm, we'll probably want to be consistent with CXXFLAGS vs OS_INCLUDES, but I'm ok with deciding that when they get moved to moz.build.

>diff --git a/netwerk/build/Makefile.in b/netwerk/build/Makefile.in
>+LOCAL_INCLUDES += \
>   -I../dns \
>-  $(foreach d,$(filter-out about,$(NECKO_PROTOCOLS)), \
>-    -I$(srcdir)/../protocol/$(d)) \
>   $(NULL)

So we still need -I../dns for an objdir include?

Looks good! Hope try is happy?
Attachment #830352 - Flags: review?(mshal) → review+
(Assignee)

Comment 13

4 years ago
(In reply to Michael Shal [:mshal] from comment #10)
> Comment on attachment 830349 [details] [diff] [review]
> Part d: Move LOCAL_INCLUDES to moz.build in image/;
> 
> >diff --git a/image/decoders/icon/gtk/Makefile.in b/image/decoders/icon/gtk/Makefile.in
> >--- a/image/decoders/icon/gtk/Makefile.in
> >+++ b/image/decoders/icon/gtk/Makefile.in
> >@@ -1,10 +1,11 @@
> > # This Source Code Form is subject to the terms of the Mozilla Public
> > # License, v. 2.0. If a copy of the MPL was not distributed with this
> > # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> > 
> >+include $(topsrcdir)/config/rules.mk
> 
> Why do we need to include config/rules.mk again? (and in
> image/decoders/icon/qt)

It needs to be included before CXXFLAGS.

(In reply to Michael Shal [:mshal] from comment #11)
> Comment on attachment 830350 [details] [diff] [review]
> Part e: Move LOCAL_INCLUDES to moz.build in layout/;
> 
> >diff --git a/layout/build/moz.build b/layout/build/moz.build
> >+if CONFIG['MOZ_B2G_RIL']:
> >+    LOCAL_INCLUDES += [
> >+        '/dom/system/gonk',
> >+    ]
> 
> This block seems to line up with 'ifeq (gonk,$(MOZ_WIDGET_TOOLKIT))' in the
> Makefile. At least, I don't see where MOZ_B2G_RIL is coming from. Is that
> intentional?

Gah. Bitrot; thanks for catching it.

(In reply to Michael Shal [:mshal] from comment #12)
> Comment on attachment 830352 [details] [diff] [review]
> Part g: Move LOCAL_INCLUDES to moz.build in netwerk/;
> 
> ># HG changeset patch
> ># User Ms2ger <ms2ger@gmail.com>
> ># Date 1384199690 -3600
> >#      Mon Nov 11 20:54:50 2013 +0100
> ># Node ID 6765fa87e6be8d5827d4d8260543b8aa83be9615
> ># Parent  716a4eae7921c3fdb33b8355b4c408f8d4cc445a
> >Bug 937224 - Part g: Move LOCAL_INCLUDES to moz.build in netwerk/; r=?mshal
> >
> >diff --git a/netwerk/base/src/Makefile.in b/netwerk/base/src/Makefile.in
> > ifdef MOZ_ENABLE_QTNETWORK
> >-	LOCAL_INCLUDES += -I$(srcdir)/../../system/qt
> >-	OS_INCLUDES += $(MOZ_QT_CFLAGS)
> >+OS_INCLUDES += $(MOZ_QT_CFLAGS)
> 
> Hmm, we'll probably want to be consistent with CXXFLAGS vs OS_INCLUDES, but
> I'm ok with deciding that when they get moved to moz.build.

I'd prefer not touching OS_INCLUDES right now.

> >diff --git a/netwerk/build/Makefile.in b/netwerk/build/Makefile.in
> >+LOCAL_INCLUDES += \
> >   -I../dns \
> >-  $(foreach d,$(filter-out about,$(NECKO_PROTOCOLS)), \
> >-    -I$(srcdir)/../protocol/$(d)) \
> >   $(NULL)
> 
> So we still need -I../dns for an objdir include?

Yep.
(Assignee)

Comment 14

4 years ago
(In reply to :Ms2ger from comment #13)
> (In reply to Michael Shal [:mshal] from comment #10)
> > >diff --git a/netwerk/build/Makefile.in b/netwerk/build/Makefile.in
> > >+LOCAL_INCLUDES += \
> > >   -I../dns \
> > >-  $(foreach d,$(filter-out about,$(NECKO_PROTOCOLS)), \
> > >-    -I$(srcdir)/../protocol/$(d)) \
> > >   $(NULL)
> > 
> > So we still need -I../dns for an objdir include?
> 
> Yep.

Actually, there's GENERATED_INCLUDES. I'll use that.

Updated

4 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.