Add support for generating jars from protos and add cacheinvalidation_java.

Add support for generating jars from protos and add cacheinvalidation_java.

The cacheinvalidation_java target is also added to build/all_android.gyp to
ensure it is always built since nothing currently depends on it upstream.
When all of Android-specific sync code is upstreamed, a target for sync
should be used instead of cacheinvalidation.

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

[nyquist, 2012-10-13 00:40:00]

PTAL

[cjhopman, 2012-10-15 22:14:24]

http://codereview.chromium.org/11146005/diff/1/build/protoc_java.gypi
File build/protoc_java.gypi (right):

http://codereview.chromium.org/11146005/diff/1/build/protoc_java.gypi#newcode33
build/protoc_java.gypi:33: # The 'output_java_files' variable specifies a list
of output files that will
Maybe mention here how the output files should be prefixed by >(java_dir).

http://codereview.chromium.org/11146005/diff/1/build/protoc_java.gypi#newcode73
build/protoc_java.gypi:73: 'message': 'Generating Java code from
<(RULE_INPUT_PATH)',
Update this message to use something other than RULE_INPUT_PATH.

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

http://codereview.chromium.org/11146005/diff/1/third_party/cacheinvalidation/...
third_party/cacheinvalidation/cacheinvalidation.gyp:196:
'com/google/protos/ipc/invalidation/AndroidChannel.java',
These need to be prefixed by >(java_dir)

[cjhopman, 2012-10-15 22:15:10]

http://codereview.chromium.org/11146005/diff/1/build/protoc_java.gypi
File build/protoc_java.gypi (right):

http://codereview.chromium.org/11146005/diff/1/build/protoc_java.gypi#newcode73
build/protoc_java.gypi:73: 'message': 'Generating Java code from
<(RULE_INPUT_PATH)',
On 2012/10/15 22:14:24, cjhopman wrote:
> Update this message to use something other than RULE_INPUT_PATH.

Done.

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

http://codereview.chromium.org/11146005/diff/1/third_party/cacheinvalidation/...
third_party/cacheinvalidation/cacheinvalidation.gyp:196:
'com/google/protos/ipc/invalidation/AndroidChannel.java',
On 2012/10/15 22:14:24, cjhopman wrote:
> These need to be prefixed by >(java_dir)

Done.

[cjhopman, 2012-10-23 15:48:25]

lgtm

http://codereview.chromium.org/11146005/diff/10001/build/protoc_java.gypi
File build/protoc_java.gypi (right):

http://codereview.chromium.org/11146005/diff/10001/build/protoc_java.gypi#new...
build/protoc_java.gypi:29: # The 'proto_out_dir' variable specifies the path
suffix that output
This was changed to just use _target_name. Also at line 38 below.

[nyquist, 2012-10-25 04:53:50]

akalin@: Could you PTAL?

[akalin, 2012-10-26 00:11:47]

lgtm, but I'd like someone more familiar with the proto gyps to take another
look: +rsleevi

http://codereview.chromium.org/11146005/diff/17001/third_party/cacheinvalidat...
File third_party/cacheinvalidation/cacheinvalidation.gyp (right):

http://codereview.chromium.org/11146005/diff/17001/third_party/cacheinvalidat...
third_party/cacheinvalidation/cacheinvalidation.gyp:152: # The Java files
generated from the cache invalidation protocol buffers.
move this to right below the _cpp version

http://codereview.chromium.org/11146005/diff/17001/third_party/cacheinvalidat...
third_party/cacheinvalidation/cacheinvalidation.gyp:154: 'target_name':
'cacheinvalidation_proto_src_java',
remove _src to match _cpp version?

[nyquist, 2012-10-26 00:32:31]

http://codereview.chromium.org/11146005/diff/17001/third_party/cacheinvalidat...
File third_party/cacheinvalidation/cacheinvalidation.gyp (right):

http://codereview.chromium.org/11146005/diff/17001/third_party/cacheinvalidat...
third_party/cacheinvalidation/cacheinvalidation.gyp:154: 'target_name':
'cacheinvalidation_proto_src_java',
On 2012/10/26 00:11:47, akalin wrote:
> remove _src to match _cpp version?

This only generates the source files. Downstream we have another target that
generates a .jar-file (that is what you want to depend on). Is it OK to keep it
as is and then later add the .jar-target as cacheinvalidation_proto_java?

[akalin, 2012-10-26 20:18:09]

On 2012/10/26 00:32:31, nyquist wrote:
>
http://codereview.chromium.org/11146005/diff/17001/third_party/cacheinvalidat...
> File third_party/cacheinvalidation/cacheinvalidation.gyp (right):
> 
>
http://codereview.chromium.org/11146005/diff/17001/third_party/cacheinvalidat...
> third_party/cacheinvalidation/cacheinvalidation.gyp:154: 'target_name':
> 'cacheinvalidation_proto_src_java',
> On 2012/10/26 00:11:47, akalin wrote:
> > remove _src to match _cpp version?
> 
> This only generates the source files. Downstream we have another target that
> generates a .jar-file (that is what you want to depend on). Is it OK to keep
it
> as is and then later add the .jar-target as cacheinvalidation_proto_java?

Ah, I see.  I think this CL is okay as-is, but eventually I'd like to see all
the .java -> .jar logic encapsulated in protoc_java.gypi, so that
cacheinvalidation.gyp only has a target that generates the jar from the proto
files.

[akalin, 2012-10-26 20:18:25]

On 2012/10/26 20:18:09, akalin wrote:
> On 2012/10/26 00:32:31, nyquist wrote:
> >
>
http://codereview.chromium.org/11146005/diff/17001/third_party/cacheinvalidat...
> > File third_party/cacheinvalidation/cacheinvalidation.gyp (right):
> > 
> >
>
http://codereview.chromium.org/11146005/diff/17001/third_party/cacheinvalidat...
> > third_party/cacheinvalidation/cacheinvalidation.gyp:154: 'target_name':
> > 'cacheinvalidation_proto_src_java',
> > On 2012/10/26 00:11:47, akalin wrote:
> > > remove _src to match _cpp version?
> > 
> > This only generates the source files. Downstream we have another target that
> > generates a .jar-file (that is what you want to depend on). Is it OK to keep
> it
> > as is and then later add the .jar-target as cacheinvalidation_proto_java?
> 
> Ah, I see.  I think this CL is okay as-is, but eventually I'd like to see all
> the .java -> .jar logic encapsulated in protoc_java.gypi, so that
> cacheinvalidation.gyp only has a target that generates the jar from the proto
> files.

Maybe add TODOs for this?

[Ryan Sleevi, 2012-10-29 08:36:36]

NACK with comments.

http://codereview.chromium.org/11146005/diff/17001/build/protoc_java.gypi
File build/protoc_java.gypi (right):

http://codereview.chromium.org/11146005/diff/17001/build/protoc_java.gypi#new...
build/protoc_java.gypi:5: # This file is meant to be included into a target to
provide a rule
We generate Python and C++ files using the single protoc.gypi.

It's worth exploring why this cannot be shared, and at least documenting in this
CL's change notes or in the file.

http://codereview.chromium.org/11146005/diff/17001/build/protoc_java.gypi#new...
build/protoc_java.gypi:11: #   'type': 'static_library',
This is not correct, is it? Can you generate a static_library target from just
jar files?

http://codereview.chromium.org/11146005/diff/17001/build/protoc_java.gypi#new...
build/protoc_java.gypi:20: #      
'>(java_out_dir)/org/chromium/package/Bar.java'
Is there ever a situation where you have .proto files belonging to multiple
packages? Does it make more sense to have the package specified in the variable,
rather than requiring every source to be repeated?

http://codereview.chromium.org/11146005/diff/17001/build/protoc_java.gypi#new...
build/protoc_java.gypi:52: 'action': [ 'mkdir', '-p', '<(java_out_dir)' ],
NACK on this. I strongly dislike embedding platform-specific commands in GYP -
particularly when they just make directories.

Seems like this should be done in protoc or in a wrapper script, but not within
GYP. For example, Android supports an NDK on Windows - thus it's reasonable to
imagine building this on Windows at some point in the future.

http://codereview.chromium.org/11146005/diff/17001/build/protoc_java.gypi#new...
build/protoc_java.gypi:77: '<(SHARED_INTERMEDIATE_DIR)/protoc_out',
Why are either of these necessary at all? You're generating a Java target. You
never use the path protoc_out in this file.

http://codereview.chromium.org/11146005/diff/17001/third_party/cacheinvalidat...
File third_party/cacheinvalidation/cacheinvalidation.gyp (right):

http://codereview.chromium.org/11146005/diff/17001/third_party/cacheinvalidat...
third_party/cacheinvalidation/cacheinvalidation.gyp:178:
'<(proto_in_dir)/proto/types.proto',
don't repeat <(proto_in_dir)/proto

1) Your GYP file already documents sources as being relative to <(proto_in_dir)
2) You're still repeating /proto/.

Just said proto_in_dir to src/proto, and set sources as appropriate. Or update
your .gypi and remove the option entirely.

[nyquist, 2012-10-30 00:30:32]

rsleevi: PTAL

Addressed comments.
Now we directly generate a .jar-file instead of using a separate target for the
.java-files.

Note, this depends on other CLs in flight and will not be submitted before they
have landed:
http://codereview.chromium.org/11341029/
http://codereview.chromium.org/11347026/

http://codereview.chromium.org/11146005/diff/17001/build/protoc_java.gypi
File build/protoc_java.gypi (right):

http://codereview.chromium.org/11146005/diff/17001/build/protoc_java.gypi#new...
build/protoc_java.gypi:5: # This file is meant to be included into a target to
provide a rule
On 2012/10/29 08:36:36, Ryan Sleevi wrote:
> We generate Python and C++ files using the single protoc.gypi.
> 
> It's worth exploring why this cannot be shared, and at least documenting in
this
> CL's change notes or in the file.

Done.

http://codereview.chromium.org/11146005/diff/17001/build/protoc_java.gypi#new...
build/protoc_java.gypi:11: #   'type': 'static_library',
On 2012/10/29 08:36:36, Ryan Sleevi wrote:
> This is not correct, is it? Can you generate a static_library target from just
> jar files?

Change to 'none'.

http://codereview.chromium.org/11146005/diff/17001/build/protoc_java.gypi#new...
build/protoc_java.gypi:20: #      
'>(java_out_dir)/org/chromium/package/Bar.java'
On 2012/10/29 08:36:36, Ryan Sleevi wrote:
> Is there ever a situation where you have .proto files belonging to multiple
> packages? Does it make more sense to have the package specified in the
variable,
> rather than requiring every source to be repeated?

Removed the shole notion of specifying the .java-files and now instead generate
a .jar-file.

http://codereview.chromium.org/11146005/diff/17001/build/protoc_java.gypi#new...
build/protoc_java.gypi:52: 'action': [ 'mkdir', '-p', '<(java_out_dir)' ],
On 2012/10/29 08:36:36, Ryan Sleevi wrote:
> NACK on this. I strongly dislike embedding platform-specific commands in GYP -
> particularly when they just make directories.
> 
> Seems like this should be done in protoc or in a wrapper script, but not
within
> GYP. For example, Android supports an NDK on Windows - thus it's reasonable to
> imagine building this on Windows at some point in the future.

Changed to use .sh file. This is also used for timestamping output.

http://codereview.chromium.org/11146005/diff/17001/build/protoc_java.gypi#new...
build/protoc_java.gypi:77: '<(SHARED_INTERMEDIATE_DIR)/protoc_out',
On 2012/10/29 08:36:36, Ryan Sleevi wrote:
> Why are either of these necessary at all? You're generating a Java target. You
> never use the path protoc_out in this file.

Done.

http://codereview.chromium.org/11146005/diff/17001/third_party/cacheinvalidat...
File third_party/cacheinvalidation/cacheinvalidation.gyp (right):

http://codereview.chromium.org/11146005/diff/17001/third_party/cacheinvalidat...
third_party/cacheinvalidation/cacheinvalidation.gyp:152: # The Java files
generated from the cache invalidation protocol buffers.
On 2012/10/26 00:11:47, akalin wrote:
> move this to right below the _cpp version

Done.

http://codereview.chromium.org/11146005/diff/17001/third_party/cacheinvalidat...
third_party/cacheinvalidation/cacheinvalidation.gyp:178:
'<(proto_in_dir)/proto/types.proto',
On 2012/10/29 08:36:36, Ryan Sleevi wrote:
> don't repeat <(proto_in_dir)/proto
> 
> 1) Your GYP file already documents sources as being relative to
<(proto_in_dir)
> 2) You're still repeating /proto/.
> 
> Just said proto_in_dir to src/proto, and set sources as appropriate. Or update
> your .gypi and remove the option entirely.

1)
The proto directory needed to be set, and the content of the files had a form
like
import "proto/otherfile.proto", so we had to specify the paths relative to that.
2)
Added a new variable that includes /proto in the path.

[Ryan Sleevi, 2012-10-30 00:50:42]

The java_in_dir bit is weird to me, but I see you've filed a bug on it. If it's
not something to be addressed within the next few days, I think it may make more
sense specifically mentioning the bug next to the instances of the java_in_dir
hack, lest it become cargo-culted.

Other than that, I think my concerns have been addressed, so this LGTM. Some
nits below though, and please see about getting that bash code looked at by
someone with strong opinions on bash (I seem to recall digit has them, hence
I've added him as a reviewer, but I may be misremembering)

https://codereview.chromium.org/11146005/diff/24003/build/android/empty/src/....
File build/android/empty/src/.keep (right):

https://codereview.chromium.org/11146005/diff/24003/build/android/empty/src/....
build/android/empty/src/.keep:1: This is a file that needs to live here until
crbug.com/158155 has been fixed.
nit: add the http:// so it will linkify correctly in more editors.

https://codereview.chromium.org/11146005/diff/24003/build/protoc_java.gypi
File build/protoc_java.gypi (right):

https://codereview.chromium.org/11146005/diff/24003/build/protoc_java.gypi#ne...
build/protoc_java.gypi:48: 'script': '<(DEPTH)/build/protoc_java.sh',
I think I would be happier if this were (cross-platform) Python rather than
Bash, but I can live with it. I thought maruel and scottmg were trying to kill
off bash-in-build-scripts, but that may have only related to shared code.

Something to consider, at least.

https://codereview.chromium.org/11146005/diff/24003/build/protoc_java.sh
File build/protoc_java.sh (right):

https://codereview.chromium.org/11146005/diff/24003/build/protoc_java.sh#newc...
build/protoc_java.sh:13: touch $4
Seems like you will want to get someone with bash readability to look at this,
but I'm instantly nervous about the lack of quoting around file system paths
here, especially if the originally arrived quoted (eg: protoc_java.sh protoc
dummy "src /"). I realize the quotes for lines 5/8 probably mitigate this, but
it is enough to raise an eyebrow at (and wonder about how it would affect line
12, since $2 isn't quoted)

[nyquist, 2012-10-30 05:18:35]

On 2012/10/30 00:50:42, Ryan Sleevi wrote:
> The java_in_dir bit is weird to me, but I see you've filed a bug on it. If
it's
> not something to be addressed within the next few days, I think it may make
more
> sense specifically mentioning the bug next to the instances of the java_in_dir
> hack, lest it become cargo-culted.

Good idea! I will make sure I add a comment wherever this hack is used too.

> 
> Other than that, I think my concerns have been addressed, so this LGTM. Some
> nits below though, and please see about getting that bash code looked at by
> someone with strong opinions on bash (I seem to recall digit has them, hence
> I've added him as a reviewer, but I may be misremembering)

It sounds reasonable to look into using python instead of bash for this. I will
take a stab at that unless digit tells me otherwise.

> 
>
https://codereview.chromium.org/11146005/diff/24003/build/android/empty/src/....
> File build/android/empty/src/.keep (right):
> 
>
https://codereview.chromium.org/11146005/diff/24003/build/android/empty/src/....
> build/android/empty/src/.keep:1: This is a file that needs to live here until
> crbug.com/158155 has been fixed.
> nit: add the http:// so it will linkify correctly in more editors.
> 
> https://codereview.chromium.org/11146005/diff/24003/build/protoc_java.gypi
> File build/protoc_java.gypi (right):
> 
>
https://codereview.chromium.org/11146005/diff/24003/build/protoc_java.gypi#ne...
> build/protoc_java.gypi:48: 'script': '<(DEPTH)/build/protoc_java.sh',
> I think I would be happier if this were (cross-platform) Python rather than
> Bash, but I can live with it. I thought maruel and scottmg were trying to kill
> off bash-in-build-scripts, but that may have only related to shared code.
> 
> Something to consider, at least.
> 
> https://codereview.chromium.org/11146005/diff/24003/build/protoc_java.sh
> File build/protoc_java.sh (right):
> 
>
https://codereview.chromium.org/11146005/diff/24003/build/protoc_java.sh#newc...
> build/protoc_java.sh:13: touch $4
> Seems like you will want to get someone with bash readability to look at this,
> but I'm instantly nervous about the lack of quoting around file system paths
> here, especially if the originally arrived quoted (eg: protoc_java.sh protoc
> dummy "src /"). I realize the quotes for lines 5/8 probably mitigate this, but
> it is enough to raise an eyebrow at (and wonder about how it would affect line
> 12, since $2 isn't quoted)

[nyquist, 2012-10-30 05:20:07]

On 2012/10/30 05:18:35, nyquist wrote:
> On 2012/10/30 00:50:42, Ryan Sleevi wrote:
> > The java_in_dir bit is weird to me, but I see you've filed a bug on it. If
> it's
> > not something to be addressed within the next few days, I think it may make
> more
> > sense specifically mentioning the bug next to the instances of the
java_in_dir
> > hack, lest it become cargo-culted.
> 
> Good idea! I will make sure I add a comment wherever this hack is used too.
> 
> > 
> > Other than that, I think my concerns have been addressed, so this LGTM. Some
> > nits below though, and please see about getting that bash code looked at by
> > someone with strong opinions on bash (I seem to recall digit has them, hence
> > I've added him as a reviewer, but I may be misremembering)
> 
> It sounds reasonable to look into using python instead of bash for this. I
will
> take a stab at that unless digit tells me otherwise.
> 
> > 
> >
>
https://codereview.chromium.org/11146005/diff/24003/build/android/empty/src/....
> > File build/android/empty/src/.keep (right):
> > 
> >
>
https://codereview.chromium.org/11146005/diff/24003/build/android/empty/src/....
> > build/android/empty/src/.keep:1: This is a file that needs to live here
until
> > crbug.com/158155 has been fixed.
> > nit: add the http:// so it will linkify correctly in more editors.
> > 
> > https://codereview.chromium.org/11146005/diff/24003/build/protoc_java.gypi
> > File build/protoc_java.gypi (right):
> > 
> >
>
https://codereview.chromium.org/11146005/diff/24003/build/protoc_java.gypi#ne...
> > build/protoc_java.gypi:48: 'script': '<(DEPTH)/build/protoc_java.sh',
> > I think I would be happier if this were (cross-platform) Python rather than
> > Bash, but I can live with it. I thought maruel and scottmg were trying to
kill
> > off bash-in-build-scripts, but that may have only related to shared code.
> > 
> > Something to consider, at least.
> > 
> > https://codereview.chromium.org/11146005/diff/24003/build/protoc_java.sh
> > File build/protoc_java.sh (right):
> > 
> >
>
https://codereview.chromium.org/11146005/diff/24003/build/protoc_java.sh#newc...
> > build/protoc_java.sh:13: touch $4
> > Seems like you will want to get someone with bash readability to look at
this,
> > but I'm instantly nervous about the lack of quoting around file system paths
> > here, especially if the originally arrived quoted (eg: protoc_java.sh protoc
> > dummy "src /"). I realize the quotes for lines 5/8 probably mitigate this,
but
> > it is enough to raise an eyebrow at (and wonder about how it would affect
line
> > 12, since $2 isn't quoted)

Also, a quick note: all files in src/third_party/protobuf should be removed from
this CL. They should only be part of http://codereview.chromium.org/11347026/
Local branch dependencies made them become part of this CL. Will remove in next
update.

[nyquist, 2012-10-31 17:46:49]

rsleevi: PTAL.
Now using Python script instead.
Also using bundled jars for guava and gcm.
In the future, we should see if we could build guava ourselves instead, and
possibly get gcm as part of the SDK.

[nyquist, 2012-10-31 17:49:33]

addressed comments

http://codereview.chromium.org/11146005/diff/24003/build/android/empty/src/.keep
File build/android/empty/src/.keep (right):

http://codereview.chromium.org/11146005/diff/24003/build/android/empty/src/.k...
build/android/empty/src/.keep:1: This is a file that needs to live here until
crbug.com/158155 has been fixed.
On 2012/10/30 00:50:43, Ryan Sleevi wrote:
> nit: add the http:// so it will linkify correctly in more editors.

Done.

http://codereview.chromium.org/11146005/diff/24003/build/protoc_java.gypi
File build/protoc_java.gypi (right):

http://codereview.chromium.org/11146005/diff/24003/build/protoc_java.gypi#new...
build/protoc_java.gypi:48: 'script': '<(DEPTH)/build/protoc_java.sh',
On 2012/10/30 00:50:43, Ryan Sleevi wrote:
> I think I would be happier if this were (cross-platform) Python rather than
> Bash, but I can live with it. I thought maruel and scottmg were trying to kill
> off bash-in-build-scripts, but that may have only related to shared code.
> 
> Something to consider, at least.

Done.

http://codereview.chromium.org/11146005/diff/24003/build/protoc_java.sh
File build/protoc_java.sh (right):

http://codereview.chromium.org/11146005/diff/24003/build/protoc_java.sh#newco...
build/protoc_java.sh:13: touch $4
On 2012/10/30 00:50:43, Ryan Sleevi wrote:
> Seems like you will want to get someone with bash readability to look at this,
> but I'm instantly nervous about the lack of quoting around file system paths
> here, especially if the originally arrived quoted (eg: protoc_java.sh protoc
> dummy "src /"). I realize the quotes for lines 5/8 probably mitigate this, but
> it is enough to raise an eyebrow at (and wonder about how it would affect line
> 12, since $2 isn't quoted)

Changed to use Python.

[Ryan Sleevi, 2012-10-31 20:11:17]

This all LGTM from a GYP purity standpoint, I would just ask:

1) Does protoc_java.gypi belong in build or build/android?
2) Can you get a build/android OWNER to stamp to make sure they're happy on the
Android side of things. There's not a physical OWNERS file, but I believe there
are a few people responsible for the vast code there (and I suspect digit is one
of them), so please double check with them.

[Isaac (away), 2012-10-31 21:32:47]

build/android files are actually not used in the build, but rather have various
utilities and extensive python runners to managing and running tests on devices.
 So I think this code is better placed in build/ than in build/android.

Maybe we should start a discussion about long term locations for android files. 
Some of this stuff might be better hosted in testing/android.

This location lgtm.  I have some comments about the py file, I didn't look at
the gypi ones.

https://chromiumcodereview.appspot.com/11146005/diff/39001/build/protoc_java.py
File build/protoc_java.py (right):

https://chromiumcodereview.appspot.com/11146005/diff/39001/build/protoc_java....
build/protoc_java.py:30: protoc_path = argv[1]
nit: lines 30-33 can be shortened to:

protoc_path, proto_path, java_out, stamp_file = argv[1:5]

https://chromiumcodereview.appspot.com/11146005/diff/39001/build/protoc_java....
build/protoc_java.py:44: protoc_command = [protoc_path,
nit: you could inline this:

subprocess.call(
  [protoc_path, '--proto_path', proto_path, '--java_out', java_out]
  + proto_files)

https://chromiumcodereview.appspot.com/11146005/diff/39001/build/protoc_java....
build/protoc_java.py:53: with file(stamp_file, 'a'):
nit, you could always return ret (delete lines 49/50 and change line 56)

if ret == 0:
  # Create a stamp file
  ...

return ret

[nyquist, 2012-10-31 23:14:20]

addressed all comments.
Can not land this until protobuf java library target is available.

http://codereview.chromium.org/11146005/diff/39001/build/protoc_java.py
File build/protoc_java.py (right):

http://codereview.chromium.org/11146005/diff/39001/build/protoc_java.py#newco...
build/protoc_java.py:30: protoc_path = argv[1]
On 2012/10/31 21:32:48, Isaac wrote:
> nit: lines 30-33 can be shortened to:
> 
> protoc_path, proto_path, java_out, stamp_file = argv[1:5]

Done.

http://codereview.chromium.org/11146005/diff/39001/build/protoc_java.py#newco...
build/protoc_java.py:44: protoc_command = [protoc_path,
On 2012/10/31 21:32:48, Isaac wrote:
> nit: you could inline this:
> 
> subprocess.call(
>   [protoc_path, '--proto_path', proto_path, '--java_out', java_out]
>   + proto_files)

Done.

http://codereview.chromium.org/11146005/diff/39001/build/protoc_java.py#newco...
build/protoc_java.py:53: with file(stamp_file, 'a'):
On 2012/10/31 21:32:48, Isaac wrote:
> nit, you could always return ret (delete lines 49/50 and change line 56)
> 
> if ret == 0:
>   # Create a stamp file
>   ...
> 
> return ret

Done.

[nyquist, 2012-11-14 06:48:25]

rsleevi@: Could you PTAL at changes after patchset 13?

There has been some small changes after the protobuf lite has been submitted. 
I had to move the android-specific targets into a gyp condition.
Also, the android_gcm from
src/third_party/android_tools/android_tools.gyp:android_gcm has become available
in the meantime.

Also, the cacheinvalidation_javalib target has been added to all targets for
Android, so it always builds on bots to ensure we do not break.

[Ryan Sleevi, 2012-11-14 18:49:44]

lgtm

[commit-bot: I haz the power, 2012-11-14 20:00:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nyquist@chromium.org/11146005/35015

[commit-bot: I haz the power, 2012-11-14 21:20:49]

Change committed as 167746