Protobuf: Add new paramater to filter `import_prefix` usage

[ss-limafriyadi]

Hi guys,

We are using DDD as our development method. We are separate our protobuf files according to our domains in a monorepo environment. There are some proto files that need to import other proto.

// proto/domain/b/service.proto
syntax="proto3";

package com.example.proto.domain.a;

import "google/protobuf/wrappers.proto";

message TestA {
    string id = 1;
}

// proto/domain/b/service.proto
syntax="proto3";

package com.example.proto.domain.b;

import "google/protobuf/wrappers.proto";
import "proto/domain/a/service.proto";

message TestB {
    string id = 1;
    TestA a = 2;
}

• Generated source without import_prefix

// repo/service/proto/domain/b/service.pb.go
// ...
import google_protobuf "github.com/golang/protobuf/proto";
// this is problematic in our monorepo environment,
// since we need to use the full repository path to import correctly 
import com_example_proto_domain_a "proto/domain/b";

• Generated source with import_prefix=github.com/user/repo/

// repo/service/proto/domain/b/service.pb.go
// ...
// The import prefix applied to google wrappers too
// This generate compile error since the path does not exists
import google_protobuf "github.com/user/repo/github.com/golang/protobuf/wrappers";
// This import works correctly
import com_example_proto_domain_a "github.com/user/repo/proto/domain/b";

Can we add an option (e.g path_prefix) to make the import_prefix applied ONLY when the import path match the prefix defined in path_prefix?

[neild]

I believe you can get the behavior you want by including a go_package option in the source file:

option go_package = "github.com/user/repo/proto/domain/b";

This will have three effects:

• It sets the Go package name to b. (Which it already was, so a no-op.)
• Imports of this file will use import "github.com/user/repo/proto/domain/b". (The desired behavior.)
• The proto compiler will write the output file to github.com/user/repo/proto/domain/b/service.pb.go. (Possibly not the most desired behavior (#515), but not too difficult to work around.)

[ss-limafriyadi]

The 3rd effect was exactly why I request the feature. Since the problem is related to our development environment (monorepo and centralize protobuf location) I guess your suggestion works for most people. But thanks anyway for your reply @neild. :bowing_man:

[neild]

I agree that the organization of output files is inconvenient in some (many? most?) cases, and we should do something to fix that. If you have any thoughts on whether any of my proposals in #515 would help, I'd love to know. Thanks!

[FreGalle]

@neild I just wanted to elaborate on the setup my company uses in which we deliberately don't have a go_package option specified in every one of our protobuf source files.

We have one single repository containing all of our protobuf definition files. This repo is included as a submodule in multiple other projects. Those projects are responsible for generating and maintaining their own generated code from those protobuf definitions.

Because these generated files end up at different locations inside the $GOPATH, no single go_package value will accomodate this.

Being able to specify an import path prefix (in conjunction with the relative naming implemented in this PR) as a command-line option when generating the code seems like a good solution here as every project would be able to provide its own distinct destination for the code to live in as well as the importable package reference.

This sounds like the kind of problem import_prefix was meant to resolve, but its indiscriminate rewriting of all import paths is often unwanted and leads to some creative workarounds.

Currently there doesn't seem to be a clean or obvious way only using go_package (the advice given in #515) to allow multiple consumers of a proto file to use different import paths. Am I missing something obvious? Happy to discuss this further.

[neild]

Currently there doesn't seem to be a clean or obvious way only using go_package (the advice given in #515) to allow multiple consumers of a proto file to use different import paths. Am I missing something obvious?

I don't understand why you would want to use different import paths for the same proto package. This will prevent different consumers of that package from interoperating, since they'll each have a different copy of it.

If you do want a local copy of a package, you could use a vendor directory in the same way as with any other Go package. (I'm not certain what the vgo equivalent of vendor directories is, but I presume there is one.)

If you do need to change the import path associated with a specific source .proto file, you can use the Mfoo.proto=import/path compiler flag to do so.

I think that the best approach for almost everyone, however, is to use a consistent import path for each generated package.

[FreGalle]

I don't understand why you would want to use different import paths for the same proto package. This will prevent different consumers of that package from interoperating, since they'll each have a different copy of it.

The resulting packages would not be used by different consumers, they generally live somewhere as an internal package.

If you do want a local copy of a package, you could use a vendor directory in the same way as with any other Go package. (I'm not certain what the vgo equivalent of vendor directories is, but I presume there is one.)

Fair point. vendor is the logical answer to maintaining project-specific and externally inaccessible copies of packages. We did look into this, but currently dep, the de facto versioning solution in the go-space, prevents us from doing this without also maintaining an upstream mirror of the generated code. This points to a limitation in dep's dealing with vendor, for sure. It also means vendoring (using dep) is currently not a viable solution for anyone who wants to generate similar protobuf code in different projects under different import paths.

If you do need to change the import path associated with a specific source .proto file, you can use the Mfoo.proto=import/path compiler flag to do so.

This does however lead to less-maintainable generator code. Dependencies between protobuf packages (and hence import paths) have to be maintained both inside the protobuf definitions as well as in any dependant project's protobuf generation instructions.

[neild]

This does however lead to less-maintainable generator code. Dependencies between protobuf packages (and hence import paths) have to be maintained both inside the protobuf definitions as well as in any dependant project's protobuf generation instructions.

It's true that generating a rename of each source .proto is messy, but I don't think import_prefix is the answer.

Leaving aside the question of whether any of your own proto packages have multiple consumers, import_prefix will affect the import path of well-known files such as google/protobuf/any.proto. These are types that really should have a single, canonical implementation with a stable, defined Go import path.

[FreGalle]

The current behaviour of import_prefix is indeed not the answer. The adjustment of the import path of well-known proto files or the github.com/golang/protobuf/proto package seems like a mostly unintended side-effect that should not be the default behaviour of this option.

Unfortunately it currently is the most maintainable solution to the problem I described above. Most often it will have to be used in conjunction with one or more additional sed-replacements to fix the import paths of anything that wasn't included in the compilation. It's what I'll end up doing and what high-profile projects like cockroachdb seem to do.

paths=source_relative as defined in #515 is a good start to a saner output and import management solution. The deprecation of import_prefix however leaves us with only Mfoo.proto=import/path to steer import paths on a per-project basis (i.e. not based on go_package)

Don't get me wrong, I'm not advocating keeping import_prefix as-is, but combining paths=source_relative with a saner import_prefix (only applied to imports inside --proto-path) looks like it might solve the problem. Granted, this is only my point of view based on the way we use protobuf files and I'm sure there's a reason why import_prefix works the way it does, but to me this is still a valuable discussion in order to try and accomodate the different ways of compiling protobuf files.

[ss-limafriyadi]

Well, I think my case (and maybe @FreGalle) is not a common use case. But still, I made a fork to the generator and added a new option to apply import_prefix just as I proposed. And it proves to be useful and working well in our current repository setup (monorepo). @FreGalle if you're interested about my work, you can see here

[neild]

I'm open to making changes to better support other use cases, but I strongly feel that any new features need to be both very clear in what they do and how they are intended to be used.

I don't really want to tweak import_prefix because it already fails on both these points: The exact effect it has is fairly obscure and it's not at all clear how you're supposed to use it. I'd say we should just remove it, but that would doubtless break someone relying on the current behavior. I don't see a way to change it that makes it clear and generally useful:

• Changing it to not affect fmt and other stdlib package imports leaves imports of any.proto and other WKTs broken.
• Changing it to not affect any.proto starts us down a path where it isn't at all clear which imports are affected and which are not. (How about google.rpc.*? And so on.)

The proto compiler does currently give you full control over the Go import path associated with a given .proto file, in two ways:

• Specify a go_package option in the .proto file. Does precisely the right thing when a .proto file is associated with a single, canonical Go package. Almost everyone should use this.
• Provide a Mx.proto=importpath flag to the compiler. Gives you complete control, at the expense of requiring you to be very explicit. Most useful for build systems which have their own conception of import paths (e.g., bazel).

If we're to add an additional option to that list, it needs to be clearly specified and not break imports of WKT protos and the like.