Rework handling of os and cpu_arch in GN.

Rework handling of os and cpu_arch in GN.

NaCl and V8 have more complicated requirements for handling architectures
than GN could previously handle. This CL reworks things to meet their
needs.

NaCl occasionally has the need to be able to override build_cpu_arch. This
comes up when they want to use a 32-bit host toolchain (i.e., build
32-bit "host" objects) on a 64-bit kernel. In addition, it is conceivable
that other projects will need this functionality on systems capable of
building for running programs for multiple architectures (any system
that can run both 32-bit and 64-bit binaries), and there is no way that
GN can always correctly guess which arch to use by default.

V8 has the need to be able to run a host toolchain that knows what the
requested target cpu_arch is; we can not use the existing "cpu_arch" for
this because that needs to be the cpu_arch of the current toolchain.
The concrete example is running a host binary on Linux x86 that needs to
get specific flags to target arm (rather than x86), for example. We could
solve this in the build configs by passing custom variables across the
toolchain, but this suggests that we should have a general solution to
track these things, which is what this CL does.

This CL introduces two new predefined variables -- target_cpu and
target_os -- and renames cpu_arch and os to to 'current_cpu' and
'current_os', and renames build_cpu_arch and build_os to
host_cpu and host_os for consistency.

current_cpu and target_cpu default to the same value as
host_cpu, and current_os and target_os default to the same value
as host_os.

Any of these variables is (and should be) overridable on the command line
or in a build config file. We want them to be overridable because (a) it's conceivable
that some projects might always want fixed values for target_os and
target_cpu_arch regardless of what platform GN is running on, and (b) we might
want to set the values based on other values (i.e., have target_cpu_arch
default to "arm" if target_os == "android").

Due to the renaming of "os" and "cpu_arch", this CL is likely to break any and
all existing project builds; projects will need to update their build configs when
rolling in the new binary.

R=brettw@chromium.org
BUG=344767
Committed: https://crrev.com/74add9fc39c276817d91ae88a79746ba686fd330
Cr-Commit-Position: refs/heads/master@{#317120}

[Dirk Pranke, 2015-02-11 02:34:32]

https://codereview.chromium.org/914873002/diff/1/tools/gn/args.cc
File tools/gn/args.cc (right):

https://codereview.chromium.org/914873002/diff/1/tools/gn/args.cc#newcode146
tools/gn/args.cc:146: 
Brett: I couldn't figure out how to make this work. Would it be better to
"undeclare" the predefined variable and "redeclare" it like it was a normal
declare_args() variable, or do some magic surgery to evaluate the value and
store it back in the original location?

https://codereview.chromium.org/914873002/diff/1/tools/gn/format_test_data/03...
File tools/gn/format_test_data/030.golden (right):

https://codereview.chromium.org/914873002/diff/1/tools/gn/format_test_data/03...
tools/gn/format_test_data/030.golden:4: 
This is an unrelated fix I discovered while trying to run gn_unittests, which I
guess we're still not running on the bots (apart from the gn_upload bots).

[scottmg, 2015-02-11 03:06:20]

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

[scottmg, 2015-02-11 03:06:21]

Sorry to bikeshed (and maybe it's too annoying to change now anyway...) but does
"host_cpu_arch" make more sense than build_cpu_arch?

I find the fact that there's 3 a bit confusing, but I think I understand why
they're all useful.

https://codereview.chromium.org/914873002/diff/1/tools/gn/format_test_data/03...
File tools/gn/format_test_data/030.golden (right):

https://codereview.chromium.org/914873002/diff/1/tools/gn/format_test_data/03...
tools/gn/format_test_data/030.golden:4: 
On 2015/02/11 02:34:31, Dirk Pranke wrote:
> This is an unrelated fix I discovered while trying to run gn_unittests, which
I
> guess we're still not running on the bots (apart from the gn_upload bots).

Weird, oops. :/

Er, no... was part of a mistaken change in a big refactor.
https://codereview.chromium.org/824263004 Yeah, we should get the tests on the
main bots.

https://codereview.chromium.org/914873002/diff/1/tools/gn/misc/vim/syntax/gn.vim
File tools/gn/misc/vim/syntax/gn.vim (right):

https://codereview.chromium.org/914873002/diff/1/tools/gn/misc/vim/syntax/gn....
tools/gn/misc/vim/syntax/gn.vim:22: syn keyword     gnPredefVar target_cpu_arch
target_gen_dir target_out_dir
Thanks for updating this!

https://codereview.chromium.org/914873002/diff/1/tools/gn/variables.cc
File tools/gn/variables.cc (right):

https://codereview.chromium.org/914873002/diff/1/tools/gn/variables.cc#newcode22
tools/gn/variables.cc:22: "  host architecture (e.g., if you can do either
32-bit or 64-bit builds.\n"
) missing here

[tfarina, 2015-02-11 10:43:22]

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

[tfarina, 2015-02-11 10:43:23]

Sorry to enter the bikeshed party, but +1!!! for 'host_cpu_arch'. I didn't
realize build_cpu_arch was for that until Scott mentioned it.

I like the symmetry of host vs target.

[tfarina, 2015-02-11 10:43:38]

tfarina@chromium.org changed reviewers:
- tfarina@chromium.org

[tfarina, 2015-02-11 13:04:48]

https://codereview.chromium.org/914873002/diff/1/tools/gn/format_test_data/03...
File tools/gn/format_test_data/030.golden (right):

https://codereview.chromium.org/914873002/diff/1/tools/gn/format_test_data/03...
tools/gn/format_test_data/030.golden:4: 
On 2015/02/11 02:34:31, Dirk Pranke wrote:
> This is an unrelated fix I discovered while trying to run gn_unittests, which
I
> guess we're still not running on the bots (apart from the gn_upload bots).

I looked into this and it was also disabled on gn_upload.

I made CL https://codereview.chromium.org/920493002 to address this.

[Dirk Pranke, 2015-02-11 22:05:21]

Actually, in this case bikeshedding is welcome. If we're going to change these
names, we shouldn't do it many times, but doing it now seems appropriate.

Brett and I discussed this in person and agreed to change things as:

build_os -> host_os
build_cpu_arch -> host_cpu

os -> current_os
cpu_arch -> current_cpu

(the last two for consistency w/ current_toolchain)

and add

target_os
target_cpu

This will require people to update their build configs. I will be doing all of
the updating of the ones I can find.

We don't expect it to be a big deal that os becomes a much longer variable name,
since people should use is_linux, is_mac, etc. instead of testing against os
directly.

[Dirk Pranke, 2015-02-11 22:05:33]

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

[Dirk Pranke, 2015-02-11 22:05:52]

+cjhopman for his thoughts/approval/paint preferences as well.

[scottmg, 2015-02-11 22:15:01]

I like those, seems clearer to me what they all mean when they're all
prefixed. cpu seems better than cpu_arch too.

One more idea, feel free to decline: maybe toolchain_os and toolchain_cpu
instead of "current"? current feels like it could be interpreted as "host"
-- i.e. what you're "currently" running on. There does seem to be a decent
amount of |cpu_arch|s so maybe length is a consideration here too.


On Wed, Feb 11, 2015 at 2:05 PM, <dpranke@chromium.org> wrote:

> Actually, in this case bikeshedding is welcome. If we're going to change
> these
> names, we shouldn't do it many times, but doing it now seems appropriate.
>
> Brett and I discussed this in person and agreed to change things as:
>
> build_os -> host_os
> build_cpu_arch -> host_cpu
>
> os -> current_os
> cpu_arch -> current_cpu
>
> (the last two for consistency w/ current_toolchain)
>
> and add
>
> target_os
> target_cpu
>
> This will require people to update their build configs. I will be doing
> all of
> the updating of the ones I can find.
>
> We don't expect it to be a big deal that os becomes a much longer variable
> name,
> since people should use is_linux, is_mac, etc. instead of testing against
> os
> directly.
>
>
>
>
> https://codereview.chromium.org/914873002/
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Dirk Pranke, 2015-02-11 22:51:11]

https://codereview.chromium.org/914873002/diff/1/tools/gn/variables.cc
File tools/gn/variables.cc (right):

https://codereview.chromium.org/914873002/diff/1/tools/gn/variables.cc#newcode22
tools/gn/variables.cc:22: "  host architecture (e.g., if you can do either
32-bit or 64-bit builds.\n"
On 2015/02/11 03:06:20, scottmg wrote:
> ) missing here

Acknowledged.

[tfarina, 2015-02-11 22:59:40]

On Wednesday, February 11, 2015, Scott Graham <scottmg@chromium.org> wrote:

> I like those, seems clearer to me what they all mean when they're all
> prefixed. cpu seems better than cpu_arch too.
>
> One more idea, feel free to decline: maybe toolchain_os and toolchain_cpu
> instead of "current"? current feels like it could be interpreted as "host"
> -- i.e. what you're "currently" running on. There does seem to be a decent
> amount of |cpu_arch|s so maybe length is a consideration here too.
>
I'm with Scott here too. I didn't like much the current_ variables. I also
thought 'current' was 'host'.


-- 
Thiago Farina

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Dirk Pranke, 2015-02-11 23:53:26]

https://codereview.chromium.org/914873002/diff/20001/tools/gn/format_test_dat...
File tools/gn/format_test_data/030.golden (right):

https://codereview.chromium.org/914873002/diff/20001/tools/gn/format_test_dat...
tools/gn/format_test_data/030.golden:4: 
This change is unrelated to the CL, but is needed to fix gn_unittests :).

[Dirk Pranke, 2015-02-12 00:01:24]

Patchset #2 (id:20001) has been deleted

[Dirk Pranke, 2015-02-12 00:01:32]

Patchset #2 (id:40001) has been deleted

[cjhopman, 2015-02-12 00:53:20]

On 2015/02/11 23:53:26, Dirk Pranke wrote:
>
https://codereview.chromium.org/914873002/diff/20001/tools/gn/format_test_dat...
> File tools/gn/format_test_data/030.golden (right):
> 
>
https://codereview.chromium.org/914873002/diff/20001/tools/gn/format_test_dat...
> tools/gn/format_test_data/030.golden:4: 
> This change is unrelated to the CL, but is needed to fix gn_unittests :).

What's the benefit of building all of this into the gn binary? Afaik, the gn
binary doesn't actually enforce how you use any of these variables. Can't we get
basically the same behavior by having gn just set defaults for host_os and
host_cpu and then defaulting the others in a declare_args block in the
BUILDCONFIG.gn file? If someone wants to do something interesting with these
variables they have to do it in their build files anyway.

[Dirk Pranke, 2015-02-12 02:02:01]

On 2015/02/12 00:53:20, cjhopman wrote:
> What's the benefit of building all of this into the gn binary?

Great question. The real answer is that we wanted all of the values to show up
in 'gn help',
and the only way to do that right now is to bake the variables into gn.

If/when we get around to implementing crbug.com/363452, I would suspect that we
would push all of this into BUILDCONFIG.gn et al.

[Dirk Pranke, 2015-02-19 00:16:58]

Brett, please take a look? I think this is ready to land now that I've worked
out the other CLs as well.

[brettw, 2015-02-19 17:51:27]

lgtm

https://codereview.chromium.org/914873002/diff/120001/tools/gn/variables.cc
File tools/gn/variables.cc (right):

https://codereview.chromium.org/914873002/diff/120001/tools/gn/variables.cc#n...
tools/gn/variables.cc:9: // Built-in variables
----------------------------------------------------------
CAn you keep the blank line below this?

https://codereview.chromium.org/914873002/diff/120001/tools/gn/variables.cc#n...
tools/gn/variables.cc:17: "  still access the host architecture when needed.\n"
delete "still"

https://codereview.chromium.org/914873002/diff/120001/tools/gn/variables.cc#n...
tools/gn/variables.cc:19: "  The value should generally be considered read-only,
but it is\n"
it is -> it can be

https://codereview.chromium.org/914873002/diff/120001/tools/gn/variables.cc#n...
tools/gn/variables.cc:22: "  you can do either 32-bit\n or 64-bit builds). The
value is not used\n"
Delete the \n after "32-bit"

https://codereview.chromium.org/914873002/diff/120001/tools/gn/variables.cc#n...
tools/gn/variables.cc:36: "  build system's settings when need be in order to
ensure that objects\n"
I'd just end this paragraph after "settings" and delete the rest.

https://codereview.chromium.org/914873002/diff/120001/tools/gn/variables.cc#n...
tools/gn/variables.cc:55: "  the primary objects of the build. In many cases,
this is the same as\n"
Add "It will match the cpu architecture of the defailt toolchain." after
"...build." and then start a new paragraph after that.

https://codereview.chromium.org/914873002/diff/120001/tools/gn/variables.cc#n...
tools/gn/variables.cc:85: "  for the primary object(s) of the build. In many
cases, this is the\n"
As above "It will match the OS of the default toolchain." and then new para.

[Dirk Pranke, 2015-02-19 19:46:49]

New patchsets have been uploaded after l-g-t-m from brettw@chromium.org

[Dirk Pranke, 2015-02-19 19:47:09]

https://codereview.chromium.org/914873002/diff/120001/tools/gn/variables.cc
File tools/gn/variables.cc (right):

https://codereview.chromium.org/914873002/diff/120001/tools/gn/variables.cc#n...
tools/gn/variables.cc:9: // Built-in variables
----------------------------------------------------------
On 2015/02/19 17:51:27, brettw wrote:
> CAn you keep the blank line below this?

Done.

https://codereview.chromium.org/914873002/diff/120001/tools/gn/variables.cc#n...
tools/gn/variables.cc:17: "  still access the host architecture when needed.\n"
On 2015/02/19 17:51:26, brettw wrote:
> delete "still"

Done.

https://codereview.chromium.org/914873002/diff/120001/tools/gn/variables.cc#n...
tools/gn/variables.cc:19: "  The value should generally be considered read-only,
but it is\n"
On 2015/02/19 17:51:26, brettw wrote:
> it is -> it can be

Done, but that required changing "overridable" to "overridden" :)

https://codereview.chromium.org/914873002/diff/120001/tools/gn/variables.cc#n...
tools/gn/variables.cc:22: "  you can do either 32-bit\n or 64-bit builds). The
value is not used\n"
On 2015/02/19 17:51:27, brettw wrote:
> Delete the \n after "32-bit"

Done.

https://codereview.chromium.org/914873002/diff/120001/tools/gn/variables.cc#n...
tools/gn/variables.cc:36: "  build system's settings when need be in order to
ensure that objects\n"
On 2015/02/19 17:51:27, brettw wrote:
> I'd just end this paragraph after "settings" and delete the rest.

Done.

https://codereview.chromium.org/914873002/diff/120001/tools/gn/variables.cc#n...
tools/gn/variables.cc:55: "  the primary objects of the build. In many cases,
this is the same as\n"
On 2015/02/19 17:51:26, brettw wrote:
> Add "It will match the cpu architecture of the defailt toolchain." after
> "...build." and then start a new paragraph after that.

Done.

https://codereview.chromium.org/914873002/diff/120001/tools/gn/variables.cc#n...
tools/gn/variables.cc:85: "  for the primary object(s) of the build. In many
cases, this is the\n"
On 2015/02/19 17:51:26, brettw wrote:
> As above "It will match the OS of the default toolchain." and then new para.

Done.

[Dirk Pranke, 2015-02-19 19:47:19]

The CQ bit was checked by dpranke@chromium.org

[commit-bot: I haz the power, 2015-02-19 19:48:43]

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

[commit-bot: I haz the power, 2015-02-19 20:21:14]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2015-02-19 20:22:01]

Patchset 6 (id:??) landed as
https://crrev.com/74add9fc39c276817d91ae88a79746ba686fd330
Cr-Commit-Position: refs/heads/master@{#317120}