More changes to support hermetic Xcode toolchain in GN.

More changes to support hermetic Xcode toolchain in GN.

BUG=651267
Committed: https://crrev.com/717ca1227414e7ea84b4676ec4b1572491654ae1
Cr-Commit-Position: refs/heads/master@{#424307}

[erikchen, 2016-10-07 20:35:49]

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

[commit-bot: I haz the power, 2016-10-07 20:36:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-07 22:27:48]

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

[commit-bot: I haz the power, 2016-10-07 22:27:49]

Dry run: This issue passed the CQ dry run.

[erikchen, 2016-10-08 01:14:17]

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

[erikchen, 2016-10-08 01:14:18]

thakis: Please review.

[Nico, 2016-10-08 19:03:38]

https://codereview.chromium.org/2403583002/diff/20001/build/secondary/third_p...
File build/secondary/third_party/crashpad/crashpad/util/BUILD.gn (right):

https://codereview.chromium.org/2403583002/diff/20001/build/secondary/third_p...
build/secondary/third_party/crashpad/crashpad/util/BUILD.gn:37: args += [
hermetic_xcode_path ]
doesn't this need an

 if (!use_system_xcode)

too? else i'd expect this'd pass an empty string as additional argument, and
then the mig.py script thinks that it gets 3 args

https://codereview.chromium.org/2403583002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/build/scripts/gperf.py (right):

https://codereview.chromium.org/2403583002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/gperf.py:17: if sys.argv[1] ==
"--developer_dir":
i'd use argparse. it's maybe two lines more code and much nicer.

https://codereview.chromium.org/2403583002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/BUILD.gn (right):

https://codereview.chromium.org/2403583002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/BUILD.gn:751: if (!use_system_xcode) {
do you need to check of is_mac (or (is_mac || is_ios)) too? This runs on all
platforms, right?

[erikchen, 2016-10-10 19:20:57]

thakis: PTAL

https://codereview.chromium.org/2403583002/diff/20001/build/secondary/third_p...
File build/secondary/third_party/crashpad/crashpad/util/BUILD.gn (right):

https://codereview.chromium.org/2403583002/diff/20001/build/secondary/third_p...
build/secondary/third_party/crashpad/crashpad/util/BUILD.gn:37: args += [
hermetic_xcode_path ]
On 2016/10/08 19:03:37, Nico wrote:
> doesn't this need an
> 
>  if (!use_system_xcode)
> 
> too? else i'd expect this'd pass an empty string as additional argument, and
> then the mig.py script thinks that it gets 3 args

Done.

https://codereview.chromium.org/2403583002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/build/scripts/gperf.py (right):

https://codereview.chromium.org/2403583002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/gperf.py:17: if sys.argv[1] ==
"--developer_dir":
On 2016/10/08 19:03:37, Nico wrote:
> i'd use argparse. it's maybe two lines more code and much nicer.

Done.

https://codereview.chromium.org/2403583002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/BUILD.gn (right):

https://codereview.chromium.org/2403583002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/BUILD.gn:751: if (!use_system_xcode) {
On 2016/10/08 19:03:37, Nico wrote:
> do you need to check of is_mac (or (is_mac || is_ios)) too? This runs on all
> platforms, right?

It's not strictly necessary, but I think it makes more sense to include the
conditional for is_mac.

[Nico, 2016-10-10 19:35:59]

lgtm, but you might be missing one is_mac

https://codereview.chromium.org/2403583002/diff/40001/build/secondary/third_p...
File build/secondary/third_party/crashpad/crashpad/util/BUILD.gn (right):

https://codereview.chromium.org/2403583002/diff/40001/build/secondary/third_p...
build/secondary/third_party/crashpad/crashpad/util/BUILD.gn:5:
import("//build/toolchain/toolchain.gni")
Do you need to upstream changes in this dir after landing? I forget. I think
their README explains how this works.

https://codereview.chromium.org/2403583002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/build/scripts/gperf.py (right):

https://codereview.chromium.org/2403583002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/gperf.py:6: # Usage: gperf.py
[--developer_dir PATH_TO_XCODE] gperf ...
(fwiw you could use add_subparsers() to make sure that --developer-dir always
prependds gperf, but as-is is fine too

https://codereview.chromium.org/2403583002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/build/scripts/scripts.gni (right):

https://codereview.chromium.org/2403583002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/scripts.gni:113: if (!use_system_xcode)
{
is_mac check?

[erikchen, 2016-10-10 22:54:58]

https://codereview.chromium.org/2403583002/diff/40001/build/secondary/third_p...
File build/secondary/third_party/crashpad/crashpad/util/BUILD.gn (right):

https://codereview.chromium.org/2403583002/diff/40001/build/secondary/third_p...
build/secondary/third_party/crashpad/crashpad/util/BUILD.gn:5:
import("//build/toolchain/toolchain.gni")
On 2016/10/10 19:35:59, Nico wrote:
> Do you need to upstream changes in this dir after landing? I forget. I think
> their README explains how this works.

There are no readmes in any of the parent directories. My understanding is that
the crashpad team is responsible for upstreaming any relevant changes.

https://codereview.chromium.org/2403583002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/build/scripts/gperf.py (right):

https://codereview.chromium.org/2403583002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/gperf.py:6: # Usage: gperf.py
[--developer_dir PATH_TO_XCODE] gperf ...
On 2016/10/10 19:35:59, Nico wrote:
> (fwiw you could use add_subparsers() to make sure that --developer-dir always
> prependds gperf, but as-is is fine too

noted.

https://codereview.chromium.org/2403583002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/build/scripts/scripts.gni (right):

https://codereview.chromium.org/2403583002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/scripts.gni:113: if (!use_system_xcode)
{
On 2016/10/10 19:35:59, Nico wrote:
> is_mac check?

Done.

[erikchen, 2016-10-10 22:55:03]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2016-10-10 22:55:04]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/2403583002/#ps60001
(title: "Comments from thakis.")

[commit-bot: I haz the power, 2016-10-10 22:55:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-11 00:39:25]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-11 00:43:19]

Description was changed from

==========
More changes to support hermetic Xcode toolchain in GN.

BUG=651267
==========

to

==========
More changes to support hermetic Xcode toolchain in GN.

BUG=651267

Committed: https://crrev.com/717ca1227414e7ea84b4676ec4b1572491654ae1
Cr-Commit-Position: refs/heads/master@{#424307}
==========

[commit-bot: I haz the power, 2016-10-11 00:43:20]

Patchset 4 (id:??) landed as
https://crrev.com/717ca1227414e7ea84b4676ec4b1572491654ae1
Cr-Commit-Position: refs/heads/master@{#424307}

[Nico, 2016-10-11 00:46:51]

https://codereview.chromium.org/2403583002/diff/40001/build/secondary/third_p...
File build/secondary/third_party/crashpad/crashpad/util/BUILD.gn (right):

https://codereview.chromium.org/2403583002/diff/40001/build/secondary/third_p...
build/secondary/third_party/crashpad/crashpad/util/BUILD.gn:5:
import("//build/toolchain/toolchain.gni")
On 2016/10/10 22:54:57, erikchen wrote:
> On 2016/10/10 19:35:59, Nico wrote:
> > Do you need to upstream changes in this dir after landing? I forget. I think
> > their README explains how this works.
> 
> There are no readmes in any of the parent directories. My understanding is
that
> the crashpad team is responsible for upstreaming any relevant changes.

i have one at src/third_party/crashpad/README.chromium

[erikchen, 2016-10-11 00:49:55]

On 2016/10/11 00:46:51, Nico wrote:
>
https://codereview.chromium.org/2403583002/diff/40001/build/secondary/third_p...
> File build/secondary/third_party/crashpad/crashpad/util/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2403583002/diff/40001/build/secondary/third_p...
> build/secondary/third_party/crashpad/crashpad/util/BUILD.gn:5:
> import("//build/toolchain/toolchain.gni")
> On 2016/10/10 22:54:57, erikchen wrote:
> > On 2016/10/10 19:35:59, Nico wrote:
> > > Do you need to upstream changes in this dir after landing? I forget. I
think
> > > their README explains how this works.
> > 
> > There are no readmes in any of the parent directories. My understanding is
> that
> > the crashpad team is responsible for upstreaming any relevant changes.
> 
> i have one at src/third_party/crashpad/README.chromium

isn't that for changes to src/third_party/crashpad? crashpad/util/BUILD.gn is
not present in the crashpad repo.

[Nico, 2016-10-11 01:22:37]

On 2016/10/11 00:49:55, erikchen wrote:
> On 2016/10/11 00:46:51, Nico wrote:
> >
>
https://codereview.chromium.org/2403583002/diff/40001/build/secondary/third_p...
> > File build/secondary/third_party/crashpad/crashpad/util/BUILD.gn (right):
> > 
> >
>
https://codereview.chromium.org/2403583002/diff/40001/build/secondary/third_p...
> > build/secondary/third_party/crashpad/crashpad/util/BUILD.gn:5:
> > import("//build/toolchain/toolchain.gni")
> > On 2016/10/10 22:54:57, erikchen wrote:
> > > On 2016/10/10 19:35:59, Nico wrote:
> > > > Do you need to upstream changes in this dir after landing? I forget. I
> think
> > > > their README explains how this works.
> > > 
> > > There are no readmes in any of the parent directories. My understanding is
> > that
> > > the crashpad team is responsible for upstreaming any relevant changes.
> > 
> > i have one at src/third_party/crashpad/README.chromium
> 
> isn't that for changes to src/third_party/crashpad? crashpad/util/BUILD.gn is
> not present in the crashpad repo.

As that readme says, crashpad is checked in to src/ and then manually mirrored.
I don't know if that BUILD.gn should've been mirrored up or if crashpad doesn't
use gn yet or what. But that file describes the mirroring process at the bottom.