-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
build: move .pb.go
contents munging from make
to protoc-gen-gogoroach
#57913
Conversation
pkg/cmd/protoc-gen-gogoroach/main.go
Outdated
continue | ||
} | ||
|
||
line = strings.Replace(line, "github.com/cockroachdb/cockroach/pkg/etcd", "go.etcd.io/etcd", 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these be strings.ReplaceAll
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't really matter since there's only ever one occurrence in each line, so sure.
pkg/cmd/protoc-gen-gogoroach/main.go
Outdated
|
||
line = strings.Replace(line, "github.com/cockroachdb/cockroach/pkg/etcd", "go.etcd.io/etcd", 1) | ||
line = strings.Replace(line, "github.com/cockroachdb/cockroach/pkg/errorspb", "github.com/cockroachdb/errors/errorspb", 1) | ||
// TODO(benesch): Remove these after https://github.com/grpc/grpc-go/issues/711. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the parent issue is now closed, does this still apply?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The replacement is still necessary (introduces bad diffs if I leave it out). I removed the comment.
pkg/cmd/protoc-gen-gogoroach/main.go
Outdated
) | ||
|
||
var builtinRegex *regexp.Regexp = regexp.MustCompile("github.com/cockroachdb/cockroach/pkg/(?P<capture>(bytes|context|encoding/binary|errors|fmt|io|math|github\\.com|(google\\.)?golang\\.org)([^a-z]|$$))") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really sure with what was happening originally, but assuming you've cracked it, a comment would be helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't fully understand exactly how this works, but I added a comment explaining what I DO know. :)
pkg/cmd/protoc-gen-gogoroach/main.go
Outdated
@@ -14,8 +14,35 @@ import ( | |||
"github.com/gogo/protobuf/protoc-gen-gogo/descriptor" | |||
"github.com/gogo/protobuf/vanity" | |||
"github.com/gogo/protobuf/vanity/command" | |||
"regexp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want these stdlib imports to go above the others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gofmt
moves the imports back into alphabetical order if I make this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our linter will fail (I think what you want is to introduce a newline between the two blocks).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the tip about the newline -- I just figured that out myself the hard way :)
d76a325
to
1164297
Compare
…roach` This worked fine, but it complicated cockroachdb#56067 because [the standard Go protobuf Bazel support](https://github.com/bazelbuild/rules_go/blob/master/proto/core.rst#go_proto_library) doesn't easily allow us to just inject a random `sed` command into the middle of the (`.proto` -> `.pb.go` -> compiled Go library) pipeline. With this logic in the actual `protoc` plugin proper, we can then safely use Bazel's built-in stuff without a lot of monkey-patching or code duplication. Release note: None
1164297
to
82b1e29
Compare
bors r+ |
Build succeeded: |
This moves us closer to generating our protobuf generated code in the Bazel sandbox (see cockroachdb#56067). Currently all the `.pb.go` files are checked into tree. We hook into the standard Bazel support for protobufs, adding a few (very light) utilities along the way. Specifically: * Add a `go_proto_compiler` definition for `protoc-gen-gogoroach`. After cockroachdb#57913, this is trivial. * Define a couple utilities in `pkg/build/util.bzl` to wrap some common functionality. These utilities (`proto_lib`, `go_proto_lib`) are meant to be used almost everywhere in the CockroachDB codebase in place of the standard `proto_library` and `go_proto_library` rules, and have the same API. * Port one small example (`pkg/jobs/jobspb/schedule.proto`) from using the in-tree `.pb.go` file to a version generated in the sandbox to demonstrate that everything works as expected. The intention here is that now that the utilities are in place, the remainder of cockroachdb#56067 is rote work that can be done in a follow-up. Release note: None
This worked fine, but it complicated #56067 because the standard Go
protobuf Bazel support
doesn't easily allow us to just inject a random
sed
command into themiddle of the (
.proto
->.pb.go
-> compiled Go library) pipeline.With this logic in the actual
protoc
plugin proper, we can then safelyuse Bazel's built-in stuff without a lot of monkey-patching or code
duplication.
Release note: None