Allow WebRTC to require Mac 10.7 deployments in GN builds.

Allow WebRTC to require Mac 10.7 deployments in GN builds.

The standalone (non-Chromium) WebRTC build has some targets
that on the Mac use Objective-C features that require the
targets to be deployed to 10.7 or newer hosts (i.e., they
won't run on 10.6).

Because we share GN //build configurations between the WebRTC
and Chromium project, we need to give WebRTC to toggle the
minimum configuration needed; we do this by creating a new file called
build.gni in //build_overrides/build.gni file, and
making //build/config/mac/mac_sdk.gni import that. This introduces
a dependency from //build to //build_overrides, and means that other
projects that depend on //build will need to create a new file when
this change is rolled in..

R=brettw@chromium.org
BUG=webrtc:5453, 588513
Committed: https://crrev.com/769de4d88df49a35f42932e71fbcb3ad1a26ec8d
Cr-Commit-Position: refs/heads/master@{#376856}

[Dirk Pranke, 2016-02-11 01:59:45]

Description was changed from

==========
Allow WebRTC to require Mac 10.7 deployments in GN builds.

The standalone (non-Chromium) WebRTC build has some targets
that on the Mac use Objective-C features that require the
targets to be deployed to 10.7 or newer hosts (i.e., they
won't run on 10.6).

Because we share GN //build configurations between the WebRTC
and Chromium project, we need to give WebRTC to toggle the
minimum configuration needed; we do this by adding another
variable to the //build_overrides/webrtc.gni file, and
make //build/config/mac/mac_sdk.gni import that. This introduces
a dependency from //build to //build_overrides, but I think that
should be fine.

This CL also adds a new variable to the WebRTC build_override
file called 'rtc_build_with_chromium', to replace the existing
'build_with_chromium' variable; the latter is too generic a name
to tell what it does.

R=erikchen@chromium.org, brettw@chromium.org
BUG=webrtc:5453
==========

to

==========
Allow WebRTC to require Mac 10.7 deployments in GN builds.

The standalone (non-Chromium) WebRTC build has some targets
that on the Mac use Objective-C features that require the
targets to be deployed to 10.7 or newer hosts (i.e., they
won't run on 10.6).

Because we share GN //build configurations between the WebRTC
and Chromium project, we need to give WebRTC to toggle the
minimum configuration needed; we do this by adding another
variable to the //build_overrides/webrtc.gni file, and
make //build/config/mac/mac_sdk.gni import that. This introduces
a dependency from //build to //build_overrides, but I think that
should be fine.

This CL also adds a new variable to the WebRTC build_override
file called 'rtc_build_with_chromium', to replace the existing
'build_with_chromium' variable; the latter is too generic a name
to tell what it does.

R=erikchen@chromium.org, brettw@chromium.org
BUG=webrtc:5453
==========

[Dirk Pranke, 2016-02-11 01:59:46]

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

[Dirk Pranke, 2016-02-11 02:01:39]

See the linked bug and https://codereview.webrtc.org/1644843003/ for context.

Please take a look? With this change, it should be fairly obvious how you would
change the WebRTC-side GN files to do the right thing, but let me know if you
need additional help.

Ideally I'd like to wait until brettw@ can review this before landing it, so
that he can okay the approach. That probably won't be until next week, though.
If you really need this to land in the meantime, let me know and we can land it
and then revert it (or fix it) later if need be.

[hjon_webrtc, 2016-02-11 17:24:46]

On 2016/02/11 02:01:39, Dirk Pranke wrote:
> See the linked bug and https://codereview.webrtc.org/1644843003/ for context.
> 
> Please take a look? With this change, it should be fairly obvious how you
would
> change the WebRTC-side GN files to do the right thing, but let me know if you
> need additional help.
> 
> Ideally I'd like to wait until brettw@ can review this before landing it, so
> that he can okay the approach. That probably won't be until next week, though.
> If you really need this to land in the meantime, let me know and we can land
it
> and then revert it (or fix it) later if need be.

If I've followed all of this correctly, it looks like for
https://codereview.webrtc.org/1644843003/ we'd set
rtc_require_mac_10_7_deployment to true in build_overrides/webrtc.gni, right? Or
is there a better way?

[Dirk Pranke, 2016-02-11 17:29:04]

On 2016/02/11 17:24:46, hjon_webrtc wrote:
> If I've followed all of this correctly, it looks like for
> https://codereview.webrtc.org/1644843003/ we'd set
> rtc_require_mac_10_7_deployment to true in build_overrides/webrtc.gni, right?
Or
> is there a better way?

Correct. The //build_overrides/webrtc.gni in the webrtc repo should have 
different values than the one in the chromium repo, and when you set
that to true, everything else hopefully Just Works.

[tkchin, 2016-02-11 22:03:23]

On 2016/02/11 17:29:04, Dirk Pranke wrote:
> On 2016/02/11 17:24:46, hjon_webrtc wrote:
> > If I've followed all of this correctly, it looks like for
> > https://codereview.webrtc.org/1644843003/ we'd set
> > rtc_require_mac_10_7_deployment to true in build_overrides/webrtc.gni,
right?
> Or
> > is there a better way?
> 
> Correct. The //build_overrides/webrtc.gni in the webrtc repo should have 
> different values than the one in the chromium repo, and when you set
> that to true, everything else hopefully Just Works.

lgtm thanks

[brettw, 2016-02-16 22:46:46]

https://codereview.chromium.org/1686373002/diff/1/build/config/mac/mac_sdk.gni
File build/config/mac/mac_sdk.gni (right):

https://codereview.chromium.org/1686373002/diff/1/build/config/mac/mac_sdk.gn...
build/config/mac/mac_sdk.gni:10: import("//build_overrides/webrtc.gni")
We're trying to make the build directory independent of any other directory.
You'll notice that the current //build directory has no references to
//build_overrides/*.

The //build directory is mirrored in
https://chromium.googlesource.com/chromium/src/build/
and can be DEPSed in by other projects (e.g. standalone V8, NaCl). However, the
//build_overrides directory is necessarily different for each such project. This
change will force each project to defined a file in their repository
"webrtc.gni" with this variable set, which I don't think we want.

This mac_deployment_target is already exposed as a build variable. Can you just
configure your bot to set it?

[Dirk Pranke, 2016-02-16 22:56:36]

On 2016/02/16 22:46:46, brettw wrote:
> https://codereview.chromium.org/1686373002/diff/1/build/config/mac/mac_sdk.gni
> File build/config/mac/mac_sdk.gni (right):
> 
>
https://codereview.chromium.org/1686373002/diff/1/build/config/mac/mac_sdk.gn...
> build/config/mac/mac_sdk.gni:10: import("//build_overrides/webrtc.gni")
> We're trying to make the build directory independent of any other directory.
> You'll notice that the current //build directory has no references to
> //build_overrides/*.
> 
> The //build directory is mirrored in
> https://chromium.googlesource.com/chromium/src/build/
> and can be DEPSed in by other projects (e.g. standalone V8, NaCl). However,
the
> //build_overrides directory is necessarily different for each such project.
This
> change will force each project to defined a file in their repository
> "webrtc.gni" with this variable set, which I don't think we want.
> 
> This mac_deployment_target is already exposed as a build variable. Can you
just
> configure your bot to set it?

I don't think that's a good solution (I considered it before ever making this
change).

I think it should be possible for webrtc to share //build but customize some
things,
and not have to force every project developer (or bot) to set gn args to do so .

I would be fine w/ renaming //build_overrides/webrtc.gni to something
not-webrtc-specific,
like //build_overrides/build_overrides.gni , but that still leaves //build
depending on
//build_overrides.

I'm open to other solutions, but I couldn't think of any that I liked that
didn't end up
kinda reinventing build_overrides and/or changing GN itself.

[brettw, 2016-02-17 19:14:25]

On 2016/02/16 22:56:36, Dirk Pranke wrote:
> On 2016/02/16 22:46:46, brettw wrote:
> >
https://codereview.chromium.org/1686373002/diff/1/build/config/mac/mac_sdk.gni
> > File build/config/mac/mac_sdk.gni (right):
> > 
> >
>
https://codereview.chromium.org/1686373002/diff/1/build/config/mac/mac_sdk.gn...
> > build/config/mac/mac_sdk.gni:10: import("//build_overrides/webrtc.gni")
> > We're trying to make the build directory independent of any other directory.
> > You'll notice that the current //build directory has no references to
> > //build_overrides/*.
> > 
> > The //build directory is mirrored in
> > https://chromium.googlesource.com/chromium/src/build/
> > and can be DEPSed in by other projects (e.g. standalone V8, NaCl). However,
> the
> > //build_overrides directory is necessarily different for each such project.
> This
> > change will force each project to defined a file in their repository
> > "webrtc.gni" with this variable set, which I don't think we want.
> > 
> > This mac_deployment_target is already exposed as a build variable. Can you
> just
> > configure your bot to set it?
> 
> I don't think that's a good solution (I considered it before ever making this
> change).
> 
> I think it should be possible for webrtc to share //build but customize some
> things,
> and not have to force every project developer (or bot) to set gn args to do so
.

I agree with this general goal. I think long-term we probably need a way to set
repo-specific build args, like maybe in the .gn file or something. This might
allow us to remove the build_overrides directory completely.

I would prefer to wait until there are stronger use cases for doing this since
it's kind of a bigger change and it seems like this is a temporary workaround.

I'm not that enthusiastic about making build depend on build_overrides. How bad
is the alternative of just having the rtc bots set this flag?

Brett

[Dirk Pranke, 2016-02-18 00:05:52]

On 2016/02/17 19:14:25, brettw wrote:
> On 2016/02/16 22:56:36, Dirk Pranke wrote:
> > On 2016/02/16 22:46:46, brettw wrote:
> > >
> https://codereview.chromium.org/1686373002/diff/1/build/config/mac/mac_sdk.gni
> > > File build/config/mac/mac_sdk.gni (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1686373002/diff/1/build/config/mac/mac_sdk.gn...
> > > build/config/mac/mac_sdk.gni:10: import("//build_overrides/webrtc.gni")
> > > We're trying to make the build directory independent of any other
directory.
> > > You'll notice that the current //build directory has no references to
> > > //build_overrides/*.
> > > 
> > > The //build directory is mirrored in
> > > https://chromium.googlesource.com/chromium/src/build/
> > > and can be DEPSed in by other projects (e.g. standalone V8, NaCl).
However,
> > the
> > > //build_overrides directory is necessarily different for each such
project.
> > This
> > > change will force each project to defined a file in their repository
> > > "webrtc.gni" with this variable set, which I don't think we want.
> > > 
> > > This mac_deployment_target is already exposed as a build variable. Can you
> > just
> > > configure your bot to set it?
> > 
> > I don't think that's a good solution (I considered it before ever making
this
> > change).
> > 
> > I think it should be possible for webrtc to share //build but customize some
> > things,
> > and not have to force every project developer (or bot) to set gn args to do
so
> .
> 
> I agree with this general goal. I think long-term we probably need a way to
set
> repo-specific build args, like maybe in the .gn file or something. This might
> allow us to remove the build_overrides directory completely.
> 
> I would prefer to wait until there are stronger use cases for doing this since
> it's kind of a bigger change and it seems like this is a temporary workaround.
> 
> I'm not that enthusiastic about making build depend on build_overrides. How
bad
> is the alternative of just having the rtc bots set this flag?

I really don't like that alternative, and I think this is an good enough use
case
to justify figuring out how we'd do this differently if we didn't want build to
depend
on build_overrides.

Feel free to argue further w/ me  interactively in person :).

[brettw, 2016-02-19 22:04:40]

https://codereview.chromium.org/1686373002/diff/1/build_overrides/webrtc.gni
File build_overrides/webrtc.gni (right):

https://codereview.chromium.org/1686373002/diff/1/build_overrides/webrtc.gni#...
build_overrides/webrtc.gni:5: #
TODO(https://bugs.chromium.org/p/webrtc/issues/detail?id=5453):
Per in-person discussion we were going to rename this file to something more
generic than "webrtc"

[Dirk Pranke, 2016-02-21 22:46:39]

Description was changed from

==========
Allow WebRTC to require Mac 10.7 deployments in GN builds.

The standalone (non-Chromium) WebRTC build has some targets
that on the Mac use Objective-C features that require the
targets to be deployed to 10.7 or newer hosts (i.e., they
won't run on 10.6).

Because we share GN //build configurations between the WebRTC
and Chromium project, we need to give WebRTC to toggle the
minimum configuration needed; we do this by adding another
variable to the //build_overrides/webrtc.gni file, and
make //build/config/mac/mac_sdk.gni import that. This introduces
a dependency from //build to //build_overrides, but I think that
should be fine.

This CL also adds a new variable to the WebRTC build_override
file called 'rtc_build_with_chromium', to replace the existing
'build_with_chromium' variable; the latter is too generic a name
to tell what it does.

R=erikchen@chromium.org, brettw@chromium.org
BUG=webrtc:5453
==========

to

==========
Allow WebRTC to require Mac 10.7 deployments in GN builds.

The standalone (non-Chromium) WebRTC build has some targets
that on the Mac use Objective-C features that require the
targets to be deployed to 10.7 or newer hosts (i.e., they
won't run on 10.6).

Because we share GN //build configurations between the WebRTC
and Chromium project, we need to give WebRTC to toggle the
minimum configuration needed; we do this by creating a new file called
build.gni in //build_overrides/build.gni file, and
making //build/config/mac/mac_sdk.gni import that. This introduces
a dependency from //build to //build_overrides, and means that other
projects that depend on //build will need to create a new file when
this change is rolled in..

R=brettw@chromium.org
BUG=webrtc:5453, 588513
==========

[Dirk Pranke, 2016-02-21 22:47:53]

dpranke@chromium.org changed reviewers:
- erikchen@chromium.org, hjon@webrtc.org

[Dirk Pranke, 2016-02-21 22:47:54]

Okay, I've significantly reworked this patch as per our in-person discussion.

Brett, please take a look and let me know what you think? Feel free to suggest
different names for the new file or the variables.

[kjellander_chromium, 2016-02-22 04:18:40]

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

[kjellander_chromium, 2016-02-22 04:18:40]

lgtm and thanks for putting in the time creating a nice long-term solution that
works for other projects than Chromium.

https://codereview.chromium.org/1686373002/diff/20001/build_overrides/README.md
File build_overrides/README.md (right):

https://codereview.chromium.org/1686373002/diff/20001/build_overrides/README....
build_overrides/README.md:27: //build_overrides.gni should go away completely in
favor of some mechanism
I guess you mean //build_overrides/build.gni here? Although having the file in
the root may make more sense, since now the idea is that the build_overrides
directory won't have other more files, right?

[Dirk Pranke, 2016-02-22 04:25:04]

https://codereview.chromium.org/1686373002/diff/20001/build_overrides/README.md
File build_overrides/README.md (right):

https://codereview.chromium.org/1686373002/diff/20001/build_overrides/README....
build_overrides/README.md:27: //build_overrides.gni should go away completely in
favor of some mechanism
On 2016/02/22 04:18:40, kjellander (chromium) wrote:
> I guess you mean //build_overrides/build.gni here? Although having the file in
> the root may make more sense, since now the idea is that the build_overrides
> directory won't have other more files, right?

Good catch, I meant //build_overrides/build.gni  on line 27.

The idea is that we stuff all of the build overrides into the already-existing
.gn file, and so we don't need a separate overrides directory (for either
//build overrrides or overrides to other projects like v8 or webrtc).

Of course, the design is still TBD.

[Dirk Pranke, 2016-02-22 04:39:19]

The CQ bit was checked by dpranke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-22 04:39:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1686373002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1686373002/40001

[commit-bot: I haz the power, 2016-02-22 05:27:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-22 05:27:19]

Dry run: This issue passed the CQ dry run.

[brettw, 2016-02-22 23:34:20]

lgtm

[Dirk Pranke, 2016-02-22 23:35:22]

The CQ bit was checked by dpranke@chromium.org

[Dirk Pranke, 2016-02-22 23:35:22]

The patchset sent to the CQ was uploaded after l-g-t-m from tkchin@chromium.org,
kjellander@chromium.org
Link to the patchset: https://codereview.chromium.org/1686373002/#ps40001
(title: "update README")

[commit-bot: I haz the power, 2016-02-22 23:37:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1686373002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1686373002/40001

[commit-bot: I haz the power, 2016-02-22 23:50:08]

Description was changed from

==========
Allow WebRTC to require Mac 10.7 deployments in GN builds.

The standalone (non-Chromium) WebRTC build has some targets
that on the Mac use Objective-C features that require the
targets to be deployed to 10.7 or newer hosts (i.e., they
won't run on 10.6).

Because we share GN //build configurations between the WebRTC
and Chromium project, we need to give WebRTC to toggle the
minimum configuration needed; we do this by creating a new file called
build.gni in //build_overrides/build.gni file, and
making //build/config/mac/mac_sdk.gni import that. This introduces
a dependency from //build to //build_overrides, and means that other
projects that depend on //build will need to create a new file when
this change is rolled in..

R=brettw@chromium.org
BUG=webrtc:5453, 588513
==========

to

==========
Allow WebRTC to require Mac 10.7 deployments in GN builds.

The standalone (non-Chromium) WebRTC build has some targets
that on the Mac use Objective-C features that require the
targets to be deployed to 10.7 or newer hosts (i.e., they
won't run on 10.6).

Because we share GN //build configurations between the WebRTC
and Chromium project, we need to give WebRTC to toggle the
minimum configuration needed; we do this by creating a new file called
build.gni in //build_overrides/build.gni file, and
making //build/config/mac/mac_sdk.gni import that. This introduces
a dependency from //build to //build_overrides, and means that other
projects that depend on //build will need to create a new file when
this change is rolled in..

R=brettw@chromium.org
BUG=webrtc:5453, 588513
==========

[commit-bot: I haz the power, 2016-02-22 23:50:09]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-02-22 23:51:23]

Description was changed from

==========
Allow WebRTC to require Mac 10.7 deployments in GN builds.

The standalone (non-Chromium) WebRTC build has some targets
that on the Mac use Objective-C features that require the
targets to be deployed to 10.7 or newer hosts (i.e., they
won't run on 10.6).

Because we share GN //build configurations between the WebRTC
and Chromium project, we need to give WebRTC to toggle the
minimum configuration needed; we do this by creating a new file called
build.gni in //build_overrides/build.gni file, and
making //build/config/mac/mac_sdk.gni import that. This introduces
a dependency from //build to //build_overrides, and means that other
projects that depend on //build will need to create a new file when
this change is rolled in..

R=brettw@chromium.org
BUG=webrtc:5453, 588513
==========

to

==========
Allow WebRTC to require Mac 10.7 deployments in GN builds.

The standalone (non-Chromium) WebRTC build has some targets
that on the Mac use Objective-C features that require the
targets to be deployed to 10.7 or newer hosts (i.e., they
won't run on 10.6).

Because we share GN //build configurations between the WebRTC
and Chromium project, we need to give WebRTC to toggle the
minimum configuration needed; we do this by creating a new file called
build.gni in //build_overrides/build.gni file, and
making //build/config/mac/mac_sdk.gni import that. This introduces
a dependency from //build to //build_overrides, and means that other
projects that depend on //build will need to create a new file when
this change is rolled in..

R=brettw@chromium.org
BUG=webrtc:5453, 588513

Committed: https://crrev.com/769de4d88df49a35f42932e71fbcb3ad1a26ec8d
Cr-Commit-Position: refs/heads/master@{#376856}
==========

[commit-bot: I haz the power, 2016-02-22 23:51:24]

Patchset 3 (id:??) landed as
https://crrev.com/769de4d88df49a35f42932e71fbcb3ad1a26ec8d
Cr-Commit-Position: refs/heads/master@{#376856}

[tkchin, 2016-02-23 18:43:18]

On 2016/02/22 23:51:24, commit-bot: I haz the power wrote:
> Patchset 3 (id:??) landed as
> https://crrev.com/769de4d88df49a35f42932e71fbcb3ad1a26ec8d
> Cr-Commit-Position: refs/heads/master@{#376856}

+1 lgtm and thank you for doing this for us.

[Nico, 2016-04-04 18:01:21]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2016-04-04 18:01:22]

https://codereview.chromium.org/1686373002/diff/40001/build/config/mac/mac_sd...
File build/config/mac/mac_sdk.gni (right):

https://codereview.chromium.org/1686373002/diff/40001/build/config/mac/mac_sd...
build/config/mac/mac_sdk.gni:8: # We can drop the
rtc_require_mac_10_7_deployment flag when Chromium
rtc_require_mac_10_7_deployment isn't referenced by anything but this comment.

we're about to bump the deployment target to 10.7 here
https://codereview.chromium.org/1847533004/ and will bump it to 10.9 soon. is
this build_overrides file still needed then? does webrtc keep targeting older os
x versions once chromium stops doing so?

[tkchin_webrtc, 2016-04-04 18:49:01]

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

[tkchin_webrtc, 2016-04-04 18:49:02]

https://codereview.chromium.org/1686373002/diff/40001/build/config/mac/mac_sd...
File build/config/mac/mac_sdk.gni (right):

https://codereview.chromium.org/1686373002/diff/40001/build/config/mac/mac_sd...
build/config/mac/mac_sdk.gni:8: # We can drop the
rtc_require_mac_10_7_deployment flag when Chromium
On 2016/04/04 18:01:22, Nico wrote:
> rtc_require_mac_10_7_deployment isn't referenced by anything but this comment.
> 
> we're about to bump the deployment target to 10.7 here
> https://codereview.chromium.org/1847533004/ and will bump it to 10.9 soon. is
> this build_overrides file still needed then? does webrtc keep targeting older
os
> x versions once chromium stops doing so?

We haven't done the work on WebRTC side to fix GN builds for iOS/Mac yet. But
this override is needed to do so (we're using the GYP equivalent).

Afaik there is no reason for us to continue to target old deployment targets if
Chromium is not doing so. I would like very much to be targeting 10.9.