flatbuffers icon indicating copy to clipboard operation
flatbuffers copied to clipboard

flatc --python --grpc: error when adding input or output path [python, grpc, flatbuffer v2.0.0, gRPC v1.35.0]

Open Mark-Niemeyer opened this issue 3 years ago • 1 comments

When I have the message .fbs files and the gRPC service .fbs files in one folder and call flatc in that folder with the following command everything works fine.

flatc --python --grpc *.fbs

But when I move the message .fbs files into another folder (~/messagefolder) and call flatc with the -I argument

flatc --python --grpc -I ~/messagefolder *.fbs

I get this error:

flatc: error: Unable to generate GRPC interface forPython

Additionally, if I just add an output folder like this:

flatc --python --grpc -o ~/outputfolder *.fbs

I get the same error.

If I try all of the above flatc calls with --cpp instead of --python everything works fine.

Mark-Niemeyer avatar Jul 29 '22 08:07 Mark-Niemeyer

I would try to debug it by examining the following file: https://github.com/google/flatbuffers/blob/master/src/idl_gen_grpc.cpp#L417-L466

dbaileychess avatar Aug 06 '22 05:08 dbaileychess

This issue is stale because it has been open 6 months with no activity. Please comment or label not-stale, or this will be closed in 14 days.

github-actions[bot] avatar Mar 04 '23 01:03 github-actions[bot]

Still unresolved.

Mark-Niemeyer avatar Mar 08 '23 10:03 Mark-Niemeyer

I can also confirm this is an issue with Python and not C++.

I have this mock bidirectional streaming RPC.

// Filepath: {project_root}/schema/service.fbs

table StreamRequest {
    message:string;
}

table StreamResponse {
    message:string;
}

rpc_service Streamer {
    Stream(StreamRequest):StreamResponse (streaming: "bidi");
}

Case 1: All in same dir - Python

✅ If I am in the {project_root}/schema directory and run flatc --python --grpc service.fbs everything is correctly written out to the same directory (FlatBuffer code and gRPC stub code).

Case 2: Output dir different - Python

❌ If I am in the {project_root}/schema directory and run flatc --python --grpc -o output_dir service.fbs. The FlatBuffer code does go into {project_root}/schema/output_dir correctly, but the gRPC stub code gets generated in {project_root}/schema.

Case 3: Output dir different - C++

✅ If I am in the {project_root}/schema directory and run flatc --cpp --grpc -o output_dir service.fbs everything is correctly written out to the {project_root}/schema/output_dir directory (FlatBuffer code and gRPC stub code).

Case 4: Other languages

I can confirm Java, Go, TypeScript, and Swift are also working ok.

tasansal avatar Aug 20 '23 05:08 tasansal

Ok comparing these lines in Python (red) and Go (green) implementations are below. The path is commented out for some reason?

https://github.com/google/flatbuffers/blob/master/src/idl_gen_grpc.cpp

...
-456 bool GeneratePythonGRPC(const Parser &parser, const std::string & /*path*/,
-457                       const std::string &file_name) {
+331 bool GenerateGoGRPC(const Parser &parser, const std::string &path,
+332                       const std::string &file_name) {

Path missing here and Python thinks its swift :) and looks like member variables aren't initialized for path and file name etc.

-423       : BaseGenerator(parser, "", filename, "", "" /*Unused*/, "swift") {}
+302       : BaseGenerator(parser, path, file_name, "", "" /*Unused*/, "go"),
+303         parser_(parser),
+304         path_(path),
+305         file_name_(file_name) {}

and these protected members don't exist in Python implementation:

+326  protected:
+327  const Parser &parser_;
+328  const std::string &path_, &file_name_;

So looking at this, it makes sense why the Python version isn't in parity. However, these changes look like very deliberate things. Any idea what's going on?

Maybe @malarinv @mustiikhalil can help, as they did some work here.

tasansal avatar Aug 20 '23 05:08 tasansal

With this modification, it does output it to the right place. I can do a PR if there are no objections to this. Does it make sense to put the path before the namespace_dir; or is there a more elegant way to do this using the BaseGenerator's NamespaceDir etc.? I'm not familiar with the code base.

diff --git a/src/idl_gen_grpc.cpp b/src/idl_gen_grpc.cpp
index 2be24e71..bdb3bc51 100644
--- a/src/idl_gen_grpc.cpp
+++ b/src/idl_gen_grpc.cpp
@@ -419,8 +419,9 @@ class PythonGRPCGenerator : public flatbuffers::BaseGenerator {
   CodeWriter code_;
 
  public:
-  PythonGRPCGenerator(const Parser &parser, const std::string &filename)
-      : BaseGenerator(parser, "", filename, "", "" /*Unused*/, "swift") {}
+  PythonGRPCGenerator(const Parser &parser, const std::string &path,
+                      const std::string &file_name)
+      : BaseGenerator(parser, path, file_name, "", "" /*Unused*/, "python") {}
 
   bool generate() {
     code_.Clear();
@@ -447,13 +448,13 @@ class PythonGRPCGenerator : public flatbuffers::BaseGenerator {
       if (it != namespaces.begin()) namespace_dir += kPathSeparator;
       namespace_dir += *it;
     }
-    std::string grpc_py_filename = namespace_dir;
+    std::string grpc_py_filename = path_ + namespace_dir;
     if (!namespace_dir.empty()) grpc_py_filename += kPathSeparator;
     return grpc_py_filename + file_name_ + "_grpc_fb.py";
   }
 };
 
-bool GeneratePythonGRPC(const Parser &parser, const std::string & /*path*/,
+bool GeneratePythonGRPC(const Parser &parser, const std::string &path,
                         const std::string &file_name) {
   int nservices = 0;
   for (auto it = parser.services_.vec.begin(); it != parser.services_.vec.end();
@@ -462,7 +463,7 @@ bool GeneratePythonGRPC(const Parser &parser, const std::string & /*path*/,
   }
   if (!nservices) return true;
 
-  return PythonGRPCGenerator(parser, file_name).generate();
+  return PythonGRPCGenerator(parser, path, file_name).generate();
 }
 
 class SwiftGRPCGenerator : public flatbuffers::BaseGenerator {

tasansal avatar Aug 20 '23 06:08 tasansal

@Mark-Niemeyer can you try my PR branch and see if it fixes your issue as well?

tasansal avatar Aug 20 '23 17:08 tasansal

@tasansal I can verify that it resolves the problems. Thanks for your contribution!

jarkenau avatar Aug 21 '23 08:08 jarkenau