[Sync] Add sync_export.h (Step 1 for componentizing sync)

[Sync] Add sync_export.h (Step 1 for componentizing sync)

Add SYNC_EXPORT{,_PRIVATE} annotations to stuff in
internal_api/public/{base,engine} in the 'sync' target.

Add some componentization-related TODOs.

Annotate sync-related test executables with SYNC_TEST.

BUG=136928
TEST=

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

[akalin, 2012-07-12 02:24:51]

+rlarocque for sync stuff
+rsleevi for gyp stuff

[rlarocque, 2012-07-12 17:45:56]

Shouldn't SyncManager be exported, too?  Are those symbols being exported in
some other way?

Under the current build settings, would the linker tell you if you forgot to
mark a symbol for exporting?

https://chromiumcodereview.appspot.com/10700180/diff/5001/sync/internal_api/p...
File sync/internal_api/public/base/model_type.h (right):

https://chromiumcodereview.appspot.com/10700180/diff/5001/sync/internal_api/p...
sync/internal_api/public/base/model_type.h:121: SYNC_EXPORT bool
ShouldMaintainPosition(ModelType model_type);
Does this need to be exported?  I don't think it's used by any objects that
don't belong to the sync component.

There are probably other instances of this.  This one struck me as the most
obvious, though.

[Ryan Sleevi, 2012-07-12 18:22:12]

Please use bots that build components - eg: win (not win_rel).  I'm not sure
this will actually work yet.

https://chromiumcodereview.appspot.com/10700180/diff/5001/sync/sync.gyp
File sync/sync.gyp (right):

https://chromiumcodereview.appspot.com/10700180/diff/5001/sync/sync.gyp#newco...
sync/sync.gyp:20: 'type': 'static_library',
I don't think this CL will/should work unless/until you do change this to
<(component)

When building as a shared library, COMPONENT_BUILD will be set by
build/common.gypi. As a result, all of the symbols marked SYNC_EXPORT will be
marked declexport (when building this static lib), but marked declimport by all
dependents. As such, there will be confusion between whether or not the symbol
is really within the module, and multiple (dependent) modules may end up
inadvertantly exporting the same symbols.

[akalin, 2012-07-13 00:50:47]

On 2012/07/12 17:45:56, rlarocque wrote:
> Shouldn't SyncManager be exported, too?  Are those symbols being exported in
> some other way?

Yes.  Right now I'm only doing stuff in the 'sync' target.  Updating CL
description.

> Under the current build settings, would the linker tell you if you forgot to
> mark a symbol for exporting?

No.  When I finish marking everything, then I'll flip the switch and see what
I've missed.

https://chromiumcodereview.appspot.com/10700180/diff/5001/sync/internal_api/p...
> File sync/internal_api/public/base/model_type.h (right):
> 
>
https://chromiumcodereview.appspot.com/10700180/diff/5001/sync/internal_api/p...
> sync/internal_api/public/base/model_type.h:121: SYNC_EXPORT bool
> ShouldMaintainPosition(ModelType model_type);
> Does this need to be exported?  I don't think it's used by any objects that
> don't belong to the sync component.

Yeah, I went through all the functions and grepped for them.  This is tedious,
but I think only a few files define lots of functions so it's okay.

[akalin, 2012-07-13 00:51:43]

PTAL everyone!

http://codereview.chromium.org/10700180/diff/5001/sync/sync.gyp
File sync/sync.gyp (right):

http://codereview.chromium.org/10700180/diff/5001/sync/sync.gyp#newcode20
sync/sync.gyp:20: 'type': 'static_library',
On 2012/07/12 18:22:13, Ryan Sleevi wrote:
> I don't think this CL will/should work unless/until you do change this to
> <(component)
> 
> When building as a shared library, COMPONENT_BUILD will be set by
> build/common.gypi. As a result, all of the symbols marked SYNC_EXPORT will be
> marked declexport (when building this static lib), but marked declimport by
all
> dependents. As such, there will be confusion between whether or not the symbol
> is really within the module, and multiple (dependent) modules may end up
> inadvertantly exporting the same symbols.

I added '0 &&' to sync_export.h, so it should be a no-op for now.

[Ryan Sleevi, 2012-07-13 00:55:13]

A little weird to see SYNC_TEST outside of sync.gyp, but I understand this to be
a temporary thing, right?

The #define fix, ergo also the GYP files, LGTM.

[akalin, 2012-07-13 00:59:43]

On 2012/07/13 00:55:13, Ryan Sleevi wrote:
> A little weird to see SYNC_TEST outside of sync.gyp, but I understand this to
be
> a temporary thing, right?

I don't think so.  Those test executables outside of sync are still testing sync
code, right?

> The #define fix, ergo also the GYP files, LGTM.

[rlarocque, 2012-07-13 17:42:12]

On 2012/07/13 00:50:47, akalin wrote:
> On 2012/07/12 17:45:56, rlarocque wrote:
> > Shouldn't SyncManager be exported, too?  Are those symbols being exported in
> > some other way?
> 
> Yes.  Right now I'm only doing stuff in the 'sync' target.  Updating CL
> description.
> 
> > Under the current build settings, would the linker tell you if you forgot to
> > mark a symbol for exporting?
> 
> No.  When I finish marking everything, then I'll flip the switch and see what
> I've missed.

Wouldn't it be easier to flip the switch at the same time as adding the
annotations?  I suppose there's no harm in trying to get it right the first
time, but I expect you'll find yourself adding half of them now and half of them
in another CL when you do flip the switch.  

> 
>
https://chromiumcodereview.appspot.com/10700180/diff/5001/sync/internal_api/p...
> > File sync/internal_api/public/base/model_type.h (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/10700180/diff/5001/sync/internal_api/p...
> > sync/internal_api/public/base/model_type.h:121: SYNC_EXPORT bool
> > ShouldMaintainPosition(ModelType model_type);
> > Does this need to be exported?  I don't think it's used by any objects that
> > don't belong to the sync component.
> 
> Yeah, I went through all the functions and grepped for them.  This is tedious,
> but I think only a few files define lots of functions so it's okay.

Once you make the symbols hidden by default, I think there's a way you could ask
the linker to give you a list of exported and unused symbols.  It's still a bit
of work, but easier than using grep.

This change looks like a good first step, so LGTM.  I think you should consider
merging this step and the next one into a single CL, but I'm OK with it either
way.

[Ryan Sleevi, 2012-07-13 17:49:56]

On 2012/07/13 00:59:43, akalin wrote:
> On 2012/07/13 00:55:13, Ryan Sleevi wrote:
> > A little weird to see SYNC_TEST outside of sync.gyp, but I understand this
to
> be
> > a temporary thing, right?
> 
> I don't think so.  Those test executables outside of sync are still testing
sync
> code, right?

I think this just surprises me, as normally the _TEST macros have only been for
that modules unit tests. It seems like the code in chrome/chrome_tests are part
of a different module, and thus should ('in an ideal world') only be depending
on sync's public interface (SYNC_EXPORT). 

I was assuming this was just part of the ongoing transition phase towards
modularizing sync and moving the chrome/ code to only depend on the public
interface parts (especially given your creation of /public/ folders ala content)

[akalin, 2012-07-18 20:31:04]

> I think this just surprises me, as normally the _TEST macros have only been
for
> that modules unit tests. It seems like the code in chrome/chrome_tests are
part
> of a different module, and thus should ('in an ideal world') only be depending
> on sync's public interface (SYNC_EXPORT). 
> 
> I was assuming this was just part of the ongoing transition phase towards
> modularizing sync and moving the chrome/ code to only depend on the public
> interface parts (especially given your creation of /public/ folders ala
content)

Hmm good point.  I reverted my changes to add SYNC_TEST to the chrome
unit-tests.  Will commit soon.

[akalin, 2012-07-18 20:34:15]

On 2012/07/13 17:42:12, rlarocque wrote:
> Wouldn't it be easier to flip the switch at the same time as adding the
> annotations?  I suppose there's no harm in trying to get it right the first
> time, but I expect you'll find yourself adding half of them now and half of
them
> in another CL when you do flip the switch.

Doing it all at once seems like a big change, and I suspect there are yaks
lurking about.  I suspect I'll have to add more annotations when I do flip the
switch, but hopefully it'll be less than half. (I can also keep track of
progress by flipping the switch locally.)

I also would like to try annotating what I expect should be exported (basically,
everything marked as public), and seeing what else needs to be annotated.  We
may have some hidden dependencies lurking, and fixing those may be non-trivial.

> Once you make the symbols hidden by default, I think there's a way you could
ask
> the linker to give you a list of exported and unused symbols.  It's still a
bit
> of work, but easier than using grep.
> 
> This change looks like a good first step, so LGTM.  I think you should
consider
> merging this step and the next one into a single CL, but I'm OK with it either
> way.

I'll commit this one soon.

[commit-bot: I haz the power, 2012-07-18 20:34:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/10700180/15001

[commit-bot: I haz the power, 2012-07-18 20:54:53]

Try job failure for 10700180-15001 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-07-18 22:43:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/10700180/15001