Compile rtc_base_objc and rtc_api_objc for Mac

Compile rtc_base_objc and rtc_api_objc for Mac

BUG=webrtc:5453

[hjon_webrtc, 2016-01-28 20:22:49]

hjon@webrtc.org changed reviewers:
+ tkchin@webrtc.org

[tkchin_webrtc, 2016-01-28 20:53:54]

Description was changed from

==========
Compile rtc_base_objc and rtc_api_objc for Mac

BUG=5453
==========

to

==========
Compile rtc_base_objc and rtc_api_objc for Mac

BUG=5453
==========

[tkchin_webrtc, 2016-01-28 20:53:54]

tkchin@webrtc.org changed reviewers:
+ pbos@webrtc.org, tommi@webrtc.org

[tkchin_webrtc, 2016-01-28 21:09:43]

tkchin@webrtc.org changed reviewers:
+ kjellander@webrtc.org

[tkchin_webrtc, 2016-01-28 21:09:44]

We're using some new language features in webrtc/api/objc and webrtc/base/objc
that require us to bump the OSX target version. Bumping the version causes a
whole bunch of deprecation errors. Rather than fix them here, we'll suppress the
warning and file a bug since a bunch of them don't have good replacements /
require changing frameworks (e.g. QTKit->AVFoundation).

We'll only bump version for WebRTC standalone OS/X.

Some feedback for hjon and a question for kjellander:
In addition to OS==mac we'll need something to prevent our new bits from being
compiled in Chromium builds. I don't know if mac_deployment_target is visible,
but if it is that should be added to the condition check.

https://codereview.webrtc.org/1644843003/diff/1/webrtc/api/BUILD.gn
File webrtc/api/BUILD.gn (right):

https://codereview.webrtc.org/1644843003/diff/1/webrtc/api/BUILD.gn#newcode62
webrtc/api/BUILD.gn:62: "../build/WebRTC-Prefix.pch",
I don't think this is how precompiled headers are supposed to be added. Found
this:
https://code.google.com/p/chromium/codesearch#chromium/src/build/config/BUILD...

Can you file a bug for GN iOS builds and reference it here? You can assign it to
me and cc kjellander.

https://codereview.webrtc.org/1644843003/diff/1/webrtc/base/logging.cc
File webrtc/base/logging.cc (right):

https://codereview.webrtc.org/1644843003/diff/1/webrtc/base/logging.cc#newcod...
webrtc/base/logging.cc:172: #pragma clang diagnostic push
compiler directives have no indentation
Can you file a bug and reference it? Here and rest of CL.

Bug should be: Replace deprecated OS/X system calls.
And then add that there aren't actually good replacements.
You can file it on tommi@ for now and cc me.

https://codereview.webrtc.org/1644843003/diff/1/webrtc/modules/video_capture/...
File webrtc/modules/video_capture/mac/qtkit/video_capture_qtkit_info_objc.mm
(right):

https://codereview.webrtc.org/1644843003/diff/1/webrtc/modules/video_capture/...
webrtc/modules/video_capture/mac/qtkit/video_capture_qtkit_info_objc.mm:63:
NSAlert* alert = [[NSAlert alloc] init];
QTKit! We should remove it altogether at some point.

https://codereview.webrtc.org/1644843003/diff/1/webrtc/modules/video_capture/...
File webrtc/modules/video_capture/mac/qtkit/video_capture_qtkit_objc.mm (right):

https://codereview.webrtc.org/1644843003/diff/1/webrtc/modules/video_capture/...
webrtc/modules/video_capture/mac/qtkit/video_capture_qtkit_objc.mm:27: #pragma
clang diagnostic push
Is this because QTKit is deprecated?

https://codereview.webrtc.org/1644843003/diff/1/webrtc/supplement.gypi
File webrtc/supplement.gypi (right):

https://codereview.webrtc.org/1644843003/diff/1/webrtc/supplement.gypi#newcode6
webrtc/supplement.gypi:6: # Needed for ARCi, libc++, and nullability.
arci?

[hjon_webrtc, 2016-01-28 23:49:51]

https://codereview.webrtc.org/1644843003/diff/1/webrtc/api/BUILD.gn
File webrtc/api/BUILD.gn (right):

https://codereview.webrtc.org/1644843003/diff/1/webrtc/api/BUILD.gn#newcode62
webrtc/api/BUILD.gn:62: "../build/WebRTC-Prefix.pch",
On 2016/01/28 21:09:44, tkchin_webrtc wrote:
> I don't think this is how precompiled headers are supposed to be added. Found
> this:
>
https://code.google.com/p/chromium/codesearch#chromium/src/build/config/BUILD...
> 
> Can you file a bug for GN iOS builds and reference it here? You can assign it
to
> me and cc kjellander.

I'm not clear on what the bug should be. Could you clarify?

https://codereview.webrtc.org/1644843003/diff/1/webrtc/base/logging.cc
File webrtc/base/logging.cc (right):

https://codereview.webrtc.org/1644843003/diff/1/webrtc/base/logging.cc#newcod...
webrtc/base/logging.cc:172: #pragma clang diagnostic push
On 2016/01/28 21:09:44, tkchin_webrtc wrote:
> compiler directives have no indentation
> Can you file a bug and reference it? Here and rest of CL.
> 
> Bug should be: Replace deprecated OS/X system calls.
> And then add that there aren't actually good replacements.
> You can file it on tommi@ for now and cc me. 

Fixed indentation. Is there a preferred way to reference bugs? I think I've seen
things like http://crbug.com/<bugNumber>. Is there anything like that for
WebRTC?

https://codereview.webrtc.org/1644843003/diff/1/webrtc/modules/video_capture/...
File webrtc/modules/video_capture/mac/qtkit/video_capture_qtkit_info_objc.mm
(right):

https://codereview.webrtc.org/1644843003/diff/1/webrtc/modules/video_capture/...
webrtc/modules/video_capture/mac/qtkit/video_capture_qtkit_info_objc.mm:63:
NSAlert* alert = [[NSAlert alloc] init];
On 2016/01/28 21:09:44, tkchin_webrtc wrote:
> QTKit! We should remove it altogether at some point.

Acknowledged.

https://codereview.webrtc.org/1644843003/diff/1/webrtc/modules/video_capture/...
File webrtc/modules/video_capture/mac/qtkit/video_capture_qtkit_objc.mm (right):

https://codereview.webrtc.org/1644843003/diff/1/webrtc/modules/video_capture/...
webrtc/modules/video_capture/mac/qtkit/video_capture_qtkit_objc.mm:27: #pragma
clang diagnostic push
On 2016/01/28 21:09:44, tkchin_webrtc wrote:
> Is this because QTKit is deprecated?

Yes. There were enough errors in this file that it seemed better to just cover
it all at once.

https://codereview.webrtc.org/1644843003/diff/1/webrtc/supplement.gypi
File webrtc/supplement.gypi (right):

https://codereview.webrtc.org/1644843003/diff/1/webrtc/supplement.gypi#newcode6
webrtc/supplement.gypi:6: # Needed for ARCi, libc++, and nullability.
On 2016/01/28 21:09:44, tkchin_webrtc wrote:
> arci?

Done.

[hjon_webrtc, 2016-01-28 23:52:44]

On 2016/01/28 21:09:44, tkchin_webrtc wrote:
> We're using some new language features in webrtc/api/objc and webrtc/base/objc
> that require us to bump the OSX target version. Bumping the version causes a
> whole bunch of deprecation errors. Rather than fix them here, we'll suppress
the
> warning and file a bug since a bunch of them don't have good replacements /
> require changing frameworks (e.g. QTKit->AVFoundation).
> 
> We'll only bump version for WebRTC standalone OS/X.
> 
> Some feedback for hjon and a question for kjellander:
> In addition to OS==mac we'll need something to prevent our new bits from being
> compiled in Chromium builds. I don't know if mac_deployment_target is visible,
> but if it is that should be added to the condition check.

It looks like mac_deployment_target is visible. Would I be correct in thinking
that the condition check should change to:

'OS=="ios" or (OS=="mac" and mac_deployment_target=="10.10")'

This would be only for new webrtc/api bits, right? Is there a similar check for
GN?

[tkchin_webrtc, 2016-01-29 00:26:29]

== '10.10' seems restrictive. We can define more variables I suppose.
e.g. mac_deployment_target_major, mac_deployment_target_minor,
is_yosemite_and_above

We want >= '10.10' but that doesn't work because 10.9 > 10.10 so that's why we
might want additional vars.


for GN I don't know - I'd grep around chromium and see if there's anything
similar (I haven't used GN much)

https://codereview.webrtc.org/1644843003/diff/1/webrtc/api/BUILD.gn
File webrtc/api/BUILD.gn (right):

https://codereview.webrtc.org/1644843003/diff/1/webrtc/api/BUILD.gn#newcode62
webrtc/api/BUILD.gn:62: "../build/WebRTC-Prefix.pch",
On 2016/01/28 23:49:51, hjon_webrtc wrote:
> On 2016/01/28 21:09:44, tkchin_webrtc wrote:
> > I don't think this is how precompiled headers are supposed to be added.
Found
> > this:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/build/config/BUILD...
> > 
> > Can you file a bug for GN iOS builds and reference it here? You can assign
it
> to
> > me and cc kjellander.
> 
> I'm not clear on what the bug should be. Could you clarify?

iOS WebRTC GN Build
- Fix prefix headers
- Get it to work

Just a place to accumulate tasks.

https://codereview.webrtc.org/1644843003/diff/1/webrtc/base/logging.cc
File webrtc/base/logging.cc (right):

https://codereview.webrtc.org/1644843003/diff/1/webrtc/base/logging.cc#newcod...
webrtc/base/logging.cc:172: #pragma clang diagnostic push
On 2016/01/28 23:49:51, hjon_webrtc wrote:
> On 2016/01/28 21:09:44, tkchin_webrtc wrote:
> > compiler directives have no indentation
> > Can you file a bug and reference it? Here and rest of CL.
> > 
> > Bug should be: Replace deprecated OS/X system calls.
> > And then add that there aren't actually good replacements.
> > You can file it on tommi@ for now and cc me. 
> 
> Fixed indentation. Is there a preferred way to reference bugs? I think I've
seen
> things like http://crbug.com/%3CbugNumber%3E. Is there anything like that for
> WebRTC?

I'm not against pasting links if it fits in 80.
// TODO(hjon): Update deprecated APIs.
// See <url>
should be fine.

Or if you don't want your name attached to it... :)
// TODO(bug#): Fix deprecated APIs.

[hjon_webrtc, 2016-02-01 00:02:20]

tkchin@ kjellander@ I've added an initial proposal for a better check of the
mac_deployment_target by adding checks for the major and minor versions.

I haven't tried this yet, but it looks like something similar could be done for
GN (based on seeing 'mac_deployment_target' in this file:
https://code.google.com/p/chromium/codesearch#chromium/src/build/config/mac/m...).

Just to make sure we're on the same page, I think the basic idea is that the
'rtc_api_objc' target should only be available if the deployment target is high
enough. Someone building can just use the default deployment target (thus we
want it to be high enough) or sets it to something else when building). So there
are two pieces - the first is setting a default value for the deployment target
and the second is verifying that the deployment target is high enough,
regardless of how it was set. Is that right?

[kjellander_webrtc, 2016-02-01 06:08:29]

Nice effort in bringing the objc libraries to desktop Mac. I added a few
comments.

Also, make the BUG= line read
BUG=webrtc:5453
to get a proper link to the right bug tracker.

https://codereview.webrtc.org/1644843003/diff/40001/webrtc/api/BUILD.gn
File webrtc/api/BUILD.gn (right):

https://codereview.webrtc.org/1644843003/diff/40001/webrtc/api/BUILD.gn#newco...
webrtc/api/BUILD.gn:62: # TODO(5477): Fix prefix headers.
The format of TODOs is generally TODO(username), so please make this something
like:
TODO(hjon): Fix prefix headers (see webrtc:5477 for details).

https://codereview.webrtc.org/1644843003/diff/40001/webrtc/api/BUILD.gn#newco...
webrtc/api/BUILD.gn:95: "objc/avfoundationvideocapturer.h",
So we can't add these for Desktop Mac? AVFoundation has been available since
10.7 so since we require 10.10, shouldn't it also work for is_mac?

https://codereview.webrtc.org/1644843003/diff/40001/webrtc/api/api.gyp
File webrtc/api/api.gyp (right):

https://codereview.webrtc.org/1644843003/diff/40001/webrtc/api/api.gyp#newcode12
webrtc/api/api.gyp:12: ['OS=="ios" or (OS=="mac" and
mac_deployment_target_major>=10 and '
I guess when Apple (if ever) increments the mac_deployment_target_major to 11,
this condition will stop being true since mac_deployment_target_minor would
likely become 0.

It would make more sense to check for mac_deployment_target_major==10 and
mac_deployment_target_minor>=10.

https://codereview.webrtc.org/1644843003/diff/40001/webrtc/base/logging.cc
File webrtc/base/logging.cc (right):

https://codereview.webrtc.org/1644843003/diff/40001/webrtc/base/logging.cc#ne...
webrtc/base/logging.cc:172: // TODO(5478): Update deprecated APIs.
Use proper format for TODOs (see previous comment). Applies to rest of CL as
well.

https://codereview.webrtc.org/1644843003/diff/40001/webrtc/build/objc_common....
File webrtc/build/objc_common.gypi (right):

https://codereview.webrtc.org/1644843003/diff/40001/webrtc/build/objc_common....
webrtc/build/objc_common.gypi:1: # Copyright 2015 The WebRTC project authors.
All Rights Reserved.
I guess this should be 2016 now.

https://codereview.webrtc.org/1644843003/diff/40001/webrtc/supplement.gypi
File webrtc/supplement.gypi (right):

https://codereview.webrtc.org/1644843003/diff/40001/webrtc/supplement.gypi#ne...
webrtc/supplement.gypi:15: 'mac_deployment_target_major%': 10,
I don't like adding these variables just for comparing versions, especially as
that check will break when mac_deployment_target_major (if ever) becomes 11 and
mac_deployment_target_minor is reset to 0.

Since doing greater than string comparison with mac_deployment_target works
equally well (although a bit ugly), I suggest we keep that (and only that)
variable instead.

I've been digging around but couldn't find anything similar in the Chromium
codebase. Maybe you can reach out to the Chrome on iOS team and ask what the
best practice is for conditions based on SDK level in GYP? (since their iOS code
is closed-source).

[kjellander_webrtc, 2016-02-01 06:19:24]

On 2016/02/01 00:02:20, hjon_webrtc wrote:
> tkchin@ kjellander@ I've added an initial proposal for a better check of the
> mac_deployment_target by adding checks for the major and minor versions.
> 
> I haven't tried this yet, but it looks like something similar could be done
for
> GN (based on seeing 'mac_deployment_target' in this file:
>
https://code.google.com/p/chromium/codesearch#chromium/src/build/config/mac/m...).

Yes, that seems to be the place where they're defined for the GN build.
To override them in a similar fashion as for GYP (webrtc/supplement.gypi), you
can set defaults for the WebRTC standalone build (that won't affect Chromium)
in the build_overrides/webrtc.gni file.

> Just to make sure we're on the same page, I think the basic idea is that the
> 'rtc_api_objc' target should only be available if the deployment target is
high
> enough. Someone building can just use the default deployment target (thus we
> want it to be high enough) or sets it to something else when building). So
there
> are two pieces - the first is setting a default value for the deployment
target
> and the second is verifying that the deployment target is high enough,
> regardless of how it was set. Is that right?

Yes, I think you got it right. If someone explicitly sets it lower than our
default, they won't get
these targets available, which is what we want (since they won't build).

[kjellander_webrtc, 2016-02-01 06:20:19]

Another comment.

https://codereview.webrtc.org/1644843003/diff/40001/webrtc/supplement.gypi
File webrtc/supplement.gypi (right):

https://codereview.webrtc.org/1644843003/diff/40001/webrtc/supplement.gypi#ne...
webrtc/supplement.gypi:15: 'mac_deployment_target_major%': 10,
On 2016/02/01 06:08:29, kjellander (webrtc) wrote:
> I don't like adding these variables just for comparing versions, especially as
> that check will break when mac_deployment_target_major (if ever) becomes 11
and
> mac_deployment_target_minor is reset to 0.
> 
> Since doing greater than string comparison with mac_deployment_target works
> equally well (although a bit ugly), I suggest we keep that (and only that)
> variable instead.
> 
> I've been digging around but couldn't find anything similar in the Chromium
> codebase. Maybe you can reach out to the Chrome on iOS team and ask what the
> best practice is for conditions based on SDK level in GYP? (since their iOS
code
> is closed-source).

I found at least one greather than string comparison in the Chromium code base:
https://code.google.com/p/chromium/codesearch#chromium/src/testing/iossim/ios...
I also found that it's possible to query substrings like this:
https://code.google.com/p/chromium/codesearch#chromium/src/tools/gyp/test/win...
in GYP (which I wasn't aware of), which may be useful if we want to check for
>=10 minor version without introducing new GYP variables.

[hjon_webrtc, 2016-02-02 00:34:46]

Description was changed from

==========
Compile rtc_base_objc and rtc_api_objc for Mac

BUG=5453
==========

to

==========
Compile rtc_base_objc and rtc_api_objc for Mac

BUG=webrtc:5453
==========

[tkchin_webrtc, 2016-02-02 10:04:46]

https://codereview.webrtc.org/1644843003/diff/40001/webrtc/api/BUILD.gn
File webrtc/api/BUILD.gn (right):

https://codereview.webrtc.org/1644843003/diff/40001/webrtc/api/BUILD.gn#newco...
webrtc/api/BUILD.gn:95: "objc/avfoundationvideocapturer.h",
On 2016/02/01 06:08:29, kjellander (webrtc) wrote:
> So we can't add these for Desktop Mac? AVFoundation has been available since
> 10.7 so since we require 10.10, shouldn't it also work for is_mac?

I never tested them for Desktop Mac. Afaik I'm using iOS specific capture modes.
It should work with minor modification though.

[hjon_webrtc, 2016-02-02 18:41:49]

On 2016/02/02 10:04:46, tkchin_webrtc wrote:
> https://codereview.webrtc.org/1644843003/diff/40001/webrtc/api/BUILD.gn
> File webrtc/api/BUILD.gn (right):
> 
>
https://codereview.webrtc.org/1644843003/diff/40001/webrtc/api/BUILD.gn#newco...
> webrtc/api/BUILD.gn:95: "objc/avfoundationvideocapturer.h",
> On 2016/02/01 06:08:29, kjellander (webrtc) wrote:
> > So we can't add these for Desktop Mac? AVFoundation has been available since
> > 10.7 so since we require 10.10, shouldn't it also work for is_mac?
> 
> I never tested them for Desktop Mac. Afaik I'm using iOS specific capture
modes.
> It should work with minor modification though.

Quick comment and then I need to try some of these things. :-)

I did try getting the AVFoundation-based video capturer working for Mac. A few
small adjustments were enough to make it compile. I think some more should be
done since it currently assumes only a front and back camera and it sounds like
the capture modes might need tweaking. This sounds to me like something worth
doing in another CL?

Is this something that could/should be reused?
https://code.google.com/p/chromium/codesearch#chromium/src/media/capture/vide...

[kjellander_webrtc, 2016-02-02 20:20:11]

Don't let the AVfoundation on Mac issue block the work in this CL, that can be
dealt with in the future. This CL still brings a lot for Mac that wasn't
previously available.

I don't know anything about the capture code in Chromium and/or if it's reusable
for us.

I'd like to see some of my previous comments addressed still, though.

https://codereview.webrtc.org/1644843003/diff/40001/webrtc/api/BUILD.gn
File webrtc/api/BUILD.gn (right):

https://codereview.webrtc.org/1644843003/diff/40001/webrtc/api/BUILD.gn#newco...
webrtc/api/BUILD.gn:95: "objc/avfoundationvideocapturer.h",
On 2016/02/02 10:04:46, tkchin_webrtc wrote:
> On 2016/02/01 06:08:29, kjellander (webrtc) wrote:
> > So we can't add these for Desktop Mac? AVFoundation has been available since
> > 10.7 so since we require 10.10, shouldn't it also work for is_mac?
> 
> I never tested them for Desktop Mac. Afaik I'm using iOS specific capture
modes.
> It should work with minor modification though.

I see, I was just wondering (I didn't look in the source code). Let's not worry
about that in this CL then (possibly future improvement).

[hjon_webrtc, 2016-02-02 23:03:50]

https://codereview.webrtc.org/1644843003/diff/40001/webrtc/api/BUILD.gn
File webrtc/api/BUILD.gn (right):

https://codereview.webrtc.org/1644843003/diff/40001/webrtc/api/BUILD.gn#newco...
webrtc/api/BUILD.gn:62: # TODO(5477): Fix prefix headers.
On 2016/02/01 06:08:29, kjellander (webrtc) wrote:
> The format of TODOs is generally TODO(username), so please make this something
> like:
> TODO(hjon): Fix prefix headers (see webrtc:5477 for details).

Done.

https://codereview.webrtc.org/1644843003/diff/40001/webrtc/base/logging.cc
File webrtc/base/logging.cc (right):

https://codereview.webrtc.org/1644843003/diff/40001/webrtc/base/logging.cc#ne...
webrtc/base/logging.cc:172: // TODO(5478): Update deprecated APIs.
On 2016/02/01 06:08:29, kjellander (webrtc) wrote:
> Use proper format for TODOs (see previous comment). Applies to rest of CL as
> well.

Done.

https://codereview.webrtc.org/1644843003/diff/40001/webrtc/build/objc_common....
File webrtc/build/objc_common.gypi (right):

https://codereview.webrtc.org/1644843003/diff/40001/webrtc/build/objc_common....
webrtc/build/objc_common.gypi:1: # Copyright 2015 The WebRTC project authors.
All Rights Reserved.
On 2016/02/01 06:08:29, kjellander (webrtc) wrote:
> I guess this should be 2016 now.

Should I just go ahead and change this in all of the files I touched or is there
a preferred way to maintain consistency across different directories?

[hjon_webrtc, 2016-02-02 23:05:36]

Summarizing our goals:
Goal 1: Generate/build the 'rtc_api_objc' target for Mac only if the deployment
target is greater than 10.10.
Goal 2: The test 10.9>=10.10 should be false.
Goal 3: The test 11.0>=10.10 should be true.
Goal 4: No extra variables.

Options that I've tried so far and their results and tradeoffs:
1. String comparison

I used a 'mac_deployment_target' of '10.9' and tested
'mac_deployment_target>="10.10"'. The result was true. Goal 2 isn't met.

2. Variables for mac_deployment_target_major and mac_deployment_target_minor

This also requires the values to be numbers, not strings. This works while the
major version remains '10', but will fail if/when it becomes '11' and the minor
version is reset to '0'. Goals 3 and 4 aren't met.

3. Query substrings

With a 'mac_deployment_target' of '10.9', I tested both
'mac_deployment_target[3:]>=10' and 'mac_deployment_target[3:]>="10"'. Both
results were true.

To confirm behavior, I tried a 'mac_deployment_target' of '10.10', I tested both
'mac_deployment_target[3:]>=10' and 'mac_deployment_target[3:]>="10"'. Both
results were true.

Goal 2 isn't met.

4. Script?

I noticed a .gni file where a script is run to get some additional information
(path to SDK and signing identity):
https://code.google.com/p/chromium/codesearch#chromium/src/build/config/ios/i....
That's for GN, but is executing a script an acceptable option? All stated goals
could be met.

At the moment, Option 2 seems like the best tradeoff to me, though Option 4
would accomplish all goals and perhaps there's an option we haven't discussed
yet. But it seems to me that Goal 2 is more likely and a higher priority than
Goal 3.

Thoughts?

[kjellander_webrtc, 2016-02-03 07:57:33]

Thanks for trying hard to avoid the new variables, but let's introduce them as
it seems to be the best option.

lgtm with my new comment addressed.

https://codereview.webrtc.org/1644843003/diff/40001/webrtc/build/objc_common....
File webrtc/build/objc_common.gypi (right):

https://codereview.webrtc.org/1644843003/diff/40001/webrtc/build/objc_common....
webrtc/build/objc_common.gypi:1: # Copyright 2015 The WebRTC project authors.
All Rights Reserved.
On 2016/02/02 23:03:50, hjon_webrtc wrote:
> On 2016/02/01 06:08:29, kjellander (webrtc) wrote:
> > I guess this should be 2016 now.
> 
> Should I just go ahead and change this in all of the files I touched or is
there
> a preferred way to maintain consistency across different directories?

No it only applies to newly added files, luckily. So this one is the only one
affected (since WebRTC-Prefix.pch was moved). See
http://dev.chromium.org/developers/coding-style#TOC-File-headers for more info.

https://codereview.webrtc.org/1644843003/diff/80001/webrtc/supplement.gypi
File webrtc/supplement.gypi (right):

https://codereview.webrtc.org/1644843003/diff/80001/webrtc/supplement.gypi#ne...
webrtc/supplement.gypi:15: 'mac_deployment_target_major%': 10,
I give up and to accept these two new variables after reading your thorough
analysis - thanks for putting in the effort!

Can you add a comment here (and at mac_deployment_target above) to point out
that they need to be kept in sync?
Also mention that these latter two does not exist in Chromium.

[tkchin_webrtc, 2016-02-03 08:26:49]

Actually I think you were waiting for feedback re: vars. But fyi anyway.

https://codereview.webrtc.org/1644843003/diff/80001/webrtc/api/api_tests.gyp
File webrtc/api/api_tests.gyp (right):

https://codereview.webrtc.org/1644843003/diff/80001/webrtc/api/api_tests.gyp#...
webrtc/api/api_tests.gyp:12: ['OS=="ios" or OS=="mac"', {
update this one too, otherwise we'll have gyp failure because it'll be missing
dependency target

https://codereview.webrtc.org/1644843003/diff/80001/webrtc/base/BUILD.gn
File webrtc/base/BUILD.gn (right):

https://codereview.webrtc.org/1644843003/diff/80001/webrtc/base/BUILD.gn#newc...
webrtc/base/BUILD.gn:620: if (is_ios || is_mac) {
Might need the deployment target check here too.

[hjon_webrtc, 2016-02-04 01:43:03]

https://codereview.webrtc.org/1644843003/diff/40001/webrtc/api/api.gyp
File webrtc/api/api.gyp (right):

https://codereview.webrtc.org/1644843003/diff/40001/webrtc/api/api.gyp#newcode12
webrtc/api/api.gyp:12: ['OS=="ios" or (OS=="mac" and
mac_deployment_target_major>=10 and '
On 2016/02/01 06:08:29, kjellander (webrtc) wrote:
> I guess when Apple (if ever) increments the mac_deployment_target_major to 11,
> this condition will stop being true since mac_deployment_target_minor would
> likely become 0.
> 
> It would make more sense to check for mac_deployment_target_major==10 and
> mac_deployment_target_minor>=10.

Do you think this should still be == rather than >=?

https://codereview.webrtc.org/1644843003/diff/40001/webrtc/build/objc_common....
File webrtc/build/objc_common.gypi (right):

https://codereview.webrtc.org/1644843003/diff/40001/webrtc/build/objc_common....
webrtc/build/objc_common.gypi:1: # Copyright 2015 The WebRTC project authors.
All Rights Reserved.
On 2016/02/03 07:57:33, kjellander (webrtc) wrote:
> On 2016/02/02 23:03:50, hjon_webrtc wrote:
> > On 2016/02/01 06:08:29, kjellander (webrtc) wrote:
> > > I guess this should be 2016 now.
> > 
> > Should I just go ahead and change this in all of the files I touched or is
> there
> > a preferred way to maintain consistency across different directories?
> 
> No it only applies to newly added files, luckily. So this one is the only one
> affected (since WebRTC-Prefix.pch was moved). See
> http://dev.chromium.org/developers/coding-style#TOC-File-headers for more
info.

Done.

https://codereview.webrtc.org/1644843003/diff/80001/webrtc/api/api_tests.gyp
File webrtc/api/api_tests.gyp (right):

https://codereview.webrtc.org/1644843003/diff/80001/webrtc/api/api_tests.gyp#...
webrtc/api/api_tests.gyp:12: ['OS=="ios" or OS=="mac"', {
On 2016/02/03 08:26:49, tkchin_webrtc wrote:
> update this one too, otherwise we'll have gyp failure because it'll be missing
> dependency target

Done.

https://codereview.webrtc.org/1644843003/diff/80001/webrtc/base/BUILD.gn
File webrtc/base/BUILD.gn (right):

https://codereview.webrtc.org/1644843003/diff/80001/webrtc/base/BUILD.gn#newc...
webrtc/base/BUILD.gn:620: if (is_ios || is_mac) {
On 2016/02/03 08:26:49, tkchin_webrtc wrote:
> Might need the deployment target check here too.

I added the deployment target check and added the same variables to
build_overrides/webrtc.gni. However, when I tested this (with `gn gen out
--args='target_os="mac" target_cpu="x86"'`), I received the error listed below.
Is there a way to get around this?

ERROR at //webrtc/BUILD.gn:13:1: Value collision.
import("build/webrtc.gni")
^------------------------
This import contains "mac_deployment_target"
See //build_overrides/webrtc.gni:27:25: defined here.
mac_deployment_target = "10.10"
                        ^------
Which would clobber the one in your current scope
See //build/config/mac/mac_sdk.gni:12:27: defined here.
  mac_deployment_target = "10.6"
                          ^-----
Executing import should not conflict with anything in the current
scope unless the values are identical.

https://codereview.webrtc.org/1644843003/diff/80001/webrtc/supplement.gypi
File webrtc/supplement.gypi (right):

https://codereview.webrtc.org/1644843003/diff/80001/webrtc/supplement.gypi#ne...
webrtc/supplement.gypi:15: 'mac_deployment_target_major%': 10,
On 2016/02/03 07:57:33, kjellander (webrtc) wrote:
> I give up and to accept these two new variables after reading your thorough
> analysis - thanks for putting in the effort!
> 
> Can you add a comment here (and at mac_deployment_target above) to point out
> that they need to be kept in sync?
> Also mention that these latter two does not exist in Chromium.

Done.

[kjellander_webrtc, 2016-02-08 09:20:46]

kjellander@webrtc.org changed reviewers:
+ dpranke@chromium.org

[kjellander_webrtc, 2016-02-08 09:25:52]

I have to pull in the heavy artillery here; adding dpranke@. He's probably the
guy (together with brettw@) that knows most of GN and GYP.

https://codereview.webrtc.org/1644843003/diff/40001/webrtc/api/api.gyp
File webrtc/api/api.gyp (right):

https://codereview.webrtc.org/1644843003/diff/40001/webrtc/api/api.gyp#newcode12
webrtc/api/api.gyp:12: ['OS=="ios" or (OS=="mac" and
mac_deployment_target_major>=10 and '
On 2016/02/04 01:43:03, hjon_webrtc wrote:
> On 2016/02/01 06:08:29, kjellander (webrtc) wrote:
> > I guess when Apple (if ever) increments the mac_deployment_target_major to
11,
> > this condition will stop being true since mac_deployment_target_minor would
> > likely become 0.
> > 
> > It would make more sense to check for mac_deployment_target_major==10 and
> > mac_deployment_target_minor>=10.
> 
> Do you think this should still be == rather than >=?

I guess == is better than >= since it makes it more obvious that it'll need to
be updated when (if) mac_deployment_target_major becomes 11 in the future.

https://codereview.webrtc.org/1644843003/diff/80001/webrtc/base/BUILD.gn
File webrtc/base/BUILD.gn (right):

https://codereview.webrtc.org/1644843003/diff/80001/webrtc/base/BUILD.gn#newc...
webrtc/base/BUILD.gn:620: if (is_ios || is_mac) {
On 2016/02/04 01:43:03, hjon_webrtc wrote:
> On 2016/02/03 08:26:49, tkchin_webrtc wrote:
> > Might need the deployment target check here too.
> 
> I added the deployment target check and added the same variables to
> build_overrides/webrtc.gni. However, when I tested this (with `gn gen out
> --args='target_os="mac" target_cpu="x86"'`), I received the error listed
below.
> Is there a way to get around this?
> 
> ERROR at //webrtc/BUILD.gn:13:1: Value collision.
> import("build/webrtc.gni")
> ^------------------------
> This import contains "mac_deployment_target"
> See //build_overrides/webrtc.gni:27:25: defined here.
> mac_deployment_target = "10.10"
>                         ^------
> Which would clobber the one in your current scope
> See //build/config/mac/mac_sdk.gni:12:27: defined here.
>   mac_deployment_target = "10.6"
>                           ^-----
> Executing import should not conflict with anything in the current
> scope unless the values are identical.

Argh, I realize build_overrides/webrtc.gni can only be used to specify a
standalone-WebRTC value for variables that only exists in own GN build (i.e. not
defined in any of the Chromium parts). That is only true for the
mac_deployment_target_{major,minor} variables, not mac_deployment_target.

Dirk: Do you know what we should do here? Maybe you know a better way to do what
hjon wants here, that could avoid adding the mac_deployment_target_{major,minor}
variables as well?

[Dirk Pranke, 2016-02-08 22:06:06]

I'm not entirely sure what you're trying to do here, but I'll see if I can ask a
few questions and offer some suggestions.

As you've seen, you can't declare the same argument in two different gni files,
so that's not going to work.

Is your intent w/ changing mac_deployment_target to actually the way
*everything* is compiled (i.e., to pass -mmacosx-version-min=10.10 everywhere)?

If so, then the way to do it is to change what the default value we're using in
//build/config/mac/mac_sdk.gni is.
If you only want to do this in standalone webrtc builds, then either we'd need
to do so via args.gn, or make
mac_sdk.gni import //build_overrides/webrtc.gni and do something different based
on build_with_chromium (or
a new variable, maybe, since "build_with_chromium" is poorly named for something
referenced outside of webrtc).
I'm a bit reluctant to make //build depend on //build_config, but it may be
unavoidable for this situation.

On the other hand, if you only need to affect the way the webrtc targets are
compiled, there's probably more
localized ways of affecting this that don't require you to change
mac_deployment_target globally.

Apart from that, I don't really understand why you are adding the other
mac_deployment_target_* variables at
all, or why you have two of them, since you seem to always use them both the
same way?

[tkchin_webrtc, 2016-02-09 16:02:42]

On 2016/02/08 22:06:06, Dirk Pranke wrote:
> I'm not entirely sure what you're trying to do here, but I'll see if I can ask
a
> few questions and offer some suggestions.
> 
> As you've seen, you can't declare the same argument in two different gni
files,
> so that's not going to work.
> 
> Is your intent w/ changing mac_deployment_target to actually the way
> *everything* is compiled (i.e., to pass -mmacosx-version-min=10.10
everywhere)?
> 
> If so, then the way to do it is to change what the default value we're using
in
> //build/config/mac/mac_sdk.gni is.
> If you only want to do this in standalone webrtc builds, then either we'd need
> to do so via args.gn, or make
> mac_sdk.gni import //build_overrides/webrtc.gni and do something different
based
> on build_with_chromium (or
> a new variable, maybe, since "build_with_chromium" is poorly named for
something
> referenced outside of webrtc).
> I'm a bit reluctant to make //build depend on //build_config, but it may be
> unavoidable for this situation.
> 
> On the other hand, if you only need to affect the way the webrtc targets are
> compiled, there's probably more
> localized ways of affecting this that don't require you to change
> mac_deployment_target globally.
> 
> Apart from that, I don't really understand why you are adding the other
> mac_deployment_target_* variables at
> all, or why you have two of them, since you seem to always use them both the
> same way?

We're basically wading in unfamiliar gn+Mac+iOS land and would really appreciate
some expertise :)

Some (but not all) of our targets require a higher deployment target (>=10.10)
because we're using features like ARC and nullability. Those components are
unlikely to be used in Chromium, and should be considered part of WebRTC
standalone. However, we don't want to break Chromium so we want to filter out
those targets during a Chromium build (which uses a lower deployment target of
10.7).

The extra variables were an artifact of python string comparisons - we wanted to
filter out OSX version >= 10.10, but "10.9" > "10.10" is true so we tried
splitting up the major/minor parts of the version.

If we can affect mac_deployment_target locally instead of globally that'd be
ideal (and have the deployment level passed down to dependencies). I don't know
how to do that offhand though in either of GYP and GN land.

[Dirk Pranke, 2016-02-09 23:36:45]

+erikchen@chromium.org , who is more of mac guru than I ...

My vague understanding of these things based on a short amount of googling is
that you should be able to still use ARC (and possibly _Nullable et. al) on a
10.6 deployment target with a few gotchas, as long as you are building against
the 10.10 SDK, which is what Chromium is doing. You're sure that that won't work
for your needs?

Regardless, I suspect that having the option to set the minimum deployment
target (or even the SDK version) per target would cause no end of confusion and
weirdness, so while it would be technically possible to restructure the //build
files to allow for this, it's probably a bad idea to do so.

I think instead it's probably better to give you a way to override
mac_deployment_target globally based on the value of something in
//build_overrides/webrtc.gni. I think such a change is probably straightforward;
do you want to take a stab at it (basically import //build/webrtc.gni from
mac_sdk.gni), or do you want me to do such a change?

[erikchen, 2016-02-09 23:42:54]

On 2016/02/09 23:36:45, Dirk Pranke wrote:
> mailto:+erikchen@chromium.org , who is more of mac guru than I ...
> 
> My vague understanding of these things based on a short amount of googling is
> that you should be able to still use ARC (and possibly _Nullable et. al) on a
> 10.6 deployment target with a few gotchas, as long as you are building against
> the 10.10 SDK, which is what Chromium is doing. You're sure that that won't
work
> for your needs?
> 
> Regardless, I suspect that having the option to set the minimum deployment
> target (or even the SDK version) per target would cause no end of confusion
and
> weirdness, so while it would be technically possible to restructure the
//build
> files to allow for this, it's probably a bad idea to do so.
> 
> I think instead it's probably better to give you a way to override
> mac_deployment_target globally based on the value of something in
> //build_overrides/webrtc.gni. I think such a change is probably
straightforward;
> do you want to take a stab at it (basically import //build/webrtc.gni from
> mac_sdk.gni), or do you want me to do such a change?

It's not clear to me why the deployment target needs to be bumped? ARC only
requires 10.7:
https://developer.apple.com/library/prerelease/ios/releasenotes/ObjectiveC/Ob...

I'm under the impression that nullability is a compile-time check that doesn't
require runtime support, just a sufficiently recent compiler/SDK.

[tkchin_webrtc, 2016-02-10 16:46:07]

On 2016/02/09 23:42:54, erikchen wrote:
> On 2016/02/09 23:36:45, Dirk Pranke wrote:
> > mailto:+erikchen@chromium.org , who is more of mac guru than I ...
> > 
> > My vague understanding of these things based on a short amount of googling
is
> > that you should be able to still use ARC (and possibly _Nullable et. al) on
a
> > 10.6 deployment target with a few gotchas, as long as you are building
against
> > the 10.10 SDK, which is what Chromium is doing. You're sure that that won't
> work
> > for your needs?
> > 
> > Regardless, I suspect that having the option to set the minimum deployment
> > target (or even the SDK version) per target would cause no end of confusion
> and
> > weirdness, so while it would be technically possible to restructure the
> //build
> > files to allow for this, it's probably a bad idea to do so.
> > 
> > I think instead it's probably better to give you a way to override
> > mac_deployment_target globally based on the value of something in
> > //build_overrides/webrtc.gni. I think such a change is probably
> straightforward;
> > do you want to take a stab at it (basically import //build/webrtc.gni from
> > mac_sdk.gni), or do you want me to do such a change?
> 
> It's not clear to me why the deployment target needs to be bumped? ARC only
> requires 10.7:
>
https://developer.apple.com/library/prerelease/ios/releasenotes/ObjectiveC/Ob...
> 
> I'm under the impression that nullability is a compile-time check that doesn't
> require runtime support, just a sufficiently recent compiler/SDK.

@dirk
 I'm not well set up for the chromium repo, so if it's something that's easy for
you to do, we'd appreciate it.

@erik
We use zero-ing weak references so we do need 10.7 at least. Chromium appears to
be 10.6
(https://code.google.com/p/chromium/codesearch#chromium/src/build/common.gypi&...).
So we will still need the ability to override the deployment target, as well as
the ability to check if the deployment target is a sufficiently high version.
(Unless I'm missing something).

I'd agree that nullability is a compile time check that doesn't require runtime
support as long as we're compiling against the right SDK. So not 10.10, but 10.7
should be enough.
The bots were unhappy with me, with 10.10 SDK and nullability though:
https://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds...
Is this because the clang version in the chromium repo is old? [clang version
3.9.0 (trunk 259396) vs Apple LLVM version 7.0.2 (clang-700.1.81)]
That seems weird because the macro should just be something defined in the SDK.
We could try updating the bots to set clang_xcode=1 perhaps.

Alternately, I tried stuffing some guards into a pch before when I was looking
at this but it didn't seemed to be picked up by the bots, so I'm not sure what's
going on there:
https://codereview.webrtc.org/1564953002/diff/80001/webrtc/build/WebRTC-Prefi...

[Dirk Pranke, 2016-02-10 22:28:51]

On 2016/02/10 16:46:07, tkchin_webrtc wrote:
> @dirk
>  I'm not well set up for the chromium repo, so if it's something that's easy
for
> you to do, we'd appreciate it.

Okay, I'll post something soonish.

[Dirk Pranke, 2016-02-11 02:02:41]

I've posted a chromium CL to https://codereview.chromium.org/1686373002/ .
Please take a look and
see if it'll work for you here.

I didn't add configurable deployment variables. Hopefully chromium will move to
requiring 10.7 soon,
and then we can reconverge, so I wanted to keep things as simple as possible.

[hjon_webrtc, 2016-02-11 19:29:15]

On 2016/02/11 02:02:41, Dirk Pranke wrote:
> I've posted a chromium CL to https://codereview.chromium.org/1686373002/ .
> Please take a look and
> see if it'll work for you here.
> 
> I didn't add configurable deployment variables. Hopefully chromium will move
to
> requiring 10.7 soon,
> and then we can reconverge, so I wanted to keep things as simple as possible.

From this discussion, sounds like for GYP we should change it back to simply
setting mac_deployment_target to 10.7 in webrtc/supplement.gypi? Is that
preferred rather than adding another variable so the GYP and GN files are more
in sync?

To confirm that this should work in practice, I used a mac_deployment_target of
10.7 and tested on my machine, compiling against a Base SDK of 10.11 and using
both Xcode's version of clang [Apple LLVM version 7.0.2 (clang-700.1.81)] and
Chromium's version [for my test, clang version 3.9.0 (trunk 257955)].

With Xcode's clang, I received some warnings about warning options, but
otherwise it built just fine:

warning: unknown warning option '-Wpartial-availability'; did you mean
'-Wavailability'? [-Wunknown-warning-option]
warning: unknown warning option '-Wfor-loop-analysis'; did you mean
'-Wloop-analysis'? [-Wunknown-warning-option]
warning: unknown warning option '-Wno-shift-negative-value'; did you mean
'-Wno-shift-sign-overflow'? [-Wunknown-warning-option]

With Chromium's clang, I initially received the following error about
subscripting, but it built just fine after resolving that issue (using
-objectForKey instead of subscripting). On my machine, I didn't run into issues
with nullability.

../../../webrtc/api/objc/RTCMediaConstraints.mm:84:15: error:
'objectForKeyedSubscript:' is partial: introduced in OS X 10.8
[-Werror,-Wpartial-availability]
    NSAssert([constraints[key] isKindOfClass:[NSString class]],
              ^

Unless there's an objection, I'll update this CL to set mac_deployment_target to
10.7 and remove usage of object subscripting.

[Dirk Pranke, 2016-02-11 19:32:44]

On 2016/02/11 19:29:15, hjon_webrtc wrote:
> On 2016/02/11 02:02:41, Dirk Pranke wrote:
> > I've posted a chromium CL to https://codereview.chromium.org/1686373002/ .
> > Please take a look and
> > see if it'll work for you here.
> > 
> > I didn't add configurable deployment variables. Hopefully chromium will move
> to
> > requiring 10.7 soon,
> > and then we can reconverge, so I wanted to keep things as simple as
possible.
> 
> From this discussion, sounds like for GYP we should change it back to simply
> setting mac_deployment_target to 10.7 in webrtc/supplement.gypi? Is that
> preferred rather than adding another variable so the GYP and GN files are more
> in sync?

That seems like a good idea. 

I have no useful opinion on the object subscripting part.

[hjon_webrtc, 2016-02-12 22:46:15]

Per IRL chat with @tkchin, we decided to just get GYP going for now and deal
with GN separately, so I've removed the changes to the GN files (other than some
changes that are still relevant).