Re-land "Flip 'Linux x64' bot on chromium waterfall to GN."

Re-land "Flip 'Linux x64' bot on chromium waterfall to GN."

This re-lands #360510 w/ a build fix to  //mojo/shell:apptests
after #360604; It looks like this was broken when we changed
//mojo/shell/application_manager.mojo in #360604 but
for whatever reason //mojo didn't include //mojo/shell:apptests.

With this fix, 'ninja all' should work again in a GN Linux x64 build.

TBR=phajdan.jr@chromium.org, ben@chromium.org, msw@chromium.org
BUG=432959
CQ_EXTRA_TRYBOTS=tryserver.chromium.linux:linux_chromium_archive_rel_ng

Committed: https://crrev.com/9aed5d586469436b43556c2a996a506e8f028850
Cr-Commit-Position: refs/heads/master@{#361056}

[Dirk Pranke, 2015-11-19 19:31:47]

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

[commit-bot: I haz the power, 2015-11-19 19:32:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1460013003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1460013003/1

[commit-bot: I haz the power, 2015-11-19 20:30:36]

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

[commit-bot: I haz the power, 2015-11-19 20:30:36]

Dry run: Try jobs failed on following builders:
  linux_chromium_archive_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Dirk Pranke, 2015-11-20 03:05:19]

Description was changed from

==========
Re-land "Flip 'Linux x64' bot on chromium waterfall to GN."

This re-lands #360510 w/ fixes as necessary. For now we'll
start with building 'gn_all' rather than 'all' so that we can
be more likely that the change will stick, and then I'll switch
to 'all' in a follow-up CL.

TBR=phajdan.jr@chromium.org
BUG=432959
CQ_EXTRA_TRYBOTS=tryserver.chromium.linux:linux_chromium_archive_rel_ng
==========

to

==========
Re-land "Flip 'Linux x64' bot on chromium waterfall to GN."

This re-lands #360510 w/ a build fix to  //mojo/shell:apptests
after #360604; It looks like this was broken when we changed
//mojo/shell/application_manager.mojo in #360604 but
for whatever reason //mojo didn't include //mojo/shell:apptests.
    
With this fix, 'ninja all' should work again in a GN Linux x64 build, but let's
start w/ just building 'gn_all' and if that
sticks change it to 'all' in a follow-up CL.

TBR=phajdan.jr@chromium.org, ben@chromium.org, msw@chromium.org
BUG=432959
CQ_EXTRA_TRYBOTS=tryserver.chromium.linux:linux_chromium_archive_rel_ng
==========

[Dirk Pranke, 2015-11-20 03:05:36]

dpranke@chromium.org changed reviewers:
+ ben@chromium.org, msw@chromium.org

[Dirk Pranke, 2015-11-20 03:17:20]

Patchset #2 (id:20001) has been deleted

[Dirk Pranke, 2015-11-20 03:17:29]

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

[commit-bot: I haz the power, 2015-11-20 03:18:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1460013003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1460013003/40001

[Dirk Pranke, 2015-11-20 03:22:02]

Description was changed from

==========
Re-land "Flip 'Linux x64' bot on chromium waterfall to GN."

This re-lands #360510 w/ a build fix to  //mojo/shell:apptests
after #360604; It looks like this was broken when we changed
//mojo/shell/application_manager.mojo in #360604 but
for whatever reason //mojo didn't include //mojo/shell:apptests.
    
With this fix, 'ninja all' should work again in a GN Linux x64 build, but let's
start w/ just building 'gn_all' and if that
sticks change it to 'all' in a follow-up CL.

TBR=phajdan.jr@chromium.org, ben@chromium.org, msw@chromium.org
BUG=432959
CQ_EXTRA_TRYBOTS=tryserver.chromium.linux:linux_chromium_archive_rel_ng
==========

to

==========
Re-land "Flip 'Linux x64' bot on chromium waterfall to GN."

This re-lands #360510 w/ a build fix to  //mojo/shell:apptests
after #360604; It looks like this was broken when we changed
//mojo/shell/application_manager.mojo in #360604 but
for whatever reason //mojo didn't include //mojo/shell:apptests.
    
With this fix, 'ninja all' should work again in a GN Linux x64 build.

TBR=phajdan.jr@chromium.org, ben@chromium.org, msw@chromium.org
BUG=432959
CQ_EXTRA_TRYBOTS=tryserver.chromium.linux:linux_chromium_archive_rel_ng
==========

[commit-bot: I haz the power, 2015-11-20 05:20:55]

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

[commit-bot: I haz the power, 2015-11-20 05:20:56]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_TIMED_OUT, no build
URL)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)

[Paweł Hajdan Jr., 2015-11-20 14:05:19]

Stamp LGTM.

However, note that there's red result for linux_chromium_archive_rel_ng.

[msw, 2015-11-20 17:52:06]

lgtm

[Dirk Pranke, 2015-11-22 00:54:34]

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

[commit-bot: I haz the power, 2015-11-22 00:55:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1460013003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1460013003/140001

[commit-bot: I haz the power, 2015-11-22 04:51:50]

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

[commit-bot: I haz the power, 2015-11-22 04:51:51]

Dry run: This issue passed the CQ dry run.

[Dirk Pranke, 2015-11-22 23:18:04]

The CQ bit was checked by dpranke@chromium.org

[Dirk Pranke, 2015-11-22 23:18:04]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org,
phajdan.jr@chromium.org
Link to the patchset: https://codereview.chromium.org/1460013003/#ps140001
(title: "update perf_expectations again")

[commit-bot: I haz the power, 2015-11-22 23:18:20]

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

[commit-bot: I haz the power, 2015-11-22 23:22:15]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2015-11-22 23:22:58]

Patchset 7 (id:??) landed as
https://crrev.com/9aed5d586469436b43556c2a996a506e8f028850
Cr-Commit-Position: refs/heads/master@{#361056}

[Nico, 2015-12-08 16:26:03]

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

[Nico, 2015-12-08 16:26:04]

I just realized that this regressed static initializers. Why didn't this get
reverted? We don't tolerate static initializer regressions.

[Nico, 2015-12-08 16:28:39]

On 2015/12/08 16:26:04, Nico wrote:
> I just realized that this regressed static initializers. Why didn't this get
> reverted? We don't tolerate static initializer regressions.

Will reverting this CL do the right thing? If yes, please revert. If not, please
do nothing but https://code.google.com/p/chromium/issues/detail?id=559766 until
gn is back at parity.

[Dirk Pranke, 2015-12-08 17:08:19]

On 2015/12/08 16:28:39, Nico wrote:
> On 2015/12/08 16:26:04, Nico wrote:
> > I just realized that this regressed static initializers. Why didn't this get
> > reverted? We don't tolerate static initializer regressions.
> 
> Will reverting this CL do the right thing? If yes, please revert. If not,
please
> do nothing but https://code.google.com/p/chromium/issues/detail?id=559766
until
> gn is back at parity.

I suppose it depends on what you mean by "the right thing". Reverting it would
cause the Linux x64 bot to no longer build w/ GN, and since the GYP build uses
static libraries instead of source sets, the problem would go away for that bot.

However, lots of other bots including the official bots would still be building
w/ GN and still have the static initializers. Flipping all of the GN bots back
to GYP would be hard to do.

Brett (and to a less extent, I) have been working on this issue. If we landed
some
variant of these two CLs, the issue would go away by flipping the affected
targets
back to static libraries:

https://codereview.chromium.org/1466173002/
https://codereview.chromium.org/1466183002/

but we've been preferring to fix the underlying issues instead.

Given all that, which paths would you be okay with us following?

[Nico, 2015-12-08 17:19:42]

Land the hacks to fix the regression and the fix the underlying issues
async?

On Tue, Dec 8, 2015 at 12:08 PM, <dpranke@chromium.org> wrote:

> On 2015/12/08 16:28:39, Nico wrote:
>
>> On 2015/12/08 16:26:04, Nico wrote:
>> > I just realized that this regressed static initializers. Why didn't
>> this get
>> > reverted? We don't tolerate static initializer regressions.
>>
>
> Will reverting this CL do the right thing? If yes, please revert. If not,
>>
> please
>
>> do nothing but https://code.google.com/p/chromium/issues/detail?id=559766
>>
> until
>
>> gn is back at parity.
>>
>
> I suppose it depends on what you mean by "the right thing". Reverting it
> would
> cause the Linux x64 bot to no longer build w/ GN, and since the GYP build
> uses
> static libraries instead of source sets, the problem would go away for
> that bot.
>
> However, lots of other bots including the official bots would still be
> building
> w/ GN and still have the static initializers. Flipping all of the GN bots
> back
> to GYP would be hard to do.
>
> Brett (and to a less extent, I) have been working on this issue. If we
> landed
> some
> variant of these two CLs, the issue would go away by flipping the affected
> targets
> back to static libraries:
>
> https://codereview.chromium.org/1466173002/
> https://codereview.chromium.org/1466183002/
>
> but we've been preferring to fix the underlying issues instead.
>
> Given all that, which paths would you be okay with us following?
>
> https://codereview.chromium.org/1460013003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2015-12-08 17:21:24]

(Some of the initializers look very easy to fix, e.g.
ink_drop_animation_observer.cc and ink_drop_state.cc probably just need an
unused iostream include removed -- that's why I assumed that nothing at all
had happened here in the last 2 weeks)

On Tue, Dec 8, 2015 at 12:19 PM, Nico Weber <thakis@chromium.org> wrote:

> Land the hacks to fix the regression and the fix the underlying issues
> async?
>
> On Tue, Dec 8, 2015 at 12:08 PM, <dpranke@chromium.org> wrote:
>
>> On 2015/12/08 16:28:39, Nico wrote:
>>
>>> On 2015/12/08 16:26:04, Nico wrote:
>>> > I just realized that this regressed static initializers. Why didn't
>>> this get
>>> > reverted? We don't tolerate static initializer regressions.
>>>
>>
>> Will reverting this CL do the right thing? If yes, please revert. If not,
>>>
>> please
>>
>>> do nothing but
>>> https://code.google.com/p/chromium/issues/detail?id=559766
>>>
>> until
>>
>>> gn is back at parity.
>>>
>>
>> I suppose it depends on what you mean by "the right thing". Reverting it
>> would
>> cause the Linux x64 bot to no longer build w/ GN, and since the GYP build
>> uses
>> static libraries instead of source sets, the problem would go away for
>> that bot.
>>
>> However, lots of other bots including the official bots would still be
>> building
>> w/ GN and still have the static initializers. Flipping all of the GN bots
>> back
>> to GYP would be hard to do.
>>
>> Brett (and to a less extent, I) have been working on this issue. If we
>> landed
>> some
>> variant of these two CLs, the issue would go away by flipping the affected
>> targets
>> back to static libraries:
>>
>> https://codereview.chromium.org/1466173002/
>> https://codereview.chromium.org/1466183002/
>>
>> but we've been preferring to fix the underlying issues instead.
>>
>> Given all that, which paths would you be okay with us following?
>>
>> https://codereview.chromium.org/1460013003/
>>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Dirk Pranke, 2015-12-08 17:31:58]

On 2015/12/08 17:21:24, Nico wrote:
> (Some of the initializers look very easy to fix, e.g.
> ink_drop_animation_observer.cc and ink_drop_state.cc probably just need an
> unused iostream include removed -- that's why I assumed that nothing at all
> had happened here in the last 2 weeks)

Agree that they should be easy to fix. I'll see what all needs to be done today
if possible.

[brettw, 2015-12-09 17:39:39]

I've landed fixes for a number of these already, and have it on my plate to
figure out the rest. Of course, help is appreciated.

[Nico, 2015-12-09 17:45:14]

Right, but "we'll revert for this" guarantees that these things will be
actually fixed in a way that "we'll fix it after the fact" doesn't. Static
initializers are in the "we'l revert for this" category. Sadly, no perf
sheriff noticed that the gn switch regressed this for two weeks, so
reverting is kind of tricky by now.

On Wed, Dec 9, 2015 at 12:39 PM, <brettw@chromium.org> wrote:

> I've landed fixes for a number of these already, and have it on my plate to
> figure out the rest. Of course, help is appreciated.
>
> https://codereview.chromium.org/1460013003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Dirk Pranke, 2015-12-09 18:32:04]

A side note would be that flipping to GN caused no perf regressions,
so maybe this isn't that big a deal on Linux? Or maybe our tests are
too noisy or not precise enough?

I'm not arguing that we shouldn't fix this, of course.

[Nico, 2015-12-09 18:42:54]

1.) This is more pronounced on other platforms (win, android) where we
can't easily check for this, so Linux and Mac serve as a proxy for these
platforms
2.) This isn't just about perf but also about static initializers not
running in a deterministic order

The number of files with static initializers is 14 as of this morning, the
same number it was at yesterday morning. Please fix this much more urgently
(as said above, land the hack to get us back to where we were, then do
nicer fixes async)

On Wed, Dec 9, 2015 at 1:32 PM, <dpranke@chromium.org> wrote:

> A side note would be that flipping to GN caused no perf regressions,
> so maybe this isn't that big a deal on Linux? Or maybe our tests are
> too noisy or not precise enough?
>
> I'm not arguing that we shouldn't fix this, of course.
>
> https://codereview.chromium.org/1460013003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[brettw, 2015-12-10 18:48:20]

I think not reverting was the right thing for this case. I'm committed to fixing
these and have been making progress. The conversion here is a large complicated
thing and I think it means we need to be pragmatic when necessary to keep
moving.