Linux: change protobuf default option to allow building

Linux: change protobuf default option to allow building
with unpatched system protobuf.

This is a preparation for landing https://codereview.chromium.org/11231046/
and a solution chosen because our custom protobuf changes will not
be accepted upstream.

Setting defaults to optimize for lite runtime and retain unknown fields
makes it possible to use the same .proto files with system protobuf.

BUG=157155
TEST=none

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

[agl, 2012-10-22 23:04:41]

LGTM, although I think you should wait for ncarter too.

I feel that CL description could a little more expansive:

"In Chrome we wish to use the lite runtime but we also need unknown field
retention, which isn't supported by the lite runtime. We have hacked our in-tree
version of protobuf to eliminate this restriction, but this does mean that we
can not longer use any other protobuf compiler.

The plan is to make the lite runtime the default with our in-tree protobuf
compiler so that the .proto files themselves don't need to specify it. Then, a
standard protobuf compiler can be used because it'll generate non-lite code,
which includes unknown field retention, we required."

[ncarter (slow), 2012-10-23 17:05:40]

LGTM

[commit-bot: I haz the power, 2012-11-07 21:35:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/phajdan.jr@chromium.org/11228038/1

[commit-bot: I haz the power, 2012-11-07 21:37:03]

Presubmit check for 11228038-1 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/tools/build
    remoting
    chrome/browser/policy
    chrome
    dbus
    chrome/browser/safe_browsing
    chrome/browser/feedback
    chrome/common/metrics
    chrome/browser/sync_file_system
    chrome/browser/history
    chrome/browser/chromeos/contacts
    content/browser/speech
    chrome/browser/chromeos/drive
    chrome/browser/metrics

Presubmit checks took 3.8s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[Philippe, 2012-11-09 14:48:39]

FYI, this change breaks the Chrome for Android build downstream (when building
the 'cacheinvalidation' third-party project) :/

The problem is that we are also using Chromium's protobuf downstream to generate
Java code on the application side.

This change makes protoc generate Java code with the retain unknown fields
feature by default which is not supported by the Protobuf Java lite runtime.

I'm not sure what the right fix is. One simple thing we could do if your change
is really needed is to add 'option retain_unknown_fields = false;' to
cacheinvalidation's proto files (I'm not a big fan of this approach).

Do you know if we can override the 'retain_unknown_fields' option (or even any
option) on the command line when invoking protoc? I didn't find any option to do
that. This would be ideal.

I'm CC'ing Tommy Nyquist who knows more about cacheinvalidation and who is
currently adding protobuf Java (for cacheinvalidation I guess?) to the Chromium
tree.

[Miguel Garcia, 2012-11-09 15:23:46]

Yeah, in general it seems that these two options are contradictory. 

In order to generate things like the getUnknownFields() calls (which are only
available if you use the non lite library of the java protobuf library) the
compiler checks for

inline bool HasUnknownFields(const FileDescriptor *file) {
  return file->options().optimize_for() != FileOptions::LITE_RUNTIME ||
         file->options().retain_unknown_fields();
}

(see third_party/protobuf/src/google/protobuf/compiler/java/java_helpers.h and
third_party/protobuf/src/google/protobuf/compiler/java/java_message.cc)

So if we are trying hard to move to the lite runtime as the default it does seem
to make sense to make retain_unknown_fields true in descriptor.proto


On 2012/11/09 14:48:39, Philippe wrote:
> FYI, this change breaks the Chrome for Android build downstream (when building
> the 'cacheinvalidation' third-party project) :/
> 
> The problem is that we are also using Chromium's protobuf downstream to
generate
> Java code on the application side.
> 
> This change makes protoc generate Java code with the retain unknown fields
> feature by default which is not supported by the Protobuf Java lite runtime.
> 
> I'm not sure what the right fix is. One simple thing we could do if your
change
> is really needed is to add 'option retain_unknown_fields = false;' to
> cacheinvalidation's proto files (I'm not a big fan of this approach).
> 
> Do you know if we can override the 'retain_unknown_fields' option (or even any
> option) on the command line when invoking protoc? I didn't find any option to
do
> that. This would be ideal.
> 
> I'm CC'ing Tommy Nyquist who knows more about cacheinvalidation and who is
> currently adding protobuf Java (for cacheinvalidation I guess?) to the
Chromium
> tree.

[Paweł Hajdan Jr., 2012-11-09 17:43:47]

Sorry for the breakage. Some notes:

1. This change landed fine on Chromium waterfall, including Android bots.

2. Adding any "option retain_unknown_fields" to .proto files is going to break
my use case. :(

3. I think we just need to update our custom protobuf code for Java to retain
unknown fields by default, maybe just generate any .java files from
descriptor.proto if applicable.

What do you think?

[nyquist, 2012-11-09 17:50:20]

On 2012/11/09 17:43:47, Paweł Hajdan Jr. wrote:
> Sorry for the breakage. Some notes:
> 
> 1. This change landed fine on Chromium waterfall, including Android bots.
> 
> 2. Adding any "option retain_unknown_fields" to .proto files is going to break
> my use case. :(
> 
> 3. I think we just need to update our custom protobuf code for Java to retain
> unknown fields by default, maybe just generate any .java files from
> descriptor.proto if applicable.
> 
> What do you think?

I am working on upstreaming the cacheinvalidation_java target. Will hopefully
land very soon. That will give you visibility into java/Android next time.

The option retain_unknown_fields will only be added to Android-only protobufs.
Do you think that is OK?

[Paweł Hajdan Jr., 2012-11-09 20:38:12]

On 2012/11/09 17:50:20, nyquist wrote:
> The option retain_unknown_fields will only be added to Android-only protobufs.
> Do you think that is OK?

To Android-specific .protos that are not used at all for Chromium Linux - sounds
fine.

[nyquist, 2012-11-12 21:45:27]

Reverted in: http://codereview.chromium.org/11359146/