910253 - Move LOCAL_INCLUDES to moz.build in accessible/

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

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

Attachment: Part a: automatic moves

Comment 2

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

Attachment: Part b: manual moves

Comment 3

[Trevor Saunders (:tbsaunde)]

Comment on attachment 796676 [details] [diff] [review]
Part a: automatic moves

>diff --git a/accessible/src/base/moz.build b/accessible/src/base/moz.build
>--- a/accessible/src/base/moz.build
>+++ b/accessible/src/base/moz.build
>+LOCAL_INCLUDES += [
>+    '../../../ipc/chromium/src',
>+    '../../../layout/generic',
>+    '../../../layout/style',
>+    '../../../layout/svg',
>+    '../../../layout/xul/base/src',
>+    '../../../layout/xul/tree/',

why not use /foo/bar here  as you did in the first file?

>+++ b/accessible/src/generic/moz.build
> LIBXUL_LIBRARY = True
> 
>+LOCAL_INCLUDES += [
>+    '../../../layout/generic',
>+    '../../../layout/xul/base/src',

same

+++ b/accessible/src/html/moz.build
>+LOCAL_INCLUDES += [
>+    '../../../layout/generic',
>+    '../../../layout/tables',
>+    '../../../layout/xul/base/src',

I suspect I don't want to know why we need to include xul stuff here

> 
>+LOCAL_INCLUDES += [
>+    '../base',
>+    '../generic',
>+    '../html',
>+    '../xul',
>+    '/layout/generic',
>+    '/layout/xul/base/src',

same on both parts (not that I expect you to try removing it when you'd need to push to try)

Comment 4

[Trevor Saunders (:tbsaunde)]

Comment on attachment 796677 [details] [diff] [review]
Part b: manual moves

I'd prefer single element additions where done as LOCAL_INCLUDES += ["../foo"] but whatever

also these patch demonstrate why building all this stuff in one monolithic moz.build file would be nice

Comment 5

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

(In reply to Trevor Saunders (:tbsaunde) from comment #3)
> Comment on attachment 796676 [details] [diff] [review]
> Part a: automatic moves
> 
> >diff --git a/accessible/src/base/moz.build b/accessible/src/base/moz.build
> >--- a/accessible/src/base/moz.build
> >+++ b/accessible/src/base/moz.build
> >+LOCAL_INCLUDES += [
> >+    '../../../ipc/chromium/src',
> >+    '../../../layout/generic',
> >+    '../../../layout/style',
> >+    '../../../layout/svg',
> >+    '../../../layout/xul/base/src',
> >+    '../../../layout/xul/tree/',
> 
> why not use /foo/bar here  as you did in the first file?

Because the first file used topsrcdir in the makefile, and this one used srcdir

Comment 6

[Michael Shal [:mshal]]

Comment on attachment 796676 [details] [diff] [review]
Part a: automatic moves

Are we sure using a string ordering for LOCAL_INCLUDES is a good idea? I guess we can always use multiple assignments if we need a non-sorted ordering to pickup the right .h files.

>diff --git a/accessible/src/base/moz.build b/accessible/src/base/moz.build
>--- a/accessible/src/base/moz.build
>+++ b/accessible/src/base/moz.build
>@@ -60,8 +60,21 @@
>     CPP_SOURCES += [
>         'Logging.cpp',
>     ]
> 
> LIBRARY_NAME = 'accessibility_base_s'
> 
> LIBXUL_LIBRARY = True
> 
>+LOCAL_INCLUDES += [
>+    '../../../ipc/chromium/src',
>+    '../../../layout/generic',
>+    '../../../layout/style',
>+    '../../../layout/svg',
>+    '../../../layout/xul/base/src',
>+    '../../../layout/xul/tree/',

nit: Trailing '/' in layout/xul/tree/

>diff --git a/accessible/src/xul/moz.build b/accessible/src/xul/moz.build
>--- a/accessible/src/xul/moz.build
>+++ b/accessible/src/xul/moz.build
>@@ -20,8 +20,18 @@
>     'XULTreeAccessible.cpp',
>     'XULTreeGridAccessible.cpp',
> ]
> 
> LIBRARY_NAME = 'accessibility_xul_s'
> 
> LIBXUL_LIBRARY = True
> 
>+LOCAL_INCLUDES += [
>+    '../../../layout/generic',
>+    '../../../layout/xul/base/src',
>+    '../../../layout/xul/tree//',

nit: Trailing '//' in layout/xul/tree//

Comment 7

[Michael Shal [:mshal]]

Comment on attachment 796677 [details] [diff] [review]
Part b: manual moves

Would it work if we moved the LOCAL_INCLUDES to accessible/src/shared.mozbuild, and just include that file from all of these moz.build files? The only difference between them seems to be that some have windows/ia2 and others don't, but I doubt adding it for all of them would break things.

Comment 8

[Michael Shal [:mshal]]

Comment on attachment 796677 [details] [diff] [review]
Part b: manual moves

Ok, we can always pull it out later.

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/56fe70596574
https://hg.mozilla.org/mozilla-central/rev/321142426acc