[sync] Componentize sync: Part Final: Target 'sync' is now its own component

[sync] Componentize sync: Part Final: Target 'sync' is now its own component

One of the long term goals of the sync team has been to pull sync out of
chrome_dll and into its own component. This should result in faster link
times for component builds, and cleaner demarcation between sync code
and the rest of chrome. 

This patch does the following:

- Splits off sync.gyp into gypi files for sync_core, sync_api, sync_internal_api,
  sync_notifier and sync_proto.
- Audits the dependencies of various targets in sync.gyp, sync_tests.gyp, and other
  chrome gyp files, and makes sure all dependencies are explicitly declared.
- Makes targets declared in gyp files outside sync.gyp directly depend on 
   sync.gyp:sync instead of inner sync targets.
- Implements two versions of the target 'sync.gyp:sync':
1) In static mode, the public 'sync' target has a target type of 'none',
   and is composed of the static library targets 'sync_api', 'sync_core',
   'sync_internal_api', 'sync_notifier', and 'sync_proto'.
2) In component mode, we build the public 'sync' target into a single shared
   library, which includes the contents of sync_api.gypi, sync_core.gypi,
   sync_internal_api.gypi, sync_notifier.gypi, and sync_proto.gypi.

TBR=akalin,robertshield,thakis
BUG=136928
TEST=Set GYP_DEFINES="component=shared_library" and build the 'all' target on all platforms.

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

[Raghu Simha, 2012-11-28 02:23:27]

Guys, please review. Thanks.

Fred: Overall review.
Ryan: I'd like to avoid the dummy file hack in sync.gyp, and wanted to get your
take on how I might do so.
Tim: FYI.

[akalin, 2012-11-28 02:27:59]

On 2012/11/28 02:23:27, rsimha wrote:
> Guys, please review. Thanks.
> 
> Fred: Overall review.
> Ryan: I'd like to avoid the dummy file hack in sync.gyp, and wanted to get
your
> take on how I might do so.
> Tim: FYI.

Mmm, how does this work with the component build?  You didn't add any
SYNC_EXPORT annotations.

(I forgot which of the trybots do the component build, but you should def. try
it locally.)

[Ryan Sleevi, 2012-11-28 03:55:51]

On 2012/11/28 02:23:27, rsimha wrote:
> Guys, please review. Thanks.
> 
> Fred: Overall review.
> Ryan: I'd like to avoid the dummy file hack in sync.gyp, and wanted to get
your
> take on how I might do so.

No suggestion. Regrettably I've had to use it as well when exporting a
shared_library built from static libraries. http://crrev.com/137477 was for this
(and then subsequently, http://crrev.com/137999 ).

As mentioned on the bug you filed,
https://groups.google.com/group/gyp-developer/browse_thread/thread/d64ead9180...
has some past context.

I'm not sure if the SYNC_EXPORT will work properly, since the declspec's have no
effect when you build the static libs. I forget the details, but there's a bug
in MSVC for this (static libs with declspecs), and then for the other compilers,
various compiler/linker-specific flags to handle the visibility "re-"exporting.

The only way I know around this is to use a .DEF file to explicitly specify what
symbols need to be linked (and exported) in the DLL.

> Tim: FYI.

[Raghu Simha, 2012-11-28 23:05:56]

On 2012/11/28 02:27:59, akalin wrote:
> Mmm, how does this work with the component build?  You didn't add any
> SYNC_EXPORT annotations.
> 
> (I forgot which of the trybots do the component build, but you should def. try
> it locally.)

A local component build (with GYP_DEFINES="component=shared_library") seems to
work for me. I noticed your TODO in sync_export.h to enable SYNC_EXPORT, and
have enabled it now. But from what Ryan says below, they don't work? I'm going
to run through a few component and non component builds locally and on the
trybots to figure out if they're really needed.


On 2012/11/28 03:55:51, Ryan Sleevi wrote:
> No suggestion. Regrettably I've had to use it as well when exporting a
> shared_library built from static libraries. http://crrev.com/137477 was for
this
> (and then subsequently, http://crrev.com/137999 ).
> 
> As mentioned on the bug you filed,
>
https://groups.google.com/group/gyp-developer/browse_thread/thread/d64ead9180...
> has some past context.

I see. Thanks.
 
> I'm not sure if the SYNC_EXPORT will work properly, since the declspec's have
no
> effect when you build the static libs. I forget the details, but there's a bug
> in MSVC for this (static libs with declspecs), and then for the other
compilers,
> various compiler/linker-specific flags to handle the visibility
"re-"exporting.
> 
> The only way I know around this is to use a .DEF file to explicitly specify
what
> symbols need to be linked (and exported) in the DLL.

Interesting. It's looking like I'm able to do a local component build without a
.DEF file. I'll try this on all platforms to make sure.

[akalin, 2012-11-28 23:27:48]

On 2012/11/28 23:05:56, rsimha wrote:
> On 2012/11/28 02:27:59, akalin wrote:
> > Mmm, how does this work with the component build?  You didn't add any
> > SYNC_EXPORT annotations.
> > 
> > (I forgot which of the trybots do the component build, but you should def.
try
> > it locally.)
> 
> A local component build (with GYP_DEFINES="component=shared_library") seems to
> work for me. I noticed your TODO in sync_export.h to enable SYNC_EXPORT, and
> have enabled it now. But from what Ryan says below, they don't work? I'm going
> to run through a few component and non component builds locally and on the
> trybots to figure out if they're really needed.

Hmm, I tried building with your patch and with component=shared_library and got
the expected link errors.  Basically, if a symbol (class, function) in sync
doesn't have SYNC_EXPORT and it's used outside of sync, then that should be a
link error with the component build since the symbol is private in sync.dll.

The other problem that Ryan mentioned was that you have a component target which
depends on a bunch of static targets which contain the actual source file.  Then
SYNC_EXPORT, when used from the source files will still you're not building a
component since it's in a static lib.  Maybe this could be gotten around via
setting some defines in the various sync library targets and checking that
instead of defined(COMPONENT_BUILD)?

[Raghu Simha, 2012-12-06 06:20:39]

Fred, sorry about the delay in updating this, but I believe I've cleaned things
up and have a component build fully working locally.

I didn't need to add a def file listing the proto exports for linux and mac. I
got away with adding a couple of cc_ variables and a copy step for sync_export.h
to sync_proto.gyp. Let me know what you think.

On 2012/11/28 23:27:48, akalin wrote:
> Hmm, I tried building with your patch and with component=shared_library and
got
> the expected link errors.  Basically, if a symbol (class, function) in sync
> doesn't have SYNC_EXPORT and it's used outside of sync, then that should be a
> link error with the component build since the symbol is private in sync.dll.

You're right. I saw th

> 
> The other problem that Ryan mentioned was that you have a component target
which
> depends on a bunch of static targets which contain the actual source file. 
Then
> SYNC_EXPORT, when used from the source files will still you're not building a
> component since it's in a static lib.  Maybe this could be gotten around via
> setting some defines in the various sync library targets and checking that
> instead of defined(COMPONENT_BUILD)?

[Raghu Simha, 2012-12-06 06:23:28]

Hit send too soon. Replies inline.

On 2012/11/28 23:27:48, akalin wrote:
> Hmm, I tried building with your patch and with component=shared_library and
got
> the expected link errors.  Basically, if a symbol (class, function) in sync
> doesn't have SYNC_EXPORT and it's used outside of sync, then that should be a
> link error with the component build since the symbol is private in sync.dll.

You're right -- I noticed the link errors after blowing away my out/ directory.

> The other problem that Ryan mentioned was that you have a component target
which
> depends on a bunch of static targets which contain the actual source file. 
Then
> SYNC_EXPORT, when used from the source files will still you're not building a
> component since it's in a static lib.  Maybe this could be gotten around via
> setting some defines in the various sync library targets and checking that
> instead of defined(COMPONENT_BUILD)?

From what I can see, this isn't a problem on linux and mac. I have a windows
machine on backorder, after which I should be able to test this on Win too. I
don't notice .def files in the chromium code base corresponding to the other
FOO_EXPORT annotations for various chrome components, so I'm not sure if this is
really necessary. Let me know what you think after reviewing patch set 4.

[Ryan Sleevi, 2012-12-07 03:01:16]

On 2012/12/06 06:23:28, rsimha wrote:
> Hit send too soon. Replies inline.
> 
> On 2012/11/28 23:27:48, akalin wrote:
> > Hmm, I tried building with your patch and with component=shared_library and
> got
> > the expected link errors.  Basically, if a symbol (class, function) in sync
> > doesn't have SYNC_EXPORT and it's used outside of sync, then that should be
a
> > link error with the component build since the symbol is private in sync.dll.
> 
> You're right -- I noticed the link errors after blowing away my out/
directory.
> 
> > The other problem that Ryan mentioned was that you have a component target
> which
> > depends on a bunch of static targets which contain the actual source file. 
> Then
> > SYNC_EXPORT, when used from the source files will still you're not building
a
> > component since it's in a static lib.  Maybe this could be gotten around via
> > setting some defines in the various sync library targets and checking that
> > instead of defined(COMPONENT_BUILD)?
> 
> From what I can see, this isn't a problem on linux and mac. I have a windows
> machine on backorder, after which I should be able to test this on Win too. I
> don't notice .def files in the chromium code base corresponding to the other
> FOO_EXPORT annotations for various chrome components, so I'm not sure if this
is
> really necessary. Let me know what you think after reviewing patch set 4.

.def files are not presently used because there is no shared library that is
purely a collection of static libraries. Further, ISTR that Windows' linker does
not like "shared_library" targets (or static_library, for that matter) that have
no 'sources' - the linker complains that no object files specified.

Have you tried this on a Windows machine actually builds a shared library (eg:
the _dbg bots)?

[Ryan Sleevi, 2012-12-07 03:16:31]

I have not closely paid attention to the SYNC_EXPORTs and whether some should be
a SYNC_EXPORT_PRIVATE

I am a little nervous that MSVC may complain about dummy.cc being an empty
object/no object code being generated. Have you confirmed no errors when doing
this? If not, you may need to add a dummy function just to ensure some symbol
gets emitted.

https://codereview.chromium.org/11412211/diff/31001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/11412211/diff/31001/chrome/chrome_tests.gypi#...
chrome/chrome_tests.gypi:138: 'chrome',
BUG: You shouldn't be re-introducing this dependency. It was removed in
http://crrev.com/171373 - which makes me concerned that it may be regressing
something.

https://codereview.chromium.org/11412211/diff/31001/sync/protocol/sync_proto.gyp
File sync/protocol/sync_proto.gyp (right):

https://codereview.chromium.org/11412211/diff/31001/sync/protocol/sync_proto....
sync/protocol/sync_proto.gyp:51: 'files': ['../base/sync_export.h'],
You should not be depending on cc_dir here - that should be an implementation
detail of protoc.gypi. 

Instead:
'variables': {
  ...
  cc_include: "sync/base/protoc.gypi"
}
include_dirs: [ '../../' ]

That *SHOULD* be sufficient here.

https://codereview.chromium.org/11412211/diff/31001/sync/sync.gyp
File sync/sync.gyp (right):

https://codereview.chromium.org/11412211/diff/31001/sync/sync.gyp#newcode448
sync/sync.gyp:448: 'SYNC_IMPLEMENTATION',
This doesn't compile any sources. Why add this?

https://codereview.chromium.org/11412211/diff/31001/sync/sync.gyp#newcode451
sync/sync.gyp:451: 'protocol/sync_proto.gyp:sync_proto',
Why did you add this bit? Seems a bit overkill.

https://codereview.chromium.org/11412211/diff/31001/sync/sync.gyp#newcode467
sync/sync.gyp:467: 'dummy.cc',
So the MSVC issue I was mentioning is that MSVC will optimize out unreferenced
static initializers from static libraries that it decides are unreachable from
the shared library.

Luckily, you shouldn't be using static initializers, but I can't remember if we
got all of the protobuf ones eliminated. Just something to watch out for.

[Raghu Simha, 2012-12-07 06:08:33]

Ryan, thanks for the review. PTAL.

On 2012/12/07 03:16:31, Ryan Sleevi wrote:
> I have not closely paid attention to the SYNC_EXPORTs and whether some should
be
> a SYNC_EXPORT_PRIVATE

I first built chrome, and added SYNC_EXPORTs for all symbols that were being
consumed by other chrome components. I then built the individual test targets
one by one, and added SYNC_EXPORT_PRIVATEs for all symbols that were being
consumed by tests, but not by other chrome components. Sound right to you?

> I am a little nervous that MSVC may complain about dummy.cc being an empty
> object/no object code being generated. Have you confirmed no errors when doing
> this? If not, you may need to add a dummy function just to ensure some symbol
> gets emitted.

Keeping an eye on the latest set of trybot runs to see if there are any errors.
I also had modify third_party/protobuf/protobuf.gyp to fix a dll interface
warning on Win. Let me know what you think.

https://codereview.chromium.org/11412211/diff/31001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/11412211/diff/31001/chrome/chrome_tests.gypi#...
chrome/chrome_tests.gypi:138: 'chrome',
On 2012/12/07 03:16:31, Ryan Sleevi wrote:
> BUG: You shouldn't be re-introducing this dependency. It was removed in
> http://crrev.com/171373 - which makes me concerned that it may be regressing
> something.

I recall there being a merge conflict right about here. Pretty sure it was my
mistake. Removed.

https://codereview.chromium.org/11412211/diff/31001/sync/protocol/sync_proto.gyp
File sync/protocol/sync_proto.gyp (right):

https://codereview.chromium.org/11412211/diff/31001/sync/protocol/sync_proto....
sync/protocol/sync_proto.gyp:51: 'files': ['../base/sync_export.h'],
On 2012/12/07 03:16:31, Ryan Sleevi wrote:
> You should not be depending on cc_dir here - that should be an implementation
> detail of protoc.gypi. 
> 
> Instead:
> 'variables': {
>   ...
>   cc_include: "sync/base/protoc.gypi"
> }
> include_dirs: [ '../../' ]
> 
> That *SHOULD* be sufficient here.

Good call. Should be a lot cleaner now.

https://codereview.chromium.org/11412211/diff/31001/sync/sync.gyp
File sync/sync.gyp (right):

https://codereview.chromium.org/11412211/diff/31001/sync/sync.gyp#newcode448
sync/sync.gyp:448: 'SYNC_IMPLEMENTATION',
On 2012/12/07 03:16:31, Ryan Sleevi wrote:
> This doesn't compile any sources. Why add this?

Removed.

https://codereview.chromium.org/11412211/diff/31001/sync/sync.gyp#newcode451
sync/sync.gyp:451: 'protocol/sync_proto.gyp:sync_proto',
On 2012/12/07 03:16:31, Ryan Sleevi wrote:
> Why did you add this bit? Seems a bit overkill.

I was under the wrong impression that 'sync' needed to re-export 'sync_proto'.
You're right -- this is overkill. Removed.

https://codereview.chromium.org/11412211/diff/31001/sync/sync.gyp#newcode467
sync/sync.gyp:467: 'dummy.cc',
On 2012/12/07 03:16:31, Ryan Sleevi wrote:
> So the MSVC issue I was mentioning is that MSVC will optimize out unreferenced
> static initializers from static libraries that it decides are unreachable from
> the shared library.
> 
> Luckily, you shouldn't be using static initializers, but I can't remember if
we
> got all of the protobuf ones eliminated. Just something to watch out for.

SGTM.

[Raghu Simha, 2012-12-10 22:50:49]

After chatting with Fred, I'm splitting off the numerous SYNC_EXPORT annotations
into their own patch (https://codereview.chromium.org/11515009), and leaving the
meat of this change in here. Update to follow soon.

[Raghu Simha, 2012-12-22 02:08:13]

Fred, PTAL. I've significantly reworked this patch based on the advice on
chromium-dev, and following the example in content.gyp.

This CL can only be landed after https://codereview.chromium.org/11624037/,
which contains a bunch of exports for tests.

[Raghu Simha, 2013-01-07 12:28:39]

Fred, Tim: PTAL.

Sending this out once again, now that all the prerequisite CLs for sync
componentization have been landed. With this CL, sync is in its own component,
with a gyp file layout much like content.gyp.

In static library mode, target 'sync' merely composes its constituent static
libraries like sync_core, sync_api ,etc.. Other chrome targets continue to
depend on the constituent static libraries so that we don't have unnecessary
dependencies being introduced upstream.

In shared library mode, target 'sync' directly includes the source files and
dependencies from all the constituent gypi files, like sync_core.gypi,
sync_api.gypi, etc, and ends up as one monolithic shared library. In this mode,
we create targets for things like sync_core, sync_api, etc. that merely forward
the dependency onto the main 'sync' shared library target.

Fred, not sure if you're still available for code reviews. If so, could you
please take a fresh look?

Tim: Adding you as overall reviewer + OWNERs approver for everything under
src/sync.

Trybots seem to be happy building all targets on all platforms, in both shared
library and static library modes.

[akalin, 2013-01-07 20:14:51]

https://codereview.chromium.org/11412211/diff/130027/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/11412211/diff/130027/chrome/chrome_browser.gy...
chrome/chrome_browser.gypi:36: '../sql/sql.gyp:sql',
i assume you found a direct include of something in sql/ from browser? (just
checking)

https://codereview.chromium.org/11412211/diff/130027/chrome/chrome_browser.gy...
chrome/chrome_browser.gypi:38: '../sync/sync.gyp:sync_core',
confused -- why not depend on just sync.gyp:sync?  Will that be in a follow-up
patch?

https://codereview.chromium.org/11412211/diff/130027/chrome/chrome_browser_ex...
File chrome/chrome_browser_extensions.gypi (right):

https://codereview.chromium.org/11412211/diff/130027/chrome/chrome_browser_ex...
chrome/chrome_browser_extensions.gypi:38:
'../third_party/cacheinvalidation/cacheinvalidation.gyp:cacheinvalidation',
just curious, where's the direct dep on cacheinvalidation?

https://codereview.chromium.org/11412211/diff/130027/jingle/jingle.gyp
File jingle/jingle.gyp (right):

https://codereview.chromium.org/11412211/diff/130027/jingle/jingle.gyp#newcod...
jingle/jingle.gyp:115: 'jingle_glue',
can you list one direct dep here for posterity?

https://codereview.chromium.org/11412211/diff/130027/net/net.gyp
File net/net.gyp (left):

https://codereview.chromium.org/11412211/diff/130027/net/net.gyp#oldcode1907
net/net.gyp:1907: '../sync/protocol/sync_proto.gyp:sync_proto',
i'm pretty sure this is still needed, unless something has changed.  The trybots
may pass, but there might not be coverage.  Basically, this was needed because
the net testserver depended on sync protos for the sync server parts; without
this, the net testserver was failing because it was trying to include the proto
files even when the sync server wasn't being used.  Has that changed?

[Raghu Simha, 2013-01-08 11:27:48]

Fred, PTAL. Responses below.

https://codereview.chromium.org/11412211/diff/130027/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/11412211/diff/130027/chrome/chrome_browser.gy...
chrome/chrome_browser.gypi:36: '../sql/sql.gyp:sql',
On 2013/01/07 20:14:51, akalin wrote:
> i assume you found a direct include of something in sql/ from browser? (just
> checking)

That's right. For instance, there are numerous instances of #include
"sql/connection.h" in chrome/browser. Come to think of it, I should also add
sql.gyp:sql to export_dependent_settings. Done.

https://codereview.chromium.org/11412211/diff/130027/chrome/chrome_browser.gy...
chrome/chrome_browser.gypi:38: '../sync/sync.gyp:sync_core',
On 2013/01/07 20:14:51, akalin wrote:
> confused -- why not depend on just sync.gyp:sync?  Will that be in a follow-up
> patch?

In this specific case, you're right that it might make sense to depend on
sync.gyp:sync and not the individual targets since we depend on all of them.
However, the reason I didn't do so is because we'd then have to propagate the
entire set of dependencies upstream via export_dependent_settings, and that
would cause a major increase in the number of inherited dependencies by targets
that depend on browser. Right now, the only dependency we need to export is
sync_notifier.

Let me know if you disagree, or if you think I've understood it wrong.

https://codereview.chromium.org/11412211/diff/130027/chrome/chrome_browser_ex...
File chrome/chrome_browser_extensions.gypi (right):

https://codereview.chromium.org/11412211/diff/130027/chrome/chrome_browser_ex...
chrome/chrome_browser_extensions.gypi:38:
'../third_party/cacheinvalidation/cacheinvalidation.gyp:cacheinvalidation',
On 2013/01/07 20:14:51, akalin wrote:
> just curious, where's the direct dep on cacheinvalidation?

chrome/browser/api/push_messaging/push_message_invalidation_handler.cc includes
google/cacheinvalidation/types.pb.h.

Come to think of it, so does
chrome/browser/api/push_messaging/push_message_invalidation_handler_unittest.cc,
so I'll need to add the dependency to browser_tests too. Done.

https://codereview.chromium.org/11412211/diff/130027/jingle/jingle.gyp
File jingle/jingle.gyp (right):

https://codereview.chromium.org/11412211/diff/130027/jingle/jingle.gyp#newcod...
jingle/jingle.gyp:115: 'jingle_glue',
On 2013/01/07 20:14:51, akalin wrote:
> can you list one direct dep here for posterity?

Sure. For example, notifier/base/xmpp_connection.cc includes
jingle/glue/task_pump.h. There are several more instances.

https://codereview.chromium.org/11412211/diff/130027/net/net.gyp
File net/net.gyp (left):

https://codereview.chromium.org/11412211/diff/130027/net/net.gyp#oldcode1907
net/net.gyp:1907: '../sync/protocol/sync_proto.gyp:sync_proto',
On 2013/01/07 20:14:51, akalin wrote:
> i'm pretty sure this is still needed, unless something has changed.  The
trybots
> may pass, but there might not be coverage.  Basically, this was needed because
> the net testserver depended on sync protos for the sync server parts; without
> this, the net testserver was failing because it was trying to include the
proto
> files even when the sync server wasn't being used.  Has that changed?

You're right that this used to be the case. However, we now lazily import
chromiumsync.py in testserver.py when a sync testserver is being used. Test
suites that use a non-sync testserver remain unaffected by this change, and
suites that do use a sync testserver already depend on sync.gyp:sync_proto.

The reason I had to remove this dependency is that with component builds, the
sync_proto target had to be moved out of its own separate gyp file and into
sync.gyp, which created a cycle in the gyp dependency graph between net.gyp and
sync.gyp (gyp cycles are detected at the file level, and not at the target
level). I tried a whole bunch of gyp file hackery, but none of it was able to
remove the cycle.

In any case, it is my intent to fix crbug.com/117559 soon. Until then, the only
thing that breaks is run_testserver, and only if you were to build it without
building sync_proto, and then run it with the --sync flag. I've added a message
in run_testserver.cc to warn users of this until the bug is fixed. Let me know
if this sounds good.

[Ryan Sleevi, 2013-01-08 19:16:12]

https://codereview.chromium.org/11412211/diff/130027/net/net.gyp
File net/net.gyp (left):

https://codereview.chromium.org/11412211/diff/130027/net/net.gyp#oldcode1907
net/net.gyp:1907: '../sync/protocol/sync_proto.gyp:sync_proto',
On 2013/01/08 11:27:48, rsimha wrote:
> On 2013/01/07 20:14:51, akalin wrote:
> > i'm pretty sure this is still needed, unless something has changed.  The
> trybots
> > may pass, but there might not be coverage.  Basically, this was needed
because
> > the net testserver depended on sync protos for the sync server parts;
without
> > this, the net testserver was failing because it was trying to include the
> proto
> > files even when the sync server wasn't being used.  Has that changed?
> 
> You're right that this used to be the case. However, we now lazily import
> chromiumsync.py in testserver.py when a sync testserver is being used. Test
> suites that use a non-sync testserver remain unaffected by this change, and
> suites that do use a sync testserver already depend on sync.gyp:sync_proto.
> 
> The reason I had to remove this dependency is that with component builds, the
> sync_proto target had to be moved out of its own separate gyp file and into
> sync.gyp, which created a cycle in the gyp dependency graph between net.gyp
and
> sync.gyp (gyp cycles are detected at the file level, and not at the target
> level). I tried a whole bunch of gyp file hackery, but none of it was able to
> remove the cycle.
> 
> In any case, it is my intent to fix crbug.com/117559 soon. Until then, the
only
> thing that breaks is run_testserver, and only if you were to build it without
> building sync_proto, and then run it with the --sync flag. I've added a
message
> in run_testserver.cc to warn users of this until the bug is fixed. Let me know
> if this sounds good.

At the risk of piling on, I would prefer you fix that issue first. Such
dependency hackery is subtle for users, subtle to debug, and worse, may
intermittently cause flakes.

[Raghu Simha, 2013-01-22 08:03:44]

Fred, Ryan, Tim: PTAL.

Fred: All your comments should now be addressed.
Tim: General review, please!
Ryan: Could you take another look at the gyp changes to make sure you're
satisfied?

Thanks!

On 2013/01/08 19:16:12, Ryan Sleevi wrote:
> At the risk of piling on, I would prefer you fix that issue first. Such
> dependency hackery is subtle for users, subtle to debug, and worse, may
> intermittently cause flakes.

I'm glad you called this out. The dependency between net_test_support and
sync_proto was eliminated with r177864, after which things are a lot cleaner.
The latest patch in this CL no longer relies on the net->sync dependency, or
exposes any of its subtleties.

[Raghu Simha, 2013-01-22 09:25:41]

https://codereview.chromium.org/11412211/diff/130027/net/net.gyp
File net/net.gyp (left):

https://codereview.chromium.org/11412211/diff/130027/net/net.gyp#oldcode1907
net/net.gyp:1907: '../sync/protocol/sync_proto.gyp:sync_proto',
On 2013/01/08 11:27:48, rsimha wrote:
> You're right that this used to be the case. However, we now lazily import
> chromiumsync.py in testserver.py when a sync testserver is being used. Test
> suites that use a non-sync testserver remain unaffected by this change, and
> suites that do use a sync testserver already depend on sync.gyp:sync_proto.
> 
> The reason I had to remove this dependency is that with component builds, the
> sync_proto target had to be moved out of its own separate gyp file and into
> sync.gyp, which created a cycle in the gyp dependency graph between net.gyp
and
> sync.gyp (gyp cycles are detected at the file level, and not at the target
> level). I tried a whole bunch of gyp file hackery, but none of it was able to
> remove the cycle.
> 
> In any case, it is my intent to fix crbug.com/117559 soon. Until then, the
only
> thing that breaks is run_testserver, and only if you were to build it without
> building sync_proto, and then run it with the --sync flag. I've added a
message
> in run_testserver.cc to warn users of this until the bug is fixed. Let me know
> if this sounds good.

With http://crrev.com/177864, the above comment can be disregarded as moot.
There is no longer a dependency between net.gyp and sync.gyp.

[tim (not reviewing), 2013-01-22 23:29:58]

At a high level this patch LGTM with the nits below, but I didn't review the gyp
changes as closely as Ryan et al, so you should wait for their LGTM too.

https://codereview.chromium.org/11412211/diff/167003/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/11412211/diff/167003/chrome/chrome_browser.gy...
chrome/chrome_browser.gypi:38: '../sync/sync.gyp:sync_core',
Can you update the todo to mention that sync_core is really (I think?) the only
thing we shouldn't be depending on, long term?

https://codereview.chromium.org/11412211/diff/167003/sync/base/sync_export.h
File sync/base/sync_export.h (right):

https://codereview.chromium.org/11412211/diff/167003/sync/base/sync_export.h#...
sync/base/sync_export.h:8: #if defined(COMPONENT_BUILD)
Can we add a comment here that explains when to use SYNC_EXPORT vs
SYNC_EXPORT_PRIVATE, and what the loose relationship between those decorations
and DEPS rules are?  I think we could also have this writeup on the Sync section
of dev.chromium.

https://codereview.chromium.org/11412211/diff/167003/sync/sync_core.gypi
File sync/sync_core.gypi (right):

https://codereview.chromium.org/11412211/diff/167003/sync/sync_core.gypi#newc...
sync/sync_core.gypi:100: 'protocol/proto_enum_conversions.cc',
Should these be part of sync_proto, or not (since they're not .proto files)?

https://codereview.chromium.org/11412211/diff/167003/sync/syncable/syncable_b...
File sync/syncable/syncable_base_transaction.h (right):

https://codereview.chromium.org/11412211/diff/167003/sync/syncable/syncable_b...
sync/syncable/syncable_base_transaction.h:34: class SYNC_EXPORT BaseTransaction
{
Where do we use syncable transactions directly?  Should there be a todo here?

[Ryan Sleevi, 2013-01-23 01:05:20]

https://codereview.chromium.org/11412211/diff/167003/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/11412211/diff/167003/chrome/chrome_browser.gy...
chrome/chrome_browser.gypi:42: '../sync/sync.gyp:sync_proto',
If I understand your CL description, this should just be ../sync/sync.gyp:sync

I'm not sure why the multiple inclusions?

https://codereview.chromium.org/11412211/diff/167003/chrome/chrome_browser.gy...
chrome/chrome_browser.gypi:65: '../sync/sync.gyp:sync_notifier',
same here - why not ../sync/sync.gyp:sync

https://codereview.chromium.org/11412211/diff/167003/sync/sync.gyp
File sync/sync.gyp (right):

https://codereview.chromium.org/11412211/diff/167003/sync/sync.gyp#newcode132
sync/sync.gyp:132: 'UseLibraryDependencyInputs': "true",
This inclusion makes me think that you're trying to force symbol exposure in the
'sync' target, but given that you're using includes and a single target, it
shouldn't be needed.

Could you explain more the rationale for this?

https://codereview.chromium.org/11412211/diff/167003/sync/sync_core.gypi
File sync/sync_core.gypi (right):

https://codereview.chromium.org/11412211/diff/167003/sync/sync_core.gypi#newc...
sync/sync_core.gypi:10: '..',
style nit: You swapped the order of include_dirs and defines between
sync_api.gypi. You should keep the same ordering

https://codereview.chromium.org/11412211/diff/167003/sync/sync_tests.gypi
File sync/sync_tests.gypi (right):

https://codereview.chromium.org/11412211/diff/167003/sync/sync_tests.gypi#new...
sync/sync_tests.gypi:16: 'SYNC_IMPLEMENTATION'
What's up with the defines! in this file. Where is the SYNC_IMPLEMENTATION
define originating at?

[Raghu Simha, 2013-01-23 03:43:46]

Ryan, Tim, PTAL.

Ryan: Thanks for the useful feedback. In addition to addressing the fixes you
suggested, you'll notice that I've now changed every target outside sync.gyp to
simply depend on sync.gyp:sync, since they shouldn't have to bother with the
more fine grained inner targets. This is in keeping with the original intent of
this patch.

Tim: All your feedback is addressed, and one new bug was logged for the TODO you
suggested. Could you give it another quick glance?

https://codereview.chromium.org/11412211/diff/167003/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/11412211/diff/167003/chrome/chrome_browser.gy...
chrome/chrome_browser.gypi:38: '../sync/sync.gyp:sync_core',
On 2013/01/22 23:29:59, timsteele wrote:
> Can you update the todo to mention that sync_core is really (I think?) the
only
> thing we shouldn't be depending on, long term?

This is now moot. Targets outside sync.gyp should just depend on sync.gyp:sync
and not a list of inner targets. The real TODO here is to make sure that
sync.gyp:sync doesn't export sync_core. I've logged a bug and added a TODO in
sync.gyp.

https://codereview.chromium.org/11412211/diff/167003/chrome/chrome_browser.gy...
chrome/chrome_browser.gypi:42: '../sync/sync.gyp:sync_proto',
On 2013/01/23 01:05:20, Ryan Sleevi wrote:
> If I understand your CL description, this should just be ../sync/sync.gyp:sync
> 
> I'm not sure why the multiple inclusions?

I had tried rationalizing this in my response to Fred's comment. See
https://codereview.chromium.org/11412211/diff/130027/chrome/chrome_browser.gy....

Now that I think about it, what you and Fred suggest makes more sense than what
I've done. I've changed this list of dependencies to just 'sync' (in this and
other files).

https://codereview.chromium.org/11412211/diff/167003/chrome/chrome_browser.gy...
chrome/chrome_browser.gypi:65: '../sync/sync.gyp:sync_notifier',
On 2013/01/23 01:05:20, Ryan Sleevi wrote:
> same here - why not ../sync/sync.gyp:sync

Done.

https://codereview.chromium.org/11412211/diff/167003/sync/base/sync_export.h
File sync/base/sync_export.h (right):

https://codereview.chromium.org/11412211/diff/167003/sync/base/sync_export.h#...
sync/base/sync_export.h:8: #if defined(COMPONENT_BUILD)
On 2013/01/22 23:29:59, timsteele wrote:
> Can we add a comment here that explains when to use SYNC_EXPORT vs
> SYNC_EXPORT_PRIVATE, and what the loose relationship between those decorations
> and DEPS rules are?  I think we could also have this writeup on the Sync
section
> of dev.chromium.

Done.

https://codereview.chromium.org/11412211/diff/167003/sync/sync.gyp
File sync/sync.gyp (right):

https://codereview.chromium.org/11412211/diff/167003/sync/sync.gyp#newcode132
sync/sync.gyp:132: 'UseLibraryDependencyInputs': "true",
On 2013/01/23 01:05:20, Ryan Sleevi wrote:
> This inclusion makes me think that you're trying to force symbol exposure in
the
> 'sync' target, but given that you're using includes and a single target, it
> shouldn't be needed.
> 
> Could you explain more the rationale for this?

I added this back when I was trying to create a single sync.dll component out of
multiple static libs. You're right. Now that we have a single target with
includes, this isn't needed any longer.  Removed.

https://codereview.chromium.org/11412211/diff/167003/sync/sync_core.gypi
File sync/sync_core.gypi (right):

https://codereview.chromium.org/11412211/diff/167003/sync/sync_core.gypi#newc...
sync/sync_core.gypi:10: '..',
On 2013/01/23 01:05:20, Ryan Sleevi wrote:
> style nit: You swapped the order of include_dirs and defines between
> sync_api.gypi. You should keep the same ordering

Nice catch! Fixed.

https://codereview.chromium.org/11412211/diff/167003/sync/sync_core.gypi#newc...
sync/sync_core.gypi:100: 'protocol/proto_enum_conversions.cc',
On 2013/01/22 23:29:59, timsteele wrote:
> Should these be part of sync_proto, or not (since they're not .proto files)?

I believe they should stay here, because sync_proto.gypi includes protoc.gypi,
which contains rules for compiling .proto files into h/cc files. The rules are
applied on all files in the sources section of sync_proto.gypi. See the comments
at the top of build/protoc.gypi. Also, there is no precedent elsewhere in the
chrome codebase for including non-proto files in a *_proto.gypi file.

https://codereview.chromium.org/11412211/diff/167003/sync/sync_tests.gypi
File sync/sync_tests.gypi (right):

https://codereview.chromium.org/11412211/diff/167003/sync/sync_tests.gypi#new...
sync/sync_tests.gypi:16: 'SYNC_IMPLEMENTATION'
On 2013/01/23 01:05:20, Ryan Sleevi wrote:
> What's up with the defines! in this file. Where is the SYNC_IMPLEMENTATION
> define originating at?

I originally followed the pattern set by content.gyp, of test targets having a
defines! for their corresponding _IMPLEMENTATION define. It turns out these
aren't needed. Removed.

https://codereview.chromium.org/11412211/diff/167003/sync/syncable/syncable_b...
File sync/syncable/syncable_base_transaction.h (right):

https://codereview.chromium.org/11412211/diff/167003/sync/syncable/syncable_b...
sync/syncable/syncable_base_transaction.h:34: class SYNC_EXPORT BaseTransaction
{
On 2013/01/22 23:29:59, timsteele wrote:
> Where do we use syncable transactions directly?  Should there be a todo here?

Good catch. I don't believe we use this class directly. I think I mistakenly
exported this because of linker errors on Windows due to the non-exported base
class of syncable::{Read|Write}Transaction. The right fix is to use
NON_EXPORTED_BASE while declaring the inheritance. I've removed this export.

[Ryan Sleevi, 2013-01-23 04:06:21]

https://codereview.chromium.org/11412211/diff/159030/sync/base/sync_export.h
File sync/base/sync_export.h (right):

https://codereview.chromium.org/11412211/diff/159030/sync/base/sync_export.h#...
sync/base/sync_export.h:18: // TODO(rsimha): Include a link to a doc on sync
componentization.
Is this comment necessary? Does it deviate at all from the 'normal'
componentization process?

It seems like this is just the same old API.

https://codereview.chromium.org/11412211/diff/159030/sync/sync.gyp
File sync/sync.gyp (right):

https://codereview.chromium.org/11412211/diff/159030/sync/sync.gyp#newcode39
sync/sync.gyp:39: 'sync_proto',
Curious: Why the export_dependent_settings here?

I would expect sync_proto, in order for the protobufs to be right, but
everything else seems to be an implementation detail of sync? Is it because the
sync public API includes headers from (_notifier, _internal_api)?

What direct_dependent_settings are you relying on targets depending on sync to
pick up, beyond the protobufs (and the hard_dependency they entail)

Seems like sync_proto would suffice here, based on the rest of this file.

https://codereview.chromium.org/11412211/diff/159030/sync/sync.gyp#newcode165
sync/sync.gyp:165: },
Are these pseudo-targets still needed?

[Raghu Simha, 2013-01-23 08:04:06]

Ryan, your comments are addressed below. PTAL.

https://codereview.chromium.org/11412211/diff/159030/sync/base/sync_export.h
File sync/base/sync_export.h (right):

https://codereview.chromium.org/11412211/diff/159030/sync/base/sync_export.h#...
sync/base/sync_export.h:18: // TODO(rsimha): Include a link to a doc on sync
componentization.
On 2013/01/23 04:06:21, Ryan Sleevi wrote:
> Is this comment necessary? Does it deviate at all from the 'normal'
> componentization process?
> 
> It seems like this is just the same old API.

This is the same API as the other FOO_EXPORTs. Tim asked me to add a comment
here to help folks debug link errors. If you think this seems repetitive, I'll
let folks know offline.

https://codereview.chromium.org/11412211/diff/159030/sync/sync.gyp
File sync/sync.gyp (right):

https://codereview.chromium.org/11412211/diff/159030/sync/sync.gyp#newcode39
sync/sync.gyp:39: 'sync_proto',
On 2013/01/23 04:06:21, Ryan Sleevi wrote:
> Curious: Why the export_dependent_settings here?
> 
> I would expect sync_proto, in order for the protobufs to be right, but
> everything else seems to be an implementation detail of sync? Is it because
the
> sync public API includes headers from (_notifier, _internal_api)?
>
> What direct_dependent_settings are you relying on targets depending on sync to
> pick up, beyond the protobufs (and the hard_dependency they entail)
>
> Seems like sync_proto would suffice here, based on the rest of this file.

You are right. it turns out that the only dependencies that need to be exported
here are sync_proto and sync_notifier. (sync_notifier must be exported because
of its dependency on cacheinvalidation.gyp, which consists of the
cacheinvalidation_proto_cpp target and a custom include_path)

https://codereview.chromium.org/11412211/diff/159030/sync/sync.gyp#newcode165
sync/sync.gyp:165: },
On 2013/01/23 04:06:21, Ryan Sleevi wrote:
> Are these pseudo-targets still needed?

Yes, they are used within sync_tests.gypi, so that the test_support_* targets
can use them as dependencies.

[Ryan Sleevi, 2013-01-23 19:34:36]

All the other GYP changes look good but the one below

https://codereview.chromium.org/11412211/diff/161035/third_party/protobuf/pro...
File third_party/protobuf/protobuf.gyp (right):

https://codereview.chromium.org/11412211/diff/161035/third_party/protobuf/pro...
third_party/protobuf/protobuf.gyp:210: ],
This doesn't make sense, since on line 132, you're building protobuf_lite as a
static_library

Perhaps you meant to also update it to be type: <(component)?

And how does this affect builds for host vs target toolsets? I would somehow
expect the host target would need to be a static compile, but target dynamic?

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

Ryan, your last comment is addressed below. PTAL.

https://codereview.chromium.org/11412211/diff/161035/third_party/protobuf/pro...
File third_party/protobuf/protobuf.gyp (right):

https://codereview.chromium.org/11412211/diff/161035/third_party/protobuf/pro...
third_party/protobuf/protobuf.gyp:210: ],
On 2013/01/23 19:34:36, Ryan Sleevi wrote:
> This doesn't make sense, since on line 132, you're building protobuf_lite as a
> static_library
> 
> Perhaps you meant to also update it to be type: <(component)?
> 
> And how does this affect builds for host vs target toolsets? I would somehow
> expect the host target would need to be a static compile, but target dynamic?

Thanks for bringing this up. I took another look at this change, and it turns
out that we don't actually use any of protobuf_lite's symbols in sync/. The only
reason I had to export them was to suppress the non-dll-interface class warning
on Windows. I can just as easily do that with an entry in
msvs_disabled_warnings, and not have to worry about changing the target type of
protobuf_lite. Let me know what you think.

[Ryan Sleevi, 2013-01-23 20:44:16]

On 2013/01/23 20:23:29, rsimha wrote:
> Ryan, your last comment is addressed below. PTAL.
> 
>
https://codereview.chromium.org/11412211/diff/161035/third_party/protobuf/pro...
> File third_party/protobuf/protobuf.gyp (right):
> 
>
https://codereview.chromium.org/11412211/diff/161035/third_party/protobuf/pro...
> third_party/protobuf/protobuf.gyp:210: ],
> On 2013/01/23 19:34:36, Ryan Sleevi wrote:
> > This doesn't make sense, since on line 132, you're building protobuf_lite as
a
> > static_library
> > 
> > Perhaps you meant to also update it to be type: <(component)?
> > 
> > And how does this affect builds for host vs target toolsets? I would somehow
> > expect the host target would need to be a static compile, but target
dynamic?
> 
> Thanks for bringing this up. I took another look at this change, and it turns
> out that we don't actually use any of protobuf_lite's symbols in sync/. The
only
> reason I had to export them was to suppress the non-dll-interface class
warning
> on Windows. I can just as easily do that with an entry in
> msvs_disabled_warnings, and not have to worry about changing the target type
of
> protobuf_lite. Let me know what you think.

Please see
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/3_GPGgQpxvc/dis...

Knowing Protobuf and its lurv of statics, I'm nervous about this change, if only
because I don't know enough about Protobuf to know whether or not this is truly
a safe change. You can see that discussion on the thread in which it can cause
issues, and is the responsibility of the CL author and reviewer to know when
it's appropriate. I certainly don't have enough protobuf knowledge about its
implementation to know whether it's safe.

[Raghu Simha, 2013-01-23 20:57:23]

+Dirk, who advised me about this change in the past.

Dirk, could you please review the change I'm trying to make to protobuf.gyp?
(Between patch sets 10 and 11).

In a past CL, you had initially suggested that I also change the target type of
protobuf_lite to '<(component)', but after chatting offline, you suggested that
I not make the change. See
https://codereview.chromium.org/11316370/diff/10002/third_party/protobuf/prot...
for context.

I'm still running into linker warning-as-an-error 4275 on Windows component
builds. After reading Ryan's comment below, I'm convinced that it's not a good
idea to merely mute the linker warning 4275. What is your take on making
protobuf_lite a component?

On 2013/01/23 20:44:16, Ryan Sleevi wrote:
> Knowing Protobuf and its lurv of statics, I'm nervous about this change, if
only
> because I don't know enough about Protobuf to know whether or not this is
truly
> a safe change. You can see that discussion on the thread in which it can
cause
> issues, and is the responsibility of the CL author and reviewer to know when
> it's appropriate. I certainly don't have enough protobuf knowledge about its
> implementation to know whether it's safe.

[Raghu Simha, 2013-01-23 23:13:40]

I investigated further into changing the target type of protobuf_lite to
'<(component)', and it appears to be a bad idea. The "DLLs vs. static linking"
section in third_party/protobuf/vsprojects/readme.txt strongly discourages
building protobuf_lite into a dll, and strongly encourages building it as a
static library.

Given that the sync protos need to be part of sync.dll in component mode, the
only way I can get things to build is to suppress the non-exported base class
warning.

In the light of the above, what would you guys recommend?

[Raghu Simha, 2013-01-24 03:09:40]

+Tommy for sync/android/DEPS.

There was a deps warning due to
src/sync/android/java/src/org/chromium/sync/internal_api/pub/base/ModelType.java.
See
http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos_asan/b...

[Raghu Simha, 2013-01-24 10:55:15]

On 2013/01/23 23:13:40, rsimha wrote:
> I investigated further into changing the target type of protobuf_lite to
> '<(component)', and it appears to be a bad idea. The "DLLs vs. static linking"
> section in third_party/protobuf/vsprojects/readme.txt strongly discourages
> building protobuf_lite into a dll, and strongly encourages building it as a
> static library.
> 
> Given that the sync protos need to be part of sync.dll in component mode, the
> only way I can get things to build is to suppress the non-exported base class
> warning.
> 
> In the light of the above, what would you guys recommend?

I've tried a slightly different approach in patch set 12, which seems to
eliminate the protobuf trickiness described above. I've moved sync_proto out of
sync.dll, and changed sync.gyp so that sync_proto always gets built into a
static library. Targets that include sync proto headers will need to explicitly
declare their dependency on sync_proto. The rest of the CL remains the same, and
we don't need to touch third_party/protobuf anymore.

Please let me know what you think, and sorry about the back and forth.

[Ryan Sleevi, 2013-01-24 17:14:40]

GYP changes LGTM. The whole protobufs-in-a-DLL-interface seems quite messy, so I
think this is a fine tradeoff.

https://codereview.chromium.org/11412211/diff/166020/sync/sync.gyp
File sync/sync.gyp (right):

https://codereview.chromium.org/11412211/diff/166020/sync/sync.gyp#newcode163
sync/sync.gyp:163: },
nit: "normally" we put non-conditional targets before conditions, if only
because it makes it easier to avoid having to count braces/indents to see
whether or not the target is still part of some nested condition. eg: moving to
line 13

[Raghu Simha, 2013-01-24 19:31:35]

Thanks for the review, Ryan.

https://codereview.chromium.org/11412211/diff/166020/sync/sync.gyp
File sync/sync.gyp (right):

https://codereview.chromium.org/11412211/diff/166020/sync/sync.gyp#newcode163
sync/sync.gyp:163: },
On 2013/01/24 17:14:40, Ryan Sleevi wrote:
> nit: "normally" we put non-conditional targets before conditions, if only
> because it makes it easier to avoid having to count braces/indents to see
> whether or not the target is still part of some nested condition. eg: moving
to
> line 13

Done.

[nyquist, 2013-01-24 21:35:32]

sync/android/DEPS lgtm

[Raghu Simha, 2013-01-25 06:24:00]

+thakis@ for OWNERS approval of chrome/chrome*.gypi
+robertshield@ for OWNERS approval of chrome_frame.gyp

[Raghu Simha, 2013-01-25 07:51:31]

@timsteele: I had to exclude a handful of sync unit tests from linux component
builds because they fail and / or crash on just that specific platform. I
believe it's due to them passing strings across DLL boundaries. I've logged a
bug to investigate them, but figured it shouldn't block this CL from being
landed. Let me know what you think of patch set 15.

[Raghu Simha, 2013-01-25 21:01:20]

On 2013/01/25 07:51:31, rsimha wrote:
> @timsteele: I had to exclude a handful of sync unit tests from linux component
> builds because they fail and / or crash on just that specific platform. I
> believe it's due to them passing strings across DLL boundaries. I've logged a
> bug to investigate them, but figured it shouldn't block this CL from being
> landed. Let me know what you think of patch set 15.

Please disregard the above comment. It's not just a handful of tests that are
affected on components builds. Investigating bug 172201.

[Ryan Sleevi, 2013-01-26 04:59:26]

https://codereview.chromium.org/11412211/diff/191001/sync/sync.gyp
File sync/sync.gyp (right):

https://codereview.chromium.org/11412211/diff/191001/sync/sync.gyp#newcode136
sync/sync.gyp:136: 'export_dependent_settings': [ 'sync_proto' ],
If you're doing this, then you're saying the details of the protobuf are going
to leak out into the public interface - and that won't work for you.

It's only "safe" when you can fully make sure that sync_proto is not exported,
and all targets build. You can use the static build to test this.

[Raghu Simha, 2013-01-29 20:05:48]

The crashes were being caused by the fact that we were linking multiple
instances of protobuf_lite into various chrome components. The problem was being
exposed by sync_proto moving out of browser.

I've logged crbug.com/172800 to track the componentization of protobuf_lite. I
will update this patch once that bug is fixed.

https://codereview.chromium.org/11412211/diff/191001/sync/sync.gyp
File sync/sync.gyp (right):

https://codereview.chromium.org/11412211/diff/191001/sync/sync.gyp#newcode136
sync/sync.gyp:136: 'export_dependent_settings': [ 'sync_proto' ],
On 2013/01/26 04:59:26, Ryan Sleevi wrote:
> If you're doing this, then you're saying the details of the protobuf are going
> to leak out into the public interface - and that won't work for you.

I was able to identify protobuf_lite as the real cause of the crashes I was
seeing, I've gone ahead and moved back to the old way of 'sync' including
sync_proto.gypi in component mode. I also was able to get rid of all the dummy
targets, since all targets outside sync.gyp now just depend on 'sync'.

> It's only "safe" when you can fully make sure that sync_proto is not exported,
> and all targets build. You can use the static build to test this.

A good bit of the sync implementation relies on the fact that sync protobuf
objects can be passed between classes in sync/ (the sync engine) and
chrome/browser/sync (glue that connects sync to the rest of chrome). So we will
need to retain sync_proto in the export_dependent_settings section of 'sync' in
static mode.

[Ryan Sleevi, 2013-01-29 20:13:10]

On 2013/01/29 20:05:48, rsimha wrote:
> The crashes were being caused by the fact that we were linking multiple
> instances of protobuf_lite into various chrome components. The problem was
being
> exposed by sync_proto moving out of browser.
> 
> I've logged crbug.com/172800 to track the componentization of protobuf_lite. I
> will update this patch once that bug is fixed.
> 
> https://codereview.chromium.org/11412211/diff/191001/sync/sync.gyp
> File sync/sync.gyp (right):
> 
> https://codereview.chromium.org/11412211/diff/191001/sync/sync.gyp#newcode136
> sync/sync.gyp:136: 'export_dependent_settings': [ 'sync_proto' ],
> On 2013/01/26 04:59:26, Ryan Sleevi wrote:
> > If you're doing this, then you're saying the details of the protobuf are
going
> > to leak out into the public interface - and that won't work for you.
> 
> I was able to identify protobuf_lite as the real cause of the crashes I was
> seeing, I've gone ahead and moved back to the old way of 'sync' including
> sync_proto.gypi in component mode. I also was able to get rid of all the dummy
> targets, since all targets outside sync.gyp now just depend on 'sync'.
> 
> > It's only "safe" when you can fully make sure that sync_proto is not
exported,
> > and all targets build. You can use the static build to test this.
> 
> A good bit of the sync implementation relies on the fact that sync protobuf
> objects can be passed between classes in sync/ (the sync engine) and
> chrome/browser/sync (glue that connects sync to the rest of chrome). So we
will
> need to retain sync_proto in the export_dependent_settings section of 'sync'
in
> static mode.

Then you'll need to make sure that all of the methods on the sync_proto objects
are tagged appropriately (dllexport/dllimport), so that there's only once
instance of the constructors/destructors/etc.

Otherwise, you will either run into linker issues or this issue again. So I
suspect that, in addition to making protobuf_life a component, you'll need your
changes to make sure the sync protobufs are tagged as exported. I'm guessing
this is what you meant by "the old way" ?

[Raghu Simha, 2013-01-29 20:27:27]

On 2013/01/29 20:13:10, Ryan Sleevi wrote:
> Then you'll need to make sure that all of the methods on the sync_proto
objects
> are tagged appropriately (dllexport/dllimport), so that there's only once
> instance of the constructors/destructors/etc.
> 
> Otherwise, you will either run into linker issues or this issue again. So I
> suspect that, in addition to making protobuf_life a component, you'll need
your
> changes to make sure the sync protobufs are tagged as exported. I'm guessing
> this is what you meant by "the old way" ?

That's right. If you look at sync_proto.gypi, you'll notice that the sync
protobuf classes are already tagged as exported using "'cc_generator_options':
'dllexport_decl=SYNC_EXPORT:',".

I'm working on componentizing protobuf_lite in a separate patch since I don't
want to mix it in with this stuff. (Currently wrestling with classes that are
partially defined across protobuf_lite and protobuf_full_do_not_use).

In any case, I really appreciate your review comments. Thanks for patiently
looking at all the changes I'm sending your way :)

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

On 2013/01/29 20:27:27, rsimha wrote:
> I'm working on componentizing protobuf_lite in a separate patch since I don't
> want to mix it in with this stuff.

This was sent out for review: https://codereview.chromium.org/12090058.

[Raghu Simha, 2013-01-31 06:44:27]

Ryan, this CL is good to land, now that protobuf_lite is a component. Figured
I'd first check if you had any more comments before landing this.

Thanks.

[Raghu Simha, 2013-01-31 23:08:45]

Landing this once trybots are happy.