Update //third_party/protobuf to version 3.

third_party/protobuf/README.chromium

Index: third_party/protobuf/README.chromium
diff --git a/third_party/protobuf/README.chromium b/third_party/protobuf/README.chromium
index 507037960d0eb6407c29d9cf03cfd189f57c717d..374018debbc7d5ae11e5ee1c1efcfb30c001c459 100644
--- a/third_party/protobuf/README.chromium
+++ b/third_party/protobuf/README.chromium
@@ -1,52 +1,79 @@
 Name: Protocol Buffers
 Short Name: protobuf
-URL: http://protobuf.googlecode.com/svn/trunk
+URL: https://github.com/google/protobuf
 License: BSD
-License File: COPYING.txt
-Version: unknown
-Revision: r476
+License File: LICENSE
+Version: 3.0.0-beta-2

[Peter Kasting, 2016/04/06 22:35:39]

Nit: Add a Revision: field here as well.

[xyzzyz, 2016/04/07 18:55:02]

Done.

 Security Critical: yes
 
-Local files (not taken from upstream):
-README.chromium
-config.h
-descriptor2_pb.py
+Steps used to create the current version:
+1. Pull the release from https://github.com/google/protobuf/releases
+2. Use the cleanup_release.sh script to remove unnecessary files.
+3. Add build files (BUILD.gn, proto_library.gni, protobuf.py,

[Peter Kasting, 2016/04/06 22:35:39]

There is no proto_library.gni or protobuf.py.

[xyzzyz, 2016/04/07 18:55:02]

I mean protobuf.gyp (good catch), and with regards to proto_library.gni, it's
there, my patch just doesn't touch it.

+   protobuf_lite.gypi, protobuf_nacl.gyp).
+4. Add open-source library six.py to protobuf/third_party/six/six.py

[Peter Kasting, 2016/04/06 22:35:39]

Obtained how?

[xyzzyz, 2016/04/07 18:55:02]

Done.

+5. Apply patches in patches/ (see the description below):
 
-A protobuf.gyp file has been added for building with Chromium.
+   $ for patch in patches/*; do patch -s -p1 < $patch; done
 
-This code has been patched to support unknown field retention in protobuf-lite.
-See r62331 for the patch.
+   For future releases, it will be worth looking into which patches still need
+   to be applied.
+5. Generate descriptor_pb2.py using something like the following steps. Make
+   sure you've regenerated your buildfiles and will build protoc from the
+   newly-modified sources above.
 
-This code has been patched to ensure that files in the target protobuf_lite
-do not include headers from protobuf_full. See r173228 for the patch.
+   $ cd $SRC_DIR
+   $ ninja -C out/Debug protoc
+   $ cd third_party/protobuf/src
+   $ ../../../out/Debug/protoc --python_out=../python google/protobuf/descriptor.proto
+   $ cd ..

[Peter Kasting, 2016/04/06 22:35:39]

Nit: Omit this last line

[xyzzyz, 2016/04/07 18:55:02]

Done.

 
-This code has been patched to make the target protobuf_lite a component so that
-targets that depend on it can be componentized. See http://crbug.com/172800 for
-details, and r179806 for the patch.
+   Copy the resulting descriptor_pb2.py to protobuf/.
+6. Add empty __init__.py to protobuf/.

[Peter Kasting, 2016/04/06 22:35:39]

You should remove this step and instead make the build files copy from
python/google/__init__.py.  See e.g. the change in the copy_google step of
https://codereview.chromium.org/1291903002/diff/110001/third_party/protobuf/B...
.

[xyzzyz, 2016/04/07 18:55:03]

How do I deal with the six.py dependency then? Currently I add
pyproto/third_party/six/ to the python path in __init__.py, because protobuf
does import six here and there,

[Peter Kasting, 2016/04/07 23:25:38]

These instructions say to add an empty __init__.py, not one with content in it. 
It sounds like the instructions here are wrong?

Why not just put six.py in third_party/python/google/protobuf or some other
directory that's already in the import path, if we have to put it somewhere
anyway?

How does upstream ensure that this library gets imported successfully?

[xyzzyz, 2016/04/11 19:34:40]

Yeah, the instructions are wrong, I'll update them when we pin down what to do
with six.py.

third_party/python/google/protobuf is _not_ in runtime import path. The copy
rules in BUILD.gn/protobuf.gyp copy them to out/.../pyproto/..., and it's
pyproto that's added to import path. What's worse is that every user is setting
the python path on their own, like e.g. here:
https://code.google.com/p/chromium/codesearch#chromium/src/net/test/spawned_t...

The upstream doesn't do anything to ensure that it gets imported -- it doesn't
even ship it, it just expects it to be available.

My change ensures that anyone who imports anything from pyproto/ also has six in
their import path. Alternatively, I could have just dumped six.py straight into
obj/pyproto/. While there may be a better way of doing that, I don't see it.

[Peter Kasting, 2016/04/11 19:54:45]

I think I was thinking, "put six.py in third_party/python/google/protobuf, then
include it in the set of copied files," but if upstream just doesn't handle
this, then we can do practically anything; I would recommend doing whatever
involves the fewest steps for future maintainers upgrading to a new version of
libprotobuf.

[xyzzyz, 2016/04/11 20:13:28]

I'm keeping the current solution then, and updating the comment in
README.chromium.

+7. Update README.chromium.
 
-Revision 504 was cherry-picked from upstream.
-Revision 512 was cherry-picked from upstream.
-Revision 516 was cherry-picked from upstream.
-Revision 517 was cherry-picked from upstream.
-Revision 522 was cherry-picked from upstream.
-Revision 523 was cherry-picked from upstream.
-Revision 524 was cherry-picked from upstream.
-Revision 573 was cherry-picked from upstream.
-The `&file->options() != NULL &&` was removed from descriptor.cc
-References to `vector` were changed to `std::vector` in extension_set.h
-and unknown_field_set.h
+Description of the patches:
 
-GetEmptyString() and GoogleOnceInit() have been uninlined, for a large savings
-in binary size.
+- 0001-ignore-option-retain-unknown-fields.patch
 
-A BUILD.gn file has been added for building with GN.
+  Previous versions of protobuf in Chromium carried a local patch that retained
+  unknown fields in protobuf_lite mode. It was enabled by setting option
+  retain_unknown_fields = true in .proto file. Now that it is enabled by
+  default, this option is no longer recognized by protobuf, and so I had to
+  patch it so that I don't have to fix all .proto files in Chromium in a single
+  CL.

[Peter Kasting, 2016/04/06 22:35:39]

Unless you're going to be removing those options and then this patch immediately
after landing this, you should explicitly say that those files should be fixed
and this patch removed.

[xyzzyz, 2016/04/07 18:55:02]

Done.

 
-Cherry-pick pherl changes to make protobuf build on VS2015.
+- 0002-remove-support-for-unnecessary-languages.patch
 
-Cherry-pick c3cb53b (fix "sometimes-uninitialized" warning).
+  This patch removes support for C#, Ruby, Objective-C, JS and other languages not
+  used in Chromium from protoc, to decrease the package size.
 
-Cherry-pick https://github.com/google/protobuf/commit/56a90a2081379a5
+- 0003-remove-static-initializers.patch

[Peter Kasting, 2016/04/06 22:35:39]

For this and most other patches below, are you in the process of upstreaming
those so we can eliminate them from our local tree?

[xyzzyz, 2016/04/07 18:55:02]

Yes. However, not all of the stuff is likely to end up in the upstream. For
instance, upstreaming s/#include <iostream>/#include <ostream>/ was successful,
fixing integer types is also likely to succeed. 

On the other hand, I don't think changes to Status class are likely to be
accepted, as they change the API -- what I did was to create a POD-struct that
is implicitly convertible to Status, and replaced statically initialized
instances of Status class (that is, Status::OK etc) with these PODs. This way,
when e.g. somebody has a function that should return util::Status, and they do
`return Status::OK`, the StatusPod object Status::OK is implicitly converted to
Status instance, and everything works fine. This was enough to make all uses of
Status in protobuf work, but since StatusPod is not exactly the same as Status,
not everything is the same. For one thing, the unit tests don't pass -- e.g.
they have EXPECT_EQ(Status::OK, x), where x is StatusOr<...> instance. There's
an implicit conversion from Status to StatusOr, but since you cannot chain
implicit conversions, I would have to add an implicit conversion from StatusPod
to StatusOr. It's not the only problem, and even if I fix all of the unit tests,
we would still risk breaking users of upstream protobuf. 

See https://github.com/google/protobuf/pull/1367

[Peter Kasting, 2016/04/07 23:25:38]

So you made the Status changes as a way of fixing static initializers?

Upstream is receptive to fixing static initializers since they are banned in
google3 as well.  Depending on what you mean by "change the API" they may be
willing to accept something like your change.  If not, there ought to be another
way to fix this that is acceptable, e.g. dynamically initialize these sorts of
objects on first use instead of statically doing so.  If you can't come up with
anything workable, ask chromium-dev for help.

[xyzzyz, 2016/04/11 19:34:40]

I agree it would be nice to fix the upstream for good, and I know they're
receptive, but I think this change would be complex enough that I'd prefer to
use the current solution here and then cherry-pick upstream when it's solved
there.

[Peter Kasting, 2016/04/11 19:54:45]

I'm OK in principle with "solve locally, upstream in parallel", I just want to
make sure whatever local solution we use is the thing we believe in upstreaming.
 It sounded as if this was a patch that you never intended to upstream, in which
case my hope was to at least find what the "right way" of doing this might be,
even if we didn't wait on upstream approving/accepting that patch before using
it here.

That will minimize the chance that we land this locally, do something completely
different upstream, fail to pull down the differences and update these local
patches, and leave some future person blindly applying this change when it's not
necessary (which is exactly what happened to me with the "preserve unknown
fields" stuff).

[xyzzyz, 2016/04/11 20:13:28]

I understand your concerns. I'm having hard time though figuring out the proper
solution here. The API itself depends on things being statically initialized,
and fixing would require changes in the API -- the statically initialized
objects must ultimately be removed if we don't want to have static initializers.

[Peter Kasting, 2016/04/11 20:16:55]

Why doesn't "dynamically initialize on first use" work?  That's what I did in
similar places last time IIRC.

[xyzzyz, 2016/04/11 20:30:10]

If I understand what you mean correctly, that would involve replacing static
object with a pointer, or a function, which is a change in the API itself.

[Peter Kasting, 2016/04/11 20:54:51]

OK, I looked again at the patch to re-familiarize myself with what we're talking
about.

I think an API change is indeed the best way to deal with this.  There are
basically three ways to do this:

(1) The route you've gone
(2) Replace these objects with functions that return a Status (e.g. Status
OKStatus() const { ... }), which can either just construct the appropriate
Status each time or, if necessary, do something like a function-scope static
object that gets initted the first time
(3) Add relevant functions on Status so callers would do something like
"status.isOK()" instead of "status == Status::OK"

Maybe there are more, I dunno.

I would at least get word from the libprotobuf maintainers on whether any of
these are acceptable and what they'd prefer.

[xyzzyz, 2016/04/11 21:08:33]

I'm in the process of doing that:

https://github.com/google/protobuf/pull/1367
https://github.com/google/protobuf/issues/1404

It could take a while until we come to the conclusion though.

If this was up to me, I'd remove these altogether, and instead add constructor
"explicit Status(error::Code code)" so that one can replace "Status::OK" with
"Status(error::OK)", and similarly with Status::UNKNOWN and CANCELLED. There's
already Status::ok() method that checks if `this` is an OK status.

[Peter Kasting, 2016/04/11 21:20:49]

Thanks for those links.

Let's give them a day or two to respond; if nothing happens by then maybe ping
the right people directly internally, and if we still can't get some quick
feedback go ahead and proceed here.

In the meantime, I'll re-review.

 
-Cherry-pick https://github.com/google/protobuf/commit/69d660b39ceabea
+  This patch removes all static initializers from Chromium. The change in Status
+  class is not completely compatible, but it's compatible enough to work in

[Peter Kasting, 2016/04/06 22:35:39]

Not completely compatible with what?

[xyzzyz, 2016/04/07 18:55:03]

With upstream, see my comment above.

+  Chromium, which doesn't use this functionality yet.
 
-Cherry-pick https://github.com/google/protobuf/commit/a74e912a8be1274
+- 0004-fix-integer-types-and-shared-library-exports.patch
+
+  This patch fixes some unfortunate choices for integer types corresponding to

[Peter Kasting, 2016/04/06 22:35:39]

Nit: "unfortunate choices" is vague, be specific about the problem you're
fixing.

[xyzzyz, 2016/04/07 18:55:02]

Done.

+  proto's int64 and others. It also allows exporting protobuf symbols in Linux
+  .so libraries.
+
+- 0005-fix-no-threadlocal-on-mac.patch
+
+  Apple's Clang doesn't support thread-local variables on Mac.
+
+- 0006-fix-clang-type-errors.patch
+
+  New versions of Clang have type checking semantics that check the conformance
+  with the C++ standard more thorougly, which makes them a bit more
+  restrictive. This is actually cherry-pick from protobuf upstream.
+
+- 0007-uninline_googleonce.patch
+- 0008-uninline_get_empty_string.patch
+- 0009-uninline-arenastring.patch
+- 0010-uninline-generated-code.patch
+
+  These patches uninline some functions that results in a significant reduction

[Peter Kasting, 2016/04/06 22:35:39]

Nit: "functions that results" -> "functions, resulting"

[xyzzyz, 2016/04/07 18:55:03]

Done.

+  (somewhere between 500 KB and 1 MB) of binary size.