973405 - Move some misc LOCAL_INCLUDES to moz.build

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

[:ehsan akhgari]

Attachment: Move some misc LOCAL_INCLUDES to moz.build

Comment 2

[Mike Hommey [:glandium] (high latency)]

Comment on attachment 8376904 [details] [diff] [review]
Move some misc LOCAL_INCLUDES to moz.build

Review of attachment 8376904 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/sandbox/moz.build
@@ +127,5 @@
> +    LOCAL_INCLUDES += [
> +        '/nsprpub',
> +        '/security',
> +        '/security/sandbox/chromium',
> +        '/security/sandbox/chromium/base/shim',

The include path order needs to be kept here. I'd expect a try build to fail with this patch.

Comment 3

[:ehsan akhgari]

(In reply to Mike Hommey [:glandium] from comment #2)
> Comment on attachment 8376904 [details] [diff] [review]
> Move some misc LOCAL_INCLUDES to moz.build
> 
> Review of attachment 8376904 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: security/sandbox/moz.build
> @@ +127,5 @@
> > +    LOCAL_INCLUDES += [
> > +        '/nsprpub',
> > +        '/security',
> > +        '/security/sandbox/chromium',
> > +        '/security/sandbox/chromium/base/shim',
> 
> The include path order needs to be kept here. I'd expect a try build to fail
> with this patch.

The try run passed just fine.

LOCAL_INCLUDES enforces this ordering.  Please clarify how I should preserve the ordering.

Comment 4

[Mike Hommey [:glandium] (high latency)]

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #3)
> (In reply to Mike Hommey [:glandium] from comment #2)
> > Comment on attachment 8376904 [details] [diff] [review]
> > Move some misc LOCAL_INCLUDES to moz.build
> > 
> > Review of attachment 8376904 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: security/sandbox/moz.build
> > @@ +127,5 @@
> > > +    LOCAL_INCLUDES += [
> > > +        '/nsprpub',
> > > +        '/security',
> > > +        '/security/sandbox/chromium',
> > > +        '/security/sandbox/chromium/base/shim',
> > 
> > The include path order needs to be kept here. I'd expect a try build to fail
> > with this patch.
> 
> The try run passed just fine.

Ah, probably because the sandbox is not enabled by default yet (except on b2g).

> LOCAL_INCLUDES enforces this ordering.  Please clarify how I should preserve
> the ordering.

LOCAL_INCLUDES += [ foo ]
LOCAL_INCLUDES += [ bar ]

Comment 5

[:ehsan akhgari]

(In reply to Mike Hommey [:glandium] from comment #4)
> (In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness,
> emailapocalypse) from comment #3)
> > (In reply to Mike Hommey [:glandium] from comment #2)
> > > Comment on attachment 8376904 [details] [diff] [review]
> > > Move some misc LOCAL_INCLUDES to moz.build
> > > 
> > > Review of attachment 8376904 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > ::: security/sandbox/moz.build
> > > @@ +127,5 @@
> > > > +    LOCAL_INCLUDES += [
> > > > +        '/nsprpub',
> > > > +        '/security',
> > > > +        '/security/sandbox/chromium',
> > > > +        '/security/sandbox/chromium/base/shim',
> > > 
> > > The include path order needs to be kept here. I'd expect a try build to fail
> > > with this patch.
> > 
> > The try run passed just fine.
> 
> Ah, probably because the sandbox is not enabled by default yet (except on
> b2g).

Could be...

> > LOCAL_INCLUDES enforces this ordering.  Please clarify how I should preserve
> > the ordering.
> 
> LOCAL_INCLUDES += [ foo ]
> LOCAL_INCLUDES += [ bar ]

Ah, I see what you mean.  FWIW this is really annoying.  Why enforce LOCAL_INCLUDES ordering at all then?  I would argue that it's actually wrong semantically.

Comment 6

[:ehsan akhgari]

Attachment: Move some misc LOCAL_INCLUDES to moz.build

Comment 7

[Mike Hommey [:glandium] (high latency)]

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #5)
> Ah, I see what you mean.  FWIW this is really annoying.  Why enforce
> LOCAL_INCLUDES ordering at all then?  I would argue that it's actually wrong
> semantically.

Because most of the time order doesn't matter. And things usually quickly become a mess when no ordering is imposed.

Comment 8

[:ehsan akhgari]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[[e-mail]]

Comment on attachment 8377406 [details] [diff] [review]
Move some misc LOCAL_INCLUDES to moz.build

>-LOCAL_INCLUDES += \
>-    -I$(topsrcdir)/security/sandbox/chromium/base/shim \
>-    -I$(topsrcdir)/security/sandbox/chromium \
>-    -I$(topsrcdir)/security \
>-    -I$(topsrcdir)/nsprpub \
>-    $(NULL)
...
>+    LOCAL_INCLUDES += ['/security/sandbox/chromium/base/shim']
>+    LOCAL_INCLUDES += ['/security/sandbox/chromium']
>+    LOCAL_INCLUDES += ['/security']
>+    LOCAL_INCLUDES += ['/nsprpub']
This change breaks comm-central because you're missing topsrcdir.

Comment 11

[[e-mail]]

(In reply to comment #10)
> (From update of attachment 8377406 [details] [diff] [review])
> Move some misc LOCAL_INCLUDES to moz.build
> 
> >-LOCAL_INCLUDES += \
> >-    -I$(topsrcdir)/security/sandbox/chromium/base/shim \
> >-    -I$(topsrcdir)/security/sandbox/chromium \
> >-    -I$(topsrcdir)/security \
> >-    -I$(topsrcdir)/nsprpub \
> >-    $(NULL)
> ...
> >+    LOCAL_INCLUDES += ['/security/sandbox/chromium/base/shim']
> >+    LOCAL_INCLUDES += ['/security/sandbox/chromium']
> >+    LOCAL_INCLUDES += ['/security']
> >+    LOCAL_INCLUDES += ['/nsprpub']
> This change breaks comm-central because you're missing topsrcdir.

Sorry, I forgot to check the age of this bug. Something else must have changed to break comm-central.