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.

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

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

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

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

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