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

go module proof go:generate protoc #535

Merged
merged 4 commits into from
Jun 12, 2019
Merged

go module proof go:generate protoc #535

merged 4 commits into from
Jun 12, 2019

Conversation

gdbelvin
Copy link
Contributor

@gdbelvin gdbelvin commented Jun 11, 2019

go:generate does not invoke bash by default, it merely does some basic variable
substitution and then calls execute directly.

Go modules deprecates $GOPATH, requiring a layer of indirection to look up the
location of proto files in 3rd party repositories.

By calling "sh", we can get proper variable substitution.


The proto files themselves have been modified to remove any $GOPATH specifics, leaving the resolution of the package location to go list.

This PR adds option go_package to the protos to remove ambiguity, since their paths on disk are no longer required to be in $GOPATH, we cannot rely on their paths on disk to derive their go package names.


An implication of making these import paths project relative is that anyone attempting to use grpc_cli will need to replicate each -I on the commandline to help the tool resolve dependencies.

Previously, passing $GOPATH/src may have been sufficient as long as the -I . import was not used in a particular proto file.

* master:
  Upgrade to latest version of go 1.11.x (google#533)
@gdbelvin gdbelvin mentioned this pull request Jun 11, 2019
@codecov
Copy link

codecov bot commented Jun 11, 2019

Codecov Report

Merging #535 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #535   +/-   ##
=======================================
  Coverage   71.92%   71.92%           
=======================================
  Files          88       88           
  Lines        9550     9550           
=======================================
  Hits         6869     6869           
  Misses       2190     2190           
  Partials      491      491

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a5d8d6b...50bb865. Read the comment docs.

@@ -15,4 +15,4 @@
// Package configpb holds a config protobuf for minimal gossip binaries.
package configpb

//go:generate protoc -I=. -I=$GOPATH/src --go_out=:. config.proto
//go:generate sh -c "protoc -I=. -I$(go list -f '{{ .Dir }}' github.com/google/trillian) --go_out=:$GOPATH/src config.proto"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed in person, --go_out still using $GOPATH means that certificate-transparency-go will have to be checked out under $GOPATH (i.e. it can't be outside of the Go source tree like modules usually can). We should document this restriction, since we're not currently aware of a way to avoid it.

@gdbelvin gdbelvin merged commit 12adc78 into google:master Jun 12, 2019
@gdbelvin gdbelvin deleted the protoc branch June 12, 2019 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants