uber / prototool

Your Swiss Army Knife for Protocol Buffers
MIT License
5.04k stars 345 forks source link

Include proto files from Go modules #498

Closed fgrosse closed 4 years ago

fgrosse commented 5 years ago

I want to include Protobuf definitions from another repository. So far this works nicely when I have a vendor directory with the following prototool.yml:

protoc:
  version: 3.3.0

  includes:
    - vendor/example.com/foo/bar
    - vendor/example.com/foo/baz

…

Now I want to switch to Go modules (normal mode without vendor directory) and I am not sure how I can reference modules in the prototool includes directive. Is this a known use case somebody else already solved or is it simply not possible to include proto files from other Modules?

smaye81 commented 5 years ago

If you specify an absolute path to your $GOPATH/pkg/mod directory it should work. Obviously not ideal, but right now that is the only way to specify an include path using non-vendored Go modules.

smaye81 commented 5 years ago

We could maybe add a new config flag vendored: true/false which will look in your vendor directory if true and in pkg/mod if false.

So, the following would look in <projdir>/vendor for your includes:

protoc:
  version: 3.3.0

  includes:
    vendored: true
    - example.com/foo/bar
    - example.com/foo/baz

…

This would look in $GOPATH/pkg/mod for the includes

protoc:
  version: 3.3.0

  includes:
    vendored: false
    - example.com/foo/bar
    - example.com/foo/baz

…
fgrosse commented 5 years ago

Thanks for the tip with $GOPATH/pkg/mod directory. To use this however I would have to hard-code both the value of my concrete $GOPATH and also the version of the library in the proto tool now.

E.g. this works:

protoc:
  version: 3.3.0

  includes:
    - /home/fgrosse/pkg/mod/example.com/foo/bar@v0.6.1

…

Since this is not very practical a vendor mode in prototool would be very good. It should be possible given all information required can already be created using go list.

E.g. from the example, the following command would generate the path I used in the prototool.yml if I am inside a Go module that uses example.com/foo/bar

echo $GOPATH/pkg/mod/example.com/foo/bar@$(go list -m -f '{{.Version}}' example.com/foo/bar)
/home/fgrosse/pkg/mod/example.com/foo/bar@v0.6.1
fgrosse commented 5 years ago

Well actually looking into it, currently I don't see a way to access module information without having to execute go list (e.g. using os/exec) because the code is still an internal Go package. This would not be very elegant and I can understand that's not an approach prototool wants to follow.

However maybe we could be allowed to use environment variables in the imports?

diff --git a/internal/settings/config_provider.go b/internal/settings/config_provider.go
index e8fa72f..e4d6353 100644
--- a/internal/settings/config_provider.go
+++ b/internal/settings/config_provider.go
@@ -204,6 +204,8 @@ func externalConfigToConfig(develMode bool, e ExternalConfig, dirPath string) (C
        }
        includePaths := make([]string, 0, len(e.Protoc.Includes))
        for _, includePath := range strs.SortUniq(e.Protoc.Includes) {
+               includePath = os.ExpandEnv(includePath)
+
                if !filepath.IsAbs(includePath) {
                        includePath = filepath.Join(dirPath, includePath)
                }

Then we could reference the modules directory if we know the version:

protoc:
  version: 3.3.0

  includes:
    - $GOPATH/pkg/mod/example.com/foo/bar@v0.6.1

…

I would then put a CI validation script in place that will execute go list to check if I did not forget to update the version in the prototool.yaml in case dependencies get updated.

If that would be ok I can open the PR.

smaye81 commented 5 years ago

This seems fine. Please make sure to update any docs to indicate that env variables are possible in imports.

hit9 commented 5 years ago

Hello, how is this going?

fgrosse commented 5 years ago

Sorry, I got distracted and did not find time yet. Maybe I will have time on the weekend to return to this but if you want to submit a PR yourself go ahead :)

smaye81 commented 4 years ago

Closing for now due to inactivity. Please reopen (or reference this issue) if a PR is created