[build] Disable incremental linking for cctest and unittests

[build] Disable incremental linking for cctest and unittests

BUG=v8:5412
Committed: https://crrev.com/a18ff08b83fc8a7c474dd111822635d2b2072ca4
Cr-Commit-Position: refs/heads/master@{#40208}

[Michael Achenbach, 2016-10-11 12:41:46]

Description was changed from

==========
[build] Disable incremental linking for cctest and unittests

BUG=
==========

to

==========
[build] Disable incremental linking for cctest and unittests

BUG=v8:5412
==========

[Michael Achenbach, 2016-10-11 12:42:29]

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

[commit-bot: I haz the power, 2016-10-11 12:42:35]

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

[Michael Achenbach, 2016-10-11 13:07:26]

machenbach@chromium.org changed reviewers:
+ jochen@chromium.org, vogelheim@chromium.org

[Michael Achenbach, 2016-10-11 13:07:26]

PTAL

[vogelheim, 2016-10-11 13:11:37]

I'm double-confused:
- Why do we need this in the first place?
- If we do need it, why the undefined-value-equals-default-value thing, instead
of declaring a top-level build variable?

https://codereview.chromium.org/2409133002/diff/1/gni/v8.gni
File gni/v8.gni (right):

https://codereview.chromium.org/2409133002/diff/1/gni/v8.gni#newcode98
gni/v8.gni:98: if (defined(no_incremental_linking) && no_incremental_linking &&
is_win) {
I thought "defined(X) && X" was an anti-pattern that we're trying to avoid. Why
is this necessary here?

[commit-bot: I haz the power, 2016-10-11 13:11:51]

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

[commit-bot: I haz the power, 2016-10-11 13:11:52]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2016-10-11 13:13:08]

I'd just put the configs += -= thing in the cctest and unittest definition - I'd
rather not have more targets use this.

[Michael Achenbach, 2016-10-11 13:17:04]

On 2016/10/11 13:13:08, jochen wrote:
> I'd just put the configs += -= thing in the cctest and unittest definition -
I'd
> rather not have more targets use this.

That does unfortunately not work with gn templates. The default configs don't
exist in that scope, so we can't do -=. I could only specify a new variable like
"remove_configs" and pass it through.

[Michael Achenbach, 2016-10-12 07:32:08]

Our windows bots are showing strange very flaky errors since switching to shared
libraries and switching off incremental linking is a shot in the dark to fix it.
There's no proof, but many chromium executables switch it off too and it doesn't
cost much link time.

I don't know how adding a global flag would help. We only need this for the two
executables cctest and unittests.

I'll make a patchset that looks more like the pattern Jochen asked for.

https://codereview.chromium.org/2409133002/diff/1/gni/v8.gni
File gni/v8.gni (right):

https://codereview.chromium.org/2409133002/diff/1/gni/v8.gni#newcode98
gni/v8.gni:98: if (defined(no_incremental_linking) && no_incremental_linking &&
is_win) {
On 2016/10/11 13:11:37, vogelheim wrote:
> I thought "defined(X) && X" was an anti-pattern that we're trying to avoid.
Why
> is this necessary here?

That's how templates seem to be designed. There's no way of providing default
values. Many places in src/build actually move most fields into additional local
variables like:
https://cs.chromium.org/chromium/src/build/config/ios/rules.gni?q=template+fi...

Then we'd get the pattern:
foo = default_foo
if (defined(invoker.foo)) {
  foo = invoker.foo
}
if (foo) ...

Not sure if that's better. That's basically the same with more boilerplate.

If we want to get around the "defined" check, we'd need to write the variable in
each of the template invocations, e.g. d8, all fuzzers, etc.

See also
https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/referen...

[Michael Achenbach, 2016-10-12 07:57:51]

How about patch 2? This is close to how it looks in chromium.

[Michael Achenbach, 2016-10-12 09:26:13]

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

[Michael Achenbach, 2016-10-12 09:26:17]

The CQ bit was unchecked by machenbach@chromium.org

[commit-bot: I haz the power, 2016-10-12 09:26:19]

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

[Michael Achenbach, 2016-10-12 09:33:14]

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

[commit-bot: I haz the power, 2016-10-12 09:33:20]

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

[Michael Achenbach, 2016-10-12 09:35:17]

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

[commit-bot: I haz the power, 2016-10-12 09:35:27]

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

[Michael Achenbach, 2016-10-12 09:35:38]

PTAL at patch 4.

[jochen (gone - plz use gerrit), 2016-10-12 09:38:07]

lgtm

[Michael Achenbach, 2016-10-12 09:46:44]

The CQ bit was unchecked by machenbach@chromium.org

[Michael Achenbach, 2016-10-12 09:46:51]

The CQ bit was checked by machenbach@chromium.org

[commit-bot: I haz the power, 2016-10-12 09:46:58]

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

[vogelheim, 2016-10-12 09:53:09]

On 2016/10/12 07:32:08, machenbach (slow) wrote:
> Our windows bots are showing strange very flaky errors since switching to
shared
> libraries and switching off incremental linking is a shot in the dark to fix
it.
> There's no proof, but many chromium executables switch it off too and it
doesn't
> cost much link time.

Thanks for the explanation. Makes sense now.

[vogelheim, 2016-10-12 09:53:14]

lgtm

[commit-bot: I haz the power, 2016-10-12 09:59:34]

Description was changed from

==========
[build] Disable incremental linking for cctest and unittests

BUG=v8:5412
==========

to

==========
[build] Disable incremental linking for cctest and unittests

BUG=v8:5412
==========

[commit-bot: I haz the power, 2016-10-12 09:59:35]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-12 10:00:09]

Description was changed from

==========
[build] Disable incremental linking for cctest and unittests

BUG=v8:5412
==========

to

==========
[build] Disable incremental linking for cctest and unittests

BUG=v8:5412

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

[commit-bot: I haz the power, 2016-10-12 10:00:11]

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