Update //third_party/protobuf to version 3.

Update //third_party/protobuf to version 3.

For context, see https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/zJZPkxEFVxY

For the things to look out for while reviewing this, see the doc https://docs.google.com/document/d/1qjSBV2ioi0mykAT1eSXuMaNqO0BIS-j1PeYfxT-TbW4/edit?usp=sharing

BUG=597321
Committed: https://crrev.com/7cf8148f570a1072928fa9ede6e0760edc137cbd
Cr-Commit-Position: refs/heads/master@{#388596}

[xyzzyz, 2016-04-05 21:26:28]

Description was changed from

==========
Update //third_party/protobuf to version 3.

BUG=597321
==========

to

==========
Update //third_party/protobuf to version 3.

For context, see
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/zJZPkxEFVxY

For the things to look out for while reviewing this, see the doc
https://docs.google.com/document/d/1qjSBV2ioi0mykAT1eSXuMaNqO0BIS-j1PeYfxT-Tb...

BUG=597321
==========

[Nicolas Zea, 2016-04-05 22:47:35]

zea@chromium.org changed reviewers:
+ zea@chromium.org

[Nicolas Zea, 2016-04-05 22:47:40]

https://codereview.chromium.org/1842653006/diff/10001/sync/internal_api/sync_...
File sync/internal_api/sync_manager_impl_unittest.cc (right):

https://codereview.chromium.org/1842653006/diff/10001/sync/internal_api/sync_...
sync/internal_api/sync_manager_impl_unittest.cc:636:
::google::protobuf::io::StringOutputStream unknown_fields_stram(
stram -> stream

https://codereview.chromium.org/1842653006/diff/10001/sync/internal_api/sync_...
sync/internal_api/sync_manager_impl_unittest.cc:649:
EXPECT_FALSE(node.GetEntitySpecifics().unknown_fields().empty());
What do you think about verifying the values in the unknown fields? In
particular, that the tag and value are correct?

[xyzzyz, 2016-04-06 17:22:53]

https://codereview.chromium.org/1842653006/diff/10001/sync/internal_api/sync_...
File sync/internal_api/sync_manager_impl_unittest.cc (right):

https://codereview.chromium.org/1842653006/diff/10001/sync/internal_api/sync_...
sync/internal_api/sync_manager_impl_unittest.cc:636:
::google::protobuf::io::StringOutputStream unknown_fields_stram(
On 2016/04/05 22:47:40, Nicolas Zea wrote:
> stram -> stream

Good catch!

https://codereview.chromium.org/1842653006/diff/10001/sync/internal_api/sync_...
sync/internal_api/sync_manager_impl_unittest.cc:649:
EXPECT_FALSE(node.GetEntitySpecifics().unknown_fields().empty());
On 2016/04/05 22:47:40, Nicolas Zea wrote:
> What do you think about verifying the values in the unknown fields? In
> particular, that the tag and value are correct?

I added some verification, please take a look.

[xyzzyz, 2016-04-06 20:33:53]

xyzzyz@chromium.org changed reviewers:
+ brettw@chromium.org, pkasting@chromium.org

[Nicolas Zea, 2016-04-06 21:41:38]

sync LGTM, thanks!

[Peter Kasting, 2016-04-06 22:35:40]

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/BU...
File third_party/protobuf/BUILD.gn (right):

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/BU...
third_party/protobuf/BUILD.gn:1: # Copyright 2016 The Chromium Authors. All
rights reserved.
Nit: Don't update copyrights when touching files

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/BU...
third_party/protobuf/BUILD.gn:10: "HAVE_ZLIB",
Do you really need this?

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/BU...
third_party/protobuf/BUILD.gn:13: defines += [ "HAVE_PTHREAD" ]
I don't think you actually need to exclude Windows here; IIRC the protobuf
sources should ignore this for Windows.

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/BU...
third_party/protobuf/BUILD.gn:20: # protobuf-3 contains a few functions that are
unused.
Can these be removed upstream via a patch?

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/BU...
third_party/protobuf/BUILD.gn:44: "src/google/protobuf/arenastring.h",
See my comments in protobuf_lite.gypi regarding this filelist.

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/BU...
third_party/protobuf/BUILD.gn:97: "/wd4065",  # switch statement contains
'default' but no 'case' labels
Keep this list in sorted order.

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/BU...
third_party/protobuf/BUILD.gn:178: #     
"src/google/protobuf/io/gzip_stream.h",
Add the comment from
https://codereview.chromium.org/1291903002/diff/110001/third_party/protobuf/B...
that explains why you don't need these.  (This also explains why -- I think --
you don't need the HAVE_ZLIB defines.)

I would reduce the number of spaces between '#' and the filename to 1.

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/BU...
third_party/protobuf/BUILD.gn:352: "__init__.py",
Copy location needs to be updated, see comments in README.chromium.

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/BU...
third_party/protobuf/BUILD.gn:393: "descriptor_pb2.py",
You should move this into python/google/protobuf/ and update the README as well.
 See changes in
https://codereview.chromium.org/1291903002/diff/110001/third_party/protobuf/B...
.

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/OW...
File third_party/protobuf/OWNERS (left):

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/OW...
third_party/protobuf/OWNERS:2: scottmg@chromium.org
You shouldn't be deleting this file.  We need OWNERS.

You should probably change the contents of this to list yourself as OWNER
instead.

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/RE...
File third_party/protobuf/README.chromium (right):

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:6: Version: 3.0.0-beta-2
Nit: Add a Revision: field here as well.

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:12: 3. Add build files (BUILD.gn,
proto_library.gni, protobuf.py,
There is no proto_library.gni or protobuf.py.

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:14: 4. Add open-source library six.py to
protobuf/third_party/six/six.py
Obtained how?

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:29: $ cd ..
Nit: Omit this last line

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:32: 6. Add empty __init__.py to protobuf/.
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...
.

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:44: CL.
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.

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:51: - 0003-remove-static-initializers.patch
For this and most other patches below, are you in the process of upstreaming
those so we can eliminate them from our local tree?

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:54: class is not completely compatible, but
it's compatible enough to work in
Not completely compatible with what?

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:59: This patch fixes some unfortunate
choices for integer types corresponding to
Nit: "unfortunate choices" is vague, be specific about the problem you're
fixing.

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:78: These patches uninline some functions
that results in a significant reduction
Nit: "functions that results" -> "functions, resulting"

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/cl...
File third_party/protobuf/cleanup_release.sh (right):

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/cl...
third_party/protobuf/cleanup_release.sh:1: #!/bin/bash
Why do we need this script?

It won't affect the build or output of Chromium at all.  The only thing it will
do is slightly reduce the source code footprint on disk.

Against that small upside, we have the maintenance downside that
(a) This could break future updates if they pout a necessary file in one of
these places
(b) It's not obvious how to create this list or if it's exhaustive, so it's not
clear how maintainers should modify it in the future, so over time it will have
an increasing number of coverage gaps

I think we should remove this script entirely and you should check the full
upstream release into the tree.

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/pr...
File third_party/protobuf/protobuf.gyp (right):

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/pr...
third_party/protobuf/protobuf.gyp:1: # Copyright (c) 2012 The Chromium Authors.
All rights reserved.
For review comments for this file, see my comments in BUILD.gn, which basically
all apply here too.

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/pr...
File third_party/protobuf/protobuf_lite.gypi (right):

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/pr...
third_party/protobuf/protobuf_lite.gypi:11:
'src/google/protobuf/extension_set.cc',
This file seems to be missing a lot of the files I had added in
https://codereview.chromium.org/1291903002/diff/110001/third_party/protobuf/p...
, primarily headers (e.g. here it's missing descriptor.h and descriptor_pb.h). 
Did you determine these weren't needed?  (I had constructed my list from the
transitive #includes of all the source files needed, but another way to check
this would be to delete all those headers out of your local source tree
completely and see if the compile still works).

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/pr...
File third_party/protobuf/protobuf_nacl.gyp (right):

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/pr...
third_party/protobuf/protobuf_nacl.gyp:19: 'pnacl_compile_flags': [
You should be able to remove this block, I think.

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/pr...
third_party/protobuf/protobuf_nacl.gyp:33: 'HAVE_ZLIB',
Do you really need this?

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/pr...
third_party/protobuf/protobuf_nacl.gyp:34: 'HAVE_PTHREAD',
In my roll attempt I set this in protobuf_lite.gypi instead.  Why doesn't it
need to be set there (in which case you don't need the one here)?

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

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/BU...
File third_party/protobuf/BUILD.gn (right):

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/BU...
third_party/protobuf/BUILD.gn:1: # Copyright 2016 The Chromium Authors. All
rights reserved.
On 2016/04/06 22:35:39, Peter Kasting wrote:
> Nit: Don't update copyrights when touching files

Huh, I don't remember doing it. Weird.

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/BU...
third_party/protobuf/BUILD.gn:10: "HAVE_ZLIB",
On 2016/04/06 22:35:39, Peter Kasting wrote:
> Do you really need this?

I actually don't, thanks!

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/BU...
third_party/protobuf/BUILD.gn:13: defines += [ "HAVE_PTHREAD" ]
On 2016/04/06 22:35:40, Peter Kasting wrote:
> I don't think you actually need to exclude Windows here; IIRC the protobuf
> sources should ignore this for Windows.

Yeah, the way the ifdefs in protobuf are structured makes it unnecessary, but I
think this way it's less likely to break with future protobuf releases, should
they change it.

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/BU...
third_party/protobuf/BUILD.gn:20: # protobuf-3 contains a few functions that are
unused.
On 2016/04/06 22:35:39, Peter Kasting wrote:
> Can these be removed upstream via a patch?

This is my plan -- I already made some pull requests for things I found doing
stuff here, e.g. https://github.com/google/protobuf/pull/1366

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/BU...
third_party/protobuf/BUILD.gn:44: "src/google/protobuf/arenastring.h",
On 2016/04/06 22:35:40, Peter Kasting wrote:
> See my comments in protobuf_lite.gypi regarding this filelist.

See my answer there.

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/BU...
third_party/protobuf/BUILD.gn:97: "/wd4065",  # switch statement contains
'default' but no 'case' labels
On 2016/04/06 22:35:39, Peter Kasting wrote:
> Keep this list in sorted order.

Done.

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/BU...
third_party/protobuf/BUILD.gn:178: #     
"src/google/protobuf/io/gzip_stream.h",
On 2016/04/06 22:35:40, Peter Kasting wrote:
> Add the comment from
>
https://codereview.chromium.org/1291903002/diff/110001/third_party/protobuf/B...
> that explains why you don't need these.  (This also explains why -- I think --
> you don't need the HAVE_ZLIB defines.)
> 
> I would reduce the number of spaces between '#' and the filename to 1.

Done.

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/BU...
third_party/protobuf/BUILD.gn:352: "__init__.py",
On 2016/04/06 22:35:39, Peter Kasting wrote:
> Copy location needs to be updated, see comments in README.chromium.

See my response there.

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/BU...
third_party/protobuf/BUILD.gn:393: "descriptor_pb2.py",
On 2016/04/06 22:35:39, Peter Kasting wrote:
> You should move this into python/google/protobuf/ and update the README as
well.
>  See changes in
>
https://codereview.chromium.org/1291903002/diff/110001/third_party/protobuf/B...
> .

Done.

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/OW...
File third_party/protobuf/OWNERS (left):

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/OW...
third_party/protobuf/OWNERS:2: scottmg@chromium.org
On 2016/04/06 22:35:39, Peter Kasting wrote:
> You shouldn't be deleting this file.  We need OWNERS.
> 
> You should probably change the contents of this to list yourself as OWNER
> instead.

Ah, the deletion is an accident. 

If I list myself as an owner, I still need a second owner too. Who would be a
good candidate?

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/RE...
File third_party/protobuf/README.chromium (right):

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:6: Version: 3.0.0-beta-2
On 2016/04/06 22:35:39, Peter Kasting wrote:
> Nit: Add a Revision: field here as well.

Done.

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:12: 3. Add build files (BUILD.gn,
proto_library.gni, protobuf.py,
On 2016/04/06 22:35:39, Peter Kasting wrote:
> There is no proto_library.gni or protobuf.py.

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

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:14: 4. Add open-source library six.py to
protobuf/third_party/six/six.py
On 2016/04/06 22:35:39, Peter Kasting wrote:
> Obtained how?

Done.

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:29: $ cd ..
On 2016/04/06 22:35:39, Peter Kasting wrote:
> Nit: Omit this last line

Done.

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:32: 6. Add empty __init__.py to protobuf/.
On 2016/04/06 22:35:39, Peter Kasting wrote:
> 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...
> .

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,

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:44: CL.
On 2016/04/06 22:35:39, Peter Kasting wrote:
> 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.

Done.

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:51: - 0003-remove-static-initializers.patch
On 2016/04/06 22:35:39, Peter Kasting wrote:
> For this and most other patches below, are you in the process of upstreaming
> those so we can eliminate them from our local tree?

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

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:54: class is not completely compatible, but
it's compatible enough to work in
On 2016/04/06 22:35:39, Peter Kasting wrote:
> Not completely compatible with what?

With upstream, see my comment above.

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:59: This patch fixes some unfortunate
choices for integer types corresponding to
On 2016/04/06 22:35:39, Peter Kasting wrote:
> Nit: "unfortunate choices" is vague, be specific about the problem you're
> fixing.

Done.

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:78: These patches uninline some functions
that results in a significant reduction
On 2016/04/06 22:35:39, Peter Kasting wrote:
> Nit: "functions that results" -> "functions, resulting"

Done.

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/cl...
File third_party/protobuf/cleanup_release.sh (right):

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/cl...
third_party/protobuf/cleanup_release.sh:1: #!/bin/bash
On 2016/04/06 22:35:39, Peter Kasting wrote:
> Why do we need this script?
> 
> It won't affect the build or output of Chromium at all.  The only thing it
will
> do is slightly reduce the source code footprint on disk.
> 
> Against that small upside, we have the maintenance downside that
> (a) This could break future updates if they pout a necessary file in one of
> these places
> (b) It's not obvious how to create this list or if it's exhaustive, so it's
not
> clear how maintainers should modify it in the future, so over time it will
have
> an increasing number of coverage gaps
> 
> I think we should remove this script entirely and you should check the full
> upstream release into the tree.

Along with the patch 0002, it affects protoc slightly. I removed unnecessary
stuff, because most other libraries in third_party seem to also do it. Having
cleaner repository makes it easier to work with, and the Chromium build/code
review tools are a total pain to work with if your CLs are huge.

I don't think it's very hard to create this list. If it break future update, the
person doing the update will not have a hard time finding what went wrong.

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/pr...
File third_party/protobuf/protobuf_lite.gypi (right):

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/pr...
third_party/protobuf/protobuf_lite.gypi:11:
'src/google/protobuf/extension_set.cc',
On 2016/04/06 22:35:39, Peter Kasting wrote:
> This file seems to be missing a lot of the files I had added in
>
https://codereview.chromium.org/1291903002/diff/110001/third_party/protobuf/p...
> , primarily headers (e.g. here it's missing descriptor.h and descriptor_pb.h).

> Did you determine these weren't needed?  (I had constructed my list from the
> transitive #includes of all the source files needed, but another way to check
> this would be to delete all those headers out of your local source tree
> completely and see if the compile still works).

This list was prepared by copying the list from the upstream Bazel BUILD file,
and adding .h for every .cc file.

https://github.com/google/protobuf/blob/master/BUILD

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/pr...
File third_party/protobuf/protobuf_nacl.gyp (right):

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/pr...
third_party/protobuf/protobuf_nacl.gyp:19: 'pnacl_compile_flags': [
On 2016/04/06 22:35:39, Peter Kasting wrote:
> You should be able to remove this block, I think.

Done.

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/pr...
third_party/protobuf/protobuf_nacl.gyp:33: 'HAVE_ZLIB',
On 2016/04/06 22:35:39, Peter Kasting wrote:
> Do you really need this?

Seems like I actually don't. Thanks!

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/pr...
third_party/protobuf/protobuf_nacl.gyp:34: 'HAVE_PTHREAD',
On 2016/04/06 22:35:39, Peter Kasting wrote:
> In my roll attempt I set this in protobuf_lite.gypi instead.  Why doesn't it
> need to be set there (in which case you don't need the one here)?

I set it in protobuf.gyp, and it seems you're right that I don't need to set it
here. Do you think it would be better to set it in protobuf_lite.gyp?

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

Note that you should still fix the NaCl compile issue noted in the Google Doc.

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/OW...
File third_party/protobuf/OWNERS (left):

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/OW...
third_party/protobuf/OWNERS:2: scottmg@chromium.org
On 2016/04/07 18:55:02, xyzzyz wrote:
> On 2016/04/06 22:35:39, Peter Kasting wrote:
> > You shouldn't be deleting this file.  We need OWNERS.
> > 
> > You should probably change the contents of this to list yourself as OWNER
> > instead.
> 
> Ah, the deletion is an accident. 
> 
> If I list myself as an owner, I still need a second owner too. Who would be a
> good candidate?

List me, as I've done the most work with this dir in recent memory.

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/RE...
File third_party/protobuf/README.chromium (right):

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:32: 6. Add empty __init__.py to protobuf/.
On 2016/04/07 18:55:03, xyzzyz wrote:
> On 2016/04/06 22:35:39, Peter Kasting wrote:
> > 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...
> > .
> 
> 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,

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?

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:51: - 0003-remove-static-initializers.patch
On 2016/04/07 18:55:02, xyzzyz wrote:
> On 2016/04/06 22:35:39, Peter Kasting wrote:
> > For this and most other patches below, are you in the process of upstreaming
> > those so we can eliminate them from our local tree?
> 
> 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

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.

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/cl...
File third_party/protobuf/cleanup_release.sh (right):

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/cl...
third_party/protobuf/cleanup_release.sh:1: #!/bin/bash
On 2016/04/07 18:55:02, xyzzyz wrote:
> On 2016/04/06 22:35:39, Peter Kasting wrote:
> > Why do we need this script?
> > 
> > It won't affect the build or output of Chromium at all.  The only thing it
> will
> > do is slightly reduce the source code footprint on disk.
> > 
> > Against that small upside, we have the maintenance downside that
> > (a) This could break future updates if they pout a necessary file in one of
> > these places
> > (b) It's not obvious how to create this list or if it's exhaustive, so it's
> not
> > clear how maintainers should modify it in the future, so over time it will
> have
> > an increasing number of coverage gaps
> > 
> > I think we should remove this script entirely and you should check the full
> > upstream release into the tree.
> 
> Along with the patch 0002, it affects protoc slightly.

In what way?  protoc won't support these inputs anymore?  That shouldn't be a
meaningful change from Chromium's perspective.

> I removed unnecessary
> stuff, because most other libraries in third_party seem to also do it.

Definitely don't assume that any practice of anything in third_party/ is
desirable or should be a general pattern :).

> Having
> cleaner repository makes it easier to work with, and the Chromium build/code
> review tools are a total pain to work with if your CLs are huge.

I don't think this makes any meaningful difference.  This CL is already far
larger than the tools can deal with perfectly (e.g. "download patch" is
disabled).  The difference between "way too huge" and "slightly larger than way
too huge" is negligible.

I really can't see any meaningful benefit of this.  It's of far more value to
have a repo copy that exactly matches upstream than one that has a bunch of
diffs due to missing files.  That makes it easier to tell what's different when
you download a new copy into the old location.  I considered and rejected this
idea of pruning these bits of the repository during my previous update attempt
in part for this reason.

> I don't think it's very hard to create this list. If it break future update,
the
> person doing the update will not have a hard time finding what went wrong.

Having been the person who tried to do the previous update, I would not have
wanted to parse this list and figure out how to map it onto the current repo
state.

I continue to think you should undo this step and remove this script.  There's a
reason I didn't add it last time, and past maintainers before me didn't have
this.

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/pr...
File third_party/protobuf/protobuf_lite.gypi (right):

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/pr...
third_party/protobuf/protobuf_lite.gypi:11:
'src/google/protobuf/extension_set.cc',
On 2016/04/07 18:55:02, xyzzyz wrote:
> On 2016/04/06 22:35:39, Peter Kasting wrote:
> > This file seems to be missing a lot of the files I had added in
> >
>
https://codereview.chromium.org/1291903002/diff/110001/third_party/protobuf/p...
> > , primarily headers (e.g. here it's missing descriptor.h and
descriptor_pb.h).
> 
> > Did you determine these weren't needed?  (I had constructed my list from the
> > transitive #includes of all the source files needed, but another way to
check
> > this would be to delete all those headers out of your local source tree
> > completely and see if the compile still works).
> 
> This list was prepared by copying the list from the upstream Bazel BUILD file,
> and adding .h for every .cc file.
> 
> https://github.com/google/protobuf/blob/master/BUILD

We need to list all transitively-included headers, or when someone touches one
of those headers the project may not be rebuilt correctly.

If you have a tool that can do this, awesome, but I did it by literally looking
at all the #includes of every file manually.  It went surprisingly quickly.

When you come up with a list, compare it with the list in my previous update
attempt and ensure any differences are actually correct.

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/pr...
File third_party/protobuf/protobuf_nacl.gyp (right):

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/pr...
third_party/protobuf/protobuf_nacl.gyp:34: 'HAVE_PTHREAD',
On 2016/04/07 18:55:02, xyzzyz wrote:
> On 2016/04/06 22:35:39, Peter Kasting wrote:
> > In my roll attempt I set this in protobuf_lite.gypi instead.  Why doesn't it
> > need to be set there (in which case you don't need the one here)?
> 
> I set it in protobuf.gyp, and it seems you're right that I don't need to set
it
> here. Do you think it would be better to set it in protobuf_lite.gyp?

If it affects files listed in protobuf_lit.gypi, then it should be set in
protobuf_lite.gypi.  Otherwise if someone creates a new .gyp that includes that
.gypi, they won't get the #define, and things will be a mess.

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

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/OW...
File third_party/protobuf/OWNERS (left):

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/OW...
third_party/protobuf/OWNERS:2: scottmg@chromium.org
On 2016/04/07 23:25:38, Peter Kasting wrote:
> On 2016/04/07 18:55:02, xyzzyz wrote:
> > On 2016/04/06 22:35:39, Peter Kasting wrote:
> > > You shouldn't be deleting this file.  We need OWNERS.
> > > 
> > > You should probably change the contents of this to list yourself as OWNER
> > > instead.
> > 
> > Ah, the deletion is an accident. 
> > 
> > If I list myself as an owner, I still need a second owner too. Who would be
a
> > good candidate?
> 
> List me, as I've done the most work with this dir in recent memory.

Done.

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/RE...
File third_party/protobuf/README.chromium (right):

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:32: 6. Add empty __init__.py to protobuf/.
On 2016/04/07 23:25:38, Peter Kasting wrote:
> On 2016/04/07 18:55:03, xyzzyz wrote:
> > On 2016/04/06 22:35:39, Peter Kasting wrote:
> > > 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...
> > > .
> > 
> > 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,
> 
> 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?

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.

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:51: - 0003-remove-static-initializers.patch
On 2016/04/07 23:25:38, Peter Kasting wrote:
> On 2016/04/07 18:55:02, xyzzyz wrote:
> > On 2016/04/06 22:35:39, Peter Kasting wrote:
> > > For this and most other patches below, are you in the process of
upstreaming
> > > those so we can eliminate them from our local tree?
> > 
> > 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
> 
> 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.

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.

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/pr...
File third_party/protobuf/protobuf_lite.gypi (right):

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/pr...
third_party/protobuf/protobuf_lite.gypi:11:
'src/google/protobuf/extension_set.cc',
On 2016/04/07 23:25:38, Peter Kasting wrote:
> On 2016/04/07 18:55:02, xyzzyz wrote:
> > On 2016/04/06 22:35:39, Peter Kasting wrote:
> > > This file seems to be missing a lot of the files I had added in
> > >
> >
>
https://codereview.chromium.org/1291903002/diff/110001/third_party/protobuf/p...
> > > , primarily headers (e.g. here it's missing descriptor.h and
> descriptor_pb.h).
> > 
> > > Did you determine these weren't needed?  (I had constructed my list from
the
> > > transitive #includes of all the source files needed, but another way to
> check
> > > this would be to delete all those headers out of your local source tree
> > > completely and see if the compile still works).
> > 
> > This list was prepared by copying the list from the upstream Bazel BUILD
file,
> > and adding .h for every .cc file.
> > 
> > https://github.com/google/protobuf/blob/master/BUILD
> 
> We need to list all transitively-included headers, or when someone touches one
> of those headers the project may not be rebuilt correctly.
> 
> If you have a tool that can do this, awesome, but I did it by literally
looking
> at all the #includes of every file manually.  It went surprisingly quickly.
> 
> When you come up with a list, compare it with the list in my previous update
> attempt and ensure any differences are actually correct.

I think I got them all this time.

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/pr...
File third_party/protobuf/protobuf_nacl.gyp (right):

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/pr...
third_party/protobuf/protobuf_nacl.gyp:34: 'HAVE_PTHREAD',
On 2016/04/07 23:25:38, Peter Kasting wrote:
> On 2016/04/07 18:55:02, xyzzyz wrote:
> > On 2016/04/06 22:35:39, Peter Kasting wrote:
> > > In my roll attempt I set this in protobuf_lite.gypi instead.  Why doesn't
it
> > > need to be set there (in which case you don't need the one here)?
> > 
> > I set it in protobuf.gyp, and it seems you're right that I don't need to set
> it
> > here. Do you think it would be better to set it in protobuf_lite.gyp?
> 
> If it affects files listed in protobuf_lit.gypi, then it should be set in
> protobuf_lite.gypi.  Otherwise if someone creates a new .gyp that includes
that
> .gypi, they won't get the #define, and things will be a mess.

Done.

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

Have not re-reviewed

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/RE...
File third_party/protobuf/README.chromium (right):

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:32: 6. Add empty __init__.py to protobuf/.
On 2016/04/11 19:34:40, xyzzyz wrote:
> On 2016/04/07 23:25:38, Peter Kasting wrote:
> > On 2016/04/07 18:55:03, xyzzyz wrote:
> > > On 2016/04/06 22:35:39, Peter Kasting wrote:
> > > > 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...
> > > > .
> > > 
> > > 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,
> > 
> > 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?
> 
> 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.

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.

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:51: - 0003-remove-static-initializers.patch
On 2016/04/11 19:34:40, xyzzyz wrote:
> On 2016/04/07 23:25:38, Peter Kasting wrote:
> > On 2016/04/07 18:55:02, xyzzyz wrote:
> > > On 2016/04/06 22:35:39, Peter Kasting wrote:
> > > > For this and most other patches below, are you in the process of
> upstreaming
> > > > those so we can eliminate them from our local tree?
> > > 
> > > 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
> > 
> > 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.
> 
> 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.

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]

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/RE...
File third_party/protobuf/README.chromium (right):

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:32: 6. Add empty __init__.py to protobuf/.
On 2016/04/11 19:54:45, Peter Kasting wrote:
> On 2016/04/11 19:34:40, xyzzyz wrote:
> > On 2016/04/07 23:25:38, Peter Kasting wrote:
> > > On 2016/04/07 18:55:03, xyzzyz wrote:
> > > > On 2016/04/06 22:35:39, Peter Kasting wrote:
> > > > > 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...
> > > > > .
> > > > 
> > > > 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,
> > > 
> > > 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?
> > 
> > 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.
> 
> 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.

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

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:51: - 0003-remove-static-initializers.patch
On 2016/04/11 19:54:45, Peter Kasting wrote:
> On 2016/04/11 19:34:40, xyzzyz wrote:
> > On 2016/04/07 23:25:38, Peter Kasting wrote:
> > > On 2016/04/07 18:55:02, xyzzyz wrote:
> > > > On 2016/04/06 22:35:39, Peter Kasting wrote:
> > > > > For this and most other patches below, are you in the process of
> > upstreaming
> > > > > those so we can eliminate them from our local tree?
> > > > 
> > > > 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
> > > 
> > > 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.
> > 
> > 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.
> 
> 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).

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]

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/RE...
File third_party/protobuf/README.chromium (right):

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:51: - 0003-remove-static-initializers.patch
On 2016/04/11 20:13:28, xyzzyz wrote:
> On 2016/04/11 19:54:45, Peter Kasting wrote:
> > On 2016/04/11 19:34:40, xyzzyz wrote:
> > > On 2016/04/07 23:25:38, Peter Kasting wrote:
> > > > On 2016/04/07 18:55:02, xyzzyz wrote:
> > > > > On 2016/04/06 22:35:39, Peter Kasting wrote:
> > > > > > For this and most other patches below, are you in the process of
> > > upstreaming
> > > > > > those so we can eliminate them from our local tree?
> > > > > 
> > > > > 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
> > > > 
> > > > 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.
> > > 
> > > 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.
> > 
> > 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).
> 
> 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.

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:11]

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/RE...
File third_party/protobuf/README.chromium (right):

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:51: - 0003-remove-static-initializers.patch
On 2016/04/11 20:16:55, Peter Kasting wrote:
> On 2016/04/11 20:13:28, xyzzyz wrote:
> > On 2016/04/11 19:54:45, Peter Kasting wrote:
> > > On 2016/04/11 19:34:40, xyzzyz wrote:
> > > > On 2016/04/07 23:25:38, Peter Kasting wrote:
> > > > > On 2016/04/07 18:55:02, xyzzyz wrote:
> > > > > > On 2016/04/06 22:35:39, Peter Kasting wrote:
> > > > > > > For this and most other patches below, are you in the process of
> > > > upstreaming
> > > > > > > those so we can eliminate them from our local tree?
> > > > > > 
> > > > > > 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
> > > > > 
> > > > > 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.
> > > > 
> > > > 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.
> > > 
> > > 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).
> > 
> > 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.
> 
> Why doesn't "dynamically initialize on first use" work?  That's what I did in
> similar places last time IIRC.

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]

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/RE...
File third_party/protobuf/README.chromium (right):

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:51: - 0003-remove-static-initializers.patch
On 2016/04/11 20:30:10, xyzzyz wrote:
> On 2016/04/11 20:16:55, Peter Kasting wrote:
> > On 2016/04/11 20:13:28, xyzzyz wrote:
> > > On 2016/04/11 19:54:45, Peter Kasting wrote:
> > > > On 2016/04/11 19:34:40, xyzzyz wrote:
> > > > > On 2016/04/07 23:25:38, Peter Kasting wrote:
> > > > > > On 2016/04/07 18:55:02, xyzzyz wrote:
> > > > > > > On 2016/04/06 22:35:39, Peter Kasting wrote:
> > > > > > > > For this and most other patches below, are you in the process of
> > > > > upstreaming
> > > > > > > > those so we can eliminate them from our local tree?
> > > > > > > 
> > > > > > > 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
> > > > > > 
> > > > > > 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.
> > > > > 
> > > > > 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.
> > > > 
> > > > 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).
> > > 
> > > 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.
> > 
> > Why doesn't "dynamically initialize on first use" work?  That's what I did
in
> > similar places last time IIRC.
> 
> 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.

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:34]

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/RE...
File third_party/protobuf/README.chromium (right):

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:51: - 0003-remove-static-initializers.patch
On 2016/04/11 20:54:51, Peter Kasting wrote:
> On 2016/04/11 20:30:10, xyzzyz wrote:
> > On 2016/04/11 20:16:55, Peter Kasting wrote:
> > > On 2016/04/11 20:13:28, xyzzyz wrote:
> > > > On 2016/04/11 19:54:45, Peter Kasting wrote:
> > > > > On 2016/04/11 19:34:40, xyzzyz wrote:
> > > > > > On 2016/04/07 23:25:38, Peter Kasting wrote:
> > > > > > > On 2016/04/07 18:55:02, xyzzyz wrote:
> > > > > > > > On 2016/04/06 22:35:39, Peter Kasting wrote:
> > > > > > > > > For this and most other patches below, are you in the process
of
> > > > > > upstreaming
> > > > > > > > > those so we can eliminate them from our local tree?
> > > > > > > > 
> > > > > > > > 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
> > > > > > > 
> > > > > > > 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.
> > > > > > 
> > > > > > 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.
> > > > > 
> > > > > 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).
> > > > 
> > > > 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.
> > > 
> > > Why doesn't "dynamically initialize on first use" work?  That's what I did
> in
> > > similar places last time IIRC.
> > 
> > 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.
> 
> 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.

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]

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/RE...
File third_party/protobuf/README.chromium (right):

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:51: - 0003-remove-static-initializers.patch
On 2016/04/11 21:08:33, xyzzyz wrote:
> On 2016/04/11 20:54:51, Peter Kasting wrote:
> > On 2016/04/11 20:30:10, xyzzyz wrote:
> > > On 2016/04/11 20:16:55, Peter Kasting wrote:
> > > > On 2016/04/11 20:13:28, xyzzyz wrote:
> > > > > On 2016/04/11 19:54:45, Peter Kasting wrote:
> > > > > > On 2016/04/11 19:34:40, xyzzyz wrote:
> > > > > > > On 2016/04/07 23:25:38, Peter Kasting wrote:
> > > > > > > > On 2016/04/07 18:55:02, xyzzyz wrote:
> > > > > > > > > On 2016/04/06 22:35:39, Peter Kasting wrote:
> > > > > > > > > > For this and most other patches below, are you in the
process
> of
> > > > > > > upstreaming
> > > > > > > > > > those so we can eliminate them from our local tree?
> > > > > > > > > 
> > > > > > > > > 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
> > > > > > > > 
> > > > > > > > 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.
> > > > > > > 
> > > > > > > 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.
> > > > > > 
> > > > > > 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).
> > > > > 
> > > > > 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.
> > > > 
> > > > Why doesn't "dynamically initialize on first use" work?  That's what I
did
> > in
> > > > similar places last time IIRC.
> > > 
> > > 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.
> > 
> > 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.
> 
> 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.

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.

[Peter Kasting, 2016-04-13 02:11:45]

Assume comments in BUILD.gn also apply to the GYP files as applicable.

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/cl...
File third_party/protobuf/cleanup_release.sh (right):

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/cl...
third_party/protobuf/cleanup_release.sh:1: #!/bin/bash
On 2016/04/07 23:25:38, Peter Kasting wrote:
> On 2016/04/07 18:55:02, xyzzyz wrote:
> > On 2016/04/06 22:35:39, Peter Kasting wrote:
> > > Why do we need this script?
> > > 
> > > It won't affect the build or output of Chromium at all.  The only thing it
> > will
> > > do is slightly reduce the source code footprint on disk.
> > > 
> > > Against that small upside, we have the maintenance downside that
> > > (a) This could break future updates if they pout a necessary file in one
of
> > > these places
> > > (b) It's not obvious how to create this list or if it's exhaustive, so
it's
> > not
> > > clear how maintainers should modify it in the future, so over time it will
> > have
> > > an increasing number of coverage gaps
> > > 
> > > I think we should remove this script entirely and you should check the
full
> > > upstream release into the tree.
> > 
> > Along with the patch 0002, it affects protoc slightly.
> 
> In what way?  protoc won't support these inputs anymore?  That shouldn't be a
> meaningful change from Chromium's perspective.
> 
> > I removed unnecessary
> > stuff, because most other libraries in third_party seem to also do it.
> 
> Definitely don't assume that any practice of anything in third_party/ is
> desirable or should be a general pattern :).
> 
> > Having
> > cleaner repository makes it easier to work with, and the Chromium build/code
> > review tools are a total pain to work with if your CLs are huge.
> 
> I don't think this makes any meaningful difference.  This CL is already far
> larger than the tools can deal with perfectly (e.g. "download patch" is
> disabled).  The difference between "way too huge" and "slightly larger than
way
> too huge" is negligible.
> 
> I really can't see any meaningful benefit of this.  It's of far more value to
> have a repo copy that exactly matches upstream than one that has a bunch of
> diffs due to missing files.  That makes it easier to tell what's different
when
> you download a new copy into the old location.  I considered and rejected this
> idea of pruning these bits of the repository during my previous update attempt
> in part for this reason.
> 
> > I don't think it's very hard to create this list. If it break future update,
> the
> > person doing the update will not have a hard time finding what went wrong.
> 
> Having been the person who tried to do the previous update, I would not have
> wanted to parse this list and figure out how to map it onto the current repo
> state.
> 
> I continue to think you should undo this step and remove this script.  There's
a
> reason I didn't add it last time, and past maintainers before me didn't have
> this.

All of my above concerns still stand.  I still believe you should not have this
script, the sources in question should be part of this patch, and they should be
listed appropriately (where applicable) in the relevant build files.

https://codereview.chromium.org/1842653006/diff/80001/third_party/protobuf/BU...
File third_party/protobuf/BUILD.gn (right):

https://codereview.chromium.org/1842653006/diff/80001/third_party/protobuf/BU...
third_party/protobuf/BUILD.gn:39: protobuf_lite_sources = [
Files my earlier patch set lists here that yours does not:

src/google/protobuf/descriptor.h
src/google/protobuf/descriptor.pb.h
src/google/protobuf/generated_enum_reflection.h
src/google/protobuf/generated_enum_util.h
src/google/protobuf/io/coded_stream_inl.h
src/google/protobuf/message.h
src/google/protobuf/metadata.cc
src/google/protobuf/metadata.h
src/google/protobuf/stubs/mathlimits.h
src/google/protobuf/testing/googletest.h
src/google/protobuf/unknown_field_set.cc
src/google/protobuf/unknown_field_set.h

Some of these might have been removed, and some (e.g. unknown_field_set.*) may
have only been needed due to the now-removed "remember unknown fields" patch.

Files your patch set lists that mine does not:

src/google/protobuf/map.h
src/google/protobuf/map_entry_lite.h
src/google/protobuf/map_field_lite.h
src/google/protobuf/map_type_handler.h
src/google/protobuf/stubs/atomicops_internals_power.h
src/google/protobuf/stubs/int128.cc
src/google/protobuf/stubs/int128.h
src/google/protobuf/stubs/shared_ptr.h
src/google/protobuf/stubs/status_macros.h
src/google/protobuf/stubs/structurally_valid.cc

I assume int128.* are new files.  I know structurally_valid.cc isn't, though.

Please do verify that all the above differences are correct, especially for
headers that might only be included transitively from other headers (however
many steps deep).

https://codereview.chromium.org/1842653006/diff/80001/third_party/protobuf/BU...
third_party/protobuf/BUILD.gn:125: "/wd4065",  # switch statement contains
'default' but no 'case' labels
Nit: Remove caps/trailing periods on the existing lines here to match your new
ones and the .gyp file.

https://codereview.chromium.org/1842653006/diff/80001/third_party/protobuf/BU...
third_party/protobuf/BUILD.gn:130: "/wd4305",  # double to float truncation
FWIW, we should probably make sure at least 4146 and 4305, and maybe 4065, have
issues filed upstream, as these are generally easily fixable and often indicate
bugs.

https://codereview.chromium.org/1842653006/diff/80001/third_party/protobuf/BU...
third_party/protobuf/BUILD.gn:211: # This file pulls in zlib, but it's not
actually used by protoc, so
Nit: This comment covers two files, so maybe say "gzip_stream.cc pulls in zlib,
... exclude these."

Might want the "just be test code" clause from my patch's comment, as that
explains why this would be in this target to begin with.

https://codereview.chromium.org/1842653006/diff/80001/third_party/protobuf/BU...
third_party/protobuf/BUILD.gn:214: # "src/google/protobuf/io/gzip_stream.h",
Nit: Blank line here

https://codereview.chromium.org/1842653006/diff/80001/third_party/protobuf/BU...
third_party/protobuf/BUILD.gn:442: # generated.
Nit: This comment is somewhat inaccurate in multiple ways now.  You should
probably switch to the comment I used in my patch.

https://codereview.chromium.org/1842653006/diff/80001/third_party/protobuf/BU...
third_party/protobuf/BUILD.gn:469: 
My patch had a "copy_google_protobuf_pyext" step here; do you not need this?  I
see you still have the files in your tree.

https://codereview.chromium.org/1842653006/diff/80001/third_party/protobuf/RE...
File third_party/protobuf/README.chromium (right):

https://codereview.chromium.org/1842653006/diff/80001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:14: protobuf_lite.gypi, protobuf_nacl.gyp).
One thing I'm concerned has been lost between my patch and yours is that mine
actually patched the build files along with the source files, and included
instructions about what should work after each successive patch.  What this
meant is that the initial set of buildfiles contained a more direct
correspondence to the upstream files, and then as we applied e.g. the
componentization patch it was clear exactly what pieces of the build file had to
do with that.  This would let a future maintainer have more confidence that they
understood what in the build files should and should not be carried forward,
because the sequential process to construct them was clear.

Now we basically just say "add the build files" with no instructions about how
to do so, increasing the likelihood that people in the future simply blindly
copy the previous buildfiles in totality, even when that wouldn't be correct.

https://codereview.chromium.org/1842653006/diff/80001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:32: Copy the resulting descriptor_pb2.py to
protobuf/.
This line is inaccurate and should simply be removed.

https://codereview.chromium.org/1842653006/diff/80001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:33: 6. Copy __init__.py to protobuf/ (it
adds third_party/six/ to Python path).
Copy it from where?

https://codereview.chromium.org/1842653006/diff/80001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:59: to work in Chromium, which doesn't use
this functionality yet.
Nit: If we land with this patch, it should refer to the upstream issue where the
correct fix is debated.

https://codereview.chromium.org/1842653006/diff/80001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:65: protobuf symbols in Linux .so
libraries.
Why are the int64_t and componentization changes combined in one patch?  Those
are independent things.

The componentization patch should talk about converting to a component and refer
to http://crrev.com/179806 , since that CL and associated bug provide a lot of
useful context for those changes; see my patch's README.

https://codereview.chromium.org/1842653006/diff/80001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:75: restrictive. This is actually
cherry-pick from protobuf upstream.
If you've cherry-picked, say exactly which revision you cherry-picked.

https://codereview.chromium.org/1842653006/diff/80001/third_party/protobuf/pr...
File third_party/protobuf/protobuf_lite.gypi (right):

https://codereview.chromium.org/1842653006/diff/80001/third_party/protobuf/pr...
third_party/protobuf/protobuf_lite.gypi:94:
'GOOGLE_PROTOBUF_NO_STATIC_INITIALIZER',
Nit: I'd put a blank line above this since it's not part of the above comment
block.

https://codereview.chromium.org/1842653006/diff/80001/third_party/protobuf/pr...
third_party/protobuf/protobuf_lite.gypi:96: # protobuf requires these to be set
manually, as it no longer
This comment doesn't seem to add much, and it's not in the .gn file; I'd remove
it.

[xyzzyz, 2016-04-14 00:23:57]

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/cl...
File third_party/protobuf/cleanup_release.sh (right):

https://codereview.chromium.org/1842653006/diff/20001/third_party/protobuf/cl...
third_party/protobuf/cleanup_release.sh:1: #!/bin/bash
On 2016/04/13 02:11:44, Peter Kasting wrote:
> On 2016/04/07 23:25:38, Peter Kasting wrote:
> > On 2016/04/07 18:55:02, xyzzyz wrote:
> > > On 2016/04/06 22:35:39, Peter Kasting wrote:
> > > > Why do we need this script?
> > > > 
> > > > It won't affect the build or output of Chromium at all.  The only thing
it
> > > will
> > > > do is slightly reduce the source code footprint on disk.
> > > > 
> > > > Against that small upside, we have the maintenance downside that
> > > > (a) This could break future updates if they pout a necessary file in one
> of
> > > > these places
> > > > (b) It's not obvious how to create this list or if it's exhaustive, so
> it's
> > > not
> > > > clear how maintainers should modify it in the future, so over time it
will
> > > have
> > > > an increasing number of coverage gaps
> > > > 
> > > > I think we should remove this script entirely and you should check the
> full
> > > > upstream release into the tree.
> > > 
> > > Along with the patch 0002, it affects protoc slightly.
> > 
> > In what way?  protoc won't support these inputs anymore?  That shouldn't be
a
> > meaningful change from Chromium's perspective.
> > 
> > > I removed unnecessary
> > > stuff, because most other libraries in third_party seem to also do it.
> > 
> > Definitely don't assume that any practice of anything in third_party/ is
> > desirable or should be a general pattern :).
> > 
> > > Having
> > > cleaner repository makes it easier to work with, and the Chromium
build/code
> > > review tools are a total pain to work with if your CLs are huge.
> > 
> > I don't think this makes any meaningful difference.  This CL is already far
> > larger than the tools can deal with perfectly (e.g. "download patch" is
> > disabled).  The difference between "way too huge" and "slightly larger than
> way
> > too huge" is negligible.
> > 
> > I really can't see any meaningful benefit of this.  It's of far more value
to
> > have a repo copy that exactly matches upstream than one that has a bunch of
> > diffs due to missing files.  That makes it easier to tell what's different
> when
> > you download a new copy into the old location.  I considered and rejected
this
> > idea of pruning these bits of the repository during my previous update
attempt
> > in part for this reason.
> > 
> > > I don't think it's very hard to create this list. If it break future
update,
> > the
> > > person doing the update will not have a hard time finding what went wrong.
> > 
> > Having been the person who tried to do the previous update, I would not have
> > wanted to parse this list and figure out how to map it onto the current repo
> > state.
> > 
> > I continue to think you should undo this step and remove this script. 
There's
> a
> > reason I didn't add it last time, and past maintainers before me didn't have
> > this.
> 
> All of my above concerns still stand.  I still believe you should not have
this
> script, the sources in question should be part of this patch, and they should
be
> listed appropriately (where applicable) in the relevant build files.

Pulled full protobuf release and deleted this script.

https://codereview.chromium.org/1842653006/diff/80001/third_party/protobuf/BU...
File third_party/protobuf/BUILD.gn (right):

https://codereview.chromium.org/1842653006/diff/80001/third_party/protobuf/BU...
third_party/protobuf/BUILD.gn:39: protobuf_lite_sources = [
On 2016/04/13 02:11:44, Peter Kasting wrote:
> Files my earlier patch set lists here that yours does not:
> 
> src/google/protobuf/descriptor.h
> src/google/protobuf/descriptor.pb.h
> src/google/protobuf/generated_enum_reflection.h
> src/google/protobuf/generated_enum_util.h
> src/google/protobuf/io/coded_stream_inl.h
> src/google/protobuf/message.h
> src/google/protobuf/metadata.cc
> src/google/protobuf/metadata.h
> src/google/protobuf/stubs/mathlimits.h
> src/google/protobuf/testing/googletest.h
> src/google/protobuf/unknown_field_set.cc
> src/google/protobuf/unknown_field_set.h
> 
> Some of these might have been removed, and some (e.g. unknown_field_set.*) may
> have only been needed due to the now-removed "remember unknown fields" patch.
> 
> Files your patch set lists that mine does not:
> 
> src/google/protobuf/map.h
> src/google/protobuf/map_entry_lite.h
> src/google/protobuf/map_field_lite.h
> src/google/protobuf/map_type_handler.h
> src/google/protobuf/stubs/atomicops_internals_power.h
> src/google/protobuf/stubs/int128.cc
> src/google/protobuf/stubs/int128.h
> src/google/protobuf/stubs/shared_ptr.h
> src/google/protobuf/stubs/status_macros.h
> src/google/protobuf/stubs/structurally_valid.cc
> 
> I assume int128.* are new files.  I know structurally_valid.cc isn't, though.
> 
> Please do verify that all the above differences are correct, especially for
> headers that might only be included transitively from other headers (however
> many steps deep).

Most of the files you mention are part of protobuf_full, not protobuf_lite, so
that's why they aren't here. The ones that are actually missing are
coded_stream_inl.h, and googletest.{cc,h}, so added them here.

https://codereview.chromium.org/1842653006/diff/80001/third_party/protobuf/BU...
third_party/protobuf/BUILD.gn:125: "/wd4065",  # switch statement contains
'default' but no 'case' labels
On 2016/04/13 02:11:45, Peter Kasting wrote:
> Nit: Remove caps/trailing periods on the existing lines here to match your new
> ones and the .gyp file.

Done.

https://codereview.chromium.org/1842653006/diff/80001/third_party/protobuf/BU...
third_party/protobuf/BUILD.gn:130: "/wd4305",  # double to float truncation
On 2016/04/13 02:11:45, Peter Kasting wrote:
> FWIW, we should probably make sure at least 4146 and 4305, and maybe 4065,
have
> issues filed upstream, as these are generally easily fixable and often
indicate
> bugs.

That's a good idea. I don't have a Windows machine though, so working on this
could be kinda painful.

https://codereview.chromium.org/1842653006/diff/80001/third_party/protobuf/BU...
third_party/protobuf/BUILD.gn:211: # This file pulls in zlib, but it's not
actually used by protoc, so
On 2016/04/13 02:11:44, Peter Kasting wrote:
> Nit: This comment covers two files, so maybe say "gzip_stream.cc pulls in
zlib,
> ... exclude these."
> 
> Might want the "just be test code" clause from my patch's comment, as that
> explains why this would be in this target to begin with.

Done.

https://codereview.chromium.org/1842653006/diff/80001/third_party/protobuf/BU...
third_party/protobuf/BUILD.gn:214: # "src/google/protobuf/io/gzip_stream.h",
On 2016/04/13 02:11:44, Peter Kasting wrote:
> Nit: Blank line here

Done.

https://codereview.chromium.org/1842653006/diff/80001/third_party/protobuf/BU...
third_party/protobuf/BUILD.gn:442: # generated.
On 2016/04/13 02:11:44, Peter Kasting wrote:
> Nit: This comment is somewhat inaccurate in multiple ways now.  You should
> probably switch to the comment I used in my patch.

Done.

https://codereview.chromium.org/1842653006/diff/80001/third_party/protobuf/BU...
third_party/protobuf/BUILD.gn:469: 
On 2016/04/13 02:11:44, Peter Kasting wrote:
> My patch had a "copy_google_protobuf_pyext" step here; do you not need this? 
I
> see you still have the files in your tree.

I'm not sure if these are at all necessary. Nothing seems to be using these.

https://codereview.chromium.org/1842653006/diff/80001/third_party/protobuf/RE...
File third_party/protobuf/README.chromium (right):

https://codereview.chromium.org/1842653006/diff/80001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:14: protobuf_lite.gypi, protobuf_nacl.gyp).
On 2016/04/13 02:11:44, Peter Kasting wrote:
> One thing I'm concerned has been lost between my patch and yours is that mine
> actually patched the build files along with the source files, and included
> instructions about what should work after each successive patch.  What this
> meant is that the initial set of buildfiles contained a more direct
> correspondence to the upstream files, and then as we applied e.g. the
> componentization patch it was clear exactly what pieces of the build file had
to
> do with that.  This would let a future maintainer have more confidence that
they
> understood what in the build files should and should not be carried forward,
> because the sequential process to construct them was clear.
> 
> Now we basically just say "add the build files" with no instructions about how
> to do so, increasing the likelihood that people in the future simply blindly
> copy the previous buildfiles in totality, even when that wouldn't be correct.

I'll update the comment to not imply to copy them blindly, but apart from that,
I don't think we can predict all issues with upstream requiring updates, so we
should just hope that the future maintainer will have good judgement. I don't
think it's worth keeping the diffs to build files, as one can always use git
history and code review comments to learn some more background.

https://codereview.chromium.org/1842653006/diff/80001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:32: Copy the resulting descriptor_pb2.py to
protobuf/.
On 2016/04/13 02:11:44, Peter Kasting wrote:
> This line is inaccurate and should simply be removed.

Done.

https://codereview.chromium.org/1842653006/diff/80001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:33: 6. Copy __init__.py to protobuf/ (it
adds third_party/six/ to Python path).
On 2016/04/13 02:11:44, Peter Kasting wrote:
> Copy it from where?

I'm not sure how to best express that one should just copy the one I prepared
and put in third_party/protobuf. PTAL.

https://codereview.chromium.org/1842653006/diff/80001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:65: protobuf symbols in Linux .so
libraries.
On 2016/04/13 02:11:44, Peter Kasting wrote:
> Why are the int64_t and componentization changes combined in one patch?  Those
> are independent things.
> 
> The componentization patch should talk about converting to a component and
refer
> to http://crrev.com/179806 , since that CL and associated bug provide a lot of
> useful context for those changes; see my patch's README.

They're in a single patch, because I didn't know how to easily separate it into
two patches, and I didn't think it was really worth spending time on it. 

I'll update the comment to talk about componentization.

https://codereview.chromium.org/1842653006/diff/80001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:75: restrictive. This is actually
cherry-pick from protobuf upstream.
On 2016/04/13 02:11:44, Peter Kasting wrote:
> If you've cherry-picked, say exactly which revision you cherry-picked.

Done.

https://codereview.chromium.org/1842653006/diff/80001/third_party/protobuf/pr...
File third_party/protobuf/protobuf_lite.gypi (right):

https://codereview.chromium.org/1842653006/diff/80001/third_party/protobuf/pr...
third_party/protobuf/protobuf_lite.gypi:96: # protobuf requires these to be set
manually, as it no longer
On 2016/04/13 02:11:45, Peter Kasting wrote:
> This comment doesn't seem to add much, and it's not in the .gn file; I'd
remove
> it.

Done.

[xyzzyz, 2016-04-14 23:57:54]

https://codereview.chromium.org/1842653006/diff/80001/third_party/protobuf/BU...
File third_party/protobuf/BUILD.gn (right):

https://codereview.chromium.org/1842653006/diff/80001/third_party/protobuf/BU...
third_party/protobuf/BUILD.gn:39: protobuf_lite_sources = [
On 2016/04/14 00:23:57, xyzzyz wrote:
> On 2016/04/13 02:11:44, Peter Kasting wrote:
> > Files my earlier patch set lists here that yours does not:
> > 
> > src/google/protobuf/descriptor.h
> > src/google/protobuf/descriptor.pb.h
> > src/google/protobuf/generated_enum_reflection.h
> > src/google/protobuf/generated_enum_util.h
> > src/google/protobuf/io/coded_stream_inl.h
> > src/google/protobuf/message.h
> > src/google/protobuf/metadata.cc
> > src/google/protobuf/metadata.h
> > src/google/protobuf/stubs/mathlimits.h
> > src/google/protobuf/testing/googletest.h
> > src/google/protobuf/unknown_field_set.cc
> > src/google/protobuf/unknown_field_set.h
> > 
> > Some of these might have been removed, and some (e.g. unknown_field_set.*)
may
> > have only been needed due to the now-removed "remember unknown fields"
patch.
> > 
> > Files your patch set lists that mine does not:
> > 
> > src/google/protobuf/map.h
> > src/google/protobuf/map_entry_lite.h
> > src/google/protobuf/map_field_lite.h
> > src/google/protobuf/map_type_handler.h
> > src/google/protobuf/stubs/atomicops_internals_power.h
> > src/google/protobuf/stubs/int128.cc
> > src/google/protobuf/stubs/int128.h
> > src/google/protobuf/stubs/shared_ptr.h
> > src/google/protobuf/stubs/status_macros.h
> > src/google/protobuf/stubs/structurally_valid.cc
> > 
> > I assume int128.* are new files.  I know structurally_valid.cc isn't,
though.
> > 
> > Please do verify that all the above differences are correct, especially for
> > headers that might only be included transitively from other headers (however
> > many steps deep).
> 
> Most of the files you mention are part of protobuf_full, not protobuf_lite, so
> that's why they aren't here. The ones that are actually missing are
> coded_stream_inl.h, and googletest.{cc,h}, so added them here.

In another pass I removed testing/googletest.{cc,h} and testing/file.{cc,h}.
These seem like they have been pulled by non-unittest code by accident. I
created a pull request to remove these from upstream:
https://github.com/google/protobuf/pull/1414

[Peter Kasting, 2016-04-15 01:12:59]

LGTM

https://codereview.chromium.org/1842653006/diff/80001/third_party/protobuf/BU...
File third_party/protobuf/BUILD.gn (right):

https://codereview.chromium.org/1842653006/diff/80001/third_party/protobuf/BU...
third_party/protobuf/BUILD.gn:39: protobuf_lite_sources = [
On 2016/04/14 00:23:57, xyzzyz wrote:
> On 2016/04/13 02:11:44, Peter Kasting wrote:
> > Files my earlier patch set lists here that yours does not:
> > 
> > src/google/protobuf/descriptor.h
> > src/google/protobuf/descriptor.pb.h
> > src/google/protobuf/generated_enum_reflection.h
> > src/google/protobuf/generated_enum_util.h
> > src/google/protobuf/io/coded_stream_inl.h
> > src/google/protobuf/message.h
> > src/google/protobuf/metadata.cc
> > src/google/protobuf/metadata.h
> > src/google/protobuf/stubs/mathlimits.h
> > src/google/protobuf/testing/googletest.h
> > src/google/protobuf/unknown_field_set.cc
> > src/google/protobuf/unknown_field_set.h
> > 
> > Some of these might have been removed, and some (e.g. unknown_field_set.*)
may
> > have only been needed due to the now-removed "remember unknown fields"
patch.
> > 
> > Files your patch set lists that mine does not:
> > 
> > src/google/protobuf/map.h
> > src/google/protobuf/map_entry_lite.h
> > src/google/protobuf/map_field_lite.h
> > src/google/protobuf/map_type_handler.h
> > src/google/protobuf/stubs/atomicops_internals_power.h
> > src/google/protobuf/stubs/int128.cc
> > src/google/protobuf/stubs/int128.h
> > src/google/protobuf/stubs/shared_ptr.h
> > src/google/protobuf/stubs/status_macros.h
> > src/google/protobuf/stubs/structurally_valid.cc
> > 
> > I assume int128.* are new files.  I know structurally_valid.cc isn't,
though.
> > 
> > Please do verify that all the above differences are correct, especially for
> > headers that might only be included transitively from other headers (however
> > many steps deep).
> 
> Most of the files you mention are part of protobuf_full, not protobuf_lite, so
> that's why they aren't here.

Right -- my concern was not that they were entirely missing, but that maybe they
needed to affect this target, and not just the full one.

It's OK to leave them in protobuf_full iff they are not compiled in to the lite
release by any transitive mechanism.  I just wanted to make sure you
double-checked the #include chains.

https://codereview.chromium.org/1842653006/diff/80001/third_party/protobuf/BU...
third_party/protobuf/BUILD.gn:130: "/wd4305",  # double to float truncation
On 2016/04/14 00:23:57, xyzzyz wrote:
> On 2016/04/13 02:11:45, Peter Kasting wrote:
> > FWIW, we should probably make sure at least 4146 and 4305, and maybe 4065,
> have
> > issues filed upstream, as these are generally easily fixable and often
> indicate
> > bugs.
> 
> That's a good idea. I don't have a Windows machine though, so working on this
> could be kinda painful.

Don't necessarily worry about fixing.  Just getting something on file, that we
can refer to here, so that future enterprising people can either work on fixing
or at least just check to see if someone else fixed, is good enough.

https://codereview.chromium.org/1842653006/diff/80001/third_party/protobuf/RE...
File third_party/protobuf/README.chromium (right):

https://codereview.chromium.org/1842653006/diff/80001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:65: protobuf symbols in Linux .so
libraries.
On 2016/04/14 00:23:56, xyzzyz wrote:
> On 2016/04/13 02:11:44, Peter Kasting wrote:
> > Why are the int64_t and componentization changes combined in one patch? 
Those
> > are independent things.
> > 
> > The componentization patch should talk about converting to a component and
> refer
> > to http://crrev.com/179806 , since that CL and associated bug provide a lot
of
> > useful context for those changes; see my patch's README.
> 
> They're in a single patch, because I didn't know how to easily separate it
into
> two patches, and I didn't think it was really worth spending time on it.

Do they actually depend on each other?  I just assumed they touched the same
files, in which case it's easy enough to just make two sequential patches that
will apply in some particular order with the relevant pieces.

The patch is small, so it's probably not a big deal, it's mostly an issue if in
the future e.g. upstream accepts the int64_t changes, in which case part of the
patch will still be needed and part won't.

https://codereview.chromium.org/1842653006/diff/90001/third_party/protobuf/RE...
File third_party/protobuf/README.chromium (right):

https://codereview.chromium.org/1842653006/diff/90001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:39: 6. Add __init__.py to protobuf/ that
adds third_party/six/ to Python path.
Nit: Add -> Add an

https://codereview.chromium.org/1842653006/diff/90001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:73: protobuf symbols in Linux .so
libraries, so that protobuf can be build as a
Nit: build -> built

https://codereview.chromium.org/1842653006/diff/90001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:84: restrictive. This is actually
cherry-pick from protobuf upstream (commits
Nit: Minor grammar issues; most direct replacement is maybe just "This
cherry-picks upstream commits ___ and ___."

https://codereview.chromium.org/1842653006/diff/90001/third_party/protobuf/pa...
File
third_party/protobuf/patches/0002-remove-support-for-unnecessary-languages.patch
(right):

https://codereview.chromium.org/1842653006/diff/90001/third_party/protobuf/pa...
third_party/protobuf/patches/0002-remove-support-for-unnecessary-languages.patch:20:
-  // Proto2 Java
I wonder if the Android people might actually want this?  Similarly the ObjC
generator for the iOS folks.

Now that all these files are in our tree, do we really gain much by stripping
out these languages?  Seems like it'd be easier to just drop this patch, and
include the full filelists in the buildfiles.  That'd make the buildfiles look
more like copies of the filelists in the upstream buildfiles anyway.

[xyzzyz, 2016-04-15 20:50:23]

xyzzyz@chromium.org changed reviewers:
+ scottmg@chromium.org

[xyzzyz, 2016-04-15 20:50:23]

xyzzyz@chromium.org changed required reviewers:
+ scottmg@chromium.org

[xyzzyz, 2016-04-15 20:50:36]

https://codereview.chromium.org/1842653006/diff/90001/third_party/protobuf/RE...
File third_party/protobuf/README.chromium (right):

https://codereview.chromium.org/1842653006/diff/90001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:39: 6. Add __init__.py to protobuf/ that
adds third_party/six/ to Python path.
On 2016/04/15 01:12:59, Peter Kasting wrote:
> Nit: Add -> Add an

Done.

https://codereview.chromium.org/1842653006/diff/90001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:73: protobuf symbols in Linux .so
libraries, so that protobuf can be build as a
On 2016/04/15 01:12:59, Peter Kasting wrote:
> Nit: build -> built

Done.

https://codereview.chromium.org/1842653006/diff/90001/third_party/protobuf/RE...
third_party/protobuf/README.chromium:84: restrictive. This is actually
cherry-pick from protobuf upstream (commits
On 2016/04/15 01:12:59, Peter Kasting wrote:
> Nit: Minor grammar issues; most direct replacement is maybe just "This
> cherry-picks upstream commits ___ and ___."

Done.

https://codereview.chromium.org/1842653006/diff/90001/third_party/protobuf/pa...
File
third_party/protobuf/patches/0002-remove-support-for-unnecessary-languages.patch
(right):

https://codereview.chromium.org/1842653006/diff/90001/third_party/protobuf/pa...
third_party/protobuf/patches/0002-remove-support-for-unnecessary-languages.patch:20:
-  // Proto2 Java
On 2016/04/15 01:12:59, Peter Kasting wrote:
> I wonder if the Android people might actually want this?  Similarly the ObjC
> generator for the iOS folks.
> 
> Now that all these files are in our tree, do we really gain much by stripping
> out these languages?  Seems like it'd be easier to just drop this patch, and
> include the full filelists in the buildfiles.  That'd make the buildfiles look
> more like copies of the filelists in the upstream buildfiles anyway.

Android/iOS doesn't seem to be using it. 

The drawback of this is that it increases the build time by adding dozens of
additional targets. I don't know if this is significant, though. To keep our
changes to upstream minimal, I added these back.

[xyzzyz, 2016-04-15 22:04:18]

In the meantime, someone submitted a change that doesn't work with the updated
protobuf. I'm currently investigating the problem.

[scottmg, 2016-04-18 19:40:22]

third_party/protobuf/OWNERS lgtm

[xyzzyz, 2016-04-20 21:30:14]

The CQ bit was checked by xyzzyz@chromium.org

[xyzzyz, 2016-04-20 21:30:23]

The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org,
pkasting@chromium.org, scottmg@chromium.org
Link to the patchset: https://codereview.chromium.org/1842653006/#ps160001
(title: "merge")

[commit-bot: I haz the power, 2016-04-20 21:30:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1842653006/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1842653006/160001

[commit-bot: I haz the power, 2016-04-20 23:50:39]

Description was changed from

==========
Update //third_party/protobuf to version 3.

For context, see
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/zJZPkxEFVxY

For the things to look out for while reviewing this, see the doc
https://docs.google.com/document/d/1qjSBV2ioi0mykAT1eSXuMaNqO0BIS-j1PeYfxT-Tb...

BUG=597321
==========

to

==========
Update //third_party/protobuf to version 3.

For context, see
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/zJZPkxEFVxY

For the things to look out for while reviewing this, see the doc
https://docs.google.com/document/d/1qjSBV2ioi0mykAT1eSXuMaNqO0BIS-j1PeYfxT-Tb...

BUG=597321
==========

[commit-bot: I haz the power, 2016-04-20 23:50:50]

Committed patchset #17 (id:160001)

[commit-bot: I haz the power, 2016-04-22 19:27:19]

Description was changed from

==========
Update //third_party/protobuf to version 3.

For context, see
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/zJZPkxEFVxY

For the things to look out for while reviewing this, see the doc
https://docs.google.com/document/d/1qjSBV2ioi0mykAT1eSXuMaNqO0BIS-j1PeYfxT-Tb...

BUG=597321
==========

to

==========
Update //third_party/protobuf to version 3.

For context, see
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/zJZPkxEFVxY

For the things to look out for while reviewing this, see the doc
https://docs.google.com/document/d/1qjSBV2ioi0mykAT1eSXuMaNqO0BIS-j1PeYfxT-Tb...

BUG=597321

Committed: https://crrev.com/7cf8148f570a1072928fa9ede6e0760edc137cbd
Cr-Commit-Position: refs/heads/master@{#388596}
==========

[commit-bot: I haz the power, 2016-04-22 19:27:27]

Patchset 17 (id:??) landed as
https://crrev.com/7cf8148f570a1072928fa9ede6e0760edc137cbd
Cr-Commit-Position: refs/heads/master@{#388596}