937224 - Move more LOCAL_INCLUDES to moz.build

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

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

Attachment: Part a: Move LOCAL_INCLUDES to moz.build in gfx/;

Comment 2

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

Attachment: Part b: Move LOCAL_INCLUDES to moz.build in widget/;

Comment 3

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

Attachment: Part c: Move LOCAL_INCLUDES to moz.build in view/;

Comment 4

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

Attachment: Part d: Move LOCAL_INCLUDES to moz.build in image/;

Comment 5

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

Attachment: Part e: Move LOCAL_INCLUDES to moz.build in layout/;

Comment 6

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

Attachment: Part f: Move LOCAL_INCLUDES to moz.build in intl/;

Comment 7

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

Attachment: Part g: Move LOCAL_INCLUDES to moz.build in netwerk/;

Comment 8

[Michael Shal [: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!

Comment 9

[Michael Shal [:mshal]]

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?

Comment 10

[Michael Shal [:mshal]]

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)

Comment 11

[Michael Shal [:mshal]]

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.

Comment 12

[Michael Shal [:mshal]]

Comment on attachment 830352 [details] [diff] [review]
Part g: Move LOCAL_INCLUDES to moz.build in netwerk/;

># HG changeset patch
># User Ms2ger <[e-mail]>
># 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?

Comment 13

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

(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 <[e-mail]>
> ># 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.

Comment 14

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

(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.

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/4779e661432a
https://hg.mozilla.org/mozilla-central/rev/f46cc655a78e
https://hg.mozilla.org/mozilla-central/rev/125b9012ac85
https://hg.mozilla.org/mozilla-central/rev/ea18e92d28d6
https://hg.mozilla.org/mozilla-central/rev/6d2b1212727e
https://hg.mozilla.org/mozilla-central/rev/ed472e7df8d0
https://hg.mozilla.org/mozilla-central/rev/0836e2aa8b32