Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/vgo: non Go source files (eg descriptor.proto) and generated code (eg ) #25957

Closed
millergarym opened this issue Jun 19, 2018 · 9 comments

Comments

@millergarym
Copy link

commented Jun 19, 2018

What version of Go are you using (go version)?

go version go1.10.3 windows/amd64

Does this issue reproduce with the latest release?

yes

What did you do?

Path to non-Go source files

Using go generate or other scripts to run protoc the directory containing protobuf is needed (for includes). This is now dynamic, depending on the version of github.com/golang/protobuf. This issue exists for all non Go code, protobufs are being used as a concrete example.

The directory has changed from

$GOPATH/src/github.com/golang/protobuf/protoc-gen-go/

to

$GOPATH/src/mod/github.com/golang/protobuf@v1.1.0/protoc-gen-go/

Generated Go code (not in the top level vendor directory)

As part of a clean build multiple packages may need to generate code. Restricting generated code to all be in the top level vendor directory is constraining. Eg. protobuf extensions.

Discussion

Path to non-Go source files

  • The format rules for vgo list might be enough to construct a path to the thirdparty source.
  • A simple argument to vgo to get the path
  • Some environment variable or template expansion by go generate

Generated Go code (not in the top level vendor directory)

Any suggestions?

@gopherbot gopherbot added this to the vgo milestone Jun 19, 2018

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2018

Perhaps go generate should support $SRCDIR. Would that solve the problem?

@myitcv

This comment has been minimized.

Copy link
Member

commented Jun 19, 2018

I think this issue touches on the discussion over in #14120 (comment).

@ianlancetaylor - is $SRCDIR the working directory of go generate? If so, I had a similar suggestion in #14120 (comment).

/cc @alandonovan

@millergarym

This comment has been minimized.

Copy link
Author

commented Jun 20, 2018

What does $SRCDIR point at? Can it contain multiple paths?

I don't think it is enough, as generate and build steps;

  1. require that paths to multiple thirdparty (non go) source directories
  2. to keep package build self contained, multiple 'vendor' directories are currently taken into account.

issue 1

Currently use gopath to construct paths to protobufs.
eg

-I$GOPATH/src/github.com/grpc-ecosystem/grpc-gateway
-I$GOPATH/src/github.com/grpc-ecosystem/grpc-gateway/third_party/googleapis

This could potentially change to;

in shell
-I$GOPATH/src/mod/$(some go command "github.com/grpc-ecosystem")/grpc-gateway
with some go command "github.com/grpc-ecosystem" returning
github.com/grpc-ecosystem@v1.1.0

or

-I$(some go command with different args "github.com/grpc-ecosystem")/grpc-gateway
which could return
/home/me/go/src/mod/github.com/grpc-ecosystem@v1.1.0
or depending on command line option (and defaults) extract file to a temp location
/tmp/xxxx/github.com/grpc-ecosystem@v1.1.0

go generate specific solution
//go:generate ... -I{{index . map of versions "github.com/grpc-ecosystem"}}/grpc-gateway
This is probably out of scope as the shell solution can be used in go generate.

as an aside, it would be nice to have multi line go generate commands

issue 2

As part of a build multiple package need to generate code. The package of the generated code is not generally known by the generate step.

In the worst case, the directory tree after code gen could look like

.
├── a
│   ├── a.pb.go
│   ├── a.pb.gw.go
│   ├── a.swagger.json
│   ├── utils.go                     <-  import x.y/options
│   └── vendor
│       └── x.y
│           └── options
│               └── myannotations.pb.go
└── b
    ├── b.pb.go
    ├── b.pb.gw.go
    ├── b.swagger.json
    ├── main.go                          <- imports "a", "x.y/options", "s.t/options"
    └── vendor
        └── s.t
            └── options
                └── annotations.pb.go

Thinking about it, the above isn't the worst case. Worse is if a and b depend on different version of x.y/options.

Setting the worster case aside, could above could be solved by

  • multiple dirs in a $SRCDIR or $GOPATH?
  • using one vendor directory? If so where?

Worked example.

We keep our proto files is a separate repo. Including protos that extend descriptor.proto. The build steps go 1) generate *.go from extension .proto 2) generate go from protos 3) call protoc with other plugins (grpc-gateway and swagger_out) 4) compile go code.
Step 1-3 are in a gen.go file using go generate comments.

Directory structure / env vars
$PB_PATH points at the dir where protoc.zip was extracted
$API_WORKSPACE is where api repo is cloned to

// gen.go - in top level dir of package
package example // import "internal.repo/myproject/example"

// 1. 
//go:generate mkdir -p vendor
//go:generate protoc -I$PB_PATH/include -I$API_WORKSPACE --go_out=plugins=grpc:vendor options/myannotations.proto

//2.
//go:generate protoc -I$PB_PATH/include -I$API_WORKSPACE -I$API_WORKSPACE/myproject/example -I$GOPATH/src/github.com/grpc-ecosystem/grpc-gateway -I$GOPATH/src/github.com/grpc-ecosystem/grpc-gateway/third_party/googleapis --go_out=plugins=grpc:. example.proto

//3.
//go:generate protoc -I$PB_PATH/include -I$API_WORKSPACE -I$API_WORKSPACE/myproject/example -I$GOPATH/src/github.com/grpc-ecosystem/grpc-gateway -I$GOPATH/src/github.com/grpc-ecosystem/grpc-gateway/third_party/googleapis --grpc-gateway_out=logtostderr=true:. example.proto

//go:generate protoc -I$PB_PATH/include -I$API_WORKSPACE -I$API_WORKSPACE/myproject/example -I$GOPATH/src/github.com/grpc-ecosystem/grpc-gateway -I$GOPATH/src/github.com/grpc-ecosystem/grpc-gateway/third_party/googleapis --swagger_out=logtostderr=true:. example.proto

Then

go build

Hope this help.
Thanks
Gary

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2018

If we implemented $SRCDIR, it would be the directory of the file with the go:generate directive.

@myitcv

This comment has been minimized.

Copy link
Member

commented Jun 20, 2018

@millergarym I think the following would solve the worked example you gave:

PB_PATH per your definition. For now I'll assume your API repo is at internal.repo/api.

// gen.go - in top level dir of package
package example // import "internal.repo/myproject/example"

// import internal.repo/api as a side effect, simply 
// to ensure we have dependency visible to vgo
import _ "internal.repo/api"

// 1. 
//go:generate mkdir -p vendor
//go:generate bash -c "protoc -I$PB_PATH/include -I$$(vgo list -f '{{.Dir}}' internal.repo/api) --go_out=plugins=grpc:vendor options/myannotations.proto"

//2.
//go:generate bash -c "protoc -I$PB_PATH/include -I$$(vgo list -f '{{.Dir}}' internal.repo/api) -I$$(vgo list -f '{{.Dir}}' internal.repo/api)/myproject/example -I$$(vgo list -f '{{.Dir}}' github.com/grpc-ecosystem/grpc-gateway) -I$$(vgo list -f '{{.Dir}}' github.com/grpc-ecosystem/grpc-gateway)/third_party/googleapis --go_out=plugins=grpc:. example.proto"

//3.
//go:generate bash -c "protoc -I$PB_PATH/include -I$$(vgo list -f '{{.Dir}}' internal.repo/api) -I$$(vgo list -f '{{.Dir}}' internal.repo/api)/myproject/example -I$$(vgo list -f '{{.Dir}}' github.com/grpc-ecosystem/grpc-gateway) -I$$(vgo list -f '{{.Dir}}' github.com/grpc-ecosystem/grpc-gateway)/third_party/googleapis --grpc-gateway_out=logtostderr=true:. example.proto"

//go:generate protoc -I$PB_PATH/include -I$$(vgo list -f '{{.Dir}}' internal.repo/api) -I$$(vgo list -f '{{.Dir}}' internal.repo/api)/myproject/example -I$$(vgo list -f '{{.Dir}}' github.com/grpc-ecosystem/grpc-gateway) -I$$(vgo list -f '{{.Dir}}' github.com/grpc-ecosystem/grpc-gateway)/third_party/googleapis --swagger_out=logtostderr=true:. example.proto

Essentially:

  • each go:generate directive becomes a bash -c "" command
  • that allows us to use a sub-shell within the string argument to run vgo list
  • $API_WORKSPACE effectively becomes $$(vgo list -f '{{.Dir}}' internal.repo/api)
  • $GOPATH/src/github.com/grpc-ecosystem/grpc-gateway becomes $$(vgo list -f '{{.Dir}}' github.com/grpc-ecosystem/grpc-gateway)

Therefore I don't think you need the $SRCDIR that @ianlancetaylor is proposing. Nor do you need $GOGENERATEDIR I was proposing in #14120 (comment).

One thing I can see is that use of vgo list in a subshell is going to be a common pattern in go:generate directives, especially with protoc. A few options I can see here:

  1. have go generate (vgo generate) support a very limited form of subshell command expansion. That would allow us to drop the bash -c "", and your go:generate directive would look something like:
//go:generate protoc -I$PB_PATH/include -I$(vgo list -f '{{.Dir}}' internal.repo/api) --go_out=plugins=grpc:vendor options/myannotations.proto

This adds complexity to go generate directive parsing/handling.

  1. add another flag to protoc like -I that accepts a go/vgo import path. This would require my proposed $GOGENERATEDIR and would also require protoc to use something like the proposed go/packages to do the resolution. In this case your go:generate directive would look something like (assuming the flag is -P):
//go:generate protoc -I$PB_PATH/include -Pinternal.repo/api --go_out=plugins=grpc:vendor options/myannotations.proto

This adds some complexity to protoc, complexity which may well have to be replicated in other such commands.

  1. use go run xyz.go instead of bash -c "". More cross platform, requires relatively boilerplate creation of xyz.go-like files.

  2. do nothing and rely on bash -c "" or equivalent. Not entirely cross platform, relies on bash etc.

/cc @alandonovan for any relevant go/packages points here
/cc @dsnet for any protoc implications

@millergarym

This comment has been minimized.

Copy link
Author

commented Jun 20, 2018

There is currently one gotcha, the directory needs to contain at least one Go source file.

$ vgo list -f '{{.Dir}}' github.com/golang/protobuf/protoc-gen-go/descriptor
/home/garym/go/src/mod/github.com/golang/protobuf@v1.1.0/protoc-gen-go/descriptor
$ echo $?
0

works.

but

$ vgo list -f '{{.}}' github.com/golang/protobuf
vgo: import "github.com/golang/protobuf" [/home/garym/go/src/mod/github.com/golang/protobuf@v1.1.0]: no Go source files
$ echo $?
1

doesn't.

I really don't want to be putting dummy go source in the internal.repo/api repo.
Specifically because this is meant to be a language agnostic repo.
Placing a dummy go file in /home/garym/go/src/mod/github.com/golang/protobuf@v1.1.0 fixed the issue.

Should we post an issue again vgo list?

@myitcv

This comment has been minimized.

Copy link
Member

commented Jun 21, 2018

I really don't want to be putting dummy go source in the internal.repo/api repo.

Indeed. So then references to this repo can simply be via an environment variable then, much like PB_PATH?

@rsc

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2018

(@ianlancetaylor, go generate already runs the generator in the source directory, so $SRCDIR wouldn't add anything.)

I don't have an answer for the fascinating abuse of vendor to hold generated pb.go files, but that's at least not specific to vgo - as @millergarym pointed out above ("worster"), that wasn't safe already. Now it's just disallowed entirely.

I don't quite understand what the solution was for the lack of magic vendor tricks, but @millergarym's last message makes it sound like the only problem left is go list requiring a Go source file in the directory being sought. That's easy to fix: add -e:

go list -e -f '{{.Dir}}' github.com/golang/protobuf/protoc-gen-go/descriptor

(Note that this works will all currently supported versions of Go, not just vgo.)

@rsc rsc closed this Jul 6, 2018

@mkmik

This comment has been minimized.

Copy link

commented Nov 20, 2018

@rsc, that doesn't seem to work if the whole repo has no .go files at all

; go list -e --json github.com/googleapis/googleapis
go: finding github.com/googleapis/googleapis latest
{
	"ImportPath": "github.com/googleapis/googleapis",
	"Match": [
		"github.com/googleapis/googleapis"
	],
	"Incomplete": true,
	"Error": {
		"ImportStack": [
			"github.com/googleapis/googleapis"
		],
		"Pos": "",
		"Err": "unknown import path \"github.com/googleapis/googleapis\": cannot find module providing package github.com/googleapis/googleapis"
	}
}

E.g., I need this file https://github.com/googleapis/googleapis/blob/master/google/rpc/status.proto in order to resolve an import from another proto source in my project.

% go list -e -f '{{.Dir}}' github.com/googleapis/googleapis/google/rpc
go: finding github.com/googleapis/googleapis/google/rpc latest
go: finding github.com/googleapis/googleapis/google latest
go: finding github.com/googleapis/googleapis latest

the repos gets downloaded and unpacked, thus I managed to hack it up with:

//go:generate go get -d github.com/googleapis/googleapis@v0.0.0-20181120215828-b7a1d68ea384
//go:generate bash -c "protoc -I $GOPATH/pkg/mod/github.com/googleapis/googleapis@v0.0.0-20181120215828-b7a1d68ea384 my.proto"

It has the suboptimal effect of "polluting" go.mod with a non-Go repo, which will get cleaned up at next go mod tidy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.