Support multiple include paths. Pretty print errors. Support combined import

tools/distrib/python/grpcio_tools/grpc_tools/BUILD

@@ -96,6 +96,7 @@ py_test(
     "simpler.proto",
     "simplest.proto",
     "complicated.proto",
+    "flawed.proto",
   ],
   python_version = "PY3",
 )

tools/distrib/python/grpcio_tools/grpc_tools/_protoc_compiler.pyx

@@ -35,8 +35,8 @@ cdef extern from "grpc_tools/main.h":
     string message
 
   int protoc_main(int argc, char *argv[])
-  int protoc_get_protos(char* protobuf_path, char* include_path, vector[pair[string, string]]* files_out, vector[cProtocError]* errors, vector[cProtocWarning]* wrnings) except +
+  int protoc_get_protos(char* protobuf_path, vector[string]* include_path, vector[pair[string, string]]* files_out, vector[cProtocError]* errors, vector[cProtocWarning]* wrnings) except +
-  int protoc_get_services(char* protobuf_path, char* include_path, vector[pair[string, string]]* files_out, vector[cProtocError]* errors, vector[cProtocWarning]* wrnings) except +
+  int protoc_get_services(char* protobuf_path, vector[string]* include_path, vector[pair[string, string]]* files_out, vector[cProtocError]* errors, vector[cProtocWarning]* wrnings) except +
 
 def run_main(list args not None):
   cdef char **argv = <char **>stdlib.malloc(len(args)*sizeof(char *))
@@ -54,8 +54,8 @@ class ProtocError(Exception):
     def __repr__(self):
         return "ProtocError(filename=\"{}\", line={}, column={}, message=\"{}\")".format(self.filename, self.line, self.column, self.message)
 
-    # TODO: Maybe come up with something better than this
+    def __str__(self):
-    __str__ = __repr__
+        return "{}:{}:{} error: {}".format(self.filename.decode("ascii"), self.line, self.column, self.message.decode("ascii"))
 
 class ProtocWarning(Warning):
     def __init__(self, filename, line, column, message):
@@ -70,6 +70,20 @@ class ProtocWarning(Warning):
     # TODO: Maybe come up with something better than this
     __str__ = __repr__
 
+
+class ProtocErrors(Exception):
+    def __init__(self, errors):
+        self._errors = errors
+
+    def errors(self):
+        return self._errors
+
+    def __repr__(self):
+        return "ProtocErrors[{}]".join(repr(err) for err in self._errors)
+
+    def __str__(self):
+        return "\n".join(str(err) for err in self._errors)
+
 cdef _c_protoc_error_to_protoc_error(cProtocError c_protoc_error):
     return ProtocError(c_protoc_error.filename, c_protoc_error.line, c_protoc_error.column, c_protoc_error.message)
 
@@ -84,24 +98,26 @@ cdef _handle_errors(int rc, vector[cProtocError]* errors, vector[cProtocWarning]
        py_errors = [_c_protoc_error_to_protoc_error(c_error) for c_error in dereference(errors)]
        # TODO: Come up with a good system for printing multiple errors from
        # protoc.
-       raise Exception(py_errors)
+       raise ProtocErrors(py_errors)
     raise Exception("An unknown error occurred while compiling {}".format(protobuf_path))
 
-def get_protos(bytes protobuf_path, bytes include_path):
+def get_protos(bytes protobuf_path, list include_paths):
+  cdef vector[string] c_include_paths = include_paths
   cdef vector[pair[string, string]] files
   cdef vector[cProtocError] errors
-  # NOTE: Abbreviated name used to shadowing of the module name.
+  # NOTE: Abbreviated name used to avoid shadowing of the module name.
   cdef vector[cProtocWarning] wrnings
-  rc = protoc_get_protos(protobuf_path, include_path, &files, &errors, &wrnings)
+  rc = protoc_get_protos(protobuf_path, &c_include_paths, &files, &errors, &wrnings)
   _handle_errors(rc, &errors, &wrnings, protobuf_path)
   return files
 
-def get_services(bytes protobuf_path, bytes include_path):
+def get_services(bytes protobuf_path, list include_paths):
+  cdef vector[string] c_include_paths = include_paths
   cdef vector[pair[string, string]] files
   cdef vector[cProtocError] errors
-  # NOTE: Abbreviated name used to shadowing of the module name.
+  # NOTE: Abbreviated name used to avoid shadowing of the module name.
   cdef vector[cProtocWarning] wrnings
-  rc = protoc_get_services(protobuf_path, include_path, &files, &errors, &wrnings)
+  rc = protoc_get_services(protobuf_path, &c_include_paths, &files, &errors, &wrnings)
   _handle_errors(rc, &errors, &wrnings, protobuf_path)
   return files
 

tools/distrib/python/grpcio_tools/grpc_tools/flawed.proto

@@ -0,0 +1,9 @@
+syntax = "proto3";
+
+message Broken {
+  int32 no_field_number;
+};
+
+message Broken2 {
+  int32 no_field_number;
+};

tools/distrib/python/grpcio_tools/grpc_tools/main.cc

@@ -133,14 +133,16 @@ static void calculate_transitive_closure(const ::google::protobuf::FileDescripto
 // TODO: Handle multiple include paths.
 static int generate_code(::google::protobuf::compiler::CodeGenerator* code_generator,
                          char* protobuf_path,
-                         char* include_path,
+                         const std::vector<std::string>* include_paths,
                          std::vector<std::pair<std::string, std::string>>* files_out,
                          std::vector<ProtocError>* errors,
                          std::vector<ProtocWarning>* warnings)
 {
   std::unique_ptr<detail::ErrorCollectorImpl> error_collector(new detail::ErrorCollectorImpl(errors, warnings));
   std::unique_ptr<::google::protobuf::compiler::DiskSourceTree> source_tree(new ::google::protobuf::compiler::DiskSourceTree());
-  source_tree->MapPath("", include_path);
+  for (const auto& include_path : *include_paths) {
+    source_tree->MapPath("", include_path);
+  }
   ::google::protobuf::compiler::Importer importer(source_tree.get(), error_collector.get());
   const ::google::protobuf::FileDescriptor* parsed_file = importer.Import(protobuf_path);
   if (parsed_file == nullptr) {
@@ -159,22 +161,22 @@ static int generate_code(::google::protobuf::compiler::CodeGenerator* code_gener
 }
 
 int protoc_get_protos(char* protobuf_path,
-                     char* include_path,
+                     const std::vector<std::string>* include_paths,
                      std::vector<std::pair<std::string, std::string>>* files_out,
                      std::vector<ProtocError>* errors,
                      std::vector<ProtocWarning>* warnings)
 {
   ::google::protobuf::compiler::python::Generator python_generator;
-  return generate_code(&python_generator, protobuf_path, include_path, files_out, errors, warnings);
+  return generate_code(&python_generator, protobuf_path, include_paths, files_out, errors, warnings);
 }
 
 int protoc_get_services(char* protobuf_path,
-                     char* include_path,
+                     const std::vector<std::string>* include_paths,
                      std::vector<std::pair<std::string, std::string>>* files_out,
                      std::vector<ProtocError>* errors,
                      std::vector<ProtocWarning>* warnings)
 {
   grpc_python_generator::GeneratorConfiguration grpc_py_config;
   grpc_python_generator::PythonGrpcGenerator grpc_py_generator(grpc_py_config);
-  return generate_code(&grpc_py_generator, protobuf_path, include_path, files_out, errors, warnings);
+  return generate_code(&grpc_py_generator, protobuf_path, include_paths, files_out, errors, warnings);
 }

tools/distrib/python/grpcio_tools/grpc_tools/main.h

@@ -40,13 +40,13 @@ typedef ProtocError ProtocWarning;
 
 // TODO: Create Alias for files_out type?
 int protoc_get_protos(char* protobuf_path,
-                     char* include_path,
+                     const std::vector<std::string>* include_paths,
                      std::vector<std::pair<std::string, std::string>>* files_out,
                      std::vector<ProtocError>* errors,
                      std::vector<ProtocWarning>* warnings);
 
 int protoc_get_services(char* protobuf_path,
-                     char* include_path,
+                     const std::vector<std::string>* include_paths,
                      std::vector<std::pair<std::string, std::string>>* files_out,
                      std::vector<ProtocError>* errors,
                      std::vector<ProtocWarning>* warnings);

tools/distrib/python/grpcio_tools/grpc_tools/protoc.py

@@ -52,14 +52,27 @@ def _import_modules_from_files(files):
       modules.append(sys.modules[module_name])
   return tuple(modules)
 
-def get_protos(protobuf_path, include_path):
+# TODO: Investigate making this even more of a no-op in the case that we have
-  files = _protoc_compiler.get_protos(protobuf_path.encode('ascii'), include_path.encode('ascii'))
+# truly already imported the module.
+def get_protos(protobuf_path, include_paths=None):
+  if include_paths is None:
+    include_paths = sys.path
+  files = _protoc_compiler.get_protos(protobuf_path.encode('ascii'), [include_path.encode('ascii') for include_path in include_paths])
   return _import_modules_from_files(files)[-1]
 
-def get_services(protobuf_path, include_path):
+def get_services(protobuf_path, include_paths=None):
-  files = _protoc_compiler.get_services(protobuf_path.encode('ascii'), include_path.encode('ascii'))
+  # NOTE: This call to get_protos is a no-op in the case it has already been
+  # called.
+  get_protos(protobuf_path, include_paths)
+  if include_paths is None:
+    include_paths = sys.path
+  files = _protoc_compiler.get_services(protobuf_path.encode('ascii'), [include_path.encode('ascii') for include_path in include_paths])
   return _import_modules_from_files(files)[-1]
 
+def get_protos_and_services(protobuf_path, include_paths=None):
+  return (get_protos(protobuf_path, include_paths=include_paths),
+          get_services(protobuf_path, include_paths=include_paths))
+
 
 if __name__ == '__main__':
     proto_include = pkg_resources.resource_filename('grpc_tools', '_proto')

tools/distrib/python/grpcio_tools/grpc_tools/protoc_test.py

@@ -31,26 +31,38 @@ def _run_in_subprocess(test_case):
 def _test_import_protos():
     from grpc_tools import protoc
     proto_path = "tools/distrib/python/grpcio_tools/"
-    protos = protoc.get_protos("grpc_tools/simple.proto", proto_path)
+    protos = protoc.get_protos("grpc_tools/simple.proto", [proto_path])
     assert protos.SimpleMessage is not None
 
 
 def _test_import_services():
     from grpc_tools import protoc
     proto_path = "tools/distrib/python/grpcio_tools/"
-    # TODO: Should we make this step optional if you only want to import
+    protos = protoc.get_protos("grpc_tools/simple.proto", [proto_path])
-    # services?
+    services = protoc.get_services("grpc_tools/simple.proto", [proto_path])
-    protos = protoc.get_protos("grpc_tools/simple.proto", proto_path)
+    assert services.SimpleMessageServiceStub is not None
-    services = protoc.get_services("grpc_tools/simple.proto", proto_path)
+
+
+# NOTE: In this case, we use sys.path to determine where to look for our protos.
+def _test_import_implicit_include():
+    from grpc_tools import protoc
+    protos = protoc.get_protos("grpc_tools/simple.proto")
+    services = protoc.get_services("grpc_tools/simple.proto")
+    assert services.SimpleMessageServiceStub is not None
+
+
+def _test_import_services_without_protos():
+    from grpc_tools import protoc
+    services = protoc.get_services("grpc_tools/simple.proto")
     assert services.SimpleMessageServiceStub is not None
 
 
 def _test_proto_module_imported_once():
     from grpc_tools import protoc
     proto_path = "tools/distrib/python/grpcio_tools/"
-    protos = protoc.get_protos("grpc_tools/simple.proto", proto_path)
+    protos = protoc.get_protos("grpc_tools/simple.proto", [proto_path])
-    services = protoc.get_services("grpc_tools/simple.proto", proto_path)
+    services = protoc.get_services("grpc_tools/simple.proto", [proto_path])
-    complicated_protos = protoc.get_protos("grpc_tools/complicated.proto", proto_path)
+    complicated_protos = protoc.get_protos("grpc_tools/complicated.proto", [proto_path])
     assert (complicated_protos.grpc__tools_dot_simplest__pb2.SimplestMessage is
             protos.grpc__tools_dot_simpler__pb2.grpc__tools_dot_simplest__pb2.SimplestMessage)
 
@@ -59,14 +71,33 @@ def _test_static_dynamic_combo():
     from grpc_tools import complicated_pb2
     from grpc_tools import protoc
     proto_path = "tools/distrib/python/grpcio_tools/"
-    protos = protoc.get_protos("grpc_tools/simple.proto", proto_path)
+    protos = protoc.get_protos("grpc_tools/simple.proto", [proto_path])
     assert (complicated_pb2.grpc__tools_dot_simplest__pb2.SimplestMessage is
             protos.grpc__tools_dot_simpler__pb2.grpc__tools_dot_simplest__pb2.SimplestMessage)
 
 
-class ProtocTest(unittest.TestCase):
+def _test_combined_import():
+    from grpc_tools import protoc
+    protos, services = protoc.get_protos_and_services("grpc_tools/simple.proto")
+    assert protos.SimpleMessage is not None
+    assert services.SimpleMessageServiceStub is not None
+
+
+def _test_syntax_errors():
+    from grpc_tools import protoc
+    try:
+        protos = protoc.get_protos("grpc_tools/flawed.proto")
+    except Exception as e:
+        error_str = str(e)
+        assert "flawed.proto" in error_str
+        assert "3:23" in error_str
+        assert "7:23" in error_str
+        print(error_str)
+    else:
+        assert False, "Compile error expected. None occurred."
 
-    # TODO: Test error messages.
+
+class ProtocTest(unittest.TestCase):
 
     def test_import_protos(self):
         _run_in_subprocess(_test_import_protos)
@@ -74,12 +105,23 @@ class ProtocTest(unittest.TestCase):
     def test_import_services(self):
         _run_in_subprocess(_test_import_services)
 
+    def test_import_implicit_include_path(self):
+        _run_in_subprocess(_test_import_implicit_include)
+
+    def test_import_services_without_protos(self):
+        _run_in_subprocess(_test_import_services_without_protos)
+
     def test_proto_module_imported_once(self):
         _run_in_subprocess(_test_proto_module_imported_once)
 
     def test_static_dynamic_combo(self):
         _run_in_subprocess(_test_static_dynamic_combo)
 
+    def test_combined_import(self):
+        _run_in_subprocess(_test_combined_import)
+
+    def test_syntax_errors(self):
+        _run_in_subprocess(_test_syntax_errors)
 
 if __name__ == '__main__':
     unittest.main()