Conditionally convert V8 build overrides to declare_args.

Conditionally convert V8 build overrides to declare_args.

We're converting the build_overrides system to the new default_args list of
overrides that can be listed in the toplevel .gn file. This will allow args to
be set on a per-repo basis.

This change conditionally adds the variables currently defined in
build_overrides/v8.gni to build args. This allows V8's build to be used in both
the new and old systems. Once all Chrome and pdfium have been updated, v8's
build overrides and the conditional checks around the new args can be removed.

BUG=684096
Review-Url: https://codereview.chromium.org/2654663003
Cr-Commit-Position: refs/heads/master@{#42639}
Committed: https://chromium.googlesource.com/v8/v8/+/98dbcfde264bec28cba7cb5a6fbc13e101333b44

[brettw, 2017-01-24 00:42:18]

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

[brettw, 2017-01-24 00:42:19]

This is dependent on https://codereview.chromium.org/2648383003 landing in
Chrome first so that no defaults change when V8 is rolled into Chrome.

See the bug for an overall plan for this change.

[brettw, 2017-01-24 00:42:23]

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

[commit-bot: I haz the power, 2017-01-24 00:42:28]

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

[commit-bot: I haz the power, 2017-01-24 00:43:58]

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

[commit-bot: I haz the power, 2017-01-24 00:43:59]

Dry run: Try jobs failed on following builders:
  v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/19733)
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/21476)

[brettw, 2017-01-24 00:49:39]

Fix

[brettw, 2017-01-24 00:49:40]

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

[commit-bot: I haz the power, 2017-01-24 00:49:48]

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

[commit-bot: I haz the power, 2017-01-24 01:25:41]

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

[commit-bot: I haz the power, 2017-01-24 01:25:42]

Dry run: This issue passed the CQ dry run.

[Michael Achenbach, 2017-01-24 20:16:28]

https://codereview.chromium.org/2654663003/diff/60001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/2654663003/diff/60001/BUILD.gn#newcode96
BUILD.gn:96: v8_extra_library_files = [
So, the v8_extra_library_files and the var below are args now? They will then
also show up in the user's list of declarable args, right? Which wasn't the case
before as they were just variables that hold different values in chromium and
v8.

https://codereview.chromium.org/2654663003/diff/60001/BUILD.gn#newcode116
BUILD.gn:116: v8_enable_gdbjit = ((v8_current_cpu == "x86" || v8_current_cpu ==
"x64" ||
I think this is different now. We want this to be off in chromium. Before, we'd
run into the else case in chromium, where it is false, as the default value is
never defined in chromium. Now, I think the if case is dead code and in the else
case we switch it on in some configurations, also in chromium.

The logic before was: If the user specifies it, do what the user specified. If
not, use the default in the v8 case and switch it off in chromium (unless
chromium specified a default, which it didn't). The default is to switch it on
with the configurations that are able to use it.

[brettw, 2017-01-24 20:35:52]

https://codereview.chromium.org/2654663003/diff/60001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/2654663003/diff/60001/BUILD.gn#newcode96
BUILD.gn:96: v8_extra_library_files = [
Correct, this isn't always the greatest thing (as in this case) but it seems to
make the most sense overall in terms of configuring repositories with different
arguments.

https://codereview.chromium.org/2654663003/diff/60001/BUILD.gn#newcode116
BUILD.gn:116: v8_enable_gdbjit = ((v8_current_cpu == "x86" || v8_current_cpu ==
"x64" ||
This is what I meant to address in https://codereview.chromium.org/2648383003/
but I realized I needed to call it v8_enable_gdbjit_default to trigger the above
arm and Chrome should be in the "false" case above. I'll do an update on that
patch (https://codereview.chromium.org/2652083002/ TBR'ed to you) but I think
this patch should still be OK to go once that lands.

[Michael Achenbach, 2017-01-24 20:39:48]

On 2017/01/24 20:35:52, brettw (ping after 24h) wrote:
> https://codereview.chromium.org/2654663003/diff/60001/BUILD.gn
> File BUILD.gn (right):
> 
> https://codereview.chromium.org/2654663003/diff/60001/BUILD.gn#newcode96
> BUILD.gn:96: v8_extra_library_files = [
> Correct, this isn't always the greatest thing (as in this case) but it seems
to
> make the most sense overall in terms of configuring repositories with
different
> arguments.
> 
> https://codereview.chromium.org/2654663003/diff/60001/BUILD.gn#newcode116
> BUILD.gn:116: v8_enable_gdbjit = ((v8_current_cpu == "x86" || v8_current_cpu
==
> "x64" ||
> This is what I meant to address in https://codereview.chromium.org/2648383003/
> but I realized I needed to call it v8_enable_gdbjit_default to trigger the
above
> arm and Chrome should be in the "false" case above. I'll do an update on that
> patch (https://codereview.chromium.org/2652083002/ TBR'ed to you) but I think
> this patch should still be OK to go once that lands.

Right, with the chromium fix in, this make sense. LGTM

[brettw, 2017-01-25 00:18:26]

The CQ bit was checked by brettw@chromium.org

[commit-bot: I haz the power, 2017-01-25 00:18:34]

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

[commit-bot: I haz the power, 2017-01-25 00:20:57]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485303506871880,
"parent_rev": "e56437b630198d86a3114cba34cd181345f3a5b4", "commit_rev":
"98dbcfde264bec28cba7cb5a6fbc13e101333b44"}

[commit-bot: I haz the power, 2017-01-25 00:21:01]

Description was changed from

==========
Conditionally convert V8 build overrides to declare_args.

We're converting the build_overrides system to the new default_args list of
overrides that can be listed in the toplevel .gn file. This will allow args to
be set on a per-repo basis.

This change conditionally adds the variables currently defined in
build_overrides/v8.gni to build args. This allows V8's build to be used in both
the new and old systems. Once all Chrome and pdfium have been updated, v8's
build overrides and the conditional checks around the new args can be removed.

BUG=684096
==========

to

==========
Conditionally convert V8 build overrides to declare_args.

We're converting the build_overrides system to the new default_args list of
overrides that can be listed in the toplevel .gn file. This will allow args to
be set on a per-repo basis.

This change conditionally adds the variables currently defined in
build_overrides/v8.gni to build args. This allows V8's build to be used in both
the new and old systems. Once all Chrome and pdfium have been updated, v8's
build overrides and the conditional checks around the new args can be removed.

BUG=684096

Review-Url: https://codereview.chromium.org/2654663003
Cr-Commit-Position: refs/heads/master@{#42639}
Committed:
https://chromium.googlesource.com/v8/v8/+/98dbcfde264bec28cba7cb5a6fbc13e1013...
==========

[commit-bot: I haz the power, 2017-01-25 00:21:02]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/v8/v8/+/98dbcfde264bec28cba7cb5a6fbc13e1013...