multi-level includes do not work

[vbmithr]

For example:

import "temporal/api/enums/v1/workflow.proto";
import "temporal/api/common/v1/message.proto";
import "temporal/api/failure/v1/message.proto";
import "temporal/api/taskqueue/v1/message.proto";

Would generate a caml file with 3 times Message = Message lines generated, which is incorrect.

[vbmithr]

For instance, a protobuf like this

syntax = "proto3";

package temporal.api.workflow.v1;

option go_package = "go.temporal.io/api/workflow/v1;workflow";
option java_package = "io.temporal.api.workflow.v1";
option java_multiple_files = true;
option java_outer_classname = "MessageProto";
option ruby_package = "Temporalio::Api::Workflow::V1";
option csharp_namespace = "Temporalio.Api.Workflow.V1";

import "google/protobuf/duration.proto";
import "google/protobuf/timestamp.proto";

import "dependencies/gogoproto/gogo.proto";

import "temporal/api/enums/v1/workflow.proto";
import "temporal/api/common/v1/message.proto";
import "temporal/api/failure/v1/message.proto";
import "temporal/api/taskqueue/v1/message.proto";

message WorkflowExecutionInfo {
    temporal.api.common.v1.WorkflowExecution execution = 1;
    temporal.api.common.v1.WorkflowType type = 2;
    google.protobuf.Timestamp start_time = 3 [(gogoproto.stdtime) = true];
    google.protobuf.Timestamp close_time = 4 [(gogoproto.stdtime) = true];
    temporal.api.enums.v1.WorkflowExecutionStatus status = 5;
    int64 history_length = 6;
    string parent_namespace_id = 7;
    temporal.api.common.v1.WorkflowExecution parent_execution = 8;
    google.protobuf.Timestamp execution_time = 9 [(gogoproto.stdtime) = true];
    temporal.api.common.v1.Memo memo = 10;
    temporal.api.common.v1.SearchAttributes search_attributes = 11;
    ResetPoints auto_reset_points = 12;
    string task_queue = 13;
    int64 state_transition_count = 14;
    int64 history_size_bytes = 15;
    // If set, the most recent worker version stamp that appeared in a workflow task completion
    temporal.api.common.v1.WorkerVersionStamp most_recent_worker_version_stamp = 16;
}

would get compiled like that

(************************************************)
(*       AUTOGENERATED FILE - DO NOT EDIT!      *)
(************************************************)
(* Generated by: ocaml-protoc-plugin            *)
(* https://github.com/issuu/ocaml-protoc-plugin *)
(************************************************)
(*
  Source: v1/message.proto
  Syntax: proto3
  Parameters:
    debug=false
    annot='[@@deriving sexp]'
    opens=[Google_types]
    int64_as_int=true
    int32_as_int=true
    fixed_as_int=false
    singleton_record=false
*)

open Ocaml_protoc_plugin.Runtime [@@warning "-33"]
open Google_types [@@warning "-33"]
(**/**)
module Imported'modules = struct
  module Duration = Duration
  module Timestamp = Timestamp
  module Gogo = Gogo
  module Workflow = Workflow
  module Message = Message
  module Message = Message
  module Message = Message
end
(**/**)
module Temporal = struct
  module Api = struct
    module Workflow = struct
      module V1 = struct
        module rec WorkflowExecutionInfo : sig
…

[andersfugmann]

Thanks for the report.

The challenge is that proto files are named based on their filename and not path, hence temporal/api/enums/v1/workflow.proto is expected to be compiled into a Message.ml module, which is present for the compilation.

Unfortunately, Ocaml does not create namespaces based on directories, so even if the conflict was resolved, there is no easy way to solve this.

Maybe adding an option to change the output name to include the full path in the name so invoking protoc on temporal/api/workflow/v1/message.proto would create a file named temporal_api_workflow_v1_message.ml These would be distinct so no name clashes. The challenge would be on handling standard includes, but I think that can be managed.

I need to give this more thought. Its an interesting use-case - but possibly not an uncommon one.

[andersfugmann]

I will try to implement a solution. Currently the parameter ocaml_out is assumed to be a directory, and in this case the system will produce one file per input files.

A solution to your setup could be to change the interpretation of the ocaml_out flag. If its a file, all "implementation" will be placed into that file. So in your case you could have all files translated in one invocation, and there would be no ambiguity.

This will also require an extra flag to allow specifying complete "implementations", i.e. if google types implementation resides in a single file, all references to google types would be from this module (i.e. ocaml_package=google=Google_single_file or similar to indicate that any module in toplevel google should reference Google_single_file. Alternatively we could just completely omit any internal module and assume that the modules are available externally. I'll sketch out the details while implementing.

@vbmithr Would this solution work for you. I.e. would you be able to compile all protobuf files in one single invocation, listing all .proto files under temporal to be compiled into a single file temporal.ml that would then contain everything?

(Note that once Dune supports include_dirs qualified, that would render this new behaviour obsolete but I dont know when that will be implemented.)

[vbmithr]

I managed to hack a solution in my fork which works for my use case. Basically I use the fully qualified name for modules names I generate instead of wrapping modules inside the file. I'll try to explain in more details later but if you look at my implementation you will surely understand my changes.

Thanks for thinking at this!

https://github.com/deepmarker/ocaml-protoc-plugin/tree/vb

[andersfugmann]

Solved by https://github.com/andersfugmann/ocaml-protoc-plugin/issues/7. Closing

[andersfugmann]

Should be noted that this is solved in repo https://github.com/andersfugmann/ocaml-protoc-plugin