GN proto_libary refactoring.

tools/protoc_wrapper/protoc_wrapper.py

 #!/usr/bin/env python
-# Copyright (c) 2012 The Chromium Authors. All rights reserved.
+# Copyright (c) 2016 The Chromium Authors. All rights reserved.

[petrcermak, 2016/08/15 13:00:06]

The date should NOT be changed. See
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md....

[kraynov, 2016/08/15 13:37:49]

Acknowledged.

 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """
 A simple wrapper for protoc.
-
-- Adds includes in generated headers.
-- Handles building with system protobuf as an option.
+Script for //third_party/protobuf/proto_library.gni .
+Features:
+- Automatic include insertion.

[petrcermak, 2016/08/15 13:00:06]

Maybe add a hash ("#include") so that it's easier to understand?

[kraynov, 2016/08/15 13:37:50]

Acknowledged.

+- Preventing bad proto names.

[petrcermak, 2016/08/15 13:00:06]

s/Preventing/Prevents/

[kraynov, 2016/08/15 13:37:49]

Acknowledged.

 """
 
-import fnmatch
+from __future__ import print_function
 import optparse
 import os.path
-import shutil
 import subprocess
 import sys
 import tempfile
 
-PROTOC_INCLUDE_POINT = '// @@protoc_insertion_point(includes)\n'
-
-def ModifyHeader(header_file, extra_header):
-  """Adds |extra_header| to |header_file|. Returns 0 on success.
-
-  |extra_header| is the name of the header file to include.
-  |header_file| is a generated protobuf cpp header.
-  """
-  include_point_found = False
-  header_contents = []
-  with open(header_file) as f:
-    for line in f:
-      header_contents.append(line)
-      if line == PROTOC_INCLUDE_POINT:
-        extra_header_msg = '#include "%s"\n' % extra_header
-        header_contents.append(extra_header_msg)
-        include_point_found = True;
-  if not include_point_found:
-    return 1
-
-  with open(header_file, 'wb') as f:
-    f.write(''.join(header_contents))
-  return 0
-
-def ScanForBadFiles(scan_root):
-  """Scan for bad file names, see http://crbug.com/386125 for details.
-  Returns True if any filenames are bad. Outputs errors to stderr.
-
-  |scan_root| is the path to the directory to be recursively scanned.
-  """
-  badname = False
-  real_scan_root = os.path.realpath(scan_root)
-  for dirpath, dirnames, filenames in os.walk(real_scan_root):
-    matches = fnmatch.filter(filenames, '*-*.proto')
-    if len(matches) > 0:
-      if not badname:
-        badname = True
-        sys.stderr.write('proto files must not have hyphens in their names ('
-                         'see http://crbug.com/386125 for more information):\n')
-      for filename in matches:
-        sys.stderr.write('  ' + os.path.join(real_scan_root,
-                                             dirpath, filename) + '\n')
-  return badname
+PROTOC_INCLUDE_POINT = "// @@protoc_insertion_point(includes)"
 
 
-def RewriteProtoFilesForSystemProtobuf(path):
-  wrapper_dir = tempfile.mkdtemp()
-  try:
-    for filename in os.listdir(path):
-      if not filename.endswith('.proto'):
-        continue
-      with open(os.path.join(path, filename), 'r') as src_file:
-        with open(os.path.join(wrapper_dir, filename), 'w') as dst_file:
-          for line in src_file:
-            # Remove lines that break build with system protobuf.
-            # We cannot optimize for lite runtime, because system lite runtime
-            # does not have a Chromium-specific hack to retain unknown fields.
-            # Similarly, it does not understand corresponding option to control
-            # the usage of that hack.
-            if 'LITE_RUNTIME' in line or 'retain_unknown_fields' in line:
-              continue
-            dst_file.write(line)
+def ProtocGenOptions(options):

[petrcermak, 2016/08/15 13:00:06]

The name is a noun, so it suggests that it's a class (rather than a method).
Something like "ConvertProtocGenOptionsToString" would be better.

[petrcermak, 2016/08/15 13:00:06]

If I understand correctly, all methods in this file are not meant to be used
outside it. If that is indeed the case, please prefix their names with a single
underscore (e.g. _ConvertProtocGenOptionsToString).

[kraynov, 2016/08/15 13:37:49]

Acknowledged.

[kraynov, 2016/08/15 13:37:50]

It's standalone utility. I've followed the previous style.

+  if options is not None:

[petrcermak, 2016/08/15 13:00:06]

Unless '' (empty string) are valid |options|, you can use |not options| instead
of |options is not None|.

[petrcermak, 2016/08/15 13:00:06]

Don't use nested if statements unless necessary:

if options is None:
  return ''
if options.endswith(':'):
  return options
return options + ':'

[kraynov, 2016/08/15 13:37:49]

Acknowledged.

+    if options.endswith(":"):
+      return options
+    else:
+      return options + ":"
+  else:
+    return ""
 
-    return wrapper_dir
-  except:
-    shutil.rmtree(wrapper_dir)
-    raise
+
+def StripProtos(protos):

[petrcermak, 2016/08/15 13:00:06]

(Feel free to ignore) I would personally use a comprehension:


def StripProtos(protos):
  return [StripProto(p) for p in protos)

def StripProto(proto):
  if not proto.endswith(".proto"):
    ...
  ...
  return filename.rsplit('.', 1)[0]

[kraynov, 2016/08/15 13:37:49]

Acknowledged.

+  stripped = []
+  for filename in protos:
+    if not filename.endswith(".proto"):
+      raise RuntimeError("Invalid proto filename extension: " + filename)
+    if "-" in filename:
+      raise RuntimeError("Proto files must not have hyphens in their names "

[petrcermak, 2016/08/15 13:00:06]

suggestion "Proto file names must not contain hyphens"

[kraynov, 2016/08/15 13:37:50]

Acknowledged.

+                         "(see http://crbug.com/386125 for more information).")

[petrcermak, 2016/08/15 13:00:06]

supernit: Your error ends with a period here, but not on lines 38 and 61

[kraynov, 2016/08/15 13:37:49]

Acknowledged.

+    # Remove .proto extension.
+    stripped.append(filename.rsplit(".", 1)[0])
+  return stripped
+
+
+def WriteIncludes(headers, include):
+  for filename in headers:
+    include_point_found = False
+    contents = []
+    with open(filename) as f:
+      for line in f:
+        stripped_line = line.strip()
+        contents.append(stripped_line)
+        if stripped_line == PROTOC_INCLUDE_POINT:

[petrcermak, 2016/08/15 13:00:06]

you should add an if statement asserting that include_point_found is false here.
if not, you will insert the include twice.

[kraynov, 2016/08/15 13:37:49]

Acknowledged.

+          extra_statement = '#include "{0}"'.format(include)
+          contents.append(extra_statement)
+          include_point_found = True
+
+      if not include_point_found:
+        raise RuntimeError("Include point not found in header: " + filename)
+
+    with open(filename, "w") as f:
+      for line in contents:

[petrcermak, 2016/08/15 13:00:06]

f.writelines(contents)

[kraynov, 2016/08/15 13:37:49]

Acknowledged.

+        print(line, file=f)
 
 
 def main(argv):
   parser = optparse.OptionParser()

[petrcermak, 2016/08/15 13:00:06]

Could you use the argparse module instead? Optparse has been deprecated
(https://docs.python.org/2/library/optparse.html).

[kraynov, 2016/08/15 13:37:50]

Okay.

-  parser.add_option('--include', dest='extra_header',
-                    help='The extra header to include. This must be specified '
-                         'along with --protobuf.')
-  parser.add_option('--protobuf', dest='generated_header',
-                    help='The c++ protobuf header to add the extra header to. '
-                         'This must be specified along with --include.')
-  parser.add_option('--proto-in-dir',
-                    help='The directory containing .proto files.')
-  parser.add_option('--proto-in-file', help='Input file to compile.')
-  parser.add_option('--use-system-protobuf', type=int, default=0,
-                    help='Option to use system-installed protobuf '
-                         'instead of bundled one.')
-  (options, args) = parser.parse_args(sys.argv)
-  if len(args) < 2:
-    return 1
+  parser.add_option("--protoc",
+                    help="Relative path to compiler.")
 
-  if ScanForBadFiles(options.proto_in_dir):
-    return 1
+  parser.add_option("--proto-in-dir",
+                    help="Base directory with source protos.")
+  parser.add_option("--cc-out-dir",
+                    help="Output directory for standard C++ generator.")
+  parser.add_option("--py-out-dir",
+                    help="Output directory for standard Python generator.")
+  parser.add_option("--plugin-out-dir",
+                    help="Output directory for custom generator.")

[petrcermak, 2016/08/15 13:00:06]

I think that the help string should contain the word "plugin" since it's in the
flag name

[kraynov, 2016/08/15 13:37:49]

Acknowledged.

 
-  proto_path = options.proto_in_dir
-  if options.use_system_protobuf == 1:
-    proto_path = RewriteProtoFilesForSystemProtobuf(proto_path)
-  try:
-    # Run what is hopefully protoc.
-    protoc_args = args[1:]
-    protoc_args += ['--proto_path=%s' % proto_path,
-                    os.path.join(proto_path, options.proto_in_file)]
-    ret = subprocess.call(protoc_args)
-    if ret != 0:
-      return ret
-  finally:
-    if options.use_system_protobuf == 1:
-      # Remove temporary directory holding re-written files.
-      shutil.rmtree(proto_path)
+  parser.add_option("--plugin",
+                    help="Relative path to custom generator plugin.")
+  parser.add_option("--plugin-options",
+                    help="Custom generator options.")

[petrcermak, 2016/08/15 13:00:07]

I think that the help string should contain the word "plugin" since it's in the
flag name

[kraynov, 2016/08/15 13:37:50]

Acknowledged.

+  parser.add_option("--cc-options",
+                    help="Standard C++ generator options.")
+  parser.add_option("--include",
+                    help="Name of include to put into generated headers.")

[petrcermak, 2016/08/15 13:00:06]

supernit: s/put/insert/

[kraynov, 2016/08/15 13:37:50]

Acknowledged.

+  (options, args) = parser.parse_args(argv)

[petrcermak, 2016/08/15 13:00:06]

no need for parentheses (feel free to ignore)

[kraynov, 2016/08/15 13:37:49]

Acknowledged.

 
-  # protoc succeeded, check to see if the generated cpp header needs editing.
-  if not options.extra_header or not options.generated_header:
-    return 0
-  return ModifyHeader(options.generated_header, options.extra_header)
+  protoc = os.path.realpath(options.protoc)

[petrcermak, 2016/08/15 13:00:06]

In my opinion, there's no need to define this here when you don't need it until
line 119. (Feel free to ignore)

[kraynov, 2016/08/15 13:37:49]

Acknowledged.

+  proto_dir = os.path.relpath(options.proto_in_dir)
+  protoc_args = ["--proto_path", proto_dir]

[petrcermak, 2016/08/15 13:00:06]

To avoid pre-pending the protoc binary at the end on line 119, why not just call
this protoc_cmd:

proto_cmd = [os.path.realpath(options.protoc), "--proto_path", proto_dir]

[kraynov, 2016/08/15 13:37:50]

Acknowledged.

+
+  protos = args[1:]
+  stripped_protos = StripProtos(protos)
+  headers = []
+
+  if options.cc_out_dir is not None:

[petrcermak, 2016/08/15 13:00:06]

you could do the following:

if options.cc_out_dir is None:
  headers = []
else:
  ...
  headers = [os.path.join(...) for name in stripped_protos]

[kraynov, 2016/08/15 13:37:49]

Acknowledged.

+    cc_options = ProtocGenOptions(options.cc_options)
+    protoc_args += ["--cpp_out", cc_options + options.cc_out_dir]
+    for name in stripped_protos:
+      headers.append(os.path.join(options.cc_out_dir, name + ".pb.h"))
+
+  if options.py_out_dir is not None:
+    protoc_args += ["--python_out", options.py_out_dir]
+
+  if options.plugin_out_dir is not None:
+    plugin_options = ProtocGenOptions(options.plugin_options)
+    protoc_args += [
+      "--plugin", "protoc-gen-plugin=" + os.path.relpath(options.plugin),
+      "--plugin_out", plugin_options + options.plugin_out_dir,
+    ]
+
+  for name in protos:
+    protoc_args.append(os.path.join(proto_dir, name))

[petrcermak, 2016/08/15 13:00:06]

I personally prefer comprehensions over explicit loops

protoc_args += [os.path.join(proto_dir, name) for name in protos]

[kraynov, 2016/08/15 13:37:49]

Acknowledged.

+
+  ret = subprocess.call([protoc] + protoc_args)
+  if ret != 0:
+    raise RuntimeError("Protoc has returned non-zero status: " + str(ret))
+
+  if options.include is not None:

[petrcermak, 2016/08/15 13:00:06]

I think you can drop "is not None" here

[kraynov, 2016/08/15 13:37:49]

Acknowledged.

+    WriteIncludes(headers, options.include)
 
 
 if __name__ == '__main__':

[petrcermak, 2016/08/15 13:00:06]

Before this patch, this file used single quotes everywhere. Now, double quotes
are used everywhere but here. Either switch back to single quotes everywhere, or
change this to double quotes as well, please.

[kraynov, 2016/08/15 13:37:50]

Acknowledged.

-  sys.exit(main(sys.argv))
+  try:
+    main(sys.argv)
+  except RuntimeError as e:
+    print(e, file=sys.stderr)
+    sys.exit(1)