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

Generate new proto rules with gazelle #808

Closed
ianthehat opened this issue Sep 8, 2017 · 4 comments · Fixed by #868
Closed

Generate new proto rules with gazelle #808

ianthehat opened this issue Sep 8, 2017 · 4 comments · Fixed by #868

Comments

@ianthehat
Copy link
Contributor

Needs to generate both proto_library and go_proto_library rules when it sees a proto file.

@jayconrod
Copy link
Contributor

Some questions:

  • Should Gazelle generate go_grpc_library instead if services are present?
  • If multiple proto files are present with the same option go_package, should they always be in the same proto_library rule?
  • What if go_package is not specified? Should ignore or try to infer something reasonable?
  • What if different go_package is specified differently in multiple files? Gazelle already has some logic to deal with files in multiple packages (choose one, ignore others), so I think we'll use that for now.
  • How should import paths be translated into labels for dependencies?
  • Is there a simple way to parse .proto files in Go? I think we just need to extract a few things at the top level (go_package, import, whether there are services), so I could just implement a minimal parser, but maybe there's something better.

@ianthehat
Copy link
Contributor Author

  • Yes
  • Anything with the same go_package should go in the same rule
  • option go_package must match the importpath on the go_proto_library rule. If it is not specified, the proto compiler will assume it is the same as the path to the base, which is highly unlikely to be correct, but you should set the import path to the same thing to at least stand a chance.
  • There should be one go_proto_library per go_package/importpath (just like there should be one go_library per package ideally.
  • The importpath->label mapping is the same problem you already face for go code.
  • There is no simple way to parse a proto file that I know of. You can probably pick out the top level things with simple string matching, if you want more than that you should use protoc to generate a proto descriptor file and then read that, it's not going to be trivial though. If the proto_library rule already exists and is correct you could get bazel to build you the descriptor file (it's the only file output of the proto_library rule) but then we are in the wonderful world of bazel run launching gazelle that needs to invoke bazel (which we may have to do anyway to run bazel fetch to get dependancies when we start doing the workspace edits)

@jayconrod
Copy link
Contributor

The importpath->label mapping is the same problem you already face for go code.

Most of the time, it seems like the import is just a path within the current repository, but in examples/proto/grpc/my_svc.proto, there's an import of google/protobuf/any.proto that gets translated into @com_google_protobuf//:any_proto. Is that a special case, or is there a general rule for that mapping?

@ianthehat
Copy link
Contributor Author

That's one of the magical special rules.
There are a set of protos, all in that repository, called the "well known proto's".
In theory a general rule that knew enough to look into all the go_proto_library rules, and their corresponding proto_library rules could work it all out with no special cases, but in practice, and certainly to start with, those probably need to be a hard coded transformation.

jayconrod pushed a commit to jayconrod/rules_go that referenced this issue Oct 3, 2017
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
jayconrod added a commit that referenced this issue Oct 5, 2017
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 #808
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants