From 709143d1f52a99f5bd82d5a9cc5d951511b24fc2 Mon Sep 17 00:00:00 2001 From: Richard Belleville Date: Mon, 2 Dec 2019 11:33:18 -0800 Subject: [PATCH] Support multiple include paths. Pretty print errors. Support combined import --- .../python/grpcio_tools/grpc_tools/BUILD | 1 + .../grpc_tools/_protoc_compiler.pyx | 38 +++++++---- .../grpcio_tools/grpc_tools/flawed.proto | 9 +++ .../python/grpcio_tools/grpc_tools/main.cc | 14 ++-- .../python/grpcio_tools/grpc_tools/main.h | 4 +- .../python/grpcio_tools/grpc_tools/protoc.py | 21 ++++-- .../grpcio_tools/grpc_tools/protoc_test.py | 64 +++++++++++++++---- 7 files changed, 117 insertions(+), 34 deletions(-) create mode 100644 tools/distrib/python/grpcio_tools/grpc_tools/flawed.proto diff --git a/tools/distrib/python/grpcio_tools/grpc_tools/BUILD b/tools/distrib/python/grpcio_tools/grpc_tools/BUILD index 1e47da38f8b..979953f64a4 100644 --- a/tools/distrib/python/grpcio_tools/grpc_tools/BUILD +++ b/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", ) diff --git a/tools/distrib/python/grpcio_tools/grpc_tools/_protoc_compiler.pyx b/tools/distrib/python/grpcio_tools/grpc_tools/_protoc_compiler.pyx index c82129b8e2e..6cfe2bac597 100644 --- a/tools/distrib/python/grpcio_tools/grpc_tools/_protoc_compiler.pyx +++ b/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_services(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, 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 = 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 - __str__ = __repr__ + def __str__(self): + 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 diff --git a/tools/distrib/python/grpcio_tools/grpc_tools/flawed.proto b/tools/distrib/python/grpcio_tools/grpc_tools/flawed.proto new file mode 100644 index 00000000000..1fd3a549d55 --- /dev/null +++ b/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; +}; diff --git a/tools/distrib/python/grpcio_tools/grpc_tools/main.cc b/tools/distrib/python/grpcio_tools/grpc_tools/main.cc index 82fbf7aada7..56c3c67adc0 100644 --- a/tools/distrib/python/grpcio_tools/grpc_tools/main.cc +++ b/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* include_paths, std::vector>* files_out, std::vector* errors, std::vector* warnings) { std::unique_ptr 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* include_paths, std::vector>* files_out, std::vector* errors, std::vector* 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* include_paths, std::vector>* files_out, std::vector* errors, std::vector* 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); } diff --git a/tools/distrib/python/grpcio_tools/grpc_tools/main.h b/tools/distrib/python/grpcio_tools/grpc_tools/main.h index 33473de2b93..073f62eadef 100644 --- a/tools/distrib/python/grpcio_tools/grpc_tools/main.h +++ b/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* include_paths, std::vector>* files_out, std::vector* errors, std::vector* warnings); int protoc_get_services(char* protobuf_path, - char* include_path, + const std::vector* include_paths, std::vector>* files_out, std::vector* errors, std::vector* warnings); diff --git a/tools/distrib/python/grpcio_tools/grpc_tools/protoc.py b/tools/distrib/python/grpcio_tools/grpc_tools/protoc.py index e22f63e92a7..d11ea07eb70 100644 --- a/tools/distrib/python/grpcio_tools/grpc_tools/protoc.py +++ b/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): - files = _protoc_compiler.get_protos(protobuf_path.encode('ascii'), include_path.encode('ascii')) +# TODO: Investigate making this even more of a no-op in the case that we have +# 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): - files = _protoc_compiler.get_services(protobuf_path.encode('ascii'), include_path.encode('ascii')) +def get_services(protobuf_path, include_paths=None): + # 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') diff --git a/tools/distrib/python/grpcio_tools/grpc_tools/protoc_test.py b/tools/distrib/python/grpcio_tools/grpc_tools/protoc_test.py index fa8b5c51a05..fcbb77498dd 100644 --- a/tools/distrib/python/grpcio_tools/grpc_tools/protoc_test.py +++ b/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 - # services? - protos = protoc.get_protos("grpc_tools/simple.proto", proto_path) - services = protoc.get_services("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]) + assert services.SimpleMessageServiceStub is not None + + +# 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) - services = protoc.get_services("grpc_tools/simple.proto", proto_path) - complicated_protos = protoc.get_protos("grpc_tools/complicated.proto", proto_path) + protos = protoc.get_protos("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]) 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()