Componentize protobuf_lite

Componentize protobuf_lite

Several parts of chrome that use protobufs depend on the static library 'protobuf_lite' in third_party/protobuf. As more parts of chrome are pulled into their own components, we end up linking duplicate copies of protobuf_lite into each of them. This causes memory corruption errors due to protobuf's use of static variables to maintain state.

Componentizing protobuf_lite isn't as simple as changing its target type to '<(component)'. This is because there is a larger, more full-fledged version of protobuf in the target 'protobuf_full_do_not_use', which cannot be dynamically linked with the component protobuf_lite. This is because several classes declared in protobuf_lite are partially defined in protobuf_full_do_not_use, due to which we run into issues with dllexport/dllimport annotations.

The target 'protobuf_lite' is used as a dependency all over chrome, while the target 'protobuf_full_do_not_use' is used only in one place, where it is statically linked into the protobuf compiler executable 'protoc'.

This patch does the following:
1) Pulls out most of the target definitions of 'protobuf_lite' into a separate gypi file.
2) Defines protobuf export macros for all platforms in protobuf/stubs/common.h.
3) For the target 'protobuf_lite':
  - Changes the target type to '<(component)'.
  - Includes the contents of protobuf_lite.gypi.
  - Exports symbols via the LIBPROTOBUF_EXPORT macro.
  - Makes sure targets that consume it can import its symbols.
4) For the target 'protobuf_full_do_not_use':
  - Retains the target type of 'static_library'.
  - Includes the contents of protobuf_lite.gypi instead of depending on the target 'protobuf_lite'.

BUG=172800
TEST=Enable component builds and make sure all chrome targets build, and all tests run and pass.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=179806

[Raghu Simha, 2013-01-29 23:42:42]

Ryan, please review. Thanks.

[Ryan Sleevi, 2013-01-30 00:24:00]

I'm actually going to ask Dirk to take a look at this, given his involvement
with componentization. If he's not free, try Ricardo.

Given that protobuf advises against dynamic linking (
https://code.google.com/p/protobuf/source/browse/trunk/vsprojects/readme.txt ),
someone more familiar with the DLL gotchas. It "seems" like it should be fine,
given the conditions set out, but I'd rather have someone more knowledgeable
look into this.

https://codereview.chromium.org/12090058/diff/3001/third_party/protobuf/proto...
File third_party/protobuf/protobuf.gyp (right):

https://codereview.chromium.org/12090058/diff/3001/third_party/protobuf/proto...
third_party/protobuf/protobuf.gyp:158: 'protobuf_full_do_not_use.gypi',
It shouldn't be necessary to pull this into its own .gypi. I would rather keep
this in the .gyp file as part of the target.

[Dirk Pranke, 2013-01-30 00:26:04]

I will take a look, but I may not get to this until tomorrow.

[Raghu Simha, 2013-01-30 00:48:48]

Thanks Ryan. Your one comment is addressed below.

Dirk, tomorrow is fine. Thanks.

https://codereview.chromium.org/12090058/diff/3001/third_party/protobuf/proto...
File third_party/protobuf/protobuf.gyp (right):

https://codereview.chromium.org/12090058/diff/3001/third_party/protobuf/proto...
third_party/protobuf/protobuf.gyp:158: 'protobuf_full_do_not_use.gypi',
On 2013/01/30 00:24:00, Ryan Sleevi wrote:
> It shouldn't be necessary to pull this into its own .gypi. I would rather keep
> this in the .gyp file as part of the target.

Fair point. Restored.

[Raghu Simha, 2013-01-30 21:58:14]

Dirk: think you'll be able to look at this today?

[Dirk Pranke, 2013-01-31 02:15:06]

this basically looks good; a couple of comments to clarify my understanding, and
then we're probably all set.

https://codereview.chromium.org/12090058/diff/8006/third_party/protobuf/proto...
File third_party/protobuf/protobuf.gyp (right):

https://codereview.chromium.org/12090058/diff/8006/third_party/protobuf/proto...
third_party/protobuf/protobuf.gyp:1: # Copyright 2013 The Chromium Authors. All
rights reserved.
Nit: I think you're supposed to leave the copyright year alone these days?

https://codereview.chromium.org/12090058/diff/8006/third_party/protobuf/proto...
third_party/protobuf/protobuf.gyp:140: 'PROTOBUF_USE_DLLS',
See my comment in common.h

https://codereview.chromium.org/12090058/diff/8006/third_party/protobuf/src/g...
File third_party/protobuf/src/google/protobuf/stubs/common.h (right):

https://codereview.chromium.org/12090058/diff/8006/third_party/protobuf/src/g...
third_party/protobuf/src/google/protobuf/stubs/common.h:90: #if
defined(COMPONENT_BUILD) && defined(PROTOBUF_USE_DLLS)
are there cases where COMPONENT_BUILD != PROTOBUF_USE_DLLS? Can we just use the
latter and define PROTOBUF_USE_DLLs conditionally in the gyp file? or is
USE_DLLs windows-specific?

[Raghu Simha, 2013-01-31 03:14:40]

Dirk, PTAL. Thanks.

https://codereview.chromium.org/12090058/diff/8006/third_party/protobuf/proto...
File third_party/protobuf/protobuf.gyp (right):

https://codereview.chromium.org/12090058/diff/8006/third_party/protobuf/proto...
third_party/protobuf/protobuf.gyp:1: # Copyright 2013 The Chromium Authors. All
rights reserved.
On 2013/01/31 02:15:06, Dirk Pranke wrote:
> Nit: I think you're supposed to leave the copyright year alone these days?

You're right. Restored.

https://codereview.chromium.org/12090058/diff/8006/third_party/protobuf/proto...
third_party/protobuf/protobuf.gyp:140: 'PROTOBUF_USE_DLLS',
On 2013/01/31 02:15:06, Dirk Pranke wrote:
> See my comment in common.h

Likewise :)

https://codereview.chromium.org/12090058/diff/8006/third_party/protobuf/src/g...
File third_party/protobuf/src/google/protobuf/stubs/common.h (right):

https://codereview.chromium.org/12090058/diff/8006/third_party/protobuf/src/g...
third_party/protobuf/src/google/protobuf/stubs/common.h:90: #if
defined(COMPONENT_BUILD) && defined(PROTOBUF_USE_DLLS)
On 2013/01/31 02:15:06, Dirk Pranke wrote:
> are there cases where COMPONENT_BUILD != PROTOBUF_USE_DLLS? Can we just use
the
> latter and define PROTOBUF_USE_DLLs conditionally in the gyp file? or is
> USE_DLLs windows-specific?

Unfortunately, we must condition these exports on PROTOBUF_USE_DLLS and
COMPONENT_BUILD because protobuf_lite.gypi is consumed in two places -- by the
component protobuf_lite and by the static library protobuf_full_do_not_use.

- We must condition the exports on COMPONENT_BUILD for the obvious reason that
we don't want the export defs to be active when chrome is being built in static
mode.
- We must also condition them on PROTOBUF_USE_DLLS, because, while we want the
exports to be active in protobuf_lite for component builds, we do not want them
to be active in protobuf_full_do_not_use, even if component builds are enabled.
Otherwise, we end up with inconsistent dll linkage warnings while trying to
build protoc.

Obviously, it would be much simpler if protobuf's class definitions were not
split across the lite and full targets. Trying to fix that would involve
reorganizing a considerable amount of protobuf code, and I don't believe that's
necessary.

[Dirk Pranke, 2013-01-31 04:34:45]

Makes sense. lgtm.