[iOS/GN] Allow compilation with system clang.

[iOS/GN] Allow compilation with system clang.

The official Chrome on iOS build are build using the version of clang
shipped with Xcode (a.k.a. the system version of clang). This CL adds
a flag to allow building with this version of clang instead of the
hermetic shipped with Chromium source (i.e. ToT clang).

When building with system version of clang, some compiler warning have
to be flipped as they are unsupported and the plugins are disabled (as
they only work for a single version of clang).

Refactor the definition of the toolchain for iOS and correctly set the
default_toolchain when targetting iOS based on target_cpu (used to be
incorrectly set to "//build/toolchain/mac:ios_clang_arm").

BUG=620376
Committed: https://crrev.com/fe65591ff09b9180898e303ec4831d4e7dfb10e9
Cr-Commit-Position: refs/heads/master@{#401119}

[sdefresne, 2016-06-20 12:49:41]

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

[sdefresne, 2016-06-20 12:49:42]

Please take a look.

[Dirk Pranke, 2016-06-20 16:31:23]

I understand the need to be able to build w/ xcode clang, but I'm nervous about
the idea that we build the official builder w/ a config that isn't tested
anywhere else. Should we have public builders testing this config as well?

cc'ing a few others for their thoughts ...

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

https://codereview.chromium.org/2079283002/diff/40001/build/config/compiler/B...
build/config/compiler/BUILD.gn:1011: # Clang compiler will almost have different
version. Certain flags may
s/and system/and the system/

and

s/almost have different version/almost always have different versions/

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

https://codereview.chromium.org/2079283002/diff/40001/build/toolchain/mac/BUI...
build/toolchain/mac/BUILD.gn:391: }
Where do we use gcc toolchains on the mac? Why add these?

[sdefresne, 2016-06-21 11:31:29]

On 2016/06/20 16:31:23, Dirk Pranke wrote:
> I understand the need to be able to build w/ xcode clang, but I'm nervous
about
> the idea that we build the official builder w/ a config that isn't tested
> anywhere else. Should we have public builders testing this config as well?
> 
> cc'ing a few others for their thoughts ...

I agree that it is a bit uncomfortable to have a config not tested by bots
upstream, but this would not be worse than what we have currently with gyp where
the corresponding configuration is only tested downstream.

Over the past, we have found few problems with that setup. The only recent
breakage this revealed where due to a more recent version of libc++ with ToT
clang and failures while trying to build v8 (which we should never have been
building on iOS).

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

https://codereview.chromium.org/2079283002/diff/40001/build/config/compiler/B...
build/config/compiler/BUILD.gn:1011: # Clang compiler will almost have different
version. Certain flags may
On 2016/06/20 16:31:22, Dirk Pranke wrote:
> s/and system/and the system/
> 
> and
> 
> s/almost have different version/almost always have different versions/
> 

Done.

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

https://codereview.chromium.org/2079283002/diff/40001/build/toolchain/mac/BUI...
build/toolchain/mac/BUILD.gn:391: }
On 2016/06/20 16:31:22, Dirk Pranke wrote:
> Where do we use gcc toolchains on the mac? Why add these?

I didn't add them, I moved the ios ones below this one (it was between "arm" and
"clang_x64"). They were located around line 403-423 in the old file.

I can remove them if you think they are not useful. But I'd prefer to do it in a
followup CL (so that I do not get reverted whole if this breaks someone, though
I don't think it will as last time I tried build failed on Mac with gcc).

[Nico, 2016-06-21 12:30:58]

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

[Nico, 2016-06-21 12:30:59]

lgtm

https://codereview.chromium.org/2079283002/diff/60001/build/config/clang/BUIL...
File build/config/clang/BUILD.gn (right):

https://codereview.chromium.org/2079283002/diff/60001/build/config/clang/BUIL...
build/config/clang/BUILD.gn:65: if (use_system_clang && is_ios) {
can we call this use_xcode_clang please? i don't want people to try and add
"system clang" support for builds other than ios.

(also, have you experimented with shipping a chromium clang release? what are
blockers?)

[Nico, 2016-06-21 12:32:08]

err sorry, fingers typed "lgtm" on autopilot. basically looks fine to me, but i
think we should name the variable the same way we do in gyp.

(also, it'd be nice if we could try to get rid of this some day, once the build
system shuffling is all done)

[sdefresne, 2016-06-21 13:21:58]

PTAL (as I don't know if your second "lgtm" was intentional or not).

https://codereview.chromium.org/2079283002/diff/60001/build/config/clang/BUIL...
File build/config/clang/BUILD.gn (right):

https://codereview.chromium.org/2079283002/diff/60001/build/config/clang/BUIL...
build/config/clang/BUILD.gn:65: if (use_system_clang && is_ios) {
On 2016/06/21 12:30:59, Nico wrote:
> can we call this use_xcode_clang please? i don't want people to try and add
> "system clang" support for builds other than ios.
> 
> (also, have you experimented with shipping a chromium clang release? what are
> blockers?)

I've renamed the variable to "is_clang_xcode" to be closer to the gyp name
("clang_xcode") while still following the same naming convention than "is_clang"
(named "clang" with gyp).

Regarding shipping a build of Chrome on iOS using ToT clang, if/when we do it we
want to decouple this from the transition from gyp to gn.

[sdefresne, 2016-06-21 13:23:05]

Sorry, I fat fingered patchset 5, please look at patchset 6.

[Nico, 2016-06-21 13:25:44]

looks like ps6 undid the "xcode" bit in the name

https://codereview.chromium.org/2079283002/diff/80001/build/toolchain/toolcha...
File build/toolchain/toolchain.gni (right):

https://codereview.chromium.org/2079283002/diff/80001/build/toolchain/toolcha...
build/toolchain/toolchain.gni:30: # Compile with system version of clang instead
of hermetic version shipped
update "system" in comment as well.

if it's possible to make declare_args conditional, i'd only do this if the host
os is mac

https://codereview.chromium.org/2079283002/diff/80001/ios/chrome/browser/brow...
File ios/chrome/browser/browser_state/chrome_browser_state_io_data.cc (right):

https://codereview.chromium.org/2079283002/diff/80001/ios/chrome/browser/brow...
ios/chrome/browser/browser_state/chrome_browser_state_io_data.cc:378:
io_thread_globals->ct_policy_enforcer.get());
this looks unrelated?

https://codereview.chromium.org/2079283002/diff/100001/build/toolchain/toolch...
File build/toolchain/toolchain.gni (right):

https://codereview.chromium.org/2079283002/diff/100001/build/toolchain/toolch...
build/toolchain/toolchain.gni:33: use_system_clang = is_ios && is_official_build
xcode, not system

[sdefresne, 2016-06-21 13:44:22]

Sorry for all the recent incorrect uploads (incorrectly used "git commit
--amend" and "git rebase").

This patchset 7 is correct now. Can you take another look?

https://codereview.chromium.org/2079283002/diff/80001/build/config/clang/BUIL...
File build/config/clang/BUILD.gn (right):

https://codereview.chromium.org/2079283002/diff/80001/build/config/clang/BUIL...
build/config/clang/BUILD.gn:64: # TODO(ios): Remove once Xcode's libc++ has LLVM
r256325.
This should not have been defined here, moved instead to
build/config/ios/BUILD.gn.

https://codereview.chromium.org/2079283002/diff/100001/build/toolchain/toolch...
File build/toolchain/toolchain.gni (right):

https://codereview.chromium.org/2079283002/diff/100001/build/toolchain/toolch...
build/toolchain/toolchain.gni:33: use_system_clang = is_ios && is_official_build
On 2016/06/21 13:25:44, Nico wrote:
> xcode, not system

Done, and made the variable conditional on host_os == "mac".

[Dirk Pranke, 2016-06-21 16:33:48]

lgtm, though I actually prefer thakis's suggested name of 'use_xcode_clang' to
'use_clang_xcode'.

[Dirk Pranke, 2016-06-21 16:35:04]

On 2016/06/21 16:33:48, Dirk Pranke wrote:
> lgtm, though I actually prefer thakis's suggested name of 'use_xcode_clang' to
> 'use_clang_xcode'.

Er, thakis suggested 'is_xcode_clang', but I do agree that 'use_' is better than
'is_'.

So, typos all around :).

[Nico, 2016-06-21 16:51:14]

lgtm

[sdefresne, 2016-06-21 19:30:43]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2016-06-21 19:30:44]

The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org
Link to the patchset: https://codereview.chromium.org/2079283002/#ps160001
(title: "sed -i '' -e 's@use_clang_xcode@use_xcode_clang@g' ")

[commit-bot: I haz the power, 2016-06-21 19:31:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2079283002/160001

[commit-bot: I haz the power, 2016-06-21 19:39:41]

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

[commit-bot: I haz the power, 2016-06-21 19:39:43]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[brettw, 2016-06-21 20:24:43]

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

[brettw, 2016-06-21 20:24:44]

buildconfig.gn lgtm

[sdefresne, 2016-06-21 20:25:39]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2016-06-21 20:25:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, thakis@chromium.org, brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/2079283002/#ps180001
(title: "Rebase")

[commit-bot: I haz the power, 2016-06-21 20:26:05]

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

[commit-bot: I haz the power, 2016-06-21 21:58:23]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-06-21 22:00:45]

Description was changed from

==========
[iOS/GN] Allow compilation with system clang.

The official Chrome on iOS build are build using the version of clang
shipped with Xcode (a.k.a. the system version of clang). This CL adds
a flag to allow building with this version of clang instead of the
hermetic shipped with Chromium source (i.e. ToT clang).

When building with system version of clang, some compiler warning have
to be flipped as they are unsupported and the plugins are disabled (as
they only work for a single version of clang).

Refactor the definition of the toolchain for iOS and correctly set the
default_toolchain when targetting iOS based on target_cpu (used to be
incorrectly set to "//build/toolchain/mac:ios_clang_arm").

BUG=620376
==========

to

==========
[iOS/GN] Allow compilation with system clang.

The official Chrome on iOS build are build using the version of clang
shipped with Xcode (a.k.a. the system version of clang). This CL adds
a flag to allow building with this version of clang instead of the
hermetic shipped with Chromium source (i.e. ToT clang).

When building with system version of clang, some compiler warning have
to be flipped as they are unsupported and the plugins are disabled (as
they only work for a single version of clang).

Refactor the definition of the toolchain for iOS and correctly set the
default_toolchain when targetting iOS based on target_cpu (used to be
incorrectly set to "//build/toolchain/mac:ios_clang_arm").

BUG=620376

Committed: https://crrev.com/fe65591ff09b9180898e303ec4831d4e7dfb10e9
Cr-Commit-Position: refs/heads/master@{#401119}
==========

[commit-bot: I haz the power, 2016-06-21 22:00:46]

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

[justincohen, 2016-06-22 09:15:06]

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

[justincohen, 2016-06-22 09:15:07]

https://codereview.chromium.org/2079283002/diff/60001/build/config/clang/BUIL...
File build/config/clang/BUILD.gn (right):

https://codereview.chromium.org/2079283002/diff/60001/build/config/clang/BUIL...
build/config/clang/BUILD.gn:65: if (use_system_clang && is_ios) {
> (also, have you experimented with shipping a chromium clang release? what are
> blockers?)

We haven't tried shipping with a chromium clang.

We sometimes see features added to Xcode's clang that are required to support
some new new iOS thing.  While those features do eventually make it to chromium
clang, we'd have delay support.

I don't know of anything blocking us today, but who knows what the next Xcode
beta will include.

[Nico, 2016-06-22 14:30:28]

https://codereview.chromium.org/2079283002/diff/60001/build/config/clang/BUIL...
File build/config/clang/BUILD.gn (right):

https://codereview.chromium.org/2079283002/diff/60001/build/config/clang/BUIL...
build/config/clang/BUILD.gn:65: if (use_system_clang && is_ios) {
On 2016/06/22 09:15:07, justincohen wrote:
> > (also, have you experimented with shipping a chromium clang release? what
are
> > blockers?)
> 
> We haven't tried shipping with a chromium clang.
> 
> We sometimes see features added to Xcode's clang that are required to support
> some new new iOS thing.  While those features do eventually make it to
chromium
> clang, we'd have delay support.
> 
> I don't know of anything blocking us today, but who knows what the next Xcode
> beta will include.

The last two times (bitcode and...uh, don't remember, there was one other thing.
arm64?) it took like a month, which doesn't seem so terrible.

[Dirk Pranke, 2016-06-22 15:34:55]

I think we should set up an xcode_clang builder upstream; generally, I think we
should have an upstream/public config for every
configuration we need to support that can be public, so that we can catch and
revert things easily and transparently.

I don't know that it needs to be in the CQ, though ...