Move the LOCAL_INCLUDES in media to moz.build

RESOLVED FIXED in mozilla30

Status

defect
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

unspecified
mozilla30
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

No description provided.
Assignee: nobody → ehsan
Attachment #8376889 - Flags: review?(mshal)
Attachment #8376889 - Flags: review?(mh+mozilla)
Attachment #8376889 - Flags: review?(gps)
Attachment #8376889 - Attachment is obsolete: true
Attachment #8376889 - Flags: review?(mshal)
Attachment #8376889 - Flags: review?(mh+mozilla)
Attachment #8376889 - Flags: review?(gps)
Attachment #8376895 - Flags: review?(mshal)
Attachment #8376895 - Flags: review?(mh+mozilla)
Attachment #8376895 - Flags: review?(gps)
Comment on attachment 8376895 [details] [diff] [review]
Move the LOCAL_INCLUDES in media to moz.build

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

::: media/webrtc/signaling/test/moz.build
@@ +20,5 @@
>              'NR_SOCKET_IS_VOID_PTR', 'HAVE_STRDUP'):
>      DEFINES[var] = True
> +
> +GENERATED_INCLUDES += [
> +  '.',

I don't think this is necessary
Attachment #8376895 - Attachment is obsolete: true
Attachment #8376895 - Flags: review?(mshal)
Attachment #8376895 - Flags: review?(mh+mozilla)
Attachment #8376895 - Flags: review?(gps)
Attachment #8377194 - Flags: review?(mshal)
Attachment #8377194 - Flags: review?(mh+mozilla)
Attachment #8377194 - Flags: review?(gps)
(In reply to :Ms2ger from comment #3)
> Comment on attachment 8376895 [details] [diff] [review]
> Move the LOCAL_INCLUDES in media to moz.build
> 
> Review of attachment 8376895 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/webrtc/signaling/test/moz.build
> @@ +20,5 @@
> >              'NR_SOCKET_IS_VOID_PTR', 'HAVE_STRDUP'):
> >      DEFINES[var] = True
> > +
> > +GENERATED_INCLUDES += [
> > +  '.',
> 
> I don't think this is necessary

This is not trivial to figure out, and I have been bitten by this once already (see bug 972459.)

Please file a follow-up if you want, but I'm not volunteering to figure out which one of these includes are actually necessary.  :-)
Comment on attachment 8377194 [details] [diff] [review]
Move the LOCAL_INCLUDES in media to moz.build

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

r+ with the following fixed.

::: media/mtransport/test/moz.build
@@ +41,5 @@
> +    ]
> +
> +if CONFIG['OS_TARGET'] == 'Darwin':
> +    LOCAL_INCLUDES += [
> +        '/media/mtransport/third_party/nrappkit/src/port/darwin/include',

This path used to be added for the BSDs too.

::: media/webrtc/signaling/test/moz.build
@@ +20,5 @@
>              'NR_SOCKET_IS_VOID_PTR', 'HAVE_STRDUP'):
>      DEFINES[var] = True
> +
> +GENERATED_INCLUDES += [
> +  '.',

Adding '.' is useless.

@@ +66,5 @@
> +    ]
> +
> +if CONFIG['OS_TARGET'] == 'Darwin':
> +    LOCAL_INCLUDES += [
> +        '/media/mtransport/third_party/nrappkit/src/port/darwin/include',

Same as media/mtransport/test/moz.build
Attachment #8377194 - Flags: review?(mshal)
Attachment #8377194 - Flags: review?(mh+mozilla)
Attachment #8377194 - Flags: review?(gps)
Attachment #8377194 - Flags: review+
(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #5)
> This is not trivial to figure out, and I have been bitten by this once
> already (see bug 972459.)
> 
> Please file a follow-up if you want, but I'm not volunteering to figure out
> which one of these includes are actually necessary.  :-)

Here, it's easy: -I. is already in INCLUDES, before LOCAL_INCLUDES. All adding it to LOCAL_INCLUDES was doing was to put it twice on the command line.
(In reply to Mike Hommey [:glandium] from comment #7)
> (In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness,
> emailapocalypse) from comment #5)
> > This is not trivial to figure out, and I have been bitten by this once
> > already (see bug 972459.)
> > 
> > Please file a follow-up if you want, but I'm not volunteering to figure out
> > which one of these includes are actually necessary.  :-)
> 
> Here, it's easy: -I. is already in INCLUDES, before LOCAL_INCLUDES. All
> adding it to LOCAL_INCLUDES was doing was to put it twice on the command
> line.

Ah I was confused because I did not note that this is GENERATED_INCLUDES, not LOCAL_INCLUDES :/
https://hg.mozilla.org/mozilla-central/rev/4bc79e20fae0
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.