Add per-file mode for proto_library generation #1033
Conversation
README.rst
Outdated
| @@ -721,6 +721,8 @@ The following directives are recognized: | |||
| | * ``default``: ``proto_library``, ``go_proto_library``, and ``go_library`` | | |||
| | rules are generated using ``@io_bazel_rules_go//proto:def.bzl``. Only one | | |||
| | of each rule may be generated per directory. This is the default mode. | | |||
| | * ``file``: a ``proto_library`` rule is generated for every .proto file. | | |||
| | .proto files can be grouped using ``proto_group`` option. | | |||
There was a problem hiding this comment.
This doesn't really make sense to me. If file mode puts one .proto file in each proto_library target, it doesn't seem meaningful to group them at all. Why would proto_group have any effect?
There was a problem hiding this comment.
It was mostly a side-effect of the way I implemented the per-file option, but I think it's pretty useful. The proto_group option allows you to override the default name of the proto_library rule. If for some reason someone wanted to logically group proto files, but still default to single proto file libraries, this allows that.
You could also use this option to rename a proto_library, if that was desired for whatever reason.
I think I could improve my wording here. How about:
The
proto_librarya .proto file ends up in can be overridden by using theproto_groupoption.
There was a problem hiding this comment.
The proto_group option allows you to override the default name of the proto_library rule.
I wonder if a better way to do that in this mode would be to match on srcs when finding targets to merge? In language/proto/kinds.go, it looks like proto_library will already do that. That seems a much more intuitive workflow: users can just change the name of the target in the build file, and Gazelle keeps using the new name thereafter. proto_group seems much less intuitive to me for this use case.
If for some reason someone wanted to logically group proto files, but still default to single proto file libraries, this allows that.
Gazelle's probably a poor fit for that level of customization. I'd recommend manually maintaining build files for directories like this.
In any case, if proto_group does this, it needs to be tested to make sure it doesn't break later on.
There was a problem hiding this comment.
Thanks for bringing this up. After doing some more digging I found that the proto_group method didn't even work in file mode 🤦 (else if vs if). Untested code is broken code... Anyway, I dropped the docs about it and the (faulty) code that would have made it work, as suggested.
I was unaware of MatchAttrs, that should handle most use cases that I was thinking about.
Does it look good now?
There was a problem hiding this comment.
@jayconrod have you had a chance to look at this?
There was a problem hiding this comment.
Regrettably, I no longer have bandwidth to work on Gazelle. I'll add @bazelbuild/go-maintainers in case anyone else can take a look.
There was a problem hiding this comment.
@achew22 are you able to take a quick look at this?
| @@ -0,0 +1,3 @@ | |||
| syntax = "proto3"; | |||
|
|
|||
| package file_mode; | |||
There was a problem hiding this comment.
Can you have one of these import from the other to confirm that the dep detection logic works
There was a problem hiding this comment.
I have added an import statement to this file, which could test that I haven't broken the parsing logic, but fwiw these tests are just for the generate behavior, not resolve. I haven't made any changes to the resolve behavior, which is tested separately.
Right now I don't believe that resolve is affected by mode changes at all. I could loop through all the available modes on the config here to test that the existing tests pass with all modes? https://github.com/bazelbuild/bazel-gazelle/blob/b1e37463c2ef0997c5cf0e90cdf03e696caaed27/language/proto/resolve_test.go#L357
What do you think?
There was a problem hiding this comment.
Re: your comment in Slack
I'm not concerned about it changing the result mode, I want to make sure that it actually does the resolve correctly when there is single file target generation
I've added a simple test in 8c77987 that imports a single proto given an index of three different protos with separate proto_librarys.
The previous test that I added for the comment above has been removed.
Should be good now? @achew22
|
FWIW I've tested this branch out in a sample repo https://github.com/magma/magma and it seems to work as intended. I'm +1 to get this merged. |
|
Ah, just noticed that @achew22 responded to me about the patch in the Bazel Slack a while back. I need to just add a better test for the resolve behavior and then it should be good to go. |
wolfd
force-pushed
the
danny/proto-per-file
branch
from
379ce5c
to
8c77987
Compare
|
|
||
| proto_library( | ||
| name = "foo_proto", | ||
| srcs = ["foo.proto"], |
There was a problem hiding this comment.
Shouldn't this have a deps entry for ":bar_proto"?
There was a problem hiding this comment.
Hmm, I don't think so. The resolve tests are entirely separate from the generate tests, is that where the confusion is from?
I think I should remove the import there (just did so in a5bd0e0), as I think it's just confusing.
I have added an import statement to this file, which could test that I haven't broken the parsing logic, but fwiw these tests are just for the generate behavior, not resolve. I haven't made any changes to the resolve behavior, which is tested separately.
To be clear, it was possible to have many different proto_library packages in a directory before, but gazelle wouldn't auto-generate them for you, you would need to use different packages (in the same dir) or an option in the proto file that defined a grouping behavior (with appropriate gazelle config changes). The resolve behavior for single proto file proto_librarys should have worked before and after this change regardless of the change to the generate.go behavior.
Anyway, the new entry in resolve_test.go should be what you're asking for?
As it stands there isn't an end-to-end test for the proto language plugin. I have thought about adding something like it, but it would be a larger change and deserve it's own PR. Basic thought would be to have the testdata directory, but instead of BUILD.old and BUILD.want, you'd have BUILD.old, BUILD.generate, BUILD.resolve, (and maybe BUILD.merge?). I would need to dig into it a bit more, but it would be refactoring all of the existing tests in this plugin (and maybe also for the go plugin).
There was a problem hiding this comment.
As a counterpoint, adding a new stage for those tests might be redundant. The resolve_test.go file defines a similar structure that isn't backed by .proto source files, and it can create arbitrarily complex situations for the resolver that may not be output by generate.go.
|
@achew22 I would love to get this merged, I am pretty certain that it's ready. |
|
Thanks for the contribution @wolfd! |
|
Any chance we can cut a new release with this change? |
What type of PR is this?
What package or component does this PR mostly affect?
What does this PR do? Why is it needed?
Allows
proto_libraryrules to be instantiated per-file, which helps in situations wherepackageis too coarse. Can also alleviate pain when files between two packages form import cycles when bundled at the package level instead of the file level, if you find yourself in that unfortunate situation.Which issues(s) does this PR fix?
Fixes #138
Other notes for review
I don't know what the requirements for the
go_proto_libraryrules are. Does this mode potentially break the way those work?I tested it in my repo, and the
go_proto_librarys still seem to generate/build, but since I order thegoplugin first in the list, it doesn't generate proto libraries for go (I don't know if that is an intended way to disable those, but it's what we do for now).