-
-
Notifications
You must be signed in to change notification settings - Fork 678
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
Gazelle: support for proto rules #868
Gazelle: support for proto rules #868
Conversation
Gazelle now generates proto_library, go_proto_library, and go_grpc_library rules. proto_library will contain .proto files in a directory. go_{proto,grpc}_library are buildable libraries, linked with the proto_library. A go_library rules embeds the go_{proto,grpc}_library and includes an extra .go sources (but .pb.go files are excluded). The new proto rule generation is turned on by default in packages that don't have existing proto rules. If there are existing rules, proto rule generation will either follow previous behavior (if go_proto_library.bzl is loaded) or will be disabled (if go_proto_library is loaded from some other repo). The proto mode can be selected explicitly with the -proto command line flag or with the '# gazelle:proto' directive. Legacy rules can be migrated with 'gazelle fix'. Limitations: * Gazelle still only allows one package per directory. Gazelle infers package name and import path from .proto files. These must match other files in the directory. * Import resolution is fairly crude and is just a guess, based on the import string. * There's no way to import protos from other repositories, except for Well Known Types. Fixes bazel-contrib#808
go/tools/gazelle/config/config.go
Outdated
type ProtoMode int | ||
|
||
const ( | ||
// In DefaultProtoMode, Gazelle generates proto_library and new |
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 godoc sulk about the "In " on the front of these comments?
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.
Done
{path: "WORKSPACE"}, | ||
{ | ||
path: config.DefaultValidBuildFileNames[0], | ||
content: `load("@io_bazel_rules_go//proto:go_proto_library.bzl", "go_proto_library") |
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 would newline before the load, it makes it much easier to read
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.
Done. I need to clean up this test at some point and stick these files in testdata.
// proto mode is already set to something other than the default, or if the mode | ||
// is set explicitly in directives, this function does nothing. If the legacy | ||
// go_proto_library.bzl is loaded, or if this is the Well Known Types | ||
// repository, legacy mode is used. If go_proto_library is loaded from another |
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.
This part of the description implies it would not mantain new proto rules that it generated, but looking at the code below you stay in DefaultMode in that case.
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.
Done. This was the result of a last-minute change.
@@ -45,6 +34,10 @@ type fileInfo struct { | |||
// "_test" suffix if it was present. It is empty for non-Go files. | |||
packageName string | |||
|
|||
// importPath is the canonical import path for the package this file | |||
// belongs to. Will be empty file files that don't specify this. |
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.
This sentence makes no sense.
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.
Hopefully better now. Also added TODO to improve.
if info.packageName == "" { | ||
stem := strings.TrimSuffix(name, ".proto") | ||
fs := strings.FieldsFunc(stem, func(r rune) bool { | ||
return !('A' <= r && r <= '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.
You probably ought to be using unicode.IsLetter and unicode.IsDigit in here.
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.
Done
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!
go/tools/gazelle/config/config.go
Outdated
type ProtoMode int | ||
|
||
const ( | ||
// In DefaultProtoMode, Gazelle generates proto_library and new |
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.
Done
// proto mode is already set to something other than the default, or if the mode | ||
// is set explicitly in directives, this function does nothing. If the legacy | ||
// go_proto_library.bzl is loaded, or if this is the Well Known Types | ||
// repository, legacy mode is used. If go_proto_library is loaded from another |
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.
Done. This was the result of a last-minute change.
{path: "WORKSPACE"}, | ||
{ | ||
path: config.DefaultValidBuildFileNames[0], | ||
content: `load("@io_bazel_rules_go//proto:go_proto_library.bzl", "go_proto_library") |
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.
Done. I need to clean up this test at some point and stick these files in testdata.
@@ -45,6 +34,10 @@ type fileInfo struct { | |||
// "_test" suffix if it was present. It is empty for non-Go files. | |||
packageName string | |||
|
|||
// importPath is the canonical import path for the package this file | |||
// belongs to. Will be empty file files that don't specify this. |
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.
Hopefully better now. Also added TODO to improve.
if info.packageName == "" { | ||
stem := strings.TrimSuffix(name, ".proto") | ||
fs := strings.FieldsFunc(stem, func(r rune) bool { | ||
return !('A' <= r && r <= '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.
Done
Gazelle now generates proto_library, go_proto_library, and
go_grpc_library rules. proto_library will contain .proto files in a
directory. go_{proto,grpc}library are buildable libraries, linked
with the proto_library. A go_library rules embeds the
go{proto,grpc}_library and includes an extra .go sources (but .pb.go
files are excluded).
The new proto rule generation is turned on by default in packages that
don't have existing proto rules. If there are existing rules, proto
rule generation will either follow previous behavior (if
go_proto_library.bzl is loaded) or will be disabled (if
go_proto_library is loaded from some other repo). The proto mode can
be selected explicitly with the -proto command line flag or with the
'# gazelle:proto' directive. Legacy rules can be migrated with
'gazelle fix'.
Limitations:
package name and import path from .proto files. These must match
other files in the directory.
the import string.
Well Known Types.
Fixes #808