Closed Bug 973405 Opened 6 years ago Closed 6 years ago

Move some misc LOCAL_INCLUDES to moz.build

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla30

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

Attachments

(2 files)

No description provided.
Assignee: nobody → ehsan
Attachment #8376904 - Flags: review?(mshal)
Attachment #8376904 - Flags: review?(mh+mozilla)
Attachment #8376904 - Flags: review?(gps)
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.
Attachment #8376904 - Flags: review?(mshal)
Attachment #8376904 - Flags: review?(mh+mozilla)
Attachment #8376904 - Flags: review?(gps)
Attachment #8376904 - Flags: review-
(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.
Flags: needinfo?(mh+mozilla)
(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 ]
Flags: needinfo?(mh+mozilla)
(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.
Attachment #8377406 - Flags: review?(mshal)
Attachment #8377406 - Flags: review?(mh+mozilla)
Attachment #8377406 - Flags: review?(gps)
(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.
Attachment #8377406 - Flags: review?(mshal)
Attachment #8377406 - Flags: review?(mh+mozilla)
Attachment #8377406 - Flags: review?(gps)
Attachment #8377406 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/eed4ef04aa76
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
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.
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.