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.

Sign up for GitHub

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

Jump to bottom

Add per-file mode for proto_library generation #1033

Merged
merged 7 commits into from
Oct 28, 2021
Merged

Add per-file mode for proto_library generation #1033

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
687c903
Add per-file mode for proto_library generation
wolfd Apr 18, 2021
b08d46f
Update docs for new file proto mode
wolfd Apr 18, 2021
8f785b0
Merge remote-tracking branch 'upstream/master' into danny/proto-per-file
wolfd Jun 30, 2021
fadca26
Fix logic bug, do not allow file mode to use proto_group
wolfd Jul 1, 2021
d3e1ce8
Test that FileMode doesn't break import parsing behavior.
wolfd Sep 8, 2021
8c77987
Test resolve behavior in file mode
wolfd Sep 17, 2021
a5bd0e0
Remove confusing aspects of tests
wolfd Sep 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ the one above, the
add [tests](https://github.com/bazelbuild/bazel-gazelle/tree/master/tests).
Run the existing tests with `bazel test //...`. Update
[README.rst](https://github.com/bazelbuild/bazel-gazelle/blob/master/README.rst)
if appropriate.
if appropriate.
1. [Create a pull request](https://help.github.com/articles/creating-a-pull-request/).
This will start the code review process. **All submissions, including
submissions by project members, require review.**
Expand Down
257 changes: 129 additions & 128 deletions README.rst
View file
Open in desktop

Large diffs are not rendered by default.

7 changes: 7 additions & 0 deletions language/proto/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,9 @@ const (
// PackageMode generates a proto_library for each set of .proto files with
// the same package name in each directory.
PackageMode

// FileMode generates a proto_library for each .proto file.
FileMode
)

func ModeFromString(s string) (Mode, error) {
Expand All @@ -111,6 +114,8 @@ func ModeFromString(s string) (Mode, error) {
return LegacyMode, nil
case "package":
return PackageMode, nil
case "file":
return FileMode, nil
default:
return 0, fmt.Errorf("unrecognized proto mode: %q", s)
}
Expand All @@ -128,6 +133,8 @@ func (m Mode) String() string {
return "legacy"
case PackageMode:
return "package"
case FileMode:
return "file"
default:
log.Panicf("unknown mode %d", m)
return ""
Expand Down
13 changes: 10 additions & 3 deletions language/proto/generate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,18 +119,25 @@ func buildPackages(pc *ProtoConfig, dir, rel string, protoFiles, genFiles []stri
for _, name := range protoFiles {
info := protoFileInfo(dir, name)
key := info.PackageName
if pc.groupOption != "" {

if pc.Mode == FileMode {
key = strings.TrimSuffix(name, ".proto")
} else if pc.groupOption != "" { // implicitly PackageMode
for _, opt := range info.Options {
if opt.Key == pc.groupOption {
key = opt.Value
break
}
}
}

if packageMap[key] == nil {
packageMap[key] = newPackage(info.PackageName)
}
packageMap[key].addFile(info)
if key != info.PackageName {
packageMap[key].RuleName = key
}
}

switch pc.Mode {
Expand All @@ -147,7 +154,7 @@ func buildPackages(pc *ProtoConfig, dir, rel string, protoFiles, genFiles []stri
}
return []*Package{pkg}

case PackageMode:
case PackageMode, FileMode:
pkgs := make([]*Package, 0, len(packageMap))
for _, pkg := range packageMap {
pkgs = append(pkgs, pkg)
Expand Down Expand Up @@ -212,7 +219,7 @@ func generateProto(pc *ProtoConfig, rel string, pkg *Package, shouldSetVisibilit
if pc.Mode == DefaultMode {
name = RuleName(goPackageName(pkg), pc.GoPrefix, rel)
} else {
name = RuleName(pkg.Options[pc.groupOption], pkg.Name, rel)
name = RuleName(pkg.RuleName, pkg.Name, rel)
}
r := rule.NewRule("proto_library", name)
srcs := make([]string, 0, len(pkg.Files))
Expand Down
1 change: 1 addition & 0 deletions language/proto/package.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import "path/filepath"
// same package name. This translates to a proto_library rule.
type Package struct {
Name string
RuleName string // if not set, defaults to Name
Files map[string]FileInfo
Imports map[string]bool
Options map[string]string
Expand Down
35 changes: 35 additions & 0 deletions language/proto/resolve_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,41 @@ proto_library(
name = "dep_proto",
deps = ["//foo:foo_proto"],
)
`,
}, {
desc: "test single file resolution in file mode",
index: []buildFile{{
rel: "somedir",
content: `
# gazelle:proto file

proto_library(
name = "foo_proto",
srcs = ["foo.proto"],
)

proto_library(
name = "bar_proto",
srcs = ["bar.proto"],
)

proto_library(
name = "baz_proto",
srcs = ["baz.proto"],
)
`,
}},
old: `
proto_library(
name = "other_proto",
_imports = ["somedir/bar.proto"],
)
`,
want: `
proto_library(
name = "other_proto",
deps = ["//somedir:bar_proto"],
)
`,
},
} {
Expand Down
1 change: 1 addition & 0 deletions language/proto/testdata/file_mode/BUILD.old
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
# gazelle:proto file
13 changes: 13 additions & 0 deletions language/proto/testdata/file_mode/BUILD.want
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
load("@rules_proto//proto:defs.bzl", "proto_library")

proto_library(
name = "bar_proto",
srcs = ["bar.proto"],
visibility = ["//visibility:public"],
)

proto_library(
name = "foo_proto",
srcs = ["foo.proto"],
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this have a deps entry for ":bar_proto"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

visibility = ["//visibility:public"],
)
3 changes: 3 additions & 0 deletions language/proto/testdata/file_mode/bar.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
syntax = "proto3";

package file_mode;
3 changes: 3 additions & 0 deletions language/proto/testdata/file_mode/foo.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
syntax = "proto3";

package file_mode;
Copy link
Member

Choose a reason for hiding this comment

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

Can you have one of these import from the other to confirm that the dep detection logic works

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@danny-skydio danny-skydio Sep 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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