Fix gcc_version.gni to determine target GCC toolchain version for Chrome OS.

Fix gcc_version.gni to determine target GCC toolchain version for Chrome OS.

This CL modifies gcc_version.gni to set the gcc_version to the target
GCC toolchain version when building for Chrome OS.

BUG=427726
TEST=Manually verify that the correct GCC toolchain is used when building with 'os="chromeos"'.

Committed: https://crrev.com/98ee835a49b59183909c730bf7d4283ea11b8281
Cr-Commit-Position: refs/heads/master@{#301883}

[Ben Chan, 2014-10-27 22:41:46]

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

[Ben Chan, 2014-10-27 23:33:48]

benchan@chromium.org changed reviewers:
+ jamesr@chromium.org

[jamesr, 2014-10-27 23:48:52]

https://codereview.chromium.org/678183002/diff/1/build/config/gcc/gcc_version...
File build/config/gcc/gcc_version.gni (right):

https://codereview.chromium.org/678183002/diff/1/build/config/gcc/gcc_version...
build/config/gcc/gcc_version.gni:14: } else if (is_chromeos) {
what does being chromeos have to do with whether you are cross-compiling or not?
 I'm pretty sure the answer is nothing.  Did you mean to check if the host
toolchain == the current toolchain or something like that?

[jamesr, 2014-10-27 23:49:18]

Take a look at 'gn help current_toolchain' and 'gn help default_toolchain'

[Ben Chan, 2014-10-28 00:21:21]

https://codereview.chromium.org/678183002/diff/1/build/config/gcc/gcc_version...
File build/config/gcc/gcc_version.gni (right):

https://codereview.chromium.org/678183002/diff/1/build/config/gcc/gcc_version...
build/config/gcc/gcc_version.gni:14: } else if (is_chromeos) {
On 2014/10/27 23:48:52, jamesr wrote:
> what does being chromeos have to do with whether you are cross-compiling or
not?
>  I'm pretty sure the answer is nothing.  Did you mean to check if the host
> toolchain == the current toolchain or something like that?

After looking at common/build.gypi, it seems like the right fix is to always
check the target toolchain version as a fallback:

  gcc_version = exec_script("../../compiler_version.py", [ "target", "compiler"
], "value")

In common/build.gypi, most of the code uses gcc_version (instead of
host_gcc_version), which is based on `compiler_version target compiler`.

Do you agree?

[jamesr, 2014-10-28 00:26:02]

jamesr@chromium.org changed reviewers:
+ cmasone@chromium.org

[jamesr, 2014-10-28 00:26:03]

Sorry, I'm not familiar enough with how the cross-compile works to say. 
cmasone@'s been looking at some chromeos cross-compile issues in GN - maybe he
can chime in with some insight.

[Chris Masone, 2014-10-28 14:36:03]

This looks to me like it lines up with the GYP notion of what to populate
'gcc_version' with: the version of the target compiler.

That said, Ben, you should look at the script this is running and ensure that
it'll Do The Right Thing when the GN build is run inside the chroot. How does
this script find the target toolchain to check its version?

In the chroot, I intend to be injecting the target toolchain using GN args
consumed here:
https://code.google.com/p/chromium/codesearch#chromium/src/build/toolchain/cr...

[Ben Chan, 2014-10-28 15:43:43]

On 2014/10/28 14:36:03, Chris Masone wrote:
> This looks to me like it lines up with the GYP notion of what to populate
> 'gcc_version' with: the version of the target compiler.
> 
> That said, Ben, you should look at the script this is running and ensure that
> it'll Do The Right Thing when the GN build is run inside the chroot. How does
> this script find the target toolchain to check its version?
> 
> In the chroot, I intend to be injecting the target toolchain using GN args
> consumed here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/build/toolchain/cr...

I think we need to sort out two issues:

(1) Currently, the chromeos chroot and simple chrome workflow both define
CC,CXX,AR to be the target toolchain.
The simple chrome workflow further defines {CC,CXX}_host to be the host
toolchain, which are turned into host_gcc_version by common.gypi.

`compiler_version.py host` checks CXX_host, CXX_target, CXX in order, while
`compiler_version.py target` checks CXX_target, CXX in order.

So this CL brings the gcc_version.gni more consistent with the GYP flow in near
term. Then we should look into (2) below to figure a more generalized solution.

(2) IIUC, cros_target_{cc,cxx,ar} allows CC,CXX,AR to be overridden via GN args.
I wonder if this should be generalized to all OS targets beyond ChromeOS (e.g.
having declare_args target_{cc,cxx,ar}). If that's the case, gcc_version /
compiler_version.py can then invoke the user specified toolchain to determine
the gcc version.

WDYT?

[Ben Chan, 2014-10-28 15:44:09]

+vapier

[Chris Masone, 2014-10-28 17:24:05]

On 2014/10/28 15:43:43, Ben Chan wrote:
> On 2014/10/28 14:36:03, Chris Masone wrote:
> > This looks to me like it lines up with the GYP notion of what to populate
> > 'gcc_version' with: the version of the target compiler.
> > 
> > That said, Ben, you should look at the script this is running and ensure
that
> > it'll Do The Right Thing when the GN build is run inside the chroot. How
does
> > this script find the target toolchain to check its version?
> > 
> > In the chroot, I intend to be injecting the target toolchain using GN args
> > consumed here:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/build/toolchain/cr...
> 
> I think we need to sort out two issues:
> 
> (1) Currently, the chromeos chroot and simple chrome workflow both define
> CC,CXX,AR to be the target toolchain.
> The simple chrome workflow further defines {CC,CXX}_host to be the host
> toolchain, which are turned into host_gcc_version by common.gypi.
> 
> `compiler_version.py host` checks CXX_host, CXX_target, CXX in order, while
> `compiler_version.py target` checks CXX_target, CXX in order.
> 
> So this CL brings the gcc_version.gni more consistent with the GYP flow in
near
> term. Then we should look into (2) below to figure a more generalized
solution.
> 
> (2) IIUC, cros_target_{cc,cxx,ar} allows CC,CXX,AR to be overridden via GN
args.
> I wonder if this should be generalized to all OS targets beyond ChromeOS (e.g.
> having declare_args target_{cc,cxx,ar}). If that's the case, gcc_version /
> compiler_version.py can then invoke the user specified toolchain to determine
> the gcc version.
> 
> WDYT?

I think that no one who maintains other platforms has any interest in passing
toolchain names in like we do. I think that 1) is fine, as this brings us in
line with GYP.

LGTM

[Ben Chan, 2014-10-28 17:42:17]

On 2014/10/28 17:24:05, Chris Masone wrote:
> On 2014/10/28 15:43:43, Ben Chan wrote:
> > On 2014/10/28 14:36:03, Chris Masone wrote:
> > > This looks to me like it lines up with the GYP notion of what to populate
> > > 'gcc_version' with: the version of the target compiler.
> > > 
> > > That said, Ben, you should look at the script this is running and ensure
> that
> > > it'll Do The Right Thing when the GN build is run inside the chroot. How
> does
> > > this script find the target toolchain to check its version?
> > > 
> > > In the chroot, I intend to be injecting the target toolchain using GN args
> > > consumed here:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/build/toolchain/cr...
> > 
> > I think we need to sort out two issues:
> > 
> > (1) Currently, the chromeos chroot and simple chrome workflow both define
> > CC,CXX,AR to be the target toolchain.
> > The simple chrome workflow further defines {CC,CXX}_host to be the host
> > toolchain, which are turned into host_gcc_version by common.gypi.
> > 
> > `compiler_version.py host` checks CXX_host, CXX_target, CXX in order, while
> > `compiler_version.py target` checks CXX_target, CXX in order.
> > 
> > So this CL brings the gcc_version.gni more consistent with the GYP flow in
> near
> > term. Then we should look into (2) below to figure a more generalized
> solution.
> > 
> > (2) IIUC, cros_target_{cc,cxx,ar} allows CC,CXX,AR to be overridden via GN
> args.
> > I wonder if this should be generalized to all OS targets beyond ChromeOS
(e.g.
> > having declare_args target_{cc,cxx,ar}). If that's the case, gcc_version /
> > compiler_version.py can then invoke the user specified toolchain to
determine
> > the gcc version.
> > 
> > WDYT?
> 
> I think that no one who maintains other platforms has any interest in passing
> toolchain names in like we do. I think that 1) is fine, as this brings us in
> line with GYP.
> 
> LGTM

jamesr, brettw: WDYT?

[jamesr, 2014-10-28 17:43:51]

I defer to cmasone@'s review

[Ben Chan, 2014-10-28 22:46:45]

On 2014/10/28 17:43:51, jamesr wrote:
> I defer to cmasone@'s review

brettw: could you give OWNER lgtm if the CL lgtm, thanks!

[brettw, 2014-10-29 00:35:22]

https://codereview.chromium.org/678183002/diff/20001/build/config/gcc/gcc_ver...
File build/config/gcc/gcc_version.gni (right):

https://codereview.chromium.org/678183002/diff/20001/build/config/gcc/gcc_ver...
build/config/gcc/gcc_version.gni:15: gcc_version =
exec_script("../../compiler_version.py", [ "target", "compiler" ], "value")
Sorry for the delay. This is a bit different than GYP because GYP runs one pass
and gcc_version means target compiler. In GN, this code will run multiple times,
once for each toolchain. This is a bit sticky since it could be any number of
values.

Currently, the toolchain might be nacl, anything defined in
//build/config/linux/BUILD.gn, or //build/toolchain/cros:target

We should define this value for each of those options, defaulting to 0 for cases
we don't handle. The cros:target is clearly the "target" GCC version.

So I'd rewrite this whole condition, removing the is_clang check at the top:

if (is_android) {
  ... current code ...
} else if (current_toolchain == "//build/toolchain/cros:target") {
  gcc_version = exec_script("../../compiler_version.py", [ "target", "compiler"
], "value")
} else if (current_toolchain == "//build/toolchain/linux:x64" ||
           current_toolchain == "//build/toolchain/linux:x86) {
  # These are both the same and just use the default gcc on the system.
  gcc_version = exec_script(.... [ "host", ...)
} else {
  gcc_version = 0
}

[vapier, 2014-10-29 05:01:11]

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

[vapier, 2014-10-29 05:01:11]

i think people are cross-compiling for more than CrOS.  people often
cross-compile Linux/ARM, and certainly they're doing that to build for
iOS/Android.  i think people are also actively investigating getting
Linux->Windows to build so they can simplify the bots (Linux is always the build
system, but can target everything else).

i also think that generalizing toolchain selection across all targets makes more
sense than restricting it to random specific OSs.  in my experience, it also
simplifies the code and leads to less bitrot.  after all, if everyone is
actively developing Linux native, then landing changes that breaks
cross-compiling would break all CrOS targets.

[Ben Chan, 2014-10-29 16:33:47]

On 2014/10/29 00:35:22, brettw wrote:
>
https://codereview.chromium.org/678183002/diff/20001/build/config/gcc/gcc_ver...
> File build/config/gcc/gcc_version.gni (right):
> 
>
https://codereview.chromium.org/678183002/diff/20001/build/config/gcc/gcc_ver...
> build/config/gcc/gcc_version.gni:15: gcc_version =
> exec_script("../../compiler_version.py", [ "target", "compiler" ], "value")
> Sorry for the delay. This is a bit different than GYP because GYP runs one
pass
> and gcc_version means target compiler. In GN, this code will run multiple
times,
> once for each toolchain. This is a bit sticky since it could be any number of
> values.
> 
> Currently, the toolchain might be nacl, anything defined in
> //build/config/linux/BUILD.gn, or //build/toolchain/cros:target
> 
> We should define this value for each of those options, defaulting to 0 for
cases
> we don't handle. The cros:target is clearly the "target" GCC version.
> 
> So I'd rewrite this whole condition, removing the is_clang check at the top:
> 
> if (is_android) {
>   ... current code ...
> } else if (current_toolchain == "//build/toolchain/cros:target") {
>   gcc_version = exec_script("../../compiler_version.py", [ "target",
"compiler"
> ], "value")
> } else if (current_toolchain == "//build/toolchain/linux:x64" ||
>            current_toolchain == "//build/toolchain/linux:x86) {
>   # These are both the same and just use the default gcc on the system.
>   gcc_version = exec_script(.... [ "host", ...)
> } else {
>   gcc_version = 0
> }

These checks seem to base on the fact that cros:target, linux:x64, linux:x86 use
gcc. I guess we could have linux:arm, linux:mips, or even mac using gcc. Perhaps
we could later indicate whether gcc is used and then further generalize the
check?

[Ben Chan, 2014-10-29 16:48:42]

On 2014/10/29 16:33:47, Ben Chan wrote:
> On 2014/10/29 00:35:22, brettw wrote:
> >
>
https://codereview.chromium.org/678183002/diff/20001/build/config/gcc/gcc_ver...
> > File build/config/gcc/gcc_version.gni (right):
> > 
> >
>
https://codereview.chromium.org/678183002/diff/20001/build/config/gcc/gcc_ver...
> > build/config/gcc/gcc_version.gni:15: gcc_version =
> > exec_script("../../compiler_version.py", [ "target", "compiler" ], "value")
> > Sorry for the delay. This is a bit different than GYP because GYP runs one
> pass
> > and gcc_version means target compiler. In GN, this code will run multiple
> times,
> > once for each toolchain. This is a bit sticky since it could be any number
of
> > values.
> > 
> > Currently, the toolchain might be nacl, anything defined in
> > //build/config/linux/BUILD.gn, or //build/toolchain/cros:target
> > 
> > We should define this value for each of those options, defaulting to 0 for
> cases
> > we don't handle. The cros:target is clearly the "target" GCC version.
> > 
> > So I'd rewrite this whole condition, removing the is_clang check at the top:
> > 
> > if (is_android) {
> >   ... current code ...
> > } else if (current_toolchain == "//build/toolchain/cros:target") {
> >   gcc_version = exec_script("../../compiler_version.py", [ "target",
> "compiler"
> > ], "value")
> > } else if (current_toolchain == "//build/toolchain/linux:x64" ||
> >            current_toolchain == "//build/toolchain/linux:x86) {
> >   # These are both the same and just use the default gcc on the system.
> >   gcc_version = exec_script(.... [ "host", ...)
> > } else {
> >   gcc_version = 0
> > }
> 
> These checks seem to base on the fact that cros:target, linux:x64, linux:x86
use
> gcc. I guess we could have linux:arm, linux:mips, or even mac using gcc.
Perhaps
> we could later indicate whether gcc is used and then further generalize the
> check?

I revise the CL based on brettw's suggestion. We can later generalize the check
beyond Chrome OS. PTAL

[brettw, 2014-10-29 16:53:38]

Yes, long term the script should take a path to some gcc (the one used by the
toolchain) and return the version of it.

LGTM for this.

[Ben Chan, 2014-10-29 17:38:22]

The CQ bit was checked by benchan@chromium.org

[commit-bot: I haz the power, 2014-10-29 17:39:58]

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

[commit-bot: I haz the power, 2014-10-29 18:29:47]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-10-29 18:30:28]

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