735561 - Bugs in webrtc drop gyp files exposed by building on linux builders

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Fix two bugs in webrtc gyp files

Two bugs in webrtc's latest drop:

1) ISOC99 needs to be set for C files (it is already for Mac/Xcode)
2) the expat include library path in libjingle was wrong and it would pick up system expat includes.

Comment 1

[Timothy B. Terriberry (:derf)]

Comment on attachment 605643 [details] [diff] [review]
Fix two bugs in webrtc gyp files

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

f+ with one nit.

::: media/webrtc/trunk/build/common.gypi
@@ +1073,5 @@
>          'defines': ['_GLIBCXX_DEBUG=1',],
>          'cflags_cc!': ['-fno-rtti'],
>          'cflags_cc+': ['-frtti', '-g'],
>        }],
> +      ['OS=="linux"', {

You should probably add a comment saying why this is needed.

Comment 2

[(not currently active) Ted Mielczarek]

Comment on attachment 605643 [details] [diff] [review]
Fix two bugs in webrtc gyp files

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

::: media/webrtc/trunk/build/common.gypi
@@ +1074,5 @@
>          'cflags_cc!': ['-fno-rtti'],
>          'cflags_cc+': ['-frtti', '-g'],
>        }],
> +      ['OS=="linux"', {
> +        'defines': ['_ISOC99_SOURCE=1'],

I would feel better if we had any idea why this was needed, but I can deal with it.

::: media/webrtc/trunk/third_party/libjingle/libjingle.gyp
@@ +118,5 @@
>          'include_dirs': [
>            '<(overrides)',
>            'source',
>            '../..',  # the third_party folder for webrtc includes
> +          '../../third_party/expat/files/lib',

Did this get shuffled around in the source?

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

C99 is needed so get get lrint() on the builders; the gyp files specify C99 for Mac but not for linux (and it appears to default to C99, or at least expose lrint(), for most of our desktop linux machines).

I haven't checked if the expat files moved; it doesn't really matter.  That's where they are now.