Fix the `sizes` regressions in Linux GN.

Fix the `sizes` regressions in Linux GN.

The Linux GN build introduces a bunch of static initializers into
Chrome as a result of linking a number of components as source_sets
instead of static libraries. This CL flips the relevant components
back to static libraries accordingly until we can actually get rid of
the statics.

R=thakis@chromium.org, brettw@chromium.org
BUG=559766
Committed: https://crrev.com/cbe5b1fc629ab93631e1ba069c05936576ca93e9
Cr-Commit-Position: refs/heads/master@{#364845}

[Dirk Pranke, 2015-12-09 23:12:51]

Description was changed from

==========
Hacks to fix the `sizes` regressions in Linux GN.

The Linux GN build introduces a bunch of static initializers into
Chrome as a result of linking a number of components as source_sets
instead of static libraries. This CL flips most of the components
back to static libraries accordingly.

It does not fix all of the sizes regressions; there is another in
pdfium, and one in webrtc, and a strange issue in gin that
the static library doesn't fix.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Fix the `sizes` regressions in Linux GN.

The Linux GN build introduces a bunch of static initializers into
Chrome as a result of linking a number of components as source_sets
instead of static libraries. This CL flips most of the components
back to static libraries accordingly.

R=thakis@chromium.org, brettw@chromium.org
BUG=559766
==========

[Dirk Pranke, 2015-12-09 23:12:51]

dpranke@chromium.org changed reviewers:
+ brettw@chromium.org, thakis@chromium.org

[Dirk Pranke, 2015-12-09 23:13:16]

Description was changed from

==========
Fix the `sizes` regressions in Linux GN.

The Linux GN build introduces a bunch of static initializers into
Chrome as a result of linking a number of components as source_sets
instead of static libraries. This CL flips most of the components
back to static libraries accordingly.

R=thakis@chromium.org, brettw@chromium.org
BUG=559766
==========

to

==========
Fix the `sizes` regressions in Linux GN.

The Linux GN build introduces a bunch of static initializers into
Chrome as a result of linking a number of components as source_sets
instead of static libraries. This CL flips the relevant components
back to static libraries accordingly until we can actually get rid of
the statics.

R=thakis@chromium.org, brettw@chromium.org
BUG=559766
==========

[Dirk Pranke, 2015-12-09 23:14:16]

Please take a look. I think with this plus the fix to pdfium in
https://codereview.chromium.org/1510353003/ we'll be back down to where we were
w/ GYP.

This fix can't land before the pdfium change is rolled into chromium, though, or
the step will still fail.

[Dirk Pranke, 2015-12-09 23:14:21]

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

[commit-bot: I haz the power, 2015-12-09 23:16:18]

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

[danakj, 2015-12-09 23:26:50]

https://codereview.chromium.org/1466173002/diff/20001/cc/BUILD.gn
File cc/BUILD.gn (right):

https://codereview.chromium.org/1466173002/diff/20001/cc/BUILD.gn#newcode7
cc/BUILD.gn:7: static_library("cc") {
is there a list of the static initializers that are being a problem?

[Nico, 2015-12-09 23:30:27]

On 2015/12/09 23:26:50, danakj (behind on reviews) wrote:
> https://codereview.chromium.org/1466173002/diff/20001/cc/BUILD.gn
> File cc/BUILD.gn (right):
> 
> https://codereview.chromium.org/1466173002/diff/20001/cc/BUILD.gn#newcode7
> cc/BUILD.gn:7: static_library("cc") {
> is there a list of the static initializers that are being a problem?

Thanks! Can you add todos to the targets that should move back? At least all the
components should become components again, right?

[Dirk Pranke, 2015-12-09 23:35:12]

I will add TODOs.

https://codereview.chromium.org/1466173002/diff/20001/cc/BUILD.gn
File cc/BUILD.gn (right):

https://codereview.chromium.org/1466173002/diff/20001/cc/BUILD.gn#newcode7
cc/BUILD.gn:7: static_library("cc") {
On 2015/12/09 23:26:50, danakj (behind on reviews) wrote:
> is there a list of the static initializers that are being a problem?

There's a list in crbug.com/559766 . In cc, I think they're from __sse_zero and
__sse_max_int in //cc/raster/texture_compressor_etc1_sse.cc.

[Dirk Pranke, 2015-12-09 23:49:55]

Description was changed from

==========
Fix the `sizes` regressions in Linux GN.

The Linux GN build introduces a bunch of static initializers into
Chrome as a result of linking a number of components as source_sets
instead of static libraries. This CL flips the relevant components
back to static libraries accordingly until we can actually get rid of
the statics.

R=thakis@chromium.org, brettw@chromium.org
BUG=559766
==========

to

==========
Fix the `sizes` regressions in Linux GN.

The Linux GN build introduces a bunch of static initializers into
Chrome as a result of linking a number of components as source_sets
instead of static libraries. This CL flips the relevant components
back to static libraries accordingly until we can actually get rid of
the statics.

R=thakis@chromium.org, brettw@chromium.org
BUG=559766
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Dirk Pranke, 2015-12-10 02:44:33]

The CQ bit was checked by dpranke@chromium.org

[commit-bot: I haz the power, 2015-12-10 02:45:42]

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

[commit-bot: I haz the power, 2015-12-10 02:45:43]

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

[commit-bot: I haz the power, 2015-12-10 02:45:44]

No L-G-T-M from a valid reviewer yet. Only full committers are accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Dirk Pranke, 2015-12-10 02:48:29]

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

[commit-bot: I haz the power, 2015-12-10 02:49:47]

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

[Nico, 2015-12-10 03:10:46]

Lgtm with comment.

https://codereview.chromium.org/1466173002/diff/40001/cc/BUILD.gn
File cc/BUILD.gn (right):

https://codereview.chromium.org/1466173002/diff/40001/cc/BUILD.gn#newcode9
cc/BUILD.gn:9: # there are static initializers that get accidentally linked
This one should be a component, actually

[commit-bot: I haz the power, 2015-12-10 04:41:31]

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

[commit-bot: I haz the power, 2015-12-10 04:41:32]

Dry run: Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/8...)

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

https://codereview.chromium.org/1466173002/diff/40001/cc/BUILD.gn
File cc/BUILD.gn (right):

https://codereview.chromium.org/1466173002/diff/40001/cc/BUILD.gn#newcode9
cc/BUILD.gn:9: # there are static initializers that get accidentally linked
On 2015/12/10 03:10:46, Nico wrote:
> This one should be a component, actually

components are source sets in the non-component build. I think we should not
hack around the problem with a static library for this one (and the views one
with a similar problem) and actually fix the problems.

https://codereview.chromium.org/1466173002/diff/40001/components/translate/co...
File components/translate/content/browser/BUILD.gn (right):

https://codereview.chromium.org/1466173002/diff/40001/components/translate/co...
components/translate/content/browser/BUILD.gn:8: # TODO(crbug.com/559766) this
should be a source_set but
This should be 565129 (separate bug for this).

[Dirk Pranke, 2015-12-10 18:51:01]

https://codereview.chromium.org/1466173002/diff/40001/cc/BUILD.gn
File cc/BUILD.gn (right):

https://codereview.chromium.org/1466173002/diff/40001/cc/BUILD.gn#newcode9
cc/BUILD.gn:9: # there are static initializers that get accidentally linked
On 2015/12/10 18:48:36, brettw wrote:
> On 2015/12/10 03:10:46, Nico wrote:
> > This one should be a component, actually
> 
> components are source sets in the non-component build. I think we should not
> hack around the problem with a static library for this one (and the views one
> with a similar problem) and actually fix the problems.

I think Nico was just pointing out that my comment was wrong; we would want 
to change it back to a component, not a source_set.

[Nico, 2015-12-10 18:52:04]

https://codereview.chromium.org/1466173002/diff/40001/cc/BUILD.gn
File cc/BUILD.gn (right):

https://codereview.chromium.org/1466173002/diff/40001/cc/BUILD.gn#newcode9
cc/BUILD.gn:9: # there are static initializers that get accidentally linked
On 2015/12/10 18:48:36, brettw wrote:
> On 2015/12/10 03:10:46, Nico wrote:
> > This one should be a component, actually
> 
> components are source sets in the non-component build. I think we should not
> hack around the problem with a static library for this one (and the views one
> with a similar problem) and actually fix the problems.

Sure, but you can do this aynchronously after the regression is fixed.

(yes, i meant the comment should say "this should be a component", not "this
should be a source_set")

[brettw, 2015-12-10 18:53:54]

On 2015/12/10 18:51:01, Dirk Pranke wrote:
> https://codereview.chromium.org/1466173002/diff/40001/cc/BUILD.gn
> File cc/BUILD.gn (right):
> 
> https://codereview.chromium.org/1466173002/diff/40001/cc/BUILD.gn#newcode9
> cc/BUILD.gn:9: # there are static initializers that get accidentally linked
> On 2015/12/10 18:48:36, brettw wrote:
> > On 2015/12/10 03:10:46, Nico wrote:
> > > This one should be a component, actually
> > 
> > components are source sets in the non-component build. I think we should not
> > hack around the problem with a static library for this one (and the views
one
> > with a similar problem) and actually fix the problems.
> 
> I think Nico was just pointing out that my comment was wrong; we would want 
> to change it back to a component, not a source_set.

Not LGTM, we really can't make components not components and have things work in
a reasonable way. The symbols will be exported from the wrong libraries, and I
think code will be duplicated. I'm really surprised the tries are green on the
component builds.

[Nico, 2015-12-10 18:56:23]

alternatively you could let "component" expand to "static_library" instead of
"source_set" until these are fixed (assuming it's defined in some .gn file)

[brettw, 2015-12-10 19:02:34]

On 2015/12/10 18:56:23, Nico wrote:
> alternatively you could let "component" expand to "static_library" instead of
> "source_set" until these are fixed (assuming it's defined in some .gn file)

That is a large disruptive change that seems worse to me than having two static
initializers for a few days.

[Nico, 2015-12-10 19:07:08]

On 2015/12/10 19:02:34, brettw wrote:
> On 2015/12/10 18:56:23, Nico wrote:
> > alternatively you could let "component" expand to "static_library" instead
of
> > "source_set" until these are fixed (assuming it's defined in some .gn file)
> 
> That is a large disruptive change that seems worse to me than having two
static
> initializers for a few days.

Why is it disruptive?

To reitate: Static initializer regressions _block commits_. The switch was TBR'd
for that regression. It wouldn't have gone in if anyone had realized that. The
switch landed two weeks ago, with little progress in that time.

[brettw, 2015-12-10 19:12:54]

On 2015/12/10 19:07:08, Nico wrote:
> On 2015/12/10 19:02:34, brettw wrote:
> > On 2015/12/10 18:56:23, Nico wrote:
> > > alternatively you could let "component" expand to "static_library" instead
> of
> > > "source_set" until these are fixed (assuming it's defined in some .gn
file)
> > 
> > That is a large disruptive change that seems worse to me than having two
> static
> > initializers for a few days.
> 
> Why is it disruptive?
> 
> To reitate: Static initializer regressions _block commits_. The switch was
TBR'd
> for that regression. It wouldn't have gone in if anyone had realized that. The
> switch landed two weeks ago, with little progress in that time.

As I said, I think the right decision was made for this patch. Is there
currently an ongoing problem with these? I'm not aware of anybody being blocked
from committing.

I have been making progress on the initializers and o plan to continue this. O
have a good record of finishing such things that I commit to fixing.

[Nico, 2015-12-10 19:20:17]

On 2015/12/10 19:12:54, brettw wrote:
> On 2015/12/10 19:07:08, Nico wrote:
> > On 2015/12/10 19:02:34, brettw wrote:
> > > On 2015/12/10 18:56:23, Nico wrote:
> > > > alternatively you could let "component" expand to "static_library"
instead
> > of
> > > > "source_set" until these are fixed (assuming it's defined in some .gn
> file)
> > > 
> > > That is a large disruptive change that seems worse to me than having two
> > static
> > > initializers for a few days.
> > 
> > Why is it disruptive?
> > 
> > To reitate: Static initializer regressions _block commits_. The switch was
> TBR'd
> > for that regression. It wouldn't have gone in if anyone had realized that.
The
> > switch landed two weeks ago, with little progress in that time.
> 
> As I said, I think the right decision was made for this patch. Is there
> currently an ongoing problem with these? I'm not aware of anybody being
blocked
> from committing.

Adding 5MB of bloat to chrome also don't block anyone from committing, yet it's
also not allowed.

> 
> I have been making progress on the initializers and o plan to continue this. O
> have a good record of finishing such things that I commit to fixing.

That's irrelevant. I don't doubt you'll finish this eventually.

It looks like the cc initializers have since been fixed. The only other
component->static_lib change is net. Maybe the initializer there is easy to fix.
Can we land this without the component->static_lib changes? The temporary
source_set->static_lib changes are hopefully uncontroversial?

[danakj, 2015-12-10 19:20:31]

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

[danakj, 2015-12-10 19:20:31]

https://codereview.chromium.org/1466173002/diff/40001/cc/BUILD.gn
File cc/BUILD.gn (right):

https://codereview.chromium.org/1466173002/diff/40001/cc/BUILD.gn#newcode9
cc/BUILD.gn:9: # there are static initializers that get accidentally linked
On 2015/12/10 18:52:03, Nico wrote:
> On 2015/12/10 18:48:36, brettw wrote:
> > On 2015/12/10 03:10:46, Nico wrote:
> > > This one should be a component, actually
> > 
> > components are source sets in the non-component build. I think we should not
> > hack around the problem with a static library for this one (and the views
one
> > with a similar problem) and actually fix the problems.
> 
> Sure, but you can do this aynchronously after the regression is fixed.
> 
> (yes, i meant the comment should say "this should be a component", not "this
> should be a source_set")

cc/ is fixed by https://codereview.chromium.org/1515753003/ I think?

[Dirk Pranke, 2015-12-10 19:50:42]

On 2015/12/10 19:20:31, danakj (behind on reviews) wrote:
> cc/ is fixed by https://codereview.chromium.org/1515753003/ I think?

Yup, should be.

[brettw, 2015-12-10 21:01:03]

Yes, source_set -> static library is OK workaround.

[Nico, 2015-12-11 19:04:37]

Ok, the gn build is now down to 7 files with initializers, 2 more than the gyp
build. These 2 files are:

data_file_browser_cld_data_provider.cc
vdso_support.cc

These are covered by your changes to
components/translate/content/browser/BUILD.gn and base/allocator/BUILD.gn . If
you revert the rest of this CL, I think this should be ready to go.

[Dirk Pranke, 2015-12-11 22:24:33]

Description was changed from

==========
Fix the `sizes` regressions in Linux GN.

The Linux GN build introduces a bunch of static initializers into
Chrome as a result of linking a number of components as source_sets
instead of static libraries. This CL flips the relevant components
back to static libraries accordingly until we can actually get rid of
the statics.

R=thakis@chromium.org, brettw@chromium.org
BUG=559766
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Fix the `sizes` regressions in Linux GN.

The Linux GN build introduces a bunch of static initializers into
Chrome as a result of linking a number of components as source_sets
instead of static libraries. This CL flips the relevant components
back to static libraries accordingly until we can actually get rid of
the statics.

R=thakis@chromium.org, brettw@chromium.org
BUG=559766
==========

[brettw, 2015-12-11 22:27:50]

lgtm

[Dirk Pranke, 2015-12-11 22:36:49]

The CQ bit was checked by dpranke@chromium.org

[Dirk Pranke, 2015-12-11 22:36:50]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1466173002/#ps60001
(title: "just switch //base/allocator:tcmalloc and
//components/translate/content/browser to static libs")

[commit-bot: I haz the power, 2015-12-11 22:39:01]

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

[commit-bot: I haz the power, 2015-12-12 00:38:49]

Description was changed from

==========
Fix the `sizes` regressions in Linux GN.

The Linux GN build introduces a bunch of static initializers into
Chrome as a result of linking a number of components as source_sets
instead of static libraries. This CL flips the relevant components
back to static libraries accordingly until we can actually get rid of
the statics.

R=thakis@chromium.org, brettw@chromium.org
BUG=559766
==========

to

==========
Fix the `sizes` regressions in Linux GN.

The Linux GN build introduces a bunch of static initializers into
Chrome as a result of linking a number of components as source_sets
instead of static libraries. This CL flips the relevant components
back to static libraries accordingly until we can actually get rid of
the statics.

R=thakis@chromium.org, brettw@chromium.org
BUG=559766
==========

[commit-bot: I haz the power, 2015-12-12 00:38:49]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-12-12 00:39:45]

Description was changed from

==========
Fix the `sizes` regressions in Linux GN.

The Linux GN build introduces a bunch of static initializers into
Chrome as a result of linking a number of components as source_sets
instead of static libraries. This CL flips the relevant components
back to static libraries accordingly until we can actually get rid of
the statics.

R=thakis@chromium.org, brettw@chromium.org
BUG=559766
==========

to

==========
Fix the `sizes` regressions in Linux GN.

The Linux GN build introduces a bunch of static initializers into
Chrome as a result of linking a number of components as source_sets
instead of static libraries. This CL flips the relevant components
back to static libraries accordingly until we can actually get rid of
the statics.

R=thakis@chromium.org, brettw@chromium.org
BUG=559766

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

[commit-bot: I haz the power, 2015-12-12 00:39:46]

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