973395 - Move the LOCAL_INCLUDES in media to moz.build

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

[(no longer active)]

Attachment: Move the LOCAL_INCLUDES in media to moz.build

Comment 2

[(no longer active)]

Attachment: Move the LOCAL_INCLUDES in media to moz.build

Comment 3

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

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

Comment 4

[(no longer active)]

Attachment: Move the LOCAL_INCLUDES in media to moz.build

Comment 5

[(no longer active)]

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

[Mike Hommey [:glandium]]

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

Comment 7

[Mike Hommey [:glandium]]

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

Comment 8

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4bc79e20fae0

Comment 9

[(no longer active)]

(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 :/

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4bc79e20fae0