Remove hand-rolled protobufs generation; enable rel paths in protoc.gypi

Remove hand-rolled protobufs generation; enable rel paths in protoc.gypi

protoc.gypi now accepts a relative path to handle the way that
protoc_out_dir is applied to build the generated path.

Remove the 'cacheinvalidation_proto_cpp' target and instead generate a
static_library to begin with. Simplify all steps by removing the actions
and including the protoc.gypi file.

BUG=113339
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=123894

[KushalP, 2012-02-13 00:15:05]

The only bad thing I see so far is that this currently produces output that
looks like the following:

  RULE cacheinvalidation_proto_genproto_0
out/Debug/pyproto//google/cacheinvalidation/v2/client_pb2.py
  RULE cacheinvalidation_proto_genproto_1
out/Debug/pyproto//google/cacheinvalidation/v2/client_gateway_pb2.py
  RULE cacheinvalidation_proto_genproto_2
out/Debug/pyproto//google/cacheinvalidation/v2/client_protocol_pb2.py
  RULE cacheinvalidation_proto_genproto_3
out/Debug/pyproto//google/cacheinvalidation/v2/client_test_internal_pb2.py
  RULE cacheinvalidation_proto_genproto_4
out/Debug/pyproto//google/cacheinvalidation/v2/types_pb2.py


  RULE sync_proto_genproto_15
out/Debug/pyproto/chrome/browser/sync/protocol/./sync_pb2.py
  RULE sync_proto_genproto_14
out/Debug/pyproto/chrome/browser/sync/protocol/./session_specifics_pb2.py
  RULE sync_proto_genproto_16
out/Debug/pyproto/chrome/browser/sync/protocol/./sync_enums_pb2.py
  RULE sync_proto_genproto_17
out/Debug/pyproto/chrome/browser/sync/protocol/./test_pb2.py

[Ryan Sleevi, 2012-02-13 19:32:12]

http://codereview.chromium.org/9378041/diff/1/build/protoc.gypi
File build/protoc.gypi (right):

http://codereview.chromium.org/9378041/diff/1/build/protoc.gypi#newcode46
build/protoc.gypi:46: 'proto_relpath%': '.',
Can you not define proto_relpath as '', and let child-implementations include
the trailing slash?

http://codereview.chromium.org/9378041/diff/1/build/protoc.gypi#newcode56
build/protoc.gypi:56: '<(py_dir)/<(proto_relpath)/<(RULE_INPUT_ROOT)_pb2.py',
So this becomes
<(py_dir)/<(proto_relpath)<(RULE_INPUT_ROOT)_pb2.py

etc

http://codereview.chromium.org/9378041/diff/1/third_party/cacheinvalidation/c...
File third_party/cacheinvalidation/cacheinvalidation.gyp (right):

http://codereview.chromium.org/9378041/diff/1/third_party/cacheinvalidation/c...
third_party/cacheinvalidation/cacheinvalidation.gyp:15: # we don't need this
variable.
akalin: Are you still planning on doing this? It may obviate the need for this
change.

http://codereview.chromium.org/9378041/diff/1/third_party/cacheinvalidation/c...
third_party/cacheinvalidation/cacheinvalidation.gyp:19: 'protoc_out_dir':
'<(SHARED_INTERMEDIATE_DIR)/protoc_out',
This variable is unused, delete it

http://codereview.chromium.org/9378041/diff/1/third_party/cacheinvalidation/c...
third_party/cacheinvalidation/cacheinvalidation.gyp:21: 'protoc':
'<(PRODUCT_DIR)/<(EXECUTABLE_PREFIX)protoc<(EXECUTABLE_SUFFIX)',
This variable is unused, delete it

http://codereview.chromium.org/9378041/diff/1/third_party/cacheinvalidation/c...
third_party/cacheinvalidation/cacheinvalidation.gyp:41: 'proto_relpath':
'<(proto_dir_relpath)',
And this becomes
'proto_relpath': '<(proto_dir_relpath)/'

[akalin, 2012-02-13 22:21:58]

sync stuff lgtm

[KushalP, 2012-02-14 11:36:07]

https://chromiumcodereview.appspot.com/9378041/diff/1/build/protoc.gypi
File build/protoc.gypi (right):

https://chromiumcodereview.appspot.com/9378041/diff/1/build/protoc.gypi#newco...
build/protoc.gypi:46: 'proto_relpath%': '.',
On 2012/02/13 19:32:12, Ryan Sleevi wrote:
> Can you not define proto_relpath as '', and let child-implementations include
> the trailing slash?

I could, but then that doesn't follow the idioms of how paths appear to be used
elsewhere: they don't require the trailing slash.

I'm fine with doing it if you feel it's the correct route to go. Just pointing
out something I've observed.

https://chromiumcodereview.appspot.com/9378041/diff/1/third_party/cacheinvali...
File third_party/cacheinvalidation/cacheinvalidation.gyp (right):

https://chromiumcodereview.appspot.com/9378041/diff/1/third_party/cacheinvali...
third_party/cacheinvalidation/cacheinvalidation.gyp:19: 'protoc_out_dir':
'<(SHARED_INTERMEDIATE_DIR)/protoc_out',
On 2012/02/13 19:32:12, Ryan Sleevi wrote:
> This variable is unused, delete it

Done.

https://chromiumcodereview.appspot.com/9378041/diff/1/third_party/cacheinvali...
third_party/cacheinvalidation/cacheinvalidation.gyp:21: 'protoc':
'<(PRODUCT_DIR)/<(EXECUTABLE_PREFIX)protoc<(EXECUTABLE_SUFFIX)',
On 2012/02/13 19:32:12, Ryan Sleevi wrote:
> This variable is unused, delete it

Done.

[akalin, 2012-02-15 01:19:56]

http://codereview.chromium.org/9378041/diff/1/third_party/cacheinvalidation/c...
File third_party/cacheinvalidation/cacheinvalidation.gyp (right):

http://codereview.chromium.org/9378041/diff/1/third_party/cacheinvalidation/c...
third_party/cacheinvalidation/cacheinvalidation.gyp:15: # we don't need this
variable.
On 2012/02/13 19:32:12, Ryan Sleevi wrote:
> akalin: Are you still planning on doing this? It may obviate the need for this
> change.

It has a low chance of happening soon. :/

[Ryan Sleevi, 2012-02-15 20:15:29]

+cc Evan Martin, in case he feels strongly about this / has any objections.

https://chromiumcodereview.appspot.com/9378041/diff/1/build/protoc.gypi
File build/protoc.gypi (right):

https://chromiumcodereview.appspot.com/9378041/diff/1/build/protoc.gypi#newco...
build/protoc.gypi:46: 'proto_relpath%': '.',
On 2012/02/14 11:36:07, KushalP wrote:
> On 2012/02/13 19:32:12, Ryan Sleevi wrote:
> > Can you not define proto_relpath as '', and let child-implementations
include
> > the trailing slash?
> 
> I could, but then that doesn't follow the idioms of how paths appear to be
used
> elsewhere: they don't require the trailing slash.
> 
> I'm fine with doing it if you feel it's the correct route to go. Just pointing
> out something I've observed.

You should be able to make this at least '' rather than '.'

The only reason proto_in_dir is '.' is because it acts as the leading component
for some variables (namely, the call to protoc), so a '/foo' would be bad.
Having a variable expand to './/' should work no different than '././', although
I don't feel strongly about it one way or the other.

https://chromiumcodereview.appspot.com/9378041/diff/7001/build/protoc.gypi
File build/protoc.gypi (right):

https://chromiumcodereview.appspot.com/9378041/diff/7001/build/protoc.gypi#ne...
build/protoc.gypi:46: 'proto_relpath%': '.',
Can you please add examples to the documentation about why this is necessary?

Note, you do not need to specifically reference cacheinvalidation, just the
pattern it uses and why this may be necessary.

https://chromiumcodereview.appspot.com/9378041/diff/7001/third_party/cacheinv...
File third_party/cacheinvalidation/cacheinvalidation.gyp (right):

https://chromiumcodereview.appspot.com/9378041/diff/7001/third_party/cacheinv...
third_party/cacheinvalidation/cacheinvalidation.gyp:35: #
out/Debug/pyproto/<(proto_out_dir)/<(proto_relpath)/file_pb2.py
I don't feel this comment is accurate for what it states.

Setting the variable doesn't change that behaviour - it always happens. It's
because the protos use fully qualified imports that this is necessary.

# Set proto_out_dir to '', because protoc will place generated files in
# subdirectories based on the relative path between proto_in_dir and
# the source. Because all the input files' relative paths start with
# <(proto_dir_relpath), the output files will effectively be placed in
# <(proto_out_dir)/<(proto_relpath)/file_pb2.py, which is the desired
# naming. This is only necessary because of how these protos import
# eachother. See protoc.gypi for more explanation.

Although feel free to wordsmith the above as appropriate, I'm trying to explain
1) Why this particular target needs to do this
2) Why it's not generally needed.
3) Explain the non-obvious surprises
4) Where to go for more information about the general pattern

[Evan Martin, 2012-02-15 20:19:51]

https://chromiumcodereview.appspot.com/9378041/diff/7001/build/protoc.gypi
File build/protoc.gypi (right):

https://chromiumcodereview.appspot.com/9378041/diff/7001/build/protoc.gypi#ne...
build/protoc.gypi:46: 'proto_relpath%': '.',
If you define this as "must have a trailing slash if provided", then you can
remove all of the "./" that show up in paths.

[KushalP, 2012-02-16 09:07:39]

Pushed: Patch Set 3 accounts for the various nits discussed previously.

Would be really interested to know what you think about the updated comment in
cacheinvalidation.gyp.

[Ryan Sleevi, 2012-02-16 09:23:49]

I think we're getting closer.

I know I'm giving a lot of push back on the comments, but since there's been
some surprising subtlety here, as we've discussed at length on IRC, I want to
make sure that it's clear for those who come after (or for those adopting
Chromium's GYP files for their own projects, which happens very regularly), why
things are the way they are and when the correct time to apply them is.

Note, you've still left what I think is the most important thing unaddressed,
which I've highlighted below. Please make sure to check all feedback before the
next draft, but also feel free to ask any questions or give push back if there's
something you disagree with.

http://codereview.chromium.org/9378041/diff/7001/build/protoc.gypi
File build/protoc.gypi (right):

http://codereview.chromium.org/9378041/diff/7001/build/protoc.gypi#newcode46
build/protoc.gypi:46: 'proto_relpath%': '.',
On 2012/02/15 20:15:29, Ryan Sleevi wrote:
> Can you please add examples to the documentation about why this is necessary?
> 
> Note, you do not need to specifically reference cacheinvalidation, just the
> pattern it uses and why this may be necessary.

This comment had not been addressed. The explanation of what "proto_relpath"
means and when and why it is necessary belongs in protoc.gypi, not
cacheinvalidation.gyp. Please see lines 5-38, which specifically explain each of
the (client-definable) variables and when and how they are to be used.

http://codereview.chromium.org/9378041/diff/11002/third_party/cacheinvalidati...
File third_party/cacheinvalidation/cacheinvalidation.gyp (right):

http://codereview.chromium.org/9378041/diff/11002/third_party/cacheinvalidati...
third_party/cacheinvalidation/cacheinvalidation.gyp:44: # eachother. See
protoc.gypi for more explanation.
There is no more explanation at this time.

Consider restructuring the comment a little more:

'proto_in_dir': '...'
# This is necessary because of these protos import with
# qualified paths, such as
# #import "google/cacheinvalidation/v2/client_protocol.proto"
# rather than the more common form of
# #import "client_protocol.proto"
# NOTE: The trailing slash is required, see protoc.gypi
'proto_relpath': '.../',
# Set proto_out_dir ...
# ...
'proto_out_dir': '',

The reason I suggest this ordering is so that you always encounter the GYP
variable BEFORE the comment block that refers to it (eg: the comment mentioning
proto_relpath appears AFTER proto_relpath has been defined to
<(proto_dir_relpath)/

[KushalP, 2012-02-17 00:10:56]

Pushed: Patch Set 4.

I've added more comments to protoc.gypi. I thought it would be useful to start
off by stating how files are generated before going straight into why
proto_relpath exists.

[Ryan Sleevi, 2012-02-17 00:31:21]

http://codereview.chromium.org/9378041/diff/16003/build/protoc.gypi
File build/protoc.gypi (right):

http://codereview.chromium.org/9378041/diff/16003/build/protoc.gypi#newcode47
build/protoc.gypi:47: # protoc.gypi will place generated files into
subdirectories based on the
As per my previous comment, this should be placed in the above block (Lines 8 ->
39). In particular, this comment now directly contradicts with Line 36-38, which
is why I wanted to call them out.

Additionally, similar to general layering goals, it is better if you can provide
an example in this file (in the comments) of how/why a user would use it, rather
than referring to a path.

For example, if this changes in third_party/cacheinvalidation, there's no
guarantee someone will remember to update this comment, nor will it be a good
starting point for others.

You can continue using the "foo/bar" examples (given in lines 37-38) to show the
difference.

http://codereview.chromium.org/9378041/diff/16003/build/protoc.gypi#newcode60
build/protoc.gypi:60: # it will be concatenated with RULE_INPUT_ROOT in
'outputs' and
You don't need the "otherwise" - it's an error to omit the trailing slash, no
need to explain why, that's an implementation detail ;-)

[KushalP, 2012-02-20 18:48:33]

Pushed Patch Set 6.

[Ryan Sleevi, 2012-02-22 05:27:29]

Thanks Kushal.

LGTM, with one comment to fix below.

http://codereview.chromium.org/9378041/diff/19001/third_party/cacheinvalidati...
File third_party/cacheinvalidation/cacheinvalidation.gyp (right):

http://codereview.chromium.org/9378041/diff/19001/third_party/cacheinvalidati...
third_party/cacheinvalidation/cacheinvalidation.gyp:130:
'../../base/base.gyp:base',
You should export_dependent_settings on cacheinvalidation_proto here, as per the
comment on line 120/121.

By doing so, you can actually delete lines 120-122 for better build parallelism.

[akalin, 2012-02-22 22:52:32]

http://codereview.chromium.org/9378041/diff/19001/third_party/cacheinvalidati...
File third_party/cacheinvalidation/cacheinvalidation.gyp (right):

http://codereview.chromium.org/9378041/diff/19001/third_party/cacheinvalidati...
third_party/cacheinvalidation/cacheinvalidation.gyp:130:
'../../base/base.gyp:base',
On 2012/02/22 05:27:30, Ryan Sleevi wrote:
> You should export_dependent_settings on cacheinvalidation_proto here, as per
the
> comment on line 120/121.
> 
> By doing so, you can actually delete lines 120-122 for better build
parallelism.

Eh?  I don't understand how the hard dep. can be removed.  If one of the users
of this library includes a cacheinvalidation header file which includes a pb.h
file, doesn't that need a hard dep?

[Ryan Sleevi, 2012-02-22 22:59:39]

On 2012/02/22 22:52:32, akalin wrote:
>
http://codereview.chromium.org/9378041/diff/19001/third_party/cacheinvalidati...
> File third_party/cacheinvalidation/cacheinvalidation.gyp (right):
> 
>
http://codereview.chromium.org/9378041/diff/19001/third_party/cacheinvalidati...
> third_party/cacheinvalidation/cacheinvalidation.gyp:130:
> '../../base/base.gyp:base',
> On 2012/02/22 05:27:30, Ryan Sleevi wrote:
> > You should export_dependent_settings on cacheinvalidation_proto here, as per
> the
> > comment on line 120/121.
> > 
> > By doing so, you can actually delete lines 120-122 for better build
> parallelism.
> 
> Eh?  I don't understand how the hard dep. can be removed.  If one of the users
> of this library includes a cacheinvalidation header file which includes a pb.h
> file, doesn't that need a hard dep?

No. It need only export a dependency on the target that generates these files
(in this case, cacheinvalidation_proto).

The hard_dependency state follows through exported dependencies. If
cacheinvalidation does export_dependent_settings of cacheinvalidation_proto,
then every target that depends on cacheinvalidation will have a synthetic
dependency on the actions of cacheinvalidation_proto. 

If such a target then export_dependent_setting's on cacheinvalidation, then all
of THOSE targets will as well have a dependency on cacheinvalidation_proto.

Generally speaking, any time you include a (generated) header from a target, you
should export_dependent_settings of that target, and the correct thing will
happen. You don't need to manually mark all the targets in the dependency chain
as hard_dependency - only the one(s) doing the generation.

[akalin, 2012-02-22 23:11:08]

On 2012/02/22 22:59:39, Ryan Sleevi wrote:
> On 2012/02/22 22:52:32, akalin wrote:
> >
>
http://codereview.chromium.org/9378041/diff/19001/third_party/cacheinvalidati...
> > File third_party/cacheinvalidation/cacheinvalidation.gyp (right):
> > 
> >
>
http://codereview.chromium.org/9378041/diff/19001/third_party/cacheinvalidati...
> > third_party/cacheinvalidation/cacheinvalidation.gyp:130:
> > '../../base/base.gyp:base',
> > On 2012/02/22 05:27:30, Ryan Sleevi wrote:
> > > You should export_dependent_settings on cacheinvalidation_proto here, as
per
> > the
> > > comment on line 120/121.
> > > 
> > > By doing so, you can actually delete lines 120-122 for better build
> > parallelism.
> > 
> > Eh?  I don't understand how the hard dep. can be removed.  If one of the
users
> > of this library includes a cacheinvalidation header file which includes a
pb.h
> > file, doesn't that need a hard dep?
> 
> No. It need only export a dependency on the target that generates these files
> (in this case, cacheinvalidation_proto).
> 
> The hard_dependency state follows through exported dependencies. If
> cacheinvalidation does export_dependent_settings of cacheinvalidation_proto,
> then every target that depends on cacheinvalidation will have a synthetic
> dependency on the actions of cacheinvalidation_proto. 
> 
> If such a target then export_dependent_setting's on cacheinvalidation, then
all
> of THOSE targets will as well have a dependency on cacheinvalidation_proto.
> 
> Generally speaking, any time you include a (generated) header from a target,
you
> should export_dependent_settings of that target, and the correct thing will
> happen. You don't need to manually mark all the targets in the dependency
chain
> as hard_dependency - only the one(s) doing the generation.

Okay, that makes sense.  I could have sworn hard_deps weren't transitive when I
wrote that file up, but maybe it's changed since then.

[KushalP, 2012-02-22 23:21:28]

On 2012/02/22 23:11:08, akalin wrote:
> Okay, that makes sense.  I could have sworn hard_deps weren't transitive when
I
> wrote that file up, but maybe it's changed since then.

In theory it makes sense, in practice it fails to generate the .a file.

My latest patch set (7) handles all of the previous changes without removing the
hard_dependency.

[Ryan Sleevi, 2012-02-22 23:23:34]

On 2012/02/22 23:21:28, KushalP wrote:
> On 2012/02/22 23:11:08, akalin wrote:
> > Okay, that makes sense.  I could have sworn hard_deps weren't transitive
when
> I
> > wrote that file up, but maybe it's changed since then.
> 
> In theory it makes sense, in practice it fails to generate the .a file.

Can you provide a bit more about your output? 'cacheinvalidation' needs only
depend on cacheinvalidation_proto AND export_dependent_settings on
cacheinvalidation_proto. If there were further failures upstream, then the same
problem should exist regardless of whether 'cacheinvalidation' is marked
hard_dependency.

[Ryan Sleevi, 2012-02-22 23:25:52]

On 2012/02/22 23:11:08, akalin wrote:
> Okay, that makes sense.  I could have sworn hard_deps weren't transitive when
I
> wrote that file up, but maybe it's changed since then.

They weren't. I changed it, because it's nasty that a 'high-level' GYP target
needs to know it needs to mark itself 'hard_dependency' because some
great-great-grandchild of a target generates headers and that those may
transitively appear in the targets public headers. The "correct" GYP syntax
was/is to export_dependent_settings on your immediate descendents if you include
them.

http://code.google.com/p/gyp/source/detail?r=1028

[KushalP, 2012-02-22 23:40:16]

Here's the build output if I remove 'hard_dependency': 1

https://gist.github.com/1a7113135e028cc7ed00

[Ryan Sleevi, 2012-02-22 23:48:39]

On 2012/02/22 23:40:16, KushalP wrote:
> Here's the build output if I remove 'hard_dependency': 1
> 
> https://gist.github.com/1a7113135e028cc7ed00

Thanks. This is particularly odd, as it indicates a failure compiling
'cacheinvalidation_proto', not 'cacheinvalidation' (which is the target that is
having hard_dependency removed). cacheinvalidation_proto would suggest some
issue with that target or with protoc.gypi.

Just to confirm, this is line 120 of Patchset you're removing, right?

[KushalP, 2012-02-23 00:03:44]

On 2012/02/22 23:48:39, Ryan Sleevi wrote:
> Just to confirm, this is line 120 of Patchset you're removing, right?

Yes. See the diff here: https://gist.github.com/1888466

[KushalP, 2012-02-23 00:47:57]

Here's a gist of the cacheinvalidation.target.mk when 'hard_dependency': 1 is
removed: https://gist.github.com/1888790

[KushalP, 2012-02-27 11:44:35]

I just ran a clobber, and removing hard_dependency works fine now. Getting ready
to run it through the trybots...

[KushalP, 2012-02-27 12:51:34]

On 2012/02/27 11:44:35, KushalP wrote:
> I just ran a clobber, and removing hard_dependency works fine now. Getting
ready
> to run it through the trybots...

The tryrun appears to have worked. The failures look like they're related to
some other CLs that are being worked on at the moment.

[commit-bot: I haz the power, 2012-02-27 23:40:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kushi.p@gmail.com/9378041/31001

[commit-bot: I haz the power, 2012-02-28 05:17:50]

Change committed as 123894

[Ryan Sleevi, 2012-02-28 05:36:44]

On 2012/02/28 05:17:50, I haz the power (commit-bot) wrote:
> Change committed as 123894

So, I had to revert this in r123896, as it broke the XCode generator.

Sample output below, I haven't yet analyzed it.

Traceback (most recent call last):
  File "src/build/gyp_chromium", line 171, in <module>
    sys.exit(gyp.main(args))
  File
"/b/build/slave/Mac_Builder__dbg_/build/src/tools/gyp/pylib/gyp/__init__.py",
line 480, in main
    generator.GenerateOutput(flat_list, targets, data, params)
  File
"/b/build/slave/Mac_Builder__dbg_/build/src/tools/gyp/pylib/gyp/generator/xcode.py",
line 1202, in GenerateOutput
    xcode_target_to_target_dict)
  File
"/b/build/slave/Mac_Builder__dbg_/build/src/tools/gyp/pylib/gyp/generator/xcode.py",
line 415, in Finalize2
    self.project_file.EnsureNoIDCollisions()
  File
"/b/build/slave/Mac_Builder__dbg_/build/src/tools/gyp/pylib/gyp/xcodeproj_file.py",
line 458, in EnsureNoIDCollisions
    str(other._properties), self._properties['rootObject'].Name())
KeyError: 'Duplicate ID E05CD4EE52E5C445FD534CDD, objects "{\'path\':
\'google/cacheinvalidation/v2\', \'children\': [<PBXFileReference
\'client_pb2.py\' at 0x102f2ce50>], \'sourceTree\': \'<group>\'}" and
"{\'path\': \'google/cacheinvalidation/v2\', \'children\': [<PBXFileReference
\'client_pb2.py\' at 0x102f2cd50>], \'sourceTree\': \'<group>\'}" in
"cacheinvalidation"'
Error: Command /usr/bin/python src/build/gyp_chromium returned non-zero exit
status 1 in /b/build/slave/Mac_Builder__dbg_/build


[KushalP, 2012-02-28 13:42:16]

On 2012/02/28 05:36:44, Ryan Sleevi wrote:
> So, I had to revert this in r123896, as it broke the XCode generator.
> 
> Sample output below, I haven't yet analyzed it.
> 
> Traceback (most recent call last):
>   File "src/build/gyp_chromium", line 171, in <module>
>     sys.exit(gyp.main(args))
>   File
> "/b/build/slave/Mac_Builder__dbg_/build/src/tools/gyp/pylib/gyp/__init__.py",
> line 480, in main
>     generator.GenerateOutput(flat_list, targets, data, params)
>   File
>
"/b/build/slave/Mac_Builder__dbg_/build/src/tools/gyp/pylib/gyp/generator/xcode.py",
> line 1202, in GenerateOutput
>     xcode_target_to_target_dict)
>   File
>
"/b/build/slave/Mac_Builder__dbg_/build/src/tools/gyp/pylib/gyp/generator/xcode.py",
> line 415, in Finalize2
>     self.project_file.EnsureNoIDCollisions()
>   File
>
"/b/build/slave/Mac_Builder__dbg_/build/src/tools/gyp/pylib/gyp/xcodeproj_file.py",
> line 458, in EnsureNoIDCollisions
>     str(other._properties), self._properties['rootObject'].Name())
> KeyError: 'Duplicate ID E05CD4EE52E5C445FD534CDD, objects "{\'path\':
> \'google/cacheinvalidation/v2\', \'children\': [<PBXFileReference
> \'client_pb2.py\' at 0x102f2ce50>], \'sourceTree\': \'<group>\'}" and
> "{\'path\': \'google/cacheinvalidation/v2\', \'children\': [<PBXFileReference
> \'client_pb2.py\' at 0x102f2cd50>], \'sourceTree\': \'<group>\'}" in
> "cacheinvalidation"'
> Error: Command /usr/bin/python src/build/gyp_chromium returned non-zero exit
> status 1 in /b/build/slave/Mac_Builder__dbg_/build
> 

I can reproduce this locally by running the following:

GYP_GENERATOR=gypd ./build/gyp_chromium

[KushalP, 2012-02-29 01:10:35]

Setting 'hard_dependency' has no effect on this. Seems there's something else
causing duplicate client_pb2.py objects to appear.

[akalin, 2012-03-02 23:04:07]

On 2012/02/29 01:10:35, KushalP wrote:
> Setting 'hard_dependency' has no effect on this. Seems there's something else
> causing duplicate client_pb2.py objects to appear.

Ryan, any idea what's going on?

[Ryan Sleevi, 2012-03-05 20:38:18]

On 2012/03/02 23:04:07, akalin wrote:
> On 2012/02/29 01:10:35, KushalP wrote:
> > Setting 'hard_dependency' has no effect on this. Seems there's something
else
> > causing duplicate client_pb2.py objects to appear.
> 
> Ryan, any idea what's going on?

No. Kushal and I discussed a little bit over IRC. My understanding is that for
scheduling reasons, he intends to pick this up next week, and we'll work through
what's up. I haven't looked at this locally, as he's using this as an
opportunity to learn GYP and its nuances :)

[KushalP, 2012-03-15 00:51:52]

I've spent the last week going through the GYP source code trying to better
understand how it generates Xcode project files. Doing this alone has been
incredibly insightful, and I'm sorry if my delaying on submitting a patch set
for this CL have annoyed you.

I still haven't got a patch set ready for you to review, but I'd like to go
through a few of the reasons that I _think_ this CL failed to apply correctly to
the tree. Many of these are pure speculation, but I wanted to write my thoughts
down to see if anything clicked with you guys. Or even if putting my thoughts to
words garnered any insights.

## Objects in Xcode and GYP

Objects in Xcode each have a 24-char identifier (hash). This identifier is only
generated once, when the object is created, and left unchanged. The generator
within Xcode (not GYP) is free to select any identifier, even at random, to
refer to the objects it creates, and Xcode will retain those identifiers and use
them when rewriting the project file. There can be issues between objects having
the same identifier, when objects are in a 'new' and 'old' state. I don't quite
get why, but it _can_ happen.

To get around this, GYP tries to make the process more deterministic: hashing a
description of the object and its parent/ancestor objects in the dependency
tree. If this is what is happening here and the error is occurring, then there's
an argument for there being a bug in GYP relating to a reference of the same
object somewhere instead of it being a duplicate. This is going along the lines
of Python's use of pass-by-reference.

Another reason could be that because it uses SHA1 internally (160 bits), but the
Xcode object takes 24 chars (96 bits), that XORing them could potentially cause
a hash collision with different objects. However, I haven't done the maths here
to test this assumption. I'm going to claim it as fairly unlikely due to the use
of XOR instead of (the much worse) discarding to fit.

The specific block which raises the exception seen in the failure, tests to see
if another object with the same hash exists in the descendants path. It does not
test against position in the tree. What's interesting is that this test doesn't
occur during addition of objects to the tree. It only occurs after the tree has
been built.

I think my next step is to attempt to dump the output of the tree (verbose mode
somewhere?) and see how/when the duplicate object is added.

[KushalP, 2012-03-23 01:03:26]

I think I've found the point where the duplicate objects (in the Python sense,
not OO) are added to the _properties and _schema instance variables for the
project. It's within the method UpdateProperties in the XCObject class.

At the end of this comment is a snippet of output that I've made from placing
'print' statements in opportune places. What this tells me is that the
_properties variable can be directly updated without an initial check for
duplicates occurring. So currently the objects are only checked for duplicates
when their respective representations are asked for.

My next step is likely to look at the 20-odd places (this class and sub-classes)
where UpdateProperties is used and see if there's an obvious point where this
target (cacheinvalidation) is used as input. From there I'll hopefully be able
to figure out WHY (I must know!) this duplication is occurring.

My current assumption: the edit to use proto_out_dir and proto_relpath is
messing with the object creation in XCode within GYP.

The following is the output I produced. It's a line-by-line output of the
various related lists that get passed in to the UpdateProperties method as
'value'. Here's the output:

google
cacheinvalidation
v2
client_pb2.py
Intermediates
$(SHARED_INTERMEDIATE_DIR)
protoc_out

google
cacheinvalidation
v2
client.pb.cc

google
cacheinvalidation
v2
client.pb.h

google
cacheinvalidation
v2
client_pb2.py

google
cacheinvalidation
v2
client.pb.cc
<PBXFileReference 'client.pb.cc' at 0x102420b10>

google
cacheinvalidation
v2
client.pb.h

google
cacheinvalidation
v2
client_gateway_pb2.py

google
cacheinvalidation
v2
client_gateway.pb.cc

google
cacheinvalidation
v2
client_gateway.pb.h

google
cacheinvalidation
v2
client_gateway_pb2.py

google
cacheinvalidation
v2
client_gateway.pb.cc
<PBXFileReference 'client_gateway.pb.cc' at 0x102421050>

google
cacheinvalidation
v2
client_gateway.pb.h

google
cacheinvalidation
v2
client_protocol_pb2.py

google
cacheinvalidation
v2
client_protocol.pb.cc

google
cacheinvalidation
v2
client_protocol.pb.h

google
cacheinvalidation
v2
client_protocol_pb2.py

google
cacheinvalidation
v2
client_protocol.pb.cc
<PBXFileReference 'client_protocol.pb.cc' at 0x102421950>

google
cacheinvalidation
v2
client_protocol.pb.h

google
cacheinvalidation
v2
client_test_internal_pb2.py

google
cacheinvalidation
v2
client_test_internal.pb.cc

google
cacheinvalidation
v2
client_test_internal.pb.h

google
cacheinvalidation
v2
client_test_internal_pb2.py

google
cacheinvalidation
v2
client_test_internal.pb.cc
<PBXFileReference 'client_test_internal.pb.cc' at 0x10242a310>

google
cacheinvalidation
v2
client_test_internal.pb.h

google
cacheinvalidation
v2
types_pb2.py

google
cacheinvalidation
v2
types.pb.cc

google
cacheinvalidation
v2
types.pb.h

google
cacheinvalidation
v2
types_pb2.py

google
cacheinvalidation
v2
types.pb.cc
<PBXFileReference 'types.pb.cc' at 0x10242a550>

The thing I find interesting here is the mixture of plain (string like) objects
and PBXFileReference objects. That's the clear duplication.

[akalin, 2012-06-18 18:28:06]

any update on this?  i'm just gonna remove myself from the reviewer list.

On 2012/03/23 01:03:26, KushalP wrote:
> I think I've found the point where the duplicate objects (in the Python sense,
> not OO) are added to the _properties and _schema instance variables for the
> project. It's within the method UpdateProperties in the XCObject class.
> 
> At the end of this comment is a snippet of output that I've made from placing
> 'print' statements in opportune places. What this tells me is that the
> _properties variable can be directly updated without an initial check for
> duplicates occurring. So currently the objects are only checked for duplicates
> when their respective representations are asked for.
> 
> My next step is likely to look at the 20-odd places (this class and
sub-classes)
> where UpdateProperties is used and see if there's an obvious point where this
> target (cacheinvalidation) is used as input. From there I'll hopefully be able
> to figure out WHY (I must know!) this duplication is occurring.
> 
> My current assumption: the edit to use proto_out_dir and proto_relpath is
> messing with the object creation in XCode within GYP.
> 
> The following is the output I produced. It's a line-by-line output of the
> various related lists that get passed in to the UpdateProperties method as
> 'value'. Here's the output:
> 
> google
> cacheinvalidation
> v2
> client_pb2.py
> Intermediates
> $(SHARED_INTERMEDIATE_DIR)
> protoc_out
> 
> google
> cacheinvalidation
> v2
> client.pb.cc
> 
> google
> cacheinvalidation
> v2
> client.pb.h
> 
> google
> cacheinvalidation
> v2
> client_pb2.py
> 
> google
> cacheinvalidation
> v2
> client.pb.cc
> <PBXFileReference 'client.pb.cc' at 0x102420b10>
> 
> google
> cacheinvalidation
> v2
> client.pb.h
> 
> google
> cacheinvalidation
> v2
> client_gateway_pb2.py
> 
> google
> cacheinvalidation
> v2
> client_gateway.pb.cc
> 
> google
> cacheinvalidation
> v2
> client_gateway.pb.h
> 
> google
> cacheinvalidation
> v2
> client_gateway_pb2.py
> 
> google
> cacheinvalidation
> v2
> client_gateway.pb.cc
> <PBXFileReference 'client_gateway.pb.cc' at 0x102421050>
> 
> google
> cacheinvalidation
> v2
> client_gateway.pb.h
> 
> google
> cacheinvalidation
> v2
> client_protocol_pb2.py
> 
> google
> cacheinvalidation
> v2
> client_protocol.pb.cc
> 
> google
> cacheinvalidation
> v2
> client_protocol.pb.h
> 
> google
> cacheinvalidation
> v2
> client_protocol_pb2.py
> 
> google
> cacheinvalidation
> v2
> client_protocol.pb.cc
> <PBXFileReference 'client_protocol.pb.cc' at 0x102421950>
> 
> google
> cacheinvalidation
> v2
> client_protocol.pb.h
> 
> google
> cacheinvalidation
> v2
> client_test_internal_pb2.py
> 
> google
> cacheinvalidation
> v2
> client_test_internal.pb.cc
> 
> google
> cacheinvalidation
> v2
> client_test_internal.pb.h
> 
> google
> cacheinvalidation
> v2
> client_test_internal_pb2.py
> 
> google
> cacheinvalidation
> v2
> client_test_internal.pb.cc
> <PBXFileReference 'client_test_internal.pb.cc' at 0x10242a310>
> 
> google
> cacheinvalidation
> v2
> client_test_internal.pb.h
> 
> google
> cacheinvalidation
> v2
> types_pb2.py
> 
> google
> cacheinvalidation
> v2
> types.pb.cc
> 
> google
> cacheinvalidation
> v2
> types.pb.h
> 
> google
> cacheinvalidation
> v2
> types_pb2.py
> 
> google
> cacheinvalidation
> v2
> types.pb.cc
> <PBXFileReference 'types.pb.cc' at 0x10242a550>
> 
> The thing I find interesting here is the mixture of plain (string like)
objects
> and PBXFileReference objects. That's the clear duplication.

[KushalP, 2012-06-19 09:08:40]

On 2012/06/18 18:28:06, akalin wrote:
> any update on this?  i'm just gonna remove myself from the reviewer list.

I haven't had a chance to work on this for about 2 months. Hoping to get back
into Chromium development within the next fortnight when the rush at work dies
down :-)

[KushalP, 2012-08-02 07:15:23]

Closing this as the work was completed by @iannucci in
https://chromiumcodereview.appspot.com/10796051