make v8_use_external_startup_data overridable

make v8_use_external_startup_data overridable

This is useful for anyone building non-standard configurations.

BUG=421063
Committed: https://crrev.com/bec137502c3f948b02f5efd68c10ddd907a600ca
Cr-Commit-Position: refs/heads/master@{#308865}

[Mostyn Bramley-Moore, 2014-12-10 09:25:55]

mostynb@opera.com changed reviewers:
+ baixo@chromium.org, rmcilroy@chromium.org

[Mostyn Bramley-Moore, 2014-12-10 09:26:39]

@baixo, rmcilroy: does this tiny adjustment look OK?

[rmcilroy, 2014-12-10 10:53:49]

On 2014/12/10 09:26:39, Mostyn Bramley-Moore wrote:
> @baixo, rmcilroy: does this tiny adjustment look OK?

Sorry but I don't think that will work.  We tried this originally however
because this variable also appears in v8.gyp as "v8_use_external_startup_data% =
0" we need to override it from Chrome's gyp file, and if you add the "%" gyp
doesn't seem to override the V8 flag (it doesn't know which declaration has
precedence and chooses randomly between them.  If you can find a better way to
make this overridable then I would be happy for it to be added.

[Mostyn Bramley-Moore, 2014-12-10 11:04:46]

mostynb@opera.com changed reviewers:
+ jkummerow@chromium.org

[Mostyn Bramley-Moore, 2014-12-10 11:10:40]

On 2014/12/10 10:53:49, rmcilroy wrote:
> Sorry but I don't think that will work.  We tried this originally however
> because this variable also appears in v8.gyp as "v8_use_external_startup_data%
=
> 0" we need to override it from Chrome's gyp file, and if you add the "%" gyp
> doesn't seem to override the V8 flag (it doesn't know which declaration has
> precedence and chooses randomly between them.  If you can find a better way to
> make this overridable then I would be happy for it to be added.

Could we set the default in a .gypi file in v8, and include that .gypi file in
build/common.gypi ?

CC'ed Jakob, it looks like he added a v8_optimized_debug setting which has a
similar arrangement- defined as overridable in both chromium's build/common.gypi
and v8's build/standalone.gypi.  Does that setting have the same problem?  If
not, can we use the same solution here?

[Jakob Kummerow, 2014-12-10 11:49:27]

The fundamental constraint is that you can't have two overridable default values
for a flag at the same time. Which means there are two options:

(1) Either there's a single place where the flag's default value is configured;
typically this would be v8.gyp or (even better) V8's build/features.gypi. For
this approach to work, Chromium and standalone V8 must agree on the default
value (which can be conditional); and it also means that external GYP files
(such as in Chromium) can't see and hence can't depend on the variable. This is
generally the preferable option, because it's simple, and because it's good when
standalone V8 behaves the same as embedded V8.

(2) Or there have to be separate default definitions in files which are not
included at the same time, such as Chromium's common.gypi and V8's
standalone.gypi. This gives full flexibility, but at the cost that every
embedder (that doesn't use V8's standalone build) must explicitly provide a
definition of the flag. It looks like this is what v8_optimized_debug is
currently doing.

A compromise involving a separate .gypi file (that's included by common.gypi and
standalone.gypi) might work, however I suspect this would make it hard or
impossible to have conditional default values, because GYP can be quite picky
about variable scopes/nesting.

[rmcilroy, 2014-12-10 11:50:51]

On 2014/12/10 11:10:40, Mostyn Bramley-Moore wrote:
> On 2014/12/10 10:53:49, rmcilroy wrote:
> > Sorry but I don't think that will work.  We tried this originally however
> > because this variable also appears in v8.gyp as
"v8_use_external_startup_data%
> =
> > 0" we need to override it from Chrome's gyp file, and if you add the "%" gyp
> > doesn't seem to override the V8 flag (it doesn't know which declaration has
> > precedence and chooses randomly between them.  If you can find a better way
to
> > make this overridable then I would be happy for it to be added.
> 
> Could we set the default in a .gypi file in v8, and include that .gypi file in
> build/common.gypi ?
>
> CC'ed Jakob, it looks like he added a v8_optimized_debug setting which has a
> similar arrangement- defined as overridable in both chromium's
build/common.gypi
> and v8's build/standalone.gypi.  Does that setting have the same problem?  If
> not, can we use the same solution here?


I'm not sure we need another gypi file to be included in build/common.gypi (as
you mentioned in the first paragraph above), but following the same model as
v8_optimized_debug sounds good to me if it works and Jakob is happy with it.

[Mostyn Bramley-Moore, 2014-12-10 12:37:21]

On 2014/12/10 11:50:51, rmcilroy wrote:
> I'm not sure we need another gypi file to be included in build/common.gypi (as
> you mentioned in the first paragraph above), but following the same model as
> v8_optimized_debug sounds good to me if it works and Jakob is happy with it.

@Jakob: would you accept a patch using method (2) described above?

(Aside: I wonder if gn's scoping/ordering rules are nicer than gyp's?)

[rmcilroy, 2014-12-10 13:05:30]

On 2014/12/10 12:37:21, Mostyn Bramley-Moore wrote:
> On 2014/12/10 11:50:51, rmcilroy wrote:
> > I'm not sure we need another gypi file to be included in build/common.gypi
(as
> > you mentioned in the first paragraph above), but following the same model as
> > v8_optimized_debug sounds good to me if it works and Jakob is happy with it.
> 
> @Jakob: would you accept a patch using method (2) described above?
> 
> (Aside: I wonder if gn's scoping/ordering rules are nicer than gyp's?)

GN actually requires you to do something similar to (2) even if you don't want
to variable to be overridable.  c.f: https://codereview.chromium.org/749213004/
and https://codereview.chromium.org/753103003/

[Jakob Kummerow, 2014-12-10 13:13:45]

> @Jakob: would you accept a patch using method (2) described above?

Sure, if (1) doesn't cut the mustard (as I assume it won't), then (2) is the way
to go.

[Mostyn Bramley-Moore, 2014-12-11 13:15:08]

v8 patch: https://codereview.chromium.org/794583002/

[Mostyn Bramley-Moore, 2014-12-12 11:05:20]

On 2014/12/11 13:15:08, Mostyn Bramley-Moore wrote:
> v8 patch: https://codereview.chromium.org/794583002/

The v8 patch landed, so I guess this CL should work now?

[Mostyn Bramley-Moore, 2014-12-12 11:05:37]

(After a v8 roll, of course.)

[rmcilroy, 2014-12-12 13:05:02]

On 2014/12/12 11:05:37, Mostyn Bramley-Moore wrote:
> (After a v8 roll, of course.)

Yes I think this should work after that change has rolled.  It looks like the
roll hasn't happened yet so please wait before submitting, and if you could
check that a standard build still enables v8_use_external_startup_data after
this change that would be great, thanks.

lgtm.

[rmcilroy, 2014-12-17 17:52:07]

On 2014/12/12 13:05:02, rmcilroy wrote:
> On 2014/12/12 11:05:37, Mostyn Bramley-Moore wrote:
> > (After a v8 roll, of course.)
> 
> Yes I think this should work after that change has rolled.  It looks like the
> roll hasn't happened yet so please wait before submitting, and if you could
> check that a standard build still enables v8_use_external_startup_data after
> this change that would be great, thanks.
> 
> lgtm.

I think you can land this now.

[Mostyn Bramley-Moore, 2014-12-17 20:07:36]

The CQ bit was checked by mostynb@opera.com

[Mostyn Bramley-Moore, 2014-12-17 20:07:51]

On 2014/12/17 17:52:07, rmcilroy wrote:
> I think you can land this now.

Thanks for the heads-up.

[commit-bot: I haz the power, 2014-12-17 20:08:36]

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

[commit-bot: I haz the power, 2014-12-17 20:16:06]

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

[commit-bot: I haz the power, 2014-12-17 20:16:07]

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

[Mostyn Bramley-Moore, 2014-12-17 21:39:32]

mostynb@opera.com changed reviewers:
+ thakis@chromium.org

[Mostyn Bramley-Moore, 2014-12-17 21:40:21]

@Nico: looks like * was removed from build/OWNERS recently - can you ok this?

[Nico, 2014-12-17 21:53:57]

rs-lgtm

[Mostyn Bramley-Moore, 2014-12-17 21:57:59]

The CQ bit was checked by mostynb@opera.com

[commit-bot: I haz the power, 2014-12-17 21:59:16]

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

[commit-bot: I haz the power, 2014-12-17 22:03:03]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2014-12-17 22:03:54]

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