From 91e6a1e1fcb728056bb10a5cc0fc062f254e154e Mon Sep 17 00:00:00 2001 From: Jay Conrod Date: Mon, 2 Jul 2018 15:07:15 -0400 Subject: [PATCH] internal/language/proto: package mode The proto extension now supports "package" mode. This can be enabled with -proto=package on the command line or "# gazelle:proto package" in a build file. In package mode, multiple proto_libraries will be generated in a directory. Source files will be grouped by package. To group source files by an option instead (i.e., option go_package), the proto extension recognizes the proto_group command line flag and directive. The go extension generates go_proto_library rules based on the proto_library rules generated by the proto extension (independent of proto mode when possible). Related bazelbuild/rules_go#1548 --- README.rst | 169 +++++----- internal/go_repository.bzl | 1 + internal/language/go/constants.go | 5 + internal/language/go/fileinfo_go_test.go | 15 +- internal/language/go/generate.go | 313 ++++++++++-------- internal/language/go/generate_test.go | 46 +++ internal/language/go/package.go | 56 +++- .../go/testdata/proto_package_mode/BUILD.old | 1 + .../go/testdata/proto_package_mode/BUILD.want | 43 +++ .../go/testdata/proto_package_mode/bar1.proto | 5 + .../go/testdata/proto_package_mode/bar2.proto | 3 + .../go/testdata/proto_package_mode/foo1.proto | 5 + .../go/testdata/proto_package_mode/foo2.proto | 5 + .../proto_package_mode_extras/BUILD.old | 3 + .../proto_package_mode_extras/BUILD.want | 54 +++ .../proto_package_mode_extras/bar1.proto | 5 + .../proto_package_mode_extras/bar2.proto | 3 + .../proto_package_mode_extras/foo1.proto | 5 + .../proto_package_mode_extras/foo2.proto | 5 + .../proto_package_mode_extras/foo_test.go | 1 + internal/language/proto/config.go | 19 +- internal/language/proto/constants.go | 9 +- internal/language/proto/generate.go | 207 ++++++++---- internal/language/proto/generate_test.go | 64 +++- internal/language/proto/package.go | 38 ++- .../multiple_packages/default_mode/BUILD.want | 5 + .../multiple_packages/default_mode/bar.proto | 3 + .../multiple_packages/default_mode/foo.proto | 3 + .../multiple_packages/package_mode/BUILD.old | 6 + .../multiple_packages/package_mode/BUILD.want | 17 + .../multiple_packages/package_mode/bar1.proto | 3 + .../multiple_packages/package_mode/bar2.proto | 3 + .../multiple_packages/package_mode/foo1.proto | 3 + .../multiple_packages/package_mode/foo2.proto | 3 + .../package_mode_group/BUILD.old | 2 + .../package_mode_group/BUILD.want | 17 + .../package_mode_group/bar1.proto | 3 + .../package_mode_group/bar2.proto | 3 + .../package_mode_group/foo1.proto | 3 + .../package_mode_group/foo2.proto | 3 + 40 files changed, 838 insertions(+), 319 deletions(-) create mode 100644 internal/language/go/testdata/proto_package_mode/BUILD.old create mode 100644 internal/language/go/testdata/proto_package_mode/BUILD.want create mode 100644 internal/language/go/testdata/proto_package_mode/bar1.proto create mode 100644 internal/language/go/testdata/proto_package_mode/bar2.proto create mode 100644 internal/language/go/testdata/proto_package_mode/foo1.proto create mode 100644 internal/language/go/testdata/proto_package_mode/foo2.proto create mode 100644 internal/language/go/testdata/proto_package_mode_extras/BUILD.old create mode 100644 internal/language/go/testdata/proto_package_mode_extras/BUILD.want create mode 100644 internal/language/go/testdata/proto_package_mode_extras/bar1.proto create mode 100644 internal/language/go/testdata/proto_package_mode_extras/bar2.proto create mode 100644 internal/language/go/testdata/proto_package_mode_extras/foo1.proto create mode 100644 internal/language/go/testdata/proto_package_mode_extras/foo2.proto create mode 100644 internal/language/go/testdata/proto_package_mode_extras/foo_test.go create mode 100644 internal/language/proto/testdata/multiple_packages/default_mode/BUILD.want create mode 100644 internal/language/proto/testdata/multiple_packages/default_mode/bar.proto create mode 100644 internal/language/proto/testdata/multiple_packages/default_mode/foo.proto create mode 100644 internal/language/proto/testdata/multiple_packages/package_mode/BUILD.old create mode 100644 internal/language/proto/testdata/multiple_packages/package_mode/BUILD.want create mode 100644 internal/language/proto/testdata/multiple_packages/package_mode/bar1.proto create mode 100644 internal/language/proto/testdata/multiple_packages/package_mode/bar2.proto create mode 100644 internal/language/proto/testdata/multiple_packages/package_mode/foo1.proto create mode 100644 internal/language/proto/testdata/multiple_packages/package_mode/foo2.proto create mode 100644 internal/language/proto/testdata/multiple_packages/package_mode_group/BUILD.old create mode 100644 internal/language/proto/testdata/multiple_packages/package_mode_group/BUILD.want create mode 100644 internal/language/proto/testdata/multiple_packages/package_mode_group/bar1.proto create mode 100644 internal/language/proto/testdata/multiple_packages/package_mode_group/bar2.proto create mode 100644 internal/language/proto/testdata/multiple_packages/package_mode_group/foo1.proto create mode 100644 internal/language/proto/testdata/multiple_packages/package_mode_group/foo2.proto diff --git a/README.rst b/README.rst index 595d90317..9723d9ee6 100644 --- a/README.rst +++ b/README.rst @@ -230,77 +230,82 @@ Subdirectories will be processed recursively. The following flags are accepted: -+------------------------------------------+-----------------------------------+ -| **Name** | **Default value** | -+==========================================+===================================+ -| :flag:`-build_file_name file1,file2,...` | :value:`BUILD.bazel,BUILD` | -+------------------------------------------+-----------------------------------+ -| Comma-separated list of file names. Gazelle recognizes these files as Bazel | -| build files. New files will use the first name in this list. Use this if | -| your project contains non-Bazel files named ``BUILD`` (or ``build`` on | -| case-insensitive file systems). | -+------------------------------------------+-----------------------------------+ -| :flag:`-build_tags tag1,tag2` | | -+------------------------------------------+-----------------------------------+ -| List of Go build tags Gazelle will consider to be true. Gazelle applies | -| constraints when generating Go rules. It assumes certain tags are true on | -| certain platforms (for example, ``amd64,linux``). It assumes all Go release | -| tags are true (for example, ``go1.8``). It considers other tags to be false | -| (for example, ``ignore``). This flag overrides that behavior. | -| | -| Bazel may still filter sources with these tags. Use | -| ``bazel build --features gotags=foo,bar`` to set tags at build time. | -+------------------------------------------+-----------------------------------+ -| :flag:`-external external|vendored` | :value:`external` | -+------------------------------------------+-----------------------------------+ -| Determines how Gazelle resolves import paths. May be :value:`external` or | -| :value:`vendored`. Gazelle translates Go import paths to Bazel labels when | -| resolving library dependencies. Import paths that start with the | -| ``go_prefix`` are resolved to local labels, but other imports | -| are resolved based on this mode. In :value:`external` mode, paths are | -| resolved using an external dependency in the WORKSPACE file (Gazelle does | -| not create or maintain these dependencies yet). In :value:`vendored` mode, | -| paths are resolved to a library in the vendor directory. | -+------------------------------------------+-----------------------------------+ -| :flag:`-go_prefix example.com/repo` | | -+------------------------------------------+-----------------------------------+ -| A prefix of import paths for libraries in the repository that corresponds to | -| the repository root. Gazelle infers this from the ``go_prefix`` rule in the | -| root BUILD.bazel file, if it exists. If not, this option is mandatory. | -| | -| This prefix is used to determine whether an import path refers to a library | -| in the current repository or an external dependency. | -+------------------------------------------+-----------------------------------+ -| :flag:`-known_import example.com` | | -+------------------------------------------+-----------------------------------+ -| Skips import path resolution for a known domain. May be repeated. | -| | -| When Gazelle resolves an import path to an external dependency, it attempts | -| to discover the remote repository root over HTTP. Gazelle skips this | -| discovery step for a few well-known domains with predictable structure, like | -| golang.org and github.com. This flag specifies additional domains to skip, | -| which is useful in situations where the lookup would fail for some reason. | -+------------------------------------------+-----------------------------------+ -| :flag:`-mode fix|print|diff` | :value:`fix` | -+------------------------------------------+-----------------------------------+ -| Method for emitting merged build files. | -| | -| In ``fix`` mode, Gazelle writes generated and merged files to disk. In | -| ``print`` mode, it prints them to stdout. In ``diff`` mode, it prints a | -| unified diff. | -+------------------------------------------+-----------------------------------+ -| :flag:`-proto default|legacy|disable` | :value:`default` | -+------------------------------------------+-----------------------------------+ -| Determines how Gazelle should generate rules for .proto files. See details | -| in `Directives`_ below. | -+------------------------------------------+-----------------------------------+ -| :flag:`-repo_root dir` | | -+------------------------------------------+-----------------------------------+ -| The root directory of the repository. Gazelle normally infers this to be the | -| directory containing the WORKSPACE file. | -| | -| Gazelle will not process packages outside this directory. | -+------------------------------------------+-----------------------------------+ ++-----------------------------------------------+-----------------------------------+ +| **Name** | **Default value** | ++===============================================+===================================+ +| :flag:`-build_file_name file1,file2,...` | :value:`BUILD.bazel,BUILD` | ++-----------------------------------------------+-----------------------------------+ +| Comma-separated list of file names. Gazelle recognizes these files as Bazel | +| build files. New files will use the first name in this list. Use this if | +| your project contains non-Bazel files named ``BUILD`` (or ``build`` on | +| case-insensitive file systems). | ++-----------------------------------------------+-----------------------------------+ +| :flag:`-build_tags tag1,tag2` | | ++-----------------------------------------------+-----------------------------------+ +| List of Go build tags Gazelle will consider to be true. Gazelle applies | +| constraints when generating Go rules. It assumes certain tags are true on | +| certain platforms (for example, ``amd64,linux``). It assumes all Go release | +| tags are true (for example, ``go1.8``). It considers other tags to be false | +| (for example, ``ignore``). This flag overrides that behavior. | +| | +| Bazel may still filter sources with these tags. Use | +| ``bazel build --features gotags=foo,bar`` to set tags at build time. | ++-----------------------------------------------+-----------------------------------+ +| :flag:`-external external|vendored` | :value:`external` | ++-----------------------------------------------+-----------------------------------+ +| Determines how Gazelle resolves import paths. May be :value:`external` or | +| :value:`vendored`. Gazelle translates Go import paths to Bazel labels when | +| resolving library dependencies. Import paths that start with the | +| ``go_prefix`` are resolved to local labels, but other imports | +| are resolved based on this mode. In :value:`external` mode, paths are | +| resolved using an external dependency in the WORKSPACE file (Gazelle does | +| not create or maintain these dependencies yet). In :value:`vendored` mode, | +| paths are resolved to a library in the vendor directory. | ++-----------------------------------------------+-----------------------------------+ +| :flag:`-go_prefix example.com/repo` | | ++-----------------------------------------------+-----------------------------------+ +| A prefix of import paths for libraries in the repository that corresponds to | +| the repository root. Gazelle infers this from the ``go_prefix`` rule in the | +| root BUILD.bazel file, if it exists. If not, this option is mandatory. | +| | +| This prefix is used to determine whether an import path refers to a library | +| in the current repository or an external dependency. | ++-----------------------------------------------+-----------------------------------+ +| :flag:`-known_import example.com` | | ++-----------------------------------------------+-----------------------------------+ +| Skips import path resolution for a known domain. May be repeated. | +| | +| When Gazelle resolves an import path to an external dependency, it attempts | +| to discover the remote repository root over HTTP. Gazelle skips this | +| discovery step for a few well-known domains with predictable structure, like | +| golang.org and github.com. This flag specifies additional domains to skip, | +| which is useful in situations where the lookup would fail for some reason. | ++-----------------------------------------------+-----------------------------------+ +| :flag:`-mode fix|print|diff` | :value:`fix` | ++-----------------------------------------------+-----------------------------------+ +| Method for emitting merged build files. | +| | +| In ``fix`` mode, Gazelle writes generated and merged files to disk. In | +| ``print`` mode, it prints them to stdout. In ``diff`` mode, it prints a | +| unified diff. | ++-----------------------------------------------+-----------------------------------+ +| :flag:`-proto default|package|legacy|disable` | :value:`default` | ++-----------------------------------------------+-----------------------------------+ +| Determines how Gazelle should generate rules for .proto files. See details | +| in `Directives`_ below. | ++-----------------------------------------------+-----------------------------------+ +| :flag:`-proto_group group` | :value:`""` | ++-----------------------------------------------+-----------------------------------+ +| Determines the proto option Gazelle uses to group .proto files into rules | +| when in ``package`` mode. See details in `Directives`_ below. | ++-----------------------------------------------+-----------------------------------+ +| :flag:`-repo_root dir` | | ++-----------------------------------------------+-----------------------------------+ +| The root directory of the repository. Gazelle normally infers this to be the | +| directory containing the WORKSPACE file. | +| | +| Gazelle will not process packages outside this directory. | ++-----------------------------------------------+-----------------------------------+ ``update-repos`` ~~~~~~~~~~~~~~~~ @@ -475,9 +480,12 @@ The following directives are recognized: +------------------------------------------+-----------------------------------+ | Tells Gazelle how to generate rules for .proto files. Valid values are: | | | -| * ``default``: ``proto_library``, ``go_proto_library``, ``go_grpc_library``, | -| and ``go_library`` rules are generated using | -| ``@io_bazel_rules_go//proto:def.bzl``. This is the default mode. | +| * ``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. | +| * ``package``: multiple ``proto_library`` and ``go_proto_library`` rules | +| may be generated in the same directory. .proto files are grouped into | +| rules based on their package name or another option (see ``proto_group``). | | * ``legacy``: ``filegroup`` rules are generated for use by | | ``@io_bazel_rules_go//proto:go_proto_library.bzl``. ``go_proto_library`` | | rules must be written by hand. Gazelle will run in this mode automatically | @@ -494,6 +502,19 @@ The following directives are recognized: | ``@io_bazel_rules_go//proto:go_proto_library.bzl`` is loaded, Gazelle | | will run in ``legacy`` mode. | +------------------------------------------+-----------------------------------+ +| :direc:`proto_group` | :value:`""` | ++------------------------------------------+-----------------------------------+ +| Specifies an option that Gazelle can use to group .proto files into rules | +| when in ``package`` mode. For example, when set to ``go_package``, .proto | +| files with the same ``option go_package`` will be grouped together. | +| | +| When this directive is set to the empty string, Gazelle will group packages | +| by their proto package statement. | +| | +| Rule names are generated based on the last run of identifier characters | +| in the package name. For example, if the package is ``"foo/bar/baz"``, the | +| ``proto_library`` rule will be named ``baz_proto``. | ++------------------------------------------+-----------------------------------+ Keep comments ~~~~~~~~~~~~~ diff --git a/internal/go_repository.bzl b/internal/go_repository.bzl index 23c46ca09..0e17503e4 100644 --- a/internal/go_repository.bzl +++ b/internal/go_repository.bzl @@ -164,6 +164,7 @@ go_repository = repository_rule( values = [ "", "default", + "package", "disable", "legacy", ], diff --git a/internal/language/go/constants.go b/internal/language/go/constants.go index c2b0103a1..8f6fc9518 100644 --- a/internal/language/go/constants.go +++ b/internal/language/go/constants.go @@ -19,4 +19,9 @@ const ( // legacyProtoFilegroupName is the anme of a filegroup created in legacy // mode for libraries that contained .pb.go files and .proto files. legacyProtoFilegroupName = "go_default_library_protos" + // wellKnownTypesGoPrefix is the import path for the Go repository containing + // pre-generated code for the Well Known Types. + wellKnownTypesGoPrefix = "github.com/golang/protobuf" + // wellKnownTypesPkg is the package name for the predefined WKTs in rules_go. + wellKnownTypesPkg = "proto/wkt" ) diff --git a/internal/language/go/fileinfo_go_test.go b/internal/language/go/fileinfo_go_test.go index 1a7e4b888..2bba53b0d 100644 --- a/internal/language/go/fileinfo_go_test.go +++ b/internal/language/go/fileinfo_go_test.go @@ -365,13 +365,16 @@ import "C" c.RepoRoot = repo gc := getGoConfig(c) gc.prefix = "example.com/repo" - got := buildPackage(c, sub, "sub", []string{"sub.go"}, nil, nil, false, "", nil) + pkgs, _ := buildPackages(c, sub, "sub", []string{"sub.go"}, false) + got, ok := pkgs["sub"] + if !ok { + t.Fatal("did not build package 'sub'") + } want := &goPackage{ - name: "sub", - dir: sub, - rel: "sub", - importPath: "example.com/repo/sub", - library: goTarget{cgo: true}, + name: "sub", + dir: sub, + rel: "sub", + library: goTarget{cgo: true}, } want.library.sources.addGenericString("sub.go") want.library.copts.addGenericString("-Isub/..") diff --git a/internal/language/go/generate.go b/internal/language/go/generate.go index db5f31f6a..36e25806d 100644 --- a/internal/language/go/generate.go +++ b/internal/language/go/generate.go @@ -21,6 +21,7 @@ import ( "log" "path" "path/filepath" + "sort" "strings" "sync" @@ -33,31 +34,37 @@ import ( func (gl *goLang) GenerateRules(c *config.Config, dir, rel string, f *rule.File, subdirs, regularFiles, genFiles []string, otherEmpty, otherGen []*rule.Rule) (empty, gen []*rule.Rule) { // Extract information about proto files. We need this to exclude .pb.go // files and generate go_proto_library rules. + gc := getGoConfig(c) pc := proto.GetProtoConfig(c) - var protoName string + var protoPackages map[string]proto.Package var protoFileInfo map[string]proto.FileInfo + var protoRuleNames []string if pc.Mode != proto.DisableMode { + protoPackages = make(map[string]proto.Package) protoFileInfo = make(map[string]proto.FileInfo) for _, r := range otherGen { if r.Kind() != "proto_library" { continue } - if protoName != "" { - // TODO(jayconrod): Currently, the proto extension generates at most one - // proto_library rule because this extension can't handle multiple - // packages. We should remove this limitation soon. - log.Panicf("%s: cannot generate Go rules for multiple proto_library rules", dir) - } - protoName = r.Name() - for _, pfi := range r.PrivateAttr(proto.FileInfoKey).([]proto.FileInfo) { - protoFileInfo[pfi.Name] = pfi + pkg := r.PrivateAttr(proto.PackageKey).(proto.Package) + protoPackages[r.Name()] = pkg + for name, info := range pkg.Files { + protoFileInfo[name] = info } + protoRuleNames = append(protoRuleNames, r.Name()) + } + sort.Strings(protoRuleNames) + } + var emptyProtoRuleNames []string + for _, r := range otherEmpty { + if r.Kind() == "proto_library" { + emptyProtoRuleNames = append(emptyProtoRuleNames, r.Name()) } } // If proto rule generation is enabled, exclude .pb.go files that correspond // to any .proto files present. - if pc.Mode != proto.DisableMode { + if pc.Mode != proto.DisableMode && pc.Mode != proto.LegacyMode { keep := func(f string) bool { if strings.HasSuffix(f, ".pb.go") { _, ok := protoFileInfo[strings.TrimSuffix(f, ".pb.go")+".proto"] @@ -71,11 +78,10 @@ func (gl *goLang) GenerateRules(c *config.Config, dir, rel string, f *rule.File, // Split regular files into files which can determine the package name and // import path and other files. - var pkgFiles, otherFiles []string + var goFiles, otherFiles []string for _, f := range regularFiles { - if strings.HasSuffix(f, ".go") || - pc.Mode != proto.DisableMode && strings.HasSuffix(f, ".proto") { - pkgFiles = append(pkgFiles, f) + if strings.HasSuffix(f, ".go") { + goFiles = append(goFiles, f) } else { otherFiles = append(otherFiles, f) } @@ -92,17 +98,144 @@ func (gl *goLang) GenerateRules(c *config.Config, dir, rel string, f *rule.File, } } - // Build a package from files in this directory. - pkg := buildPackage(c, dir, rel, pkgFiles, otherFiles, genFiles, hasTestdata, protoName, protoFileInfo) - if pkg != nil && path.Base(rel) == "testdata" { - gl.testdataPkgs[rel] = true + // Build a set of packages from files in this directory. + goPackageMap, goFilesWithUnknownPackage := buildPackages(c, dir, rel, goFiles, hasTestdata) + + // Select a package to generate rules for. If there is no package, create + // an empty package so we can generate empty rules. + var protoName string + pkg, err := selectPackage(c, dir, goPackageMap) + if err != nil { + if _, ok := err.(*build.NoGoError); ok { + if len(protoPackages) == 1 { + for name, ppkg := range protoPackages { + pkg = &goPackage{ + name: goProtoPackageName(ppkg), + importPath: goProtoImportPath(gc, ppkg, rel), + proto: protoTargetFromProtoPackage(name, ppkg), + } + protoName = name + break + } + } else { + pkg = emptyPackage(c, dir, rel) + } + } else { + log.Print(err) + } } - if pkg == nil { - pkg = emptyPackage(c, dir, rel) + + // Try to link the selected package with a proto package. + if pkg != nil { + if pkg.importPath == "" { + if err := pkg.inferImportPath(c); err != nil && pkg.firstGoFile() != "" { + inferImportPathErrorOnce.Do(func() { log.Print(err) }) + } + } + for _, name := range protoRuleNames { + ppkg := protoPackages[name] + if pkg.importPath == goProtoImportPath(gc, ppkg, rel) { + protoName = name + pkg.proto = protoTargetFromProtoPackage(name, ppkg) + break + } + } } + // Generate rules for proto packages. These should come before the other + // Go rules. g := newGenerator(c, f, rel) - return g.generateRules(pkg) + var rules []*rule.Rule + var protoEmbed string + for _, name := range protoRuleNames { + ppkg := protoPackages[name] + var rs []*rule.Rule + if name == protoName { + protoEmbed, rs = g.generateProto(pc.Mode, pkg.proto, pkg.importPath) + } else { + target := protoTargetFromProtoPackage(name, ppkg) + importPath := goProtoImportPath(gc, ppkg, rel) + _, rs = g.generateProto(pc.Mode, target, importPath) + } + rules = append(rules, rs...) + } + for _, name := range emptyProtoRuleNames { + goProtoName := strings.TrimSuffix(name, "_proto") + "_go_proto" + empty = append(empty, rule.NewRule("go_proto_library", goProtoName)) + } + if pkg != nil && pc.Mode == proto.PackageMode && pkg.firstGoFile() == "" { + // In proto package mode, don't generate a go_library embedding a + // go_proto_library unless there are actually go files. + protoEmbed = "" + } + + // Complete the Go package and generate rules for that. + if pkg != nil { + // Add files with unknown packages. This happens when there are parse + // or I/O errors. We should keep the file in the srcs list and let the + // compiler deal with the error. + cgo := pkg.haveCgo() + for _, info := range goFilesWithUnknownPackage { + if err := pkg.addFile(c, info, cgo); err != nil { + log.Print(err) + } + } + + // Process the other static files. + for _, file := range otherFiles { + info := otherFileInfo(filepath.Join(dir, file)) + if err := pkg.addFile(c, info, cgo); err != nil { + log.Print(err) + } + } + + // Process generated files. Note that generated files may have the same names + // as static files. Bazel will use the generated files, but we will look at + // the content of static files, assuming they will be the same. + regularFileSet := make(map[string]bool) + for _, f := range regularFiles { + regularFileSet[f] = true + } + for _, f := range genFiles { + if regularFileSet[f] { + continue + } + info := fileNameInfo(filepath.Join(dir, f)) + if err := pkg.addFile(c, info, cgo); err != nil { + log.Print(err) + } + } + + // Generate Go rules. + if protoName == "" { + // Empty proto rules for deletion. + _, rs := g.generateProto(pc.Mode, pkg.proto, pkg.importPath) + rules = append(rules, rs...) + } + lib := g.generateLib(pkg, protoEmbed) + var libName string + if !lib.IsEmpty(goKinds[lib.Kind()]) { + libName = lib.Name() + } + rules = append(rules, lib) + rules = append(rules, + g.generateBin(pkg, libName), + g.generateTest(pkg, libName)) + } + + for _, r := range rules { + if r.IsEmpty(goKinds[r.Kind()]) { + empty = append(empty, r) + } else { + gen = append(gen, r) + } + } + + if path.Base(rel) == "testdata" && (f != nil || len(gen) > 0) { + gl.testdataPkgs[rel] = true + } + + return empty, gen } func filterFiles(files *[]string, pred func(string) bool) { @@ -117,29 +250,14 @@ func filterFiles(files *[]string, pred func(string) bool) { *files = (*files)[:w] } -// buildPackage reads source files in a given directory and returns a goPackage -// containing information about those files and how to build them. -// -// If no buildable .go files are found in the directory, nil will be returned. -// If the directory contains multiple buildable packages, the package whose -// name matches the directory base name will be returned. If there is no such -// package or if an error occurs, an error will be logged, and nil will be -// returned. -func buildPackage(c *config.Config, dir, rel string, pkgFiles, otherFiles, genFiles []string, hasTestdata bool, protoName string, protoInfo map[string]proto.FileInfo) *goPackage { +func buildPackages(c *config.Config, dir, rel string, goFiles []string, hasTestdata bool) (packageMap map[string]*goPackage, goFilesWithUnknownPackage []fileInfo) { // Process .go and .proto files first, since these determine the package name. - packageMap := make(map[string]*goPackage) - cgo := false - var pkgFilesWithUnknownPackage []fileInfo - for _, f := range pkgFiles { + packageMap = make(map[string]*goPackage) + for _, f := range goFiles { path := filepath.Join(dir, f) - var info fileInfo - if strings.HasSuffix(f, ".go") { - info = goFileInfo(path, rel) - } else { - info = protoFileInfo(path, protoInfo[f]) - } + info := goFileInfo(path, rel) if info.packageName == "" { - pkgFilesWithUnknownPackage = append(pkgFilesWithUnknownPackage, info) + goFilesWithUnknownPackage = append(goFilesWithUnknownPackage, info) continue } if info.packageName == "documentation" { @@ -147,8 +265,6 @@ func buildPackage(c *config.Config, dir, rel string, pkgFiles, otherFiles, genFi continue } - cgo = cgo || info.isCgo - if _, ok := packageMap[info.packageName]; !ok { packageMap[info.packageName] = &goPackage{ name: info.packageName, @@ -161,61 +277,7 @@ func buildPackage(c *config.Config, dir, rel string, pkgFiles, otherFiles, genFi log.Print(err) } } - - // Select a package to generate rules for. - pkg, err := selectPackage(c, dir, packageMap) - if err != nil { - if _, ok := err.(*build.NoGoError); !ok { - log.Print(err) - } - return nil - } - - // Add files with unknown packages. This happens when there are parse - // or I/O errors. We should keep the file in the srcs list and let the - // compiler deal with the error. - for _, info := range pkgFilesWithUnknownPackage { - if err := pkg.addFile(c, info, cgo); err != nil { - log.Print(err) - } - } - - // Process the other static files. - for _, file := range otherFiles { - info := otherFileInfo(filepath.Join(dir, file)) - if err := pkg.addFile(c, info, cgo); err != nil { - log.Print(err) - } - } - - // Process generated files. Note that generated files may have the same names - // as static files. Bazel will use the generated files, but we will look at - // the content of static files, assuming they will be the same. - staticFiles := make(map[string]bool) - for _, f := range pkgFiles { - staticFiles[f] = true - } - for _, f := range otherFiles { - staticFiles[f] = true - } - for _, f := range genFiles { - if staticFiles[f] { - continue - } - info := fileNameInfo(filepath.Join(dir, f)) - if err := pkg.addFile(c, info, cgo); err != nil { - log.Print(err) - } - } - - if pkg.importPath == "" { - if err := pkg.inferImportPath(c); err != nil { - inferImportPathErrorOnce.Do(func() { log.Print(err) }) - return nil - } - } - pkg.proto.name = protoName - return pkg + return packageMap, goFilesWithUnknownPackage } var inferImportPathErrorOnce sync.Once @@ -306,29 +368,7 @@ func newGenerator(c *config.Config, f *rule.File, rel string) *generator { return &generator{c: c, rel: rel, shouldSetVisibility: shouldSetVisibility} } -func (g *generator) generateRules(pkg *goPackage) (empty, gen []*rule.Rule) { - protoMode := proto.GetProtoConfig(g.c).Mode - protoEmbed, rules := g.generateProto(protoMode, pkg) - var libName string - lib := g.generateLib(pkg, protoEmbed) - rules = append(rules, lib) - if !lib.IsEmpty(goKinds[lib.Kind()]) { - libName = lib.Name() - } - rules = append(rules, - g.generateBin(pkg, libName), - g.generateTest(pkg, libName)) - for _, r := range rules { - if !r.IsEmpty(goKinds[r.Kind()]) { - gen = append(gen, r) - } else { - empty = append(empty, r) - } - } - return empty, gen -} - -func (g *generator) generateProto(mode proto.Mode, pkg *goPackage) (string, []*rule.Rule) { +func (g *generator) generateProto(mode proto.Mode, target protoTarget, importPath string) (string, []*rule.Rule) { if mode == proto.DisableMode { // Don't create or delete proto rules in this mode. Any existing rules // are likely hand-written. @@ -336,26 +376,27 @@ func (g *generator) generateProto(mode proto.Mode, pkg *goPackage) (string, []*r } filegroupName := config.DefaultProtosName - protoName := pkg.proto.name + protoName := target.name if protoName == "" { - protoName = proto.RuleName("", g.rel, getGoConfig(g.c).prefix) + importPath := inferImportPath(getGoConfig(g.c), g.rel) + protoName = proto.RuleName(importPath) } goProtoName := strings.TrimSuffix(protoName, "_proto") + "_go_proto" - visibility := []string{checkInternalVisibility(pkg.rel, "//visibility:public")} + visibility := []string{checkInternalVisibility(g.rel, "//visibility:public")} if mode == proto.LegacyMode { filegroup := rule.NewRule("filegroup", filegroupName) - if pkg.proto.sources.isEmpty() { + if target.sources.isEmpty() { return "", []*rule.Rule{filegroup} } - filegroup.SetAttr("srcs", pkg.proto.sources.build()) + filegroup.SetAttr("srcs", target.sources.build()) if g.shouldSetVisibility { filegroup.SetAttr("visibility", visibility) } return "", []*rule.Rule{filegroup} } - if pkg.proto.sources.isEmpty() { + if target.sources.isEmpty() { return "", []*rule.Rule{ rule.NewRule("filegroup", filegroupName), rule.NewRule("go_proto_library", goProtoName), @@ -364,14 +405,14 @@ func (g *generator) generateProto(mode proto.Mode, pkg *goPackage) (string, []*r goProtoLibrary := rule.NewRule("go_proto_library", goProtoName) goProtoLibrary.SetAttr("proto", ":"+protoName) - g.setImportAttrs(goProtoLibrary, pkg) - if pkg.proto.hasServices { + g.setImportAttrs(goProtoLibrary, importPath) + if target.hasServices { goProtoLibrary.SetAttr("compilers", []string{"@io_bazel_rules_go//proto:go_grpc"}) } if g.shouldSetVisibility { goProtoLibrary.SetAttr("visibility", visibility) } - goProtoLibrary.SetPrivateAttr(config.GazelleImportsKey, pkg.proto.imports.build()) + goProtoLibrary.SetPrivateAttr(config.GazelleImportsKey, target.imports.build()) return goProtoName, []*rule.Rule{goProtoLibrary} } @@ -388,7 +429,7 @@ func (g *generator) generateLib(pkg *goPackage, embed string) *rule.Rule { visibility = checkInternalVisibility(pkg.rel, "//visibility:public") } g.setCommonAttrs(goLibrary, pkg.rel, visibility, pkg.library, embed) - g.setImportAttrs(goLibrary, pkg) + g.setImportAttrs(goLibrary, pkg.importPath) return goLibrary } @@ -437,13 +478,13 @@ func (g *generator) setCommonAttrs(r *rule.Rule, pkgRel, visibility string, targ r.SetPrivateAttr(config.GazelleImportsKey, target.imports.build()) } -func (g *generator) setImportAttrs(r *rule.Rule, pkg *goPackage) { - r.SetAttr("importpath", pkg.importPath) +func (g *generator) setImportAttrs(r *rule.Rule, importPath string) { + r.SetAttr("importpath", importPath) goConf := getGoConfig(g.c) if goConf.importMapPrefix != "" { - fromPrefixRel := pathtools.TrimPrefix(pkg.rel, goConf.importMapPrefixRel) + fromPrefixRel := pathtools.TrimPrefix(g.rel, goConf.importMapPrefixRel) importMap := path.Join(goConf.importMapPrefix, fromPrefixRel) - if importMap != pkg.importPath { + if importMap != importPath { r.SetAttr("importmap", importMap) } } diff --git a/internal/language/go/generate_test.go b/internal/language/go/generate_test.go index a0a9102d5..9685e17c5 100644 --- a/internal/language/go/generate_test.go +++ b/internal/language/go/generate_test.go @@ -36,6 +36,7 @@ func TestGenerateRules(t *testing.T) { c.ValidBuildFileNames = []string{"BUILD.old"} gc := getGoConfig(c) gc.prefix = "example.com/repo" + gc.prefixSet = true cexts := make([]config.Configurer, len(langs)) var loads []rule.LoadInfo @@ -127,6 +128,51 @@ func TestGenerateRulesEmptyLegacyProto(t *testing.T) { } } +func TestGenerateRulesEmptyPackageProto(t *testing.T) { + c, _, langs := testConfig() + pc := proto.GetProtoConfig(c) + pc.Mode = proto.PackageMode + oldContent := []byte(` +proto_library( + name = "dead_proto", + srcs = ["dead.proto"], +) +`) + old, err := rule.LoadData("BUILD.bazel", oldContent) + if err != nil { + t.Fatal(err) + } + var empty []*rule.Rule + for _, lang := range langs { + es, _ := lang.GenerateRules(c, "./foo", "foo", old, nil, nil, nil, empty, nil) + empty = append(empty, es...) + } + f := rule.EmptyFile("test") + for _, r := range empty { + r.Insert(f) + } + f.Sync() + got := strings.TrimSpace(string(bzl.Format(f.File))) + want := strings.TrimSpace(` +proto_library(name = "dead_proto") + +go_proto_library(name = "dead_go_proto") + +filegroup(name = "go_default_library_protos") + +go_proto_library(name = "foo_go_proto") + +go_library(name = "go_default_library") + +go_binary(name = "foo") + +go_test(name = "go_default_test") +`) + if got != want { + t.Errorf("got:\n%s\nwant:\n%s", got, want) + } +} + // convertImportsAttrs copies private attributes to regular attributes, which // will later be written out to build files. This allows tests to check the // values of private attributes with simple string comparison. diff --git a/internal/language/go/package.go b/internal/language/go/package.go index 56140f6bf..5d6d7ec00 100644 --- a/internal/language/go/package.go +++ b/internal/language/go/package.go @@ -23,6 +23,7 @@ import ( "strings" "github.com/bazelbuild/bazel-gazelle/internal/config" + "github.com/bazelbuild/bazel-gazelle/internal/language/proto" "github.com/bazelbuild/bazel-gazelle/internal/rule" ) @@ -91,7 +92,12 @@ func (pkg *goPackage) addFile(c *config.Config, info fileInfo, cgo bool) error { case info.ext == unknownExt || !cgo && (info.ext == cExt || info.ext == csExt): return nil case info.ext == protoExt: - pkg.proto.addFile(c, info) + if proto.GetProtoConfig(c).Mode == proto.LegacyMode { + // Only add files in legacy mode. This is used to generate a filegroup + // that contains all protos. In order modes, we get the .proto files + // from information emitted by the proto language extension. + pkg.proto.addFile(c, info) + } case info.isTest: if info.isCgo { return fmt.Errorf("%s: use of cgo in test not supported", info.path) @@ -136,15 +142,19 @@ func (pkg *goPackage) firstGoFile() string { return "" } +func (pkg *goPackage) haveCgo() bool { + return pkg.library.cgo || pkg.binary.cgo || pkg.test.cgo +} + func (pkg *goPackage) inferImportPath(c *config.Config) error { if pkg.importPath != "" { log.Panic("importPath already set") } gc := getGoConfig(c) - pkg.importPath = inferImportPath(gc, pkg.rel) - if pkg.importPath == "" { + if !gc.prefixSet { return fmt.Errorf("%s: go prefix is not set, so importpath can't be determined for rules. Set a prefix with a '# gazelle:prefix' comment or with -go_prefix on the command line.", pkg.dir) } + pkg.importPath = inferImportPath(gc, pkg.rel) return nil if pkg.rel == gc.prefixRel { @@ -165,6 +175,34 @@ func inferImportPath(gc *goConfig, rel string) string { } } +func goProtoPackageName(pkg proto.Package) string { + if value, ok := pkg.Options["go_package"]; ok { + if strings.LastIndexByte(value, '/') == -1 { + return value + } else { + if i := strings.LastIndexByte(value, ';'); i != -1 { + return value[i+1:] + } else { + return path.Base(value) + } + } + } + return strings.Replace(pkg.Name, ".", "_", -1) +} + +func goProtoImportPath(gc *goConfig, pkg proto.Package, rel string) string { + if value, ok := pkg.Options["go_package"]; ok { + if strings.LastIndexByte(value, '/') == -1 { + return inferImportPath(gc, rel) + } else if i := strings.LastIndexByte(value, ';'); i != -1 { + return value[:i] + } else { + return value + } + } + return inferImportPath(gc, rel) +} + func (t *goTarget) addFile(c *config.Config, info fileInfo) { t.cgo = t.cgo || info.isCgo add := getPlatformStringsAddFunction(c, info, nil) @@ -186,6 +224,18 @@ func (t *goTarget) addFile(c *config.Config, info fileInfo) { } } +func protoTargetFromProtoPackage(name string, pkg proto.Package) protoTarget { + target := protoTarget{name: name} + for f := range pkg.Files { + target.sources.addGenericString(f) + } + for i := range pkg.Imports { + target.imports.addGenericString(i) + } + target.hasServices = pkg.HasServices + return target +} + func (t *protoTarget) addFile(c *config.Config, info fileInfo) { t.sources.addGenericString(info.name) for _, imp := range info.imports { diff --git a/internal/language/go/testdata/proto_package_mode/BUILD.old b/internal/language/go/testdata/proto_package_mode/BUILD.old new file mode 100644 index 000000000..7508fcf00 --- /dev/null +++ b/internal/language/go/testdata/proto_package_mode/BUILD.old @@ -0,0 +1 @@ +# gazelle:proto package diff --git a/internal/language/go/testdata/proto_package_mode/BUILD.want b/internal/language/go/testdata/proto_package_mode/BUILD.want new file mode 100644 index 000000000..b2cc1331f --- /dev/null +++ b/internal/language/go/testdata/proto_package_mode/BUILD.want @@ -0,0 +1,43 @@ +load("@io_bazel_rules_go//proto:def.bzl", "go_proto_library") + +proto_library( + name = "bar_proto", + srcs = [ + "bar1.proto", + "bar2.proto", + ], + _gazelle_imports = [], + visibility = ["//visibility:public"], +) + +proto_library( + name = "foo_proto", + srcs = [ + "foo1.proto", + "foo2.proto", + ], + _gazelle_imports = [ + "google/protobuf/any.proto", + "proto_package_mode/bar1.proto", + ], + visibility = ["//visibility:public"], +) + +go_proto_library( + name = "bar_go_proto", + _gazelle_imports = [], + importpath = "example.com/repo/proto_package_mode/bar", + proto = ":bar_proto", + visibility = ["//visibility:public"], +) + +go_proto_library( + name = "foo_go_proto", + _gazelle_imports = [ + "google/protobuf/any.proto", + "proto_package_mode/bar1.proto", + ], + importpath = "example.com/repo/proto_package_mode", + proto = ":foo_proto", + visibility = ["//visibility:public"], +) diff --git a/internal/language/go/testdata/proto_package_mode/bar1.proto b/internal/language/go/testdata/proto_package_mode/bar1.proto new file mode 100644 index 000000000..31c75324c --- /dev/null +++ b/internal/language/go/testdata/proto_package_mode/bar1.proto @@ -0,0 +1,5 @@ +syntax = "proto3"; + +package bar; + +option go_package = "example.com/repo/proto_package_mode/bar"; diff --git a/internal/language/go/testdata/proto_package_mode/bar2.proto b/internal/language/go/testdata/proto_package_mode/bar2.proto new file mode 100644 index 000000000..c44368c63 --- /dev/null +++ b/internal/language/go/testdata/proto_package_mode/bar2.proto @@ -0,0 +1,3 @@ +syntax = "proto3"; + +package bar; diff --git a/internal/language/go/testdata/proto_package_mode/foo1.proto b/internal/language/go/testdata/proto_package_mode/foo1.proto new file mode 100644 index 000000000..859cd47fb --- /dev/null +++ b/internal/language/go/testdata/proto_package_mode/foo1.proto @@ -0,0 +1,5 @@ +syntax = "proto3"; + +package foo; + +import "google/protobuf/any.proto"; diff --git a/internal/language/go/testdata/proto_package_mode/foo2.proto b/internal/language/go/testdata/proto_package_mode/foo2.proto new file mode 100644 index 000000000..542a12b4e --- /dev/null +++ b/internal/language/go/testdata/proto_package_mode/foo2.proto @@ -0,0 +1,5 @@ +syntax = "proto3"; + +package foo; + +import "proto_package_mode/bar1.proto"; diff --git a/internal/language/go/testdata/proto_package_mode_extras/BUILD.old b/internal/language/go/testdata/proto_package_mode_extras/BUILD.old new file mode 100644 index 000000000..2e069bc86 --- /dev/null +++ b/internal/language/go/testdata/proto_package_mode_extras/BUILD.old @@ -0,0 +1,3 @@ +# gazelle:proto package + +package(default_visibility = ["//visibility:public"]) diff --git a/internal/language/go/testdata/proto_package_mode_extras/BUILD.want b/internal/language/go/testdata/proto_package_mode_extras/BUILD.want new file mode 100644 index 000000000..cc77fd8a3 --- /dev/null +++ b/internal/language/go/testdata/proto_package_mode_extras/BUILD.want @@ -0,0 +1,54 @@ +load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") +load("@io_bazel_rules_go//proto:def.bzl", "go_proto_library") + +proto_library( + name = "bar_proto", + srcs = [ + "bar1.proto", + "bar2.proto", + ], + _gazelle_imports = [], +) + +proto_library( + name = "foo_proto", + srcs = [ + "foo1.proto", + "foo2.proto", + ], + _gazelle_imports = [ + "google/protobuf/any.proto", + "proto_package_mode_extras/bar1.proto", + ], +) + +go_proto_library( + name = "bar_go_proto", + _gazelle_imports = [], + importpath = "example.com/repo/proto_package_mode_extras/bar", + proto = ":bar_proto", +) + +go_proto_library( + name = "foo_go_proto", + _gazelle_imports = [ + "google/protobuf/any.proto", + "proto_package_mode_extras/bar1.proto", + ], + importpath = "example.com/repo/proto_package_mode_extras", + proto = ":foo_proto", +) + +go_library( + name = "go_default_library", + _gazelle_imports = [], + embed = [":foo_go_proto"], + importpath = "example.com/repo/proto_package_mode_extras", +) + +go_test( + name = "go_default_test", + srcs = ["foo_test.go"], + _gazelle_imports = [], + embed = [":go_default_library"], +) diff --git a/internal/language/go/testdata/proto_package_mode_extras/bar1.proto b/internal/language/go/testdata/proto_package_mode_extras/bar1.proto new file mode 100644 index 000000000..7278f4609 --- /dev/null +++ b/internal/language/go/testdata/proto_package_mode_extras/bar1.proto @@ -0,0 +1,5 @@ +syntax = "proto3"; + +package bar; + +option go_package = "example.com/repo/proto_package_mode_extras/bar"; diff --git a/internal/language/go/testdata/proto_package_mode_extras/bar2.proto b/internal/language/go/testdata/proto_package_mode_extras/bar2.proto new file mode 100644 index 000000000..c44368c63 --- /dev/null +++ b/internal/language/go/testdata/proto_package_mode_extras/bar2.proto @@ -0,0 +1,3 @@ +syntax = "proto3"; + +package bar; diff --git a/internal/language/go/testdata/proto_package_mode_extras/foo1.proto b/internal/language/go/testdata/proto_package_mode_extras/foo1.proto new file mode 100644 index 000000000..859cd47fb --- /dev/null +++ b/internal/language/go/testdata/proto_package_mode_extras/foo1.proto @@ -0,0 +1,5 @@ +syntax = "proto3"; + +package foo; + +import "google/protobuf/any.proto"; diff --git a/internal/language/go/testdata/proto_package_mode_extras/foo2.proto b/internal/language/go/testdata/proto_package_mode_extras/foo2.proto new file mode 100644 index 000000000..655c6cae8 --- /dev/null +++ b/internal/language/go/testdata/proto_package_mode_extras/foo2.proto @@ -0,0 +1,5 @@ +syntax = "proto3"; + +package foo; + +import "proto_package_mode_extras/bar1.proto"; diff --git a/internal/language/go/testdata/proto_package_mode_extras/foo_test.go b/internal/language/go/testdata/proto_package_mode_extras/foo_test.go new file mode 100644 index 000000000..f52652b1b --- /dev/null +++ b/internal/language/go/testdata/proto_package_mode_extras/foo_test.go @@ -0,0 +1 @@ +package foo diff --git a/internal/language/proto/config.go b/internal/language/proto/config.go index 42fadae79..1edaaf705 100644 --- a/internal/language/proto/config.go +++ b/internal/language/proto/config.go @@ -42,6 +42,10 @@ type ProtoConfig struct { // can't be determined. // TODO(jayconrod): deprecate and remove Go-specific behavior. GoPrefix string + + // groupOption is an option name that Gazelle will use to group .proto + // files into proto_library rules. If unset, the proto package name is used. + groupOption string } func GetProtoConfig(c *config.Config) *ProtoConfig { @@ -66,6 +70,10 @@ const ( // LegacyMode generates filegroups for .proto files if .pb.go files are // present in the same directory. LegacyMode + + // PackageMode generates a proto_library for each set of .proto files with + // the same package name in each directory. + PackageMode ) func ModeFromString(s string) (Mode, error) { @@ -76,6 +84,8 @@ func ModeFromString(s string) (Mode, error) { return DisableMode, nil case "legacy": return LegacyMode, nil + case "package": + return PackageMode, nil default: return 0, fmt.Errorf("unrecognized proto mode: %q", s) } @@ -89,6 +99,8 @@ func (m Mode) String() string { return "disable" case LegacyMode: return "legacy" + case PackageMode: + return "package" default: log.Panicf("unknown mode %d", m) return "" @@ -123,7 +135,8 @@ func (_ *protoLang) RegisterFlags(fs *flag.FlagSet, cmd string, c *config.Config // Note: the -proto flag does not set the ModeExplicit flag. We want to // be able to switch to DisableMode in vendor directories, even when // this is set for compatibility with older versions. - fs.Var(&modeFlag{&pc.Mode}, "proto", "default: generates new proto rules\n\tdisable: does not touch proto rules\n\t") + fs.Var(&modeFlag{&pc.Mode}, "proto", "default: generates a proto_library rule for one package\n\tpackage: generates a proto_library rule for for each package\n\tdisable: does not touch proto rules\n\t") + fs.StringVar(&pc.groupOption, "proto_group", "", "option name used to group .proto files into proto_library rules") } func (_ *protoLang) CheckFlags(fs *flag.FlagSet, c *config.Config) error { @@ -131,7 +144,7 @@ func (_ *protoLang) CheckFlags(fs *flag.FlagSet, c *config.Config) error { } func (_ *protoLang) KnownDirectives() []string { - return []string{"proto"} + return []string{"proto", "proto_group"} } func (_ *protoLang) Configure(c *config.Config, rel string, f *rule.File) { @@ -149,6 +162,8 @@ func (_ *protoLang) Configure(c *config.Config, rel string, f *rule.File) { } pc.Mode = mode pc.ModeExplicit = true + case "proto_group": + pc.groupOption = d.Value } } } diff --git a/internal/language/proto/constants.go b/internal/language/proto/constants.go index d1a9a47c5..be6bb4c8d 100644 --- a/internal/language/proto/constants.go +++ b/internal/language/proto/constants.go @@ -16,10 +16,11 @@ limitations under the License. package proto const ( - // FileInfoKey is the name of a private attribute set on generate - // proto_library rules. This attribute contains a slice of FileInfo - // records, one for each .proto file. - FileInfoKey = "_fileinfo" + // PackageInfoKey is the name of a private attribute set on generated + // proto_library rules. This attribute contains a Package record which + // describes the library and its sources. + PackageKey = "_package" + // wellKnownTypesGoPrefix is the import path for the Go repository containing // pre-generated code for the Well Known Types. wellKnownTypesGoPrefix = "github.com/golang/protobuf" diff --git a/internal/language/proto/generate.go b/internal/language/proto/generate.go index 38677ccf8..03bafbb42 100644 --- a/internal/language/proto/generate.go +++ b/internal/language/proto/generate.go @@ -22,13 +22,12 @@ import ( "strings" "github.com/bazelbuild/bazel-gazelle/internal/config" - "github.com/bazelbuild/bazel-gazelle/internal/pathtools" "github.com/bazelbuild/bazel-gazelle/internal/rule" ) func (_ *protoLang) GenerateRules(c *config.Config, dir, rel string, f *rule.File, subdirs, regularFiles, genFiles []string, otherEmpty, otherGen []*rule.Rule) (empty, gen []*rule.Rule) { pc := GetProtoConfig(c) - if pc.Mode != DefaultMode { + if pc.Mode == DisableMode || pc.Mode == LegacyMode { // Don't create or delete proto rules in this mode. Any existing rules // are likely hand-written. return nil, nil @@ -46,50 +45,42 @@ func (_ *protoLang) GenerateRules(c *config.Config, dir, rel string, f *rule.Fil genProtoFiles = append(genFiles, name) } } - pkg := buildPackage(dir, rel, regularProtoFiles, genProtoFiles) - - if pkg == nil { - empty = append(empty, rule.NewRule("proto_library", RuleName("", rel, pc.GoPrefix))) - return empty, gen - } - - name := RuleName(goPackageName(pkg), rel, pc.GoPrefix) - r := rule.NewRule("proto_library", name) - srcs := make([]string, 0, len(pkg.files)) - for f := range pkg.files { - srcs = append(srcs, f) - } - sort.Strings(srcs) - r.SetAttr("srcs", srcs) - info := make([]FileInfo, len(srcs)) - for i, src := range srcs { - info[i] = pkg.files[src] - } - r.SetPrivateAttr(FileInfoKey, info) - imports := make([]string, 0, len(pkg.imports)) - for i := range pkg.imports { - imports = append(imports, i) - } - sort.Strings(imports) - r.SetPrivateAttr(config.GazelleImportsKey, imports) - for k, v := range pkg.options { - r.SetPrivateAttr(k, v) - } - if !hasDefaultVisibility(f) { - vis := checkInternalVisibility(rel, "//visibility:public") - r.SetAttr("visibility", []string{vis}) + pkgs := buildPackages(pc, dir, rel, regularProtoFiles, genProtoFiles) + shouldSetVisibility := !hasDefaultVisibility(f) + for _, pkg := range pkgs { + r := generateProto(pc, rel, pkg, shouldSetVisibility) + if r.IsEmpty(protoKinds[r.Kind()]) { + empty = append(empty, r) + } else { + gen = append(gen, r) + } } - gen = append(gen, r) + sort.SliceStable(gen, func(i, j int) bool { + return gen[i].Name() < gen[j].Name() + }) + empty = append(empty, generateEmpty(f, regularProtoFiles, genProtoFiles)...) return empty, gen } -// RuleName returns a name for the proto_library in the given directory. -// TODO(jayconrod): remove Go-specific functionality. This is here temporarily -// for compatibility. -func RuleName(goPkgName, rel, goPrefix string) string { - base := goPkgName - if base == "" { - base = pathtools.RelBaseName(rel, goPrefix, "") +// RuleName returns a name for a proto_library derived from the given strings. +// For each string, RuleName will look for a non-empty suffix of identifier +// characters and then append "_proto" to that. +func RuleName(names ...string) string { + base := "root" + for _, name := range names { + notIdent := func(c rune) bool { + return !('A' <= c && c <= 'Z' || + 'a' <= c && c <= 'z' || + '0' <= c && c <= '9' || + c == '_') + } + if i := strings.LastIndexFunc(name, notIdent); i >= 0 { + name = name[i+1:] + } + if name != "" { + base = name + break + } } return base + "_proto" } @@ -97,31 +88,53 @@ func RuleName(goPkgName, rel, goPrefix string) string { // buildPackage extracts metadata from the .proto files in a directory and // constructs possibly several packages, then selects a package to generate // a proto_library rule for. -func buildPackage(dir, rel string, protoFiles, genFiles []string) *protoPackage { - packageMap := make(map[string]*protoPackage) +func buildPackages(pc *ProtoConfig, dir, rel string, protoFiles, genFiles []string) []*Package { + packageMap := make(map[string]*Package) for _, name := range protoFiles { info := protoFileInfo(dir, name) - if packageMap[info.PackageName] == nil { - packageMap[info.PackageName] = newProtoPackage(info.PackageName) + key := info.PackageName + if pc.groupOption != "" { + for _, opt := range info.Options { + if opt.Key == pc.groupOption { + key = opt.Value + break + } + } + } + if packageMap[key] == nil { + packageMap[key] = newPackage(info.PackageName) } - packageMap[info.PackageName].addFile(info) + packageMap[key].addFile(info) } - pkg, err := selectPackage(dir, rel, packageMap) - if err != nil { - log.Print(err) - return nil - } - if pkg != nil { + switch pc.Mode { + case DefaultMode: + pkg, err := selectPackage(dir, rel, packageMap) + if err != nil { + log.Print(err) + } + if pkg == nil { + return nil // empty rule created in generateEmpty + } for _, name := range genFiles { pkg.addGenFile(dir, name) } + return []*Package{pkg} + + case PackageMode: + pkgs := make([]*Package, 0, len(packageMap)) + for _, pkg := range packageMap { + pkgs = append(pkgs, pkg) + } + return pkgs + + default: + return nil } - return pkg } // selectPackage chooses a package to generate rules for. -func selectPackage(dir, rel string, packageMap map[string]*protoPackage) (*protoPackage, error) { +func selectPackage(dir, rel string, packageMap map[string]*Package) (*Package, error) { if len(packageMap) == 0 { return nil, nil } @@ -130,7 +143,7 @@ func selectPackage(dir, rel string, packageMap map[string]*protoPackage) (*proto return pkg, nil } } - defaultPackageName := strings.Replace(rel, "/", ".", -1) + defaultPackageName := strings.Replace(rel, "/", "_", -1) for _, pkg := range packageMap { if pkgName := goPackageName(pkg); pkgName != "" && pkgName == defaultPackageName { return pkg, nil @@ -145,8 +158,8 @@ func selectPackage(dir, rel string, packageMap map[string]*protoPackage) (*proto // // TODO(jayconrod): remove all Go-specific functionality. This is here // temporarily for compatibility. -func goPackageName(pkg *protoPackage) string { - if opt, ok := pkg.options["go_package"]; ok { +func goPackageName(pkg *Package) string { + if opt, ok := pkg.Options["go_package"]; ok { if i := strings.IndexByte(opt, ';'); i >= 0 { return opt[i+1:] } else if i := strings.LastIndexByte(opt, '/'); i >= 0 { @@ -155,17 +168,87 @@ func goPackageName(pkg *protoPackage) string { return opt } } - if pkg.name != "" { - return strings.Replace(pkg.name, ".", "_", -1) + if pkg.Name != "" { + return strings.Replace(pkg.Name, ".", "_", -1) } - if len(pkg.files) == 1 { - for s := range pkg.files { + if len(pkg.Files) == 1 { + for s := range pkg.Files { return strings.TrimSuffix(s, ".proto") } } return "" } +// generateProto creates a new proto_library rule for a package. The rule may +// be empty if there are no sources. +func generateProto(pc *ProtoConfig, rel string, pkg *Package, shouldSetVisibility bool) *rule.Rule { + var name string + if pc.Mode == DefaultMode { + name = RuleName(goPackageName(pkg), pc.GoPrefix, rel) + } else { + name = RuleName(pkg.Options[pc.groupOption], pkg.Name, rel) + } + r := rule.NewRule("proto_library", name) + srcs := make([]string, 0, len(pkg.Files)) + for f := range pkg.Files { + srcs = append(srcs, f) + } + sort.Strings(srcs) + if len(srcs) > 0 { + r.SetAttr("srcs", srcs) + } + r.SetPrivateAttr(PackageKey, *pkg) + imports := make([]string, 0, len(pkg.Imports)) + for i := range pkg.Imports { + imports = append(imports, i) + } + sort.Strings(imports) + r.SetPrivateAttr(config.GazelleImportsKey, imports) + for k, v := range pkg.Options { + r.SetPrivateAttr(k, v) + } + if shouldSetVisibility { + vis := checkInternalVisibility(rel, "//visibility:public") + r.SetAttr("visibility", []string{vis}) + } + return r +} + +// generateEmpty generates a list of proto_library rules that may be deleted. +// This is generated from existing proto_library rules with srcs lists that +// don't match any static or generated files. +func generateEmpty(f *rule.File, regularFiles, genFiles []string) []*rule.Rule { + if f == nil { + return nil + } + knownFiles := make(map[string]bool) + for _, f := range regularFiles { + knownFiles[f] = true + } + for _, f := range genFiles { + knownFiles[f] = true + } + var empty []*rule.Rule +outer: + for _, r := range f.Rules { + if r.Kind() != "proto_library" { + continue + } + srcs := r.AttrStrings("srcs") + if len(srcs) == 0 && r.Attr("srcs") != nil { + // srcs is not a string list; leave it alone + continue + } + for _, src := range r.AttrStrings("srcs") { + if knownFiles[src] { + continue outer + } + } + empty = append(empty, rule.NewRule("proto_library", r.Name())) + } + return empty +} + // hasDefaultVisibility returns whether oldFile contains a "package" rule with // a "default_visibility" attribute. Rules generated by Gazelle should not // have their own visibility attributes if this is the case. diff --git a/internal/language/proto/generate_test.go b/internal/language/proto/generate_test.go index 7f604320b..7a7ceb4f4 100644 --- a/internal/language/proto/generate_test.go +++ b/internal/language/proto/generate_test.go @@ -76,7 +76,30 @@ func TestGenerateRulesEmpty(t *testing.T) { c := config.New() c.Exts[protoName] = &ProtoConfig{} - empty, gen := lang.GenerateRules(c, "", "foo", nil, nil, nil, nil, nil, nil) + oldContent := []byte(` +proto_library( + name = "dead_proto", + srcs = ["foo.proto"], +) + +proto_library( + name = "live_proto", + srcs = ["bar.proto"], +) + +COMPLICATED_SRCS = ["baz.proto"] + +proto_library( + name = "complicated_proto", + srcs = COMPLICATED_SRCS, +) +`) + old, err := rule.LoadData("BUILD.bazel", oldContent) + if err != nil { + t.Fatal(err) + } + genFiles := []string{"bar.proto"} + empty, gen := lang.GenerateRules(c, "", "foo", old, nil, nil, genFiles, nil, nil) if len(gen) > 0 { t.Errorf("got %d generated rules; want 0", len(gen)) } @@ -86,32 +109,45 @@ func TestGenerateRulesEmpty(t *testing.T) { } f.Sync() got := strings.TrimSpace(string(bzl.Format(f.File))) - want := `proto_library(name = "foo_proto")` + want := `proto_library(name = "dead_proto")` if got != want { t.Errorf("got:\n%s\nwant:\n%s", got, want) } } -func TestGenerateFileInfo(t *testing.T) { +func TestGeneratePackage(t *testing.T) { lang := New() c := testConfig() dir := filepath.FromSlash("testdata/protos") _, gen := lang.GenerateRules(c, dir, "protos", nil, nil, []string{"foo.proto"}, nil, nil, nil) r := gen[0] - got := r.PrivateAttr(FileInfoKey).([]FileInfo) - want := []FileInfo{{ - Path: filepath.Join(dir, "foo.proto"), - Name: "foo.proto", - PackageName: "bar.foo", - Options: []Option{{Key: "go_package", Value: "example.com/repo/protos"}}, - Imports: []string{ - "google/protobuf/any.proto", - "protos/sub/sub.proto", + got := r.PrivateAttr(PackageKey).(Package) + want := Package{ + Name: "bar.foo", + Files: map[string]FileInfo{ + "foo.proto": { + Path: filepath.Join(dir, "foo.proto"), + Name: "foo.proto", + PackageName: "bar.foo", + Options: []Option{{Key: "go_package", Value: "example.com/repo/protos"}}, + Imports: []string{ + "google/protobuf/any.proto", + "protos/sub/sub.proto", + }, + HasServices: true, + }, + }, + Imports: map[string]bool{ + "google/protobuf/any.proto": true, + "protos/sub/sub.proto": true, + }, + Options: map[string]string{ + "go_package": "example.com/repo/protos", }, HasServices: true, - }} + } if !reflect.DeepEqual(got, want) { - t.Errorf("got %v; want %v", got, want) + t.Errorf("got %#v; want %#v", got, want) } } diff --git a/internal/language/proto/package.go b/internal/language/proto/package.go index 63a689641..fba05f0c3 100644 --- a/internal/language/proto/package.go +++ b/internal/language/proto/package.go @@ -17,36 +17,38 @@ package proto import "path/filepath" -// protoPackage contains metadata for a set of .proto files that have the +// Package contains metadata for a set of .proto files that have the // same package name. This translates to a proto_library rule. -type protoPackage struct { - name string - files map[string]FileInfo - imports map[string]bool - options map[string]string +type Package struct { + Name string + Files map[string]FileInfo + Imports map[string]bool + Options map[string]string + HasServices bool } -func newProtoPackage(name string) *protoPackage { - return &protoPackage{ - name: name, - files: map[string]FileInfo{}, - imports: map[string]bool{}, - options: map[string]string{}, +func newPackage(name string) *Package { + return &Package{ + Name: name, + Files: map[string]FileInfo{}, + Imports: map[string]bool{}, + Options: map[string]string{}, } } -func (p *protoPackage) addFile(info FileInfo) { - p.files[info.Name] = info +func (p *Package) addFile(info FileInfo) { + p.Files[info.Name] = info for _, imp := range info.Imports { - p.imports[imp] = true + p.Imports[imp] = true } for _, opt := range info.Options { - p.options[opt.Key] = opt.Value + p.Options[opt.Key] = opt.Value } + p.HasServices = p.HasServices || info.HasServices } -func (p *protoPackage) addGenFile(dir, name string) { - p.files[name] = FileInfo{ +func (p *Package) addGenFile(dir, name string) { + p.Files[name] = FileInfo{ Name: name, Path: filepath.Join(dir, filepath.FromSlash(name)), } diff --git a/internal/language/proto/testdata/multiple_packages/default_mode/BUILD.want b/internal/language/proto/testdata/multiple_packages/default_mode/BUILD.want new file mode 100644 index 000000000..d1d6089d5 --- /dev/null +++ b/internal/language/proto/testdata/multiple_packages/default_mode/BUILD.want @@ -0,0 +1,5 @@ +proto_library( + name = "multiple_packages_default_mode_proto", + srcs = ["foo.proto"], + visibility = ["//visibility:public"], +) diff --git a/internal/language/proto/testdata/multiple_packages/default_mode/bar.proto b/internal/language/proto/testdata/multiple_packages/default_mode/bar.proto new file mode 100644 index 000000000..c44368c63 --- /dev/null +++ b/internal/language/proto/testdata/multiple_packages/default_mode/bar.proto @@ -0,0 +1,3 @@ +syntax = "proto3"; + +package bar; diff --git a/internal/language/proto/testdata/multiple_packages/default_mode/foo.proto b/internal/language/proto/testdata/multiple_packages/default_mode/foo.proto new file mode 100644 index 000000000..712d3d094 --- /dev/null +++ b/internal/language/proto/testdata/multiple_packages/default_mode/foo.proto @@ -0,0 +1,3 @@ +syntax = "proto3"; + +package multiple_packages.default_mode; diff --git a/internal/language/proto/testdata/multiple_packages/package_mode/BUILD.old b/internal/language/proto/testdata/multiple_packages/package_mode/BUILD.old new file mode 100644 index 000000000..dff658352 --- /dev/null +++ b/internal/language/proto/testdata/multiple_packages/package_mode/BUILD.old @@ -0,0 +1,6 @@ +# gazelle:proto package + +genrule( + name = "genproto", + outs ["gen.proto"], +) diff --git a/internal/language/proto/testdata/multiple_packages/package_mode/BUILD.want b/internal/language/proto/testdata/multiple_packages/package_mode/BUILD.want new file mode 100644 index 000000000..70d4a3c13 --- /dev/null +++ b/internal/language/proto/testdata/multiple_packages/package_mode/BUILD.want @@ -0,0 +1,17 @@ +proto_library( + name = "bar_proto", + srcs = [ + "bar1.proto", + "bar2.proto", + ], + visibility = ["//visibility:public"], +) + +proto_library( + name = "foo_proto", + srcs = [ + "foo1.proto", + "foo2.proto", + ], + visibility = ["//visibility:public"], +) diff --git a/internal/language/proto/testdata/multiple_packages/package_mode/bar1.proto b/internal/language/proto/testdata/multiple_packages/package_mode/bar1.proto new file mode 100644 index 000000000..c44368c63 --- /dev/null +++ b/internal/language/proto/testdata/multiple_packages/package_mode/bar1.proto @@ -0,0 +1,3 @@ +syntax = "proto3"; + +package bar; diff --git a/internal/language/proto/testdata/multiple_packages/package_mode/bar2.proto b/internal/language/proto/testdata/multiple_packages/package_mode/bar2.proto new file mode 100644 index 000000000..c44368c63 --- /dev/null +++ b/internal/language/proto/testdata/multiple_packages/package_mode/bar2.proto @@ -0,0 +1,3 @@ +syntax = "proto3"; + +package bar; diff --git a/internal/language/proto/testdata/multiple_packages/package_mode/foo1.proto b/internal/language/proto/testdata/multiple_packages/package_mode/foo1.proto new file mode 100644 index 000000000..69605d73a --- /dev/null +++ b/internal/language/proto/testdata/multiple_packages/package_mode/foo1.proto @@ -0,0 +1,3 @@ +syntax = "proto3"; + +package foo; diff --git a/internal/language/proto/testdata/multiple_packages/package_mode/foo2.proto b/internal/language/proto/testdata/multiple_packages/package_mode/foo2.proto new file mode 100644 index 000000000..69605d73a --- /dev/null +++ b/internal/language/proto/testdata/multiple_packages/package_mode/foo2.proto @@ -0,0 +1,3 @@ +syntax = "proto3"; + +package foo; diff --git a/internal/language/proto/testdata/multiple_packages/package_mode_group/BUILD.old b/internal/language/proto/testdata/multiple_packages/package_mode_group/BUILD.old new file mode 100644 index 000000000..9b664bd4f --- /dev/null +++ b/internal/language/proto/testdata/multiple_packages/package_mode_group/BUILD.old @@ -0,0 +1,2 @@ +# gazelle:proto package +# gazelle:proto_group x_package diff --git a/internal/language/proto/testdata/multiple_packages/package_mode_group/BUILD.want b/internal/language/proto/testdata/multiple_packages/package_mode_group/BUILD.want new file mode 100644 index 000000000..70d4a3c13 --- /dev/null +++ b/internal/language/proto/testdata/multiple_packages/package_mode_group/BUILD.want @@ -0,0 +1,17 @@ +proto_library( + name = "bar_proto", + srcs = [ + "bar1.proto", + "bar2.proto", + ], + visibility = ["//visibility:public"], +) + +proto_library( + name = "foo_proto", + srcs = [ + "foo1.proto", + "foo2.proto", + ], + visibility = ["//visibility:public"], +) diff --git a/internal/language/proto/testdata/multiple_packages/package_mode_group/bar1.proto b/internal/language/proto/testdata/multiple_packages/package_mode_group/bar1.proto new file mode 100644 index 000000000..d9d9d1aa8 --- /dev/null +++ b/internal/language/proto/testdata/multiple_packages/package_mode_group/bar1.proto @@ -0,0 +1,3 @@ +syntax = "proto3"; + +option x_package = "x/bar"; diff --git a/internal/language/proto/testdata/multiple_packages/package_mode_group/bar2.proto b/internal/language/proto/testdata/multiple_packages/package_mode_group/bar2.proto new file mode 100644 index 000000000..d9d9d1aa8 --- /dev/null +++ b/internal/language/proto/testdata/multiple_packages/package_mode_group/bar2.proto @@ -0,0 +1,3 @@ +syntax = "proto3"; + +option x_package = "x/bar"; diff --git a/internal/language/proto/testdata/multiple_packages/package_mode_group/foo1.proto b/internal/language/proto/testdata/multiple_packages/package_mode_group/foo1.proto new file mode 100644 index 000000000..632281ff0 --- /dev/null +++ b/internal/language/proto/testdata/multiple_packages/package_mode_group/foo1.proto @@ -0,0 +1,3 @@ +syntax = "proto3"; + +option x_package = "111@foo"; diff --git a/internal/language/proto/testdata/multiple_packages/package_mode_group/foo2.proto b/internal/language/proto/testdata/multiple_packages/package_mode_group/foo2.proto new file mode 100644 index 000000000..632281ff0 --- /dev/null +++ b/internal/language/proto/testdata/multiple_packages/package_mode_group/foo2.proto @@ -0,0 +1,3 @@ +syntax = "proto3"; + +option x_package = "111@foo";