patch from chinmaygarde@ to make progress on mac, ios.

patch from chinmaygarde@ to make progress on mac, ios.

I've taken https://github.com/chinmaygarde/sky_engine/commit/ad591c629a295936206dcfde37f63c8c82bc2838
and merged it onto Chromium ToT, with a few lint and other cleanups,
and one fix to make sure that Mac still compiled (which may have broken
iOS, haven't tested yet).

R=brettw@chromium.org, sdfresne@chromium.org
BUG=459705
CQ_EXTRA_TRYBOTS=tryserver.chromium.mac:mac_chromium_gn_rel,mac_chromium_gn_dbg

Committed: https://crrev.com/2a29462a04dabad3fc20dc922227c46b36459912
Cr-Commit-Position: refs/heads/master@{#342297}

[Dirk Pranke, 2015-07-22 17:09:38]

dpranke@chromium.org changed reviewers:
+ sdefresne@chromium.org

[Dirk Pranke, 2015-07-22 17:10:03]

fyi, this needs to be synced up with your changes

[eseidel, 2015-07-23 23:15:00]

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

[eseidel, 2015-07-23 23:15:01]

https://codereview.chromium.org/1250913002/diff/1/build/config/mac/BUILD.gn
File build/config/mac/BUILD.gn (right):

https://codereview.chromium.org/1250913002/diff/1/build/config/mac/BUILD.gn#n...
build/config/mac/BUILD.gn:15: #"-stdlib=libc++",
This is likely needed to use c++11.

[Dirk Pranke, 2015-07-24 02:12:41]

Okay, with the latest patch I have I've got something that can build
base_unittests into something almost resembling the application bundle format.

The binary links fine, but I think the bundle is still missing a few files and
is probably being built wrong. If I copy the executable into a GYP build, it
runs most of the tests but crashes, but if I try to run the bundle using an
iossim from a GYP build, it refuses to run.

I'll investigate further tomorrow. 

This patch is not yet ready for review but is meant more for curiosity. Most of
the changes to most of the build files could, however, be split off into their
own patches for landing.

[Dirk Pranke, 2015-07-24 02:13:17]

On 2015/07/24 02:12:41, Dirk Pranke wrote:
> This patch is not yet ready for review but is meant more for curiosity. Most
of
> the changes to most of the build files could, however, be split off into their
> own patches for landing.

meaning the files in the patch outside of //build and //testing, that is.

[sdefresne, 2015-07-24 08:47:32]

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/BUILD.gn
File build/config/ios/BUILD.gn (right):

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/BUILD....
build/config/ios/BUILD.gn:13: # TODO(dpranke): Figure out why chinmay had this
set. Needed on ios?
Yes, iOS builds requires libc++ and c++11 and uses "-std=c++11 -stdlib=libc++"
when building with gyp.

[sdefresne, 2015-07-25 19:15:31]

https://codereview.chromium.org/1250913002/diff/40001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/1250913002/diff/40001/base/BUILD.gn#newcode1467
base/BUILD.gn:1467: "strings/sys_string_conversions_mac_unittest.mm",
It looks like this file is not build on iOS when using gyp. I guess we should
also add in base/base.gypi then.

https://codereview.chromium.org/1250913002/diff/40001/base/test/BUILD.gn
File base/test/BUILD.gn (right):

https://codereview.chromium.org/1250913002/diff/40001/base/test/BUILD.gn#newc...
base/test/BUILD.gn:26: static_library("test_support") {
Why the change from "source_set" to "static_library"? I though that "source_set"
where recommended everywhere with gn. Is this required here?

https://codereview.chromium.org/1250913002/diff/40001/build/config/compiler/B...
File build/config/compiler/BUILD.gn (right):

https://codereview.chromium.org/1250913002/diff/40001/build/config/compiler/B...
build/config/compiler/BUILD.gn:269: } else if (current_cpu == "arm") {
I think we should add support for "armv7" and "arm64" cpu here too. Being able
to build for armv7 is useful when we have to debug issues that only happens on
non-64-bit devices (i.e. anything older than an iPhone 5s).

I would even go one step further and recommend removing support of "arm" as a
cpu as this is too ambiguous. There are three different ARM architecture that
are supported on iOS "armv7", "armv7s" and "arm64". Since we make the
distinction between "x86" and "x64" on Intel architecture even though "x64" cpu
can run "x86" code (the same as "arm64" do support "armv7" code), I think we
should do the same distinction for ARM architecture.

For webrtc, we can probably change their script to set target_cpu="arm64"
instead of target_cpu="arm".

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/find_s...
File build/config/ios/find_signing_identity.py (right):

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/find_s...
build/config/ios/find_signing_identity.py:24: exp = re.compile('.*\)
([A-F|0-9]*)(.*)')
nit: I would make the regexp more strict, like '[0-9]+\) ([A-F0-9]+) "([^"]*)"'

BTW, all special characters lose their special meaning (and some other character
do gain a special meaning) in group, so the "|" should be removed from the
[A-F0-9] group (as otherwise it would also match a stray "|" in the middle of
the identifier, which may lead to meta-character passed to the shell later on).

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/ios_ap...
File build/config/ios/ios_app.py (right):

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/ios_ap...
build/config/ios/ios_app.py:6: import os
nit: sort

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/ios_ap...
build/config/ios/ios_app.py:11: PLUTIL = [
nit: why use a global for a constant that is only used once?

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/ios_ap...
build/config/ios/ios_app.py:57: application_path = os.path.join( args.dir,
args.name + ".app" )
style: no space after "(" nor before ")"

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/ios_ap...
build/config/ios/ios_app.py:61: def Main():
nit: even though Python style guide recommends capitalizing methods, "main" is
more frequent than "Main" ...

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/ios_ap...
build/config/ios/ios_app.py:104: 
nit: remove newline

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/ios_sd...
File build/config/ios/ios_sdk.gni (right):

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/ios_sd...
build/config/ios/ios_sdk.gni:14: use_ios_simulator = true
nit: since gyp always generate the four possible combination of {device,
simulator} x {release, debug}, I think the comment is misleading. IMO, I think
we should have "use_ios_simulator = (target_cpu == "x86" || target_cpu ==
"x64")" since only the simulator supports Intel opcode and only devices supports
ARM opcode.

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/ios_sd...
build/config/ios/ios_sdk.gni:17: ios_deployment_target = "6.0"
nit: gyp uses "7.0" as the default ios_deployment_target (see build/common.gypi)
so I think we should update this one too.

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/ios_sd...
build/config/ios/ios_sdk.gni:21: }
We need to provide a way to build for "armv7"/"arm64" (i.e. for devices) without
signing for two uses case, 1. for upstream bots, as they do not have a valid key
to sign, and 2. for downstream release build as the signature is done in a
second step.

I would recommend doing:

# Control whether code signing is enabled. Only used when building for real
devices
# in order to allow signing in a later stage.
ios_enable_code_signing = true

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/ios_sd...
build/config/ios/ios_sdk.gni:35: if (use_ios_simulator) {
Change to

if (use_ios_simulator || !ios_enabled_code_signin) {

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/ios_si...
File build/config/ios/ios_sim.py (right):

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/ios_si...
build/config/ios/ios_sim.py:7: import os
nit: sort

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/ios_si...
build/config/ios/ios_sim.py:27: identifier = subprocess.check_output(
PLIST_BUDDY_PATH + [
nit: no space after "(" nor before ")"

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/rules.gni
File build/config/ios/rules.gni (right):

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/rules....
build/config/ios/rules.gni:44: template("resource_copy_ios") {
I think this should be part of ios_app, otherwise there is no way to specify a
list of resources to ship with an app. This is also probably why base_unittests
is not working, it is missing its resources (at list icu dat file, but possibly
the resources used by the unit tests).

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/rules....
build/config/ios/rules.gni:74: "The entitlements path must be specified for
$target_name")
This comment is incorrect, this is checking "code_signing_identity" not
"entitlements_path". Should probably be something along "The code signing
identity must be defined for $target_name").

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/rules....
build/config/ios/rules.gni:88: 
nit: remove blank line?

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/rules....
build/config/ios/rules.gni:157: #copy_gen_target_name = target_name + "_copy"
This looks like this can be removed as the other three targets generate
everything in the correct location.

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/rules....
build/config/ios/rules.gni:197: # builds
nit: full stop?

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/rules....
build/config/ios/rules.gni:199: if (invoker.code_signing_identity == "") {
question: why not flatten the hierarchy, i.e. do the following:

deps = [
  ":$struct_gen_target_name",
  ":$plist_gen_target_name",
  ":$bin_gen_target_name",
]
if (invoker.code_signing_identity != "") {
  deps += [ ":$code_sign_gen_target_name" ]
}

Since "$code_sign_gen_target_name" also depends on the three other targets, the
generated output should be the same, shouldn't it? I think the version I
proposed is simpler to understand, since we do not have to check that
"$code_sign_gen_target_name" do depends on those targets. But maybe there is a
reason why this is done as it is now, and if so, can you share it and enlighten
me?

https://codereview.chromium.org/1250913002/diff/40001/build/config/mac/BUILD.gn
File build/config/mac/BUILD.gn (right):

https://codereview.chromium.org/1250913002/diff/40001/build/config/mac/BUILD....
build/config/mac/BUILD.gn:15: #"-stdlib=libc++",
This is probably not needed on Mac (but is on iOS).

https://codereview.chromium.org/1250913002/diff/40001/build/config/mac/mac_ap...
File build/config/mac/mac_app.py (right):

https://codereview.chromium.org/1250913002/diff/40001/build/config/mac/mac_ap...
build/config/mac/mac_app.py:6: import os
nit: sort

https://codereview.chromium.org/1250913002/diff/40001/build/config/mac/mac_ap...
build/config/mac/mac_app.py:38: return subprocess.check_call( PLUTIL + [
style: no space after "(" or before ")"

https://codereview.chromium.org/1250913002/diff/40001/build/config/mac/rules.gni
File build/config/mac/rules.gni (right):

https://codereview.chromium.org/1250913002/diff/40001/build/config/mac/rules....
build/config/mac/rules.gni:102: #        "The entitlements path must be
specified for $target_name")
nit: this comment is incorrect, but since the code is commented out, maybe it
should just be deleted?

https://codereview.chromium.org/1250913002/diff/40001/build/toolchain/mac/BUI...
File build/toolchain/mac/BUILD.gn (right):

https://codereview.chromium.org/1250913002/diff/40001/build/toolchain/mac/BUI...
build/toolchain/mac/BUILD.gn:226: mac_toolchain("ios_clang_arm") {
do we need "ios_clang_armv7" and "ios_clang_arm64"?

https://codereview.chromium.org/1250913002/diff/40001/build/toolchain/mac/BUI...
build/toolchain/mac/BUILD.gn:229: prefix =
rebase_path("//third_party/llvm-build/Release+Asserts/bin",
Question: with gyp, at multiple occasion, on iOS we had to use the version of
clang shipped with Xcode (due to features added by Apple to their version of
clang, and only landing a few weeks or months later in the open-source version
of clang).

Can we add the support for this with gn too?

https://codereview.chromium.org/1250913002/diff/40001/testing/test.gni
File testing/test.gni (right):

https://codereview.chromium.org/1250913002/diff/40001/testing/test.gni#newcod...
testing/test.gni:191: info_plist = "//testing/gtest_ios/unittest-Info.plist"
Some unit tests on iOS do specify different Info.plist, so can we make this
configurable?

https://codereview.chromium.org/1250913002/diff/40001/testing/test.gni#newcod...
testing/test.gni:236: "//build/win:default_exe_manifest",
Can we omit this since it is a noop?

[sdefresne, 2015-07-25 19:27:33]

BTW, you can use BUG=459705

[Dirk Pranke, 2015-07-27 16:09:40]

Thanks for all the feedback!

The patch wasn't really meant for review yet, but the feedback will definitely
be helpful.

I'm not sure that something like this will land as-is, as we're exploring how
much of the ios-
and mac-specific logic for things like bundles we should push directly into GN.

However, I'll be starting to split out parts of this patch today to land
individually where it makes 
sense to do so and will clean up as many things as we can; the closer we can
get, the better.

[Dirk Pranke, 2015-07-31 21:27:41]

https://codereview.chromium.org/1250913002/diff/40001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/1250913002/diff/40001/base/BUILD.gn#newcode1467
base/BUILD.gn:1467: "strings/sys_string_conversions_mac_unittest.mm",
On 2015/07/25 19:15:29, sdefresne wrote:
> It looks like this file is not build on iOS when using gyp. I guess we should
> also add in base/base.gypi then.

this looks like it is pulled in to me:

https://code.google.com/p/chromium/codesearch?q=base.gypi#chromium/src/base/b...

https://codereview.chromium.org/1250913002/diff/40001/base/test/BUILD.gn
File base/test/BUILD.gn (right):

https://codereview.chromium.org/1250913002/diff/40001/base/test/BUILD.gn#newc...
base/test/BUILD.gn:26: static_library("test_support") {
On 2015/07/25 19:15:29, sdefresne wrote:
> Why the change from "source_set" to "static_library"? I though that
"source_set"
> where recommended everywhere with gn. Is this required here?

source_set is recommended, you're right.

I had initially changed this because the test_support library was pulling in
some files that we didn't need, and changing to a static lib works around that.

I'll go back and separate the files explicitly.

https://codereview.chromium.org/1250913002/diff/40001/build/config/compiler/B...
File build/config/compiler/BUILD.gn (right):

https://codereview.chromium.org/1250913002/diff/40001/build/config/compiler/B...
build/config/compiler/BUILD.gn:269: } else if (current_cpu == "arm") {
On 2015/07/25 19:15:29, sdefresne wrote:
> I think we should add support for "armv7" and "arm64" cpu here too. Being able
> to build for armv7 is useful when we have to debug issues that only happens on
> non-64-bit devices (i.e. anything older than an iPhone 5s).
> 
> I would even go one step further and recommend removing support of "arm" as a
> cpu as this is too ambiguous. There are three different ARM architecture that
> are supported on iOS "armv7", "armv7s" and "arm64". Since we make the
> distinction between "x86" and "x64" on Intel architecture even though "x64"
cpu
> can run "x86" code (the same as "arm64" do support "armv7" code), I think we
> should do the same distinction for ARM architecture.
> 
> For webrtc, we can probably change their script to set target_cpu="arm64"
> instead of target_cpu="arm".

This seems reasonable but well out of the scope of what I want to do in this
patch. I'll add a TODO.

I would like to try and find names that work for both ios and android.

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/BUILD.gn
File build/config/ios/BUILD.gn (right):

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/BUILD....
build/config/ios/BUILD.gn:13: # TODO(dpranke): Figure out why chinmay had this
set. Needed on ios?
On 2015/07/24 08:47:31, sdefresne wrote:
> Yes, iOS builds requires libc++ and c++11 and uses "-std=c++11 -stdlib=libc++"
> when building with gyp.

Hm. Seems to work now. Will fix.

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/find_s...
File build/config/ios/find_signing_identity.py (right):

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/find_s...
build/config/ios/find_signing_identity.py:24: exp = re.compile('.*\)
([A-F|0-9]*)(.*)')
On 2015/07/25 19:15:29, sdefresne wrote:
> nit: I would make the regexp more strict, like '[0-9]+\) ([A-F0-9]+)
"([^"]*)"'
> 
> BTW, all special characters lose their special meaning (and some other
character
> do gain a special meaning) in group, so the "|" should be removed from the
> [A-F0-9] group (as otherwise it would also match a stray "|" in the middle of
> the identifier, which may lead to meta-character passed to the shell later
on).

Done.

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/ios_ap...
File build/config/ios/ios_app.py (right):

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/ios_ap...
build/config/ios/ios_app.py:6: import os
On 2015/07/25 19:15:30, sdefresne wrote:
> nit: sort

Done.

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/ios_ap...
build/config/ios/ios_app.py:11: PLUTIL = [
On 2015/07/25 19:15:30, sdefresne wrote:
> nit: why use a global for a constant that is only used once?

Good catch. Fixed.

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/ios_ap...
build/config/ios/ios_app.py:57: application_path = os.path.join( args.dir,
args.name + ".app" )
On 2015/07/25 19:15:30, sdefresne wrote:
> style: no space after "(" nor before ")"

Done.

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/ios_ap...
build/config/ios/ios_app.py:61: def Main():
On 2015/07/25 19:15:30, sdefresne wrote:
> nit: even though Python style guide recommends capitalizing methods, "main" is
> more frequent than "Main" ...

Done.

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/ios_ap...
build/config/ios/ios_app.py:104: 
On 2015/07/25 19:15:30, sdefresne wrote:
> nit: remove newline

Done (removed a few others as well).

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/ios_sd...
File build/config/ios/ios_sdk.gni (right):

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/ios_sd...
build/config/ios/ios_sdk.gni:14: use_ios_simulator = true
On 2015/07/25 19:15:30, sdefresne wrote:
> nit: since gyp always generate the four possible combination of {device,
> simulator} x {release, debug}, I think the comment is misleading. IMO, I think
> we should have "use_ios_simulator = (target_cpu == "x86" || target_cpu ==
> "x64")" since only the simulator supports Intel opcode and only devices
supports
> ARM opcode.

Agreed. Good suggestion.

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/ios_sd...
build/config/ios/ios_sdk.gni:17: ios_deployment_target = "6.0"
On 2015/07/25 19:15:30, sdefresne wrote:
> nit: gyp uses "7.0" as the default ios_deployment_target (see
build/common.gypi)
> so I think we should update this one too.

Done.

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/ios_sd...
build/config/ios/ios_sdk.gni:21: }
On 2015/07/25 19:15:30, sdefresne wrote:
> We need to provide a way to build for "armv7"/"arm64" (i.e. for devices)
without
> signing for two uses case, 1. for upstream bots, as they do not have a valid
key
> to sign, and 2. for downstream release build as the signature is done in a
> second step.
> 
> I would recommend doing:
> 
> # Control whether code signing is enabled. Only used when building for real
> devices
> # in order to allow signing in a later stage.
> ios_enable_code_signing = true

Will rework.

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/ios_sd...
build/config/ios/ios_sdk.gni:35: if (use_ios_simulator) {
On 2015/07/25 19:15:30, sdefresne wrote:
> Change to
> 
> if (use_ios_simulator || !ios_enabled_code_signin) {

Will rework.

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/ios_si...
File build/config/ios/ios_sim.py (right):

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/ios_si...
build/config/ios/ios_sim.py:7: import os
On 2015/07/25 19:15:30, sdefresne wrote:
> nit: sort

Done.

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/ios_si...
build/config/ios/ios_sim.py:27: identifier = subprocess.check_output(
PLIST_BUDDY_PATH + [
On 2015/07/25 19:15:30, sdefresne wrote:
> nit: no space after "(" nor before ")"

Done.

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/rules.gni
File build/config/ios/rules.gni (right):

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/rules....
build/config/ios/rules.gni:44: template("resource_copy_ios") {
On 2015/07/25 19:15:30, sdefresne wrote:
> I think this should be part of ios_app, otherwise there is no way to specify a
> list of resources to ship with an app. This is also probably why
base_unittests
> is not working, it is missing its resources (at list icu dat file, but
possibly
> the resources used by the unit tests).

Will investigate. marking as a TODO for now.

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/rules....
build/config/ios/rules.gni:74: "The entitlements path must be specified for
$target_name")
On 2015/07/25 19:15:30, sdefresne wrote:
> This comment is incorrect, this is checking "code_signing_identity" not
> "entitlements_path". Should probably be something along "The code signing
> identity must be defined for $target_name").

Done.

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/rules....
build/config/ios/rules.gni:88: 
On 2015/07/25 19:15:30, sdefresne wrote:
> nit: remove blank line?

Done.

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/rules....
build/config/ios/rules.gni:157: #copy_gen_target_name = target_name + "_copy"
On 2015/07/25 19:15:30, sdefresne wrote:
> This looks like this can be removed as the other three targets generate
> everything in the correct location.

Right.

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/rules....
build/config/ios/rules.gni:197: # builds
On 2015/07/25 19:15:30, sdefresne wrote:
> nit: full stop?

Done.

https://codereview.chromium.org/1250913002/diff/40001/build/config/ios/rules....
build/config/ios/rules.gni:199: if (invoker.code_signing_identity == "") {
On 2015/07/25 19:15:30, sdefresne wrote:
> question: why not flatten the hierarchy, i.e. do the following:
> 
> deps = [
>   ":$struct_gen_target_name",
>   ":$plist_gen_target_name",
>   ":$bin_gen_target_name",
> ]
> if (invoker.code_signing_identity != "") {
>   deps += [ ":$code_sign_gen_target_name" ]
> }
> 
> Since "$code_sign_gen_target_name" also depends on the three other targets,
the
> generated output should be the same, shouldn't it? I think the version I
> proposed is simpler to understand, since we do not have to check that
> "$code_sign_gen_target_name" do depends on those targets. But maybe there is a
> reason why this is done as it is now, and if so, can you share it and
enlighten
> me?

I just hadn't gotten to cleaning this up yet. Will do.

https://codereview.chromium.org/1250913002/diff/40001/build/config/mac/mac_ap...
File build/config/mac/mac_app.py (right):

https://codereview.chromium.org/1250913002/diff/40001/build/config/mac/mac_ap...
build/config/mac/mac_app.py:6: import os
On 2015/07/25 19:15:30, sdefresne wrote:
> nit: sort

Done.

https://codereview.chromium.org/1250913002/diff/40001/build/config/mac/mac_ap...
build/config/mac/mac_app.py:38: return subprocess.check_call( PLUTIL + [
On 2015/07/25 19:15:30, sdefresne wrote:
> style: no space after "(" or before ")"

Done.

https://codereview.chromium.org/1250913002/diff/40001/build/config/mac/rules.gni
File build/config/mac/rules.gni (right):

https://codereview.chromium.org/1250913002/diff/40001/build/config/mac/rules....
build/config/mac/rules.gni:102: #        "The entitlements path must be
specified for $target_name")
On 2015/07/25 19:15:30, sdefresne wrote:
> nit: this comment is incorrect, but since the code is commented out, maybe it
> should just be deleted?

Done.

https://codereview.chromium.org/1250913002/diff/40001/build/toolchain/mac/BUI...
File build/toolchain/mac/BUILD.gn (right):

https://codereview.chromium.org/1250913002/diff/40001/build/toolchain/mac/BUI...
build/toolchain/mac/BUILD.gn:229: prefix =
rebase_path("//third_party/llvm-build/Release+Asserts/bin",
On 2015/07/25 19:15:30, sdefresne wrote:
> Question: with gyp, at multiple occasion, on iOS we had to use the version of
> clang shipped with Xcode (due to features added by Apple to their version of
> clang, and only landing a few weeks or months later in the open-source version
> of clang).
> 
> Can we add the support for this with gn too?

We can definitely add it. I'll add a TODO here.

https://codereview.chromium.org/1250913002/diff/40001/testing/test.gni
File testing/test.gni (right):

https://codereview.chromium.org/1250913002/diff/40001/testing/test.gni#newcod...
testing/test.gni:191: info_plist = "//testing/gtest_ios/unittest-Info.plist"
On 2015/07/25 19:15:30, sdefresne wrote:
> Some unit tests on iOS do specify different Info.plist, so can we make this
> configurable?

Added a TODO.

https://codereview.chromium.org/1250913002/diff/40001/testing/test.gni#newcod...
testing/test.gni:236: "//build/win:default_exe_manifest",
On 2015/07/25 19:15:30, sdefresne wrote:
> Can we omit this since it is a noop?

Done.

[Dirk Pranke, 2015-07-31 21:41:01]

dpranke@chromium.org changed reviewers:
+ brettw@chromium.org
- eseidel@chromium.org

[Dirk Pranke, 2015-07-31 21:41:08]

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

[Dirk Pranke, 2015-07-31 21:41:25]

Okay, I think this is ready to land and iterate.

Please take another look?

[commit-bot: I haz the power, 2015-07-31 21:42:08]

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

[commit-bot: I haz the power, 2015-07-31 22:06:31]

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

[commit-bot: I haz the power, 2015-07-31 22:06:32]

Dry run: This issue passed the CQ dry run.

[sdefresne, 2015-08-04 07:42:02]

lgtm with comments and nits

https://codereview.chromium.org/1250913002/diff/40001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/1250913002/diff/40001/base/BUILD.gn#newcode1467
base/BUILD.gn:1467: "strings/sys_string_conversions_mac_unittest.mm",
On 2015/07/31 at 21:27:40, Dirk Pranke wrote:
> On 2015/07/25 19:15:29, sdefresne wrote:
> > It looks like this file is not build on iOS when using gyp. I guess we
should
> > also add in base/base.gypi then.
> 
> this looks like it is pulled in to me:
> 
>
https://code.google.com/p/chromium/codesearch?q=base.gypi#chromium/src/base/b...

It isn't, because the regexp does not match the file path:

$ find base -name sys_string_conversions_mac_unittest.mm
base/strings/sys_string_conversions_mac_unittest.mm

The regexp should be "^strings/sys_string_conversions_mac_unittest\\.mm$". Maybe
we need to change the gyp file too then.

https://codereview.chromium.org/1250913002/diff/100001/build/config/ios/ios_a...
File build/config/ios/ios_app.py (right):

https://codereview.chromium.org/1250913002/diff/100001/build/config/ios/ios_a...
build/config/ios/ios_app.py:49: 
nit: remove blank line

https://codereview.chromium.org/1250913002/diff/100001/build/config/ios/ios_s...
File build/config/ios/ios_sim.py (right):

https://codereview.chromium.org/1250913002/diff/100001/build/config/ios/ios_s...
build/config/ios/ios_sim.py:32: 
nit: remove blank line

https://codereview.chromium.org/1250913002/diff/100001/build/config/ios/rules...
File build/config/ios/rules.gni (right):

https://codereview.chromium.org/1250913002/diff/100001/build/config/ios/rules...
build/config/ios/rules.gni:89: action(struct_gen_target_name) {
nit: this targets only creates directories as output, however, gn gen already
creates directories for all files listed in outputs, so I'm wondering whether it
is needed at all.

https://codereview.chromium.org/1250913002/diff/100001/testing/test.gni
File testing/test.gni (right):

https://codereview.chromium.org/1250913002/diff/100001/testing/test.gni#newco...
testing/test.gni:156: if (is_ios) {
nit: redundant with the previous test

[brettw, 2015-08-04 18:07:29]

I'm super excited to make this progress!

https://codereview.chromium.org/1250913002/diff/100001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/1250913002/diff/100001/base/BUILD.gn#newcode820
base/BUILD.gn:820: sources += [ "files/file_path_watcher_mac.cc" ]
This also appears in https://codereview.chromium.org/1257303005/
FIGHT!

https://codereview.chromium.org/1250913002/diff/100001/build/config/features.gni
File build/config/features.gni (right):

https://codereview.chromium.org/1250913002/diff/100001/build/config/features....
build/config/features.gni:63: # TODO(GYP) = true
This comment won't make any sense to me later. "How about TODO(GYP) Make this
work on iOS and enable by default."

https://codereview.chromium.org/1250913002/diff/100001/build/config/ios/rules...
File build/config/ios/rules.gni (right):

https://codereview.chromium.org/1250913002/diff/100001/build/config/ios/rules...
build/config/ios/rules.gni:37: deps = invoker.deps
If you sync you can do:
  forward_variables_from(invoker, ["deps", "public_deps", "testonly",
"visibility"])
instead, which is a little simpler (also not public_deps addition which is good
practice).

https://codereview.chromium.org/1250913002/diff/100001/build/config/ios/rules...
build/config/ios/rules.gni:89: action(struct_gen_target_name) {
On 2015/08/04 07:42:01, sdefresne wrote:
> nit: this targets only creates directories as output, however, gn gen already
> creates directories for all files listed in outputs, so I'm wondering whether
it
> is needed at all.

No idea, maybe try with it removed? Whatever you think is best is fine.

https://codereview.chromium.org/1250913002/diff/100001/build/config/ios/rules...
build/config/ios/rules.gni:92: sources = []
You can just delete this.

https://codereview.chromium.org/1250913002/diff/100001/build/config/ios/rules...
build/config/ios/rules.gni:104: }
Can you set visibility on these intermediate targets so only the final group can
depend on them? You'll want to make a
  group_target_name = target_name
above since target_name will be changed inside these sub-targets.

https://codereview.chromium.org/1250913002/diff/100001/build/config/ios/rules...
build/config/ios/rules.gni:165: if (defined(invoker.testonly)) {
forward_variables_from(invoker, ["testonly"])

https://codereview.chromium.org/1250913002/diff/100001/build/config/ios/rules...
build/config/ios/rules.gni:172: deps = [
I'd do public_deps instead since you may want to take the outputs from here as
inputs to other stuff.

https://codereview.chromium.org/1250913002/diff/100001/build/config/mac/rules...
File build/config/mac/rules.gni (right):

https://codereview.chromium.org/1250913002/diff/100001/build/config/mac/rules...
build/config/mac/rules.gni:37: deps = invoker.deps
forward_variables_from(invoker, ["deps", "public_deps", "visibility"])

https://codereview.chromium.org/1250913002/diff/100001/build/config/mac/rules...
build/config/mac/rules.gni:66: }
Add:
 forward_variables_from(invoker, ["deps", "public_deps", "visibility"])
to this and the other targets below as well. Some callers will inevitably want
to set some of these things.

https://codereview.chromium.org/1250913002/diff/100001/build/config/mac/rules...
build/config/mac/rules.gni:95: "The name of Mac application for $target_name")
Is it necessary for this to be different? I would expect this to use the name of
the target by default, and use output_name instead if specified.

https://codereview.chromium.org/1250913002/diff/100001/build/config/mac/rules...
build/config/mac/rules.gni:99: # We just create a variable so we can use the
same in interpolation
This is fixed as of today. Just do
  ${invoker.app_name}

https://codereview.chromium.org/1250913002/diff/100001/build/config/mac/rules...
build/config/mac/rules.gni:105: action(struct_gen_target_name) {
Can you restrict visibility for the intermediate targets so only the final group
can depend on them?

https://codereview.chromium.org/1250913002/diff/100001/build/config/mac/rules...
build/config/mac/rules.gni:108: sources = []
You can delete this.

https://codereview.chromium.org/1250913002/diff/100001/build/config/mac/rules...
build/config/mac/rules.gni:191: deps = [
I'd do public_deps for this group and the below one so other targes can use the
outputs if they want.

https://codereview.chromium.org/1250913002/diff/100001/build/toolchain/mac/BU...
File build/toolchain/mac/BUILD.gn (right):

https://codereview.chromium.org/1250913002/diff/100001/build/toolchain/mac/BU...
build/toolchain/mac/BUILD.gn:61: version_flags =
"-mios-simulator-version-min=$ios_deployment_target"
I wonder if this can be more like Linux?

There we have a config "//build/config/linux:sdk" that gets added by default in
BUILDCONFIG.gn to all binary targets. That sets up the sysroot and related flags
when necessary.

This use of the sysroot variable is also not quite right. The toolchain
definitions are executed in the context of the default toolchain, so this might
not always refer to the thing you want. Using the above pattern will do the
right thing.

[Dirk Pranke, 2015-08-04 18:27:16]

https://codereview.chromium.org/1250913002/diff/100001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/1250913002/diff/100001/base/BUILD.gn#newcode820
base/BUILD.gn:820: sources += [ "files/file_path_watcher_mac.cc" ]
On 2015/08/04 18:07:28, brettw wrote:
> This also appears in https://codereview.chromium.org/1257303005/
> FIGHT!

Yeah, I need to merge things.

https://codereview.chromium.org/1250913002/diff/100001/build/config/features.gni
File build/config/features.gni (right):

https://codereview.chromium.org/1250913002/diff/100001/build/config/features....
build/config/features.gni:63: # TODO(GYP) = true
On 2015/08/04 18:07:28, brettw wrote:
> This comment won't make any sense to me later. "How about TODO(GYP) Make this
> work on iOS and enable by default."

Will change.

https://codereview.chromium.org/1250913002/diff/100001/build/config/ios/ios_a...
File build/config/ios/ios_app.py (right):

https://codereview.chromium.org/1250913002/diff/100001/build/config/ios/ios_a...
build/config/ios/ios_app.py:49: 
On 2015/08/04 07:42:01, sdefresne wrote:
> nit: remove blank line

will do.

https://codereview.chromium.org/1250913002/diff/100001/build/config/ios/ios_s...
File build/config/ios/ios_sim.py (right):

https://codereview.chromium.org/1250913002/diff/100001/build/config/ios/ios_s...
build/config/ios/ios_sim.py:32: 
On 2015/08/04 07:42:01, sdefresne wrote:
> nit: remove blank line

will do.

https://codereview.chromium.org/1250913002/diff/100001/build/config/ios/rules...
File build/config/ios/rules.gni (right):

https://codereview.chromium.org/1250913002/diff/100001/build/config/ios/rules...
build/config/ios/rules.gni:37: deps = invoker.deps
On 2015/08/04 18:07:28, brettw wrote:
> If you sync you can do:
>   forward_variables_from(invoker, ["deps", "public_deps", "testonly",
> "visibility"])
> instead, which is a little simpler (also not public_deps addition which is
good
> practice).

ack.

https://codereview.chromium.org/1250913002/diff/100001/build/config/ios/rules...
build/config/ios/rules.gni:89: action(struct_gen_target_name) {
On 2015/08/04 18:07:28, brettw wrote:
> On 2015/08/04 07:42:01, sdefresne wrote:
> > nit: this targets only creates directories as output, however, gn gen
already
> > creates directories for all files listed in outputs, so I'm wondering
whether
> it
> > is needed at all.
> 
> No idea, maybe try with it removed? Whatever you think is best is fine.

I also think it is unnecessary but hadn't tested to confirm yet.

https://codereview.chromium.org/1250913002/diff/100001/build/config/ios/rules...
build/config/ios/rules.gni:92: sources = []
On 2015/08/04 18:07:28, brettw wrote:
> You can just delete this.

ack.

https://codereview.chromium.org/1250913002/diff/100001/build/config/ios/rules...
build/config/ios/rules.gni:104: }
On 2015/08/04 18:07:28, brettw wrote:
> Can you set visibility on these intermediate targets so only the final group
can
> depend on them? You'll want to make a
>   group_target_name = target_name
> above since target_name will be changed inside these sub-targets.

will do.

https://codereview.chromium.org/1250913002/diff/100001/build/config/ios/rules...
build/config/ios/rules.gni:165: if (defined(invoker.testonly)) {
On 2015/08/04 18:07:28, brettw wrote:
> forward_variables_from(invoker, ["testonly"])

ack.

https://codereview.chromium.org/1250913002/diff/100001/build/config/ios/rules...
build/config/ios/rules.gni:172: deps = [
On 2015/08/04 18:07:28, brettw wrote:
> I'd do public_deps instead since you may want to take the outputs from here as
> inputs to other stuff.

Hm. I don't think so; this is declaring the ios_app(target_name) executables,
and I don't think anything will depend on the sub-targets of those. I'd prefer
to leave this as regular deps for now and change later if need be.

Unless I misunderstand something?

https://codereview.chromium.org/1250913002/diff/100001/build/config/mac/rules...
File build/config/mac/rules.gni (right):

https://codereview.chromium.org/1250913002/diff/100001/build/config/mac/rules...
build/config/mac/rules.gni:37: deps = invoker.deps
On 2015/08/04 18:07:29, brettw wrote:
> forward_variables_from(invoker, ["deps", "public_deps", "visibility"])

Acknowledged.

https://codereview.chromium.org/1250913002/diff/100001/build/config/mac/rules...
build/config/mac/rules.gni:95: "The name of Mac application for $target_name")
On 2015/08/04 18:07:28, brettw wrote:
> Is it necessary for this to be different? I would expect this to use the name
of
> the target by default, and use output_name instead if specified.

I don't think it's necessary to be different, but I think in practice it always
is ("Chrome" vs. "chrome", "Content Shell" vs. "content_shell"). 

Will fix regardless so that it can be optional.

https://codereview.chromium.org/1250913002/diff/100001/build/config/mac/rules...
build/config/mac/rules.gni:99: # We just create a variable so we can use the
same in interpolation
On 2015/08/04 18:07:28, brettw wrote:
> This is fixed as of today. Just do
>   ${invoker.app_name}

Acknowledged.

https://codereview.chromium.org/1250913002/diff/100001/build/config/mac/rules...
build/config/mac/rules.gni:105: action(struct_gen_target_name) {
On 2015/08/04 18:07:28, brettw wrote:
> Can you restrict visibility for the intermediate targets so only the final
group
> can depend on them?

Acknowledged.

https://codereview.chromium.org/1250913002/diff/100001/build/config/mac/rules...
build/config/mac/rules.gni:108: sources = []
On 2015/08/04 18:07:28, brettw wrote:
> You can delete this.

Acknowledged.

https://codereview.chromium.org/1250913002/diff/100001/build/config/mac/rules...
build/config/mac/rules.gni:191: deps = [
On 2015/08/04 18:07:28, brettw wrote:
> I'd do public_deps for this group and the below one so other targes can use
the
> outputs if they want.

Same reply (I don't think we need to expose these deps yet).

https://codereview.chromium.org/1250913002/diff/100001/build/toolchain/mac/BU...
File build/toolchain/mac/BUILD.gn (right):

https://codereview.chromium.org/1250913002/diff/100001/build/toolchain/mac/BU...
build/toolchain/mac/BUILD.gn:61: version_flags =
"-mios-simulator-version-min=$ios_deployment_target"
On 2015/08/04 18:07:29, brettw wrote:
> I wonder if this can be more like Linux?
> 
> There we have a config "//build/config/linux:sdk" that gets added by default
in
> BUILDCONFIG.gn to all binary targets. That sets up the sysroot and related
flags
> when necessary.
> 
> This use of the sysroot variable is also not quite right. The toolchain
> definitions are executed in the context of the default toolchain, so this
might
> not always refer to the thing you want. Using the above pattern will do the
> right thing.

I will add a comment and investigate that in a separate CL if that's okay?

https://codereview.chromium.org/1250913002/diff/100001/testing/test.gni
File testing/test.gni (right):

https://codereview.chromium.org/1250913002/diff/100001/testing/test.gni#newco...
testing/test.gni:156: if (is_ios) {
On 2015/08/04 07:42:02, sdefresne wrote:
> nit: redundant with the previous test

Acknowledged.

[brettw, 2015-08-04 19:20:23]

https://codereview.chromium.org/1250913002/diff/100001/build/toolchain/mac/BU...
File build/toolchain/mac/BUILD.gn (right):

https://codereview.chromium.org/1250913002/diff/100001/build/toolchain/mac/BU...
build/toolchain/mac/BUILD.gn:61: version_flags =
"-mios-simulator-version-min=$ios_deployment_target"
On 2015/08/04 18:27:15, Dirk Pranke wrote:
> On 2015/08/04 18:07:29, brettw wrote:
> > I wonder if this can be more like Linux?
> > 
> > There we have a config "//build/config/linux:sdk" that gets added by default
> in
> > BUILDCONFIG.gn to all binary targets. That sets up the sysroot and related
> flags
> > when necessary.
> > 
> > This use of the sysroot variable is also not quite right. The toolchain
> > definitions are executed in the context of the default toolchain, so this
> might
> > not always refer to the thing you want. Using the above pattern will do the
> > right thing.
> 
> I will add a comment and investigate that in a separate CL if that's okay?

This current one is definitely wrong so I'd rather not add the sysroot_flags
stuff which just needs to get deleted later. If you want to simplify this
change, I'd move this whole file's changes to a different CL.

[Dirk Pranke, 2015-08-04 19:28:06]

https://codereview.chromium.org/1250913002/diff/100001/build/toolchain/mac/BU...
File build/toolchain/mac/BUILD.gn (right):

https://codereview.chromium.org/1250913002/diff/100001/build/toolchain/mac/BU...
build/toolchain/mac/BUILD.gn:61: version_flags =
"-mios-simulator-version-min=$ios_deployment_target"
On 2015/08/04 19:20:23, brettw wrote:
> On 2015/08/04 18:27:15, Dirk Pranke wrote:
> > On 2015/08/04 18:07:29, brettw wrote:
> > > I wonder if this can be more like Linux?
> > > 
> > > There we have a config "//build/config/linux:sdk" that gets added by
default
> > in
> > > BUILDCONFIG.gn to all binary targets. That sets up the sysroot and related
> > flags
> > > when necessary.
> > > 
> > > This use of the sysroot variable is also not quite right. The toolchain
> > > definitions are executed in the context of the default toolchain, so this
> > might
> > > not always refer to the thing you want. Using the above pattern will do
the
> > > right thing.
> > 
> > I will add a comment and investigate that in a separate CL if that's okay?
> 
> This current one is definitely wrong so I'd rather not add the sysroot_flags
> stuff which just needs to get deleted later. If you want to simplify this
> change, I'd move this whole file's changes to a different CL.

Okay, I'll see what I can do, one way or another.

[Dirk Pranke, 2015-08-05 22:28:22]

dpranke@chromium.org changed reviewers:
+ sherouk@google.com

[Dirk Pranke, 2015-08-05 22:28:22]

Okay, I think I've addressed all of the review feedback, though I probably
missed some things.

Please take another look? It would be good to get this landed soon so I can quit
stomping on sherouk's changes :).

[Dirk Pranke, 2015-08-05 22:28:48]

https://codereview.chromium.org/1250913002/diff/160001/BUILD.gn
File BUILD.gn (left):

https://codereview.chromium.org/1250913002/diff/160001/BUILD.gn#oldcode68
BUILD.gn:68: "//sync:sync_unit_tests",
sync (and net) don't currently compile for me, so I've commented them back out
...

[brettw, 2015-08-06 18:07:27]

https://codereview.chromium.org/1250913002/diff/160001/build/config/ios/rules...
File build/config/ios/rules.gni (right):

https://codereview.chromium.org/1250913002/diff/160001/build/config/ios/rules...
build/config/ios/rules.gni:80: app_name = invoker.app_name
This still isn't optional (should default to target name), and I still really
think it should be output name instead of introducing a new variable.

https://codereview.chromium.org/1250913002/diff/160001/build/config/ios/rules...
build/config/ios/rules.gni:93: if (defined(invoker.testonly)) {
You forward testonly above. Is this necessary? If it is, this should also be
forward.

https://codereview.chromium.org/1250913002/diff/160001/build/config/ios/rules...
build/config/ios/rules.gni:108: deps = invoker.deps + [
":$plist_gen_target_name" ]
Can this treat the invoker deps as optional? I realize this won't really happen
in practice, but if I make some kind of test app, I'll be quite surprised if I
get an error just because I have no deps.

https://codereview.chromium.org/1250913002/diff/160001/build/config/mac/rules...
File build/config/mac/rules.gni (right):

https://codereview.chromium.org/1250913002/diff/160001/build/config/mac/rules...
build/config/mac/rules.gni:81: template("resource_copy_mac") {
This file seems weirdly the same as the iOS one. Should they be the same
(potentially the template could trigger off the current platform), or can they
share some underlying shared template?

[Dirk Pranke, 2015-08-06 18:29:46]

https://codereview.chromium.org/1250913002/diff/160001/build/config/ios/rules...
File build/config/ios/rules.gni (right):

https://codereview.chromium.org/1250913002/diff/160001/build/config/ios/rules...
build/config/ios/rules.gni:80: app_name = invoker.app_name
On 2015/08/06 18:07:27, brettw wrote:
> This still isn't optional (should default to target name), and I still really
> think it should be output name instead of introducing a new variable.

Yeah, I fixed the mac one but not the ios one. will fix.

https://codereview.chromium.org/1250913002/diff/160001/build/config/ios/rules...
build/config/ios/rules.gni:93: if (defined(invoker.testonly)) {
On 2015/08/06 18:07:27, brettw wrote:
> You forward testonly above. Is this necessary? If it is, this should also be
> forward.

I don't think this is necessary. Will fix.

https://codereview.chromium.org/1250913002/diff/160001/build/config/ios/rules...
build/config/ios/rules.gni:108: deps = invoker.deps + [
":$plist_gen_target_name" ]
On 2015/08/06 18:07:27, brettw wrote:
> Can this treat the invoker deps as optional? I realize this won't really
happen
> in practice, but if I make some kind of test app, I'll be quite surprised if I
> get an error just because I have no deps.

Will fix.

https://codereview.chromium.org/1250913002/diff/160001/build/config/mac/rules...
File build/config/mac/rules.gni (right):

https://codereview.chromium.org/1250913002/diff/160001/build/config/mac/rules...
build/config/mac/rules.gni:81: template("resource_copy_mac") {
On 2015/08/06 18:07:27, brettw wrote:
> This file seems weirdly the same as the iOS one. Should they be the same
> (potentially the template could trigger off the current platform), or can they
> share some underlying shared template?

I think they can probably be merged, but I'd like to do that in a separate
follow-up patch. Sound okay?

[brettw, 2015-08-06 19:35:11]

https://codereview.chromium.org/1250913002/diff/160001/build/config/mac/rules...
File build/config/mac/rules.gni (right):

https://codereview.chromium.org/1250913002/diff/160001/build/config/mac/rules...
build/config/mac/rules.gni:81: template("resource_copy_mac") {
Yes, we can do this in a follow-up.

[Dirk Pranke, 2015-08-06 20:32:11]

https://codereview.chromium.org/1250913002/diff/160001/build/config/ios/rules...
File build/config/ios/rules.gni (right):

https://codereview.chromium.org/1250913002/diff/160001/build/config/ios/rules...
build/config/ios/rules.gni:80: app_name = invoker.app_name
On 2015/08/06 18:29:46, Dirk Pranke wrote:
> On 2015/08/06 18:07:27, brettw wrote:
> > This still isn't optional (should default to target name), and I still
really
> > think it should be output name instead of introducing a new variable.
> 
> Yeah, I fixed the mac one but not the ios one. will fix.

I'm not sure if can use output_name; we app_name for slightly different parts of
different paths, including output_name itself (see line 92 and line 124 for some
of the differences), but I'll see what I can do in a follow-up.

[brettw, 2015-08-06 22:38:15]

LGTM for now, we can address the rest in a followup

https://codereview.chromium.org/1250913002/diff/160001/build/config/ios/rules...
File build/config/ios/rules.gni (right):

https://codereview.chromium.org/1250913002/diff/160001/build/config/ios/rules...
build/config/ios/rules.gni:80: app_name = invoker.app_name
On 2015/08/06 20:32:11, Dirk Pranke wrote:
> On 2015/08/06 18:29:46, Dirk Pranke wrote:
> > On 2015/08/06 18:07:27, brettw wrote:
> > > This still isn't optional (should default to target name), and I still
> really
> > > think it should be output name instead of introducing a new variable.
> > 
> > Yeah, I fixed the mac one but not the ios one. will fix.
> 
> I'm not sure if can use output_name; we app_name for slightly different parts
of
> different paths, including output_name itself (see line 92 and line 124 for
some
> of the differences), but I'll see what I can do in a follow-up.

Okay, we can do a follow up. I'm still quite certain we should use output_name.
The caller shouldn't be specifying a path or anything in that string (I was a
little surprised it worked for you below, but I guess it's not shocking).

[Dirk Pranke, 2015-08-06 23:01:56]

The CQ bit was checked by dpranke@chromium.org

[Dirk Pranke, 2015-08-06 23:01:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/1250913002/#ps180001
(title: "status")

[commit-bot: I haz the power, 2015-08-06 23:02:57]

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

[commit-bot: I haz the power, 2015-08-07 01:05:49]

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

[commit-bot: I haz the power, 2015-08-07 01:05:50]

Try jobs failed on following builders:
  mac_chromium_gn_dbg on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)

[Dirk Pranke, 2015-08-07 01:12:01]

The CQ bit was checked by dpranke@chromium.org

[commit-bot: I haz the power, 2015-08-07 01:12:49]

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

[commit-bot: I haz the power, 2015-08-07 03:15:05]

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

[commit-bot: I haz the power, 2015-08-07 03:15:07]

Try jobs failed on following builders:
  mac_chromium_gn_dbg on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)

[Dirk Pranke, 2015-08-07 03:39:15]

The CQ bit was checked by dpranke@chromium.org

[commit-bot: I haz the power, 2015-08-07 03:40:14]

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

[commit-bot: I haz the power, 2015-08-07 05:23:09]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-08-07 05:23:47]

Patchset 10 (id:??) landed as
https://crrev.com/2a29462a04dabad3fc20dc922227c46b36459912
Cr-Commit-Position: refs/heads/master@{#342297}

[Nico, 2015-08-11 20:41:00]

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

[Nico, 2015-08-11 20:41:01]

https://codereview.chromium.org/1250913002/diff/180001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/1250913002/diff/180001/base/BUILD.gn#newcode18
base/BUILD.gn:18: }
This does not lgtm. I explicitly not lgtm'd this on
https://codereview.chromium.org/1180583002/ . You were cc'd on that review. Why
was this added here?

[Dirk Pranke, 2015-08-11 20:46:29]

On 2015/08/11 20:41:01, Nico (hiding) wrote:
> https://codereview.chromium.org/1250913002/diff/180001/base/BUILD.gn
> File base/BUILD.gn (right):
> 
> https://codereview.chromium.org/1250913002/diff/180001/base/BUILD.gn#newcode18
> base/BUILD.gn:18: }
> This does not lgtm. I explicitly not lgtm'd this on
> https://codereview.chromium.org/1180583002/ . You were cc'd on that review.
Why
> was this added here?

I think this probably was a bad merge. I will see if I can correct the merge and
post a fix.