diff --git a/cmd/gencopy/gencopy.go b/cmd/gencopy/gencopy.go index 7513237b3..4d8b9c550 100644 --- a/cmd/gencopy/gencopy.go +++ b/cmd/gencopy/gencopy.go @@ -11,6 +11,7 @@ import ( "os" "path/filepath" "strconv" + "strings" "github.com/google/go-cmp/cmp" ) @@ -105,7 +106,7 @@ func readFileAsString(filename string) (string, error) { return string(bytes), nil } -func check(_ *Config, pkg *PackageConfig, pairs []*SrcDst) error { +func check(cfg *Config, pkg *PackageConfig, pairs []*SrcDst) error { for _, pair := range pairs { expected, err := readFileAsString(pair.Src) if err != nil { @@ -117,7 +118,10 @@ func check(_ *Config, pkg *PackageConfig, pairs []*SrcDst) error { } if diff := cmp.Diff(expected, actual); diff != "" { - return fmt.Errorf("gencopy mismatch %q vs. %q (-want +got):\n%s", pair.Src, pair.Dst, diff) + return fmt.Errorf( + "gencopy mismatch %q vs. %q (-want +got):\n%s\n%s", + pair.Src, pair.Dst, diff, regenerateProTip(pkg.TargetLabel, cfg.UpdateTargetLabelName), + ) } } @@ -129,6 +133,22 @@ func check(_ *Config, pkg *PackageConfig, pairs []*SrcDst) error { return nil } +// regenerateProTip returns a friendly hint pointing the developer at the +// `.update` target that regenerates the checked-in copies. targetLabel is the +// proto_compile rule's label (e.g. "//proto:foo_proto_compile" or +// "@@repo//proto:foo_proto_compile"); updateName is the .update target's +// rule name (e.g. "foo_proto_compile.update"). +func regenerateProTip(targetLabel, updateName string) string { + if updateName == "" { + return "" + } + updateLabel := updateName + if idx := strings.LastIndex(targetLabel, ":"); idx >= 0 { + updateLabel = targetLabel[:idx+1] + updateName + } + return fmt.Sprintf("\nProTip: to regenerate, run:\n bazel run %s\n", updateLabel) +} + func update(cfg *Config, pkg *PackageConfig, pairs []*SrcDst) error { for _, pair := range pairs { pair.Dst += cfg.Extension diff --git a/cmd/gencopy/gencopy_test.go b/cmd/gencopy/gencopy_test.go index 19b2656c5..ed4e0527e 100644 --- a/cmd/gencopy/gencopy_test.go +++ b/cmd/gencopy/gencopy_test.go @@ -144,6 +144,42 @@ func TestRunPkg(t *testing.T) { } } +func TestRegenerateProTip(t *testing.T) { + for name, tc := range map[string]struct { + targetLabel string + updateName string + want string + }{ + "empty update name": { + targetLabel: "//proto:foo_proto_compile", + updateName: "", + want: "", + }, + "local target": { + targetLabel: "//proto:foo_proto_compile", + updateName: "foo_proto_compile.update", + want: "\nProTip: to regenerate, run:\n bazel run //proto:foo_proto_compile.update\n", + }, + "external repo target": { + targetLabel: "@@some_repo//pkg:foo_proto_compile", + updateName: "foo_proto_compile.update", + want: "\nProTip: to regenerate, run:\n bazel run @@some_repo//pkg:foo_proto_compile.update\n", + }, + "target with no colon": { + targetLabel: "raw_label_no_colon", + updateName: "foo_proto_compile.update", + want: "\nProTip: to regenerate, run:\n bazel run foo_proto_compile.update\n", + }, + } { + t.Run(name, func(t *testing.T) { + got := regenerateProTip(tc.targetLabel, tc.updateName) + if got != tc.want { + t.Errorf("regenerateProTip: got %q, want %q", got, tc.want) + } + }) + } +} + // listFiles - convenience debugging function to log the files under a given dir func listFiles(t *testing.T, dir string) error { return filepath.Walk(dir, func(path string, info os.FileInfo, err error) error { diff --git a/example/thing/BUILD.bazel b/example/thing/BUILD.bazel index 89324c20f..f39ef8a9e 100644 --- a/example/thing/BUILD.bazel +++ b/example/thing/BUILD.bazel @@ -11,7 +11,7 @@ proto_library( name = "thing_proto", srcs = ["thing.proto"], visibility = ["//visibility:public"], - deps = ["@protobufapis//google/protobuf:timestamp_proto"], + deps = ["@com_google_protobuf//:timestamp_proto"], ) proto_cc_library( diff --git a/pkg/language/protobuf/BUILD.bazel b/pkg/language/protobuf/BUILD.bazel index 5a3090c98..64f3a078d 100644 --- a/pkg/language/protobuf/BUILD.bazel +++ b/pkg/language/protobuf/BUILD.bazel @@ -28,6 +28,7 @@ go_library( go_test( name = "protobuf_test", srcs = [ + "config_filemode_test.go", "generate_test.go", "override_test.go", ], @@ -37,6 +38,7 @@ go_test( "@bazel_gazelle//config", "@bazel_gazelle//label", "@bazel_gazelle//language", + "@bazel_gazelle//language/proto", "@bazel_gazelle//resolve", "@bazel_gazelle//rule", "@bazel_gazelle//testtools", diff --git a/pkg/language/protobuf/config.go b/pkg/language/protobuf/config.go index dd13aad37..18693a8b7 100644 --- a/pkg/language/protobuf/config.go +++ b/pkg/language/protobuf/config.go @@ -117,10 +117,18 @@ func (pl *protobufLang) Configure(c *config.Config, rel string, f *rule.File) { // getOrCreatePackageConfig either inserts a new config into the map under the // language name or replaces it with a clone. +// +// When cloning from the parent directory's config we must overwrite the +// embedded *config.Config pointer to the current directory's config. Without +// this, lookups that walk `cfg.Config.Exts[...]` (e.g. +// `IsProtoFileMode` reading the standard proto language's per-directory mode) +// resolve against the parent's `Exts` and miss directives like +// `# gazelle:proto file` that were applied to the child only. func (pl *protobufLang) getOrCreatePackageConfig(config *config.Config) *protoc.PackageConfig { var cfg *protoc.PackageConfig if existingExt, ok := config.Exts[pl.name]; ok { cfg = existingExt.(*protoc.PackageConfig).Clone() + cfg.Config = config } else { cfg = protoc.NewPackageConfig(config) } diff --git a/pkg/language/protobuf/config_filemode_test.go b/pkg/language/protobuf/config_filemode_test.go new file mode 100644 index 000000000..cc7751c94 --- /dev/null +++ b/pkg/language/protobuf/config_filemode_test.go @@ -0,0 +1,37 @@ +package protobuf + +import ( + "testing" + + "github.com/bazelbuild/bazel-gazelle/config" + gproto "github.com/bazelbuild/bazel-gazelle/language/proto" +) + +// Regression: getOrCreatePackageConfig must point the cloned PackageConfig's +// embedded *config.Config at the CURRENT directory's config, not the parent's. +// Otherwise IsProtoFileMode reads the parent's proto-language config and +// misses a child directory that opted into `# gazelle:proto file`. +func TestGetOrCreatePackageConfig_RebindsConfig(t *testing.T) { + pl := &protobufLang{name: "protobuf"} + + // Parent: default mode. + parent := &config.Config{Exts: map[string]interface{}{}} + parent.Exts["proto"] = &gproto.ProtoConfig{Mode: gproto.DefaultMode} + pl.getOrCreatePackageConfig(parent) + + // Child: clone via gazelle's Config.Clone, then proto-lang flips mode to FileMode + // for this dir only. + child := parent.Clone() + childProto := &gproto.ProtoConfig{Mode: gproto.FileMode} + child.Exts["proto"] = childProto + + cfg := pl.getOrCreatePackageConfig(child) + if cfg.Config != child { + t.Fatalf("cloned PackageConfig.Config not rebound to child: got %p, want %p", cfg.Config, child) + } + + // IsProtoFileMode-equivalent check. + if gproto.GetProtoConfig(cfg.Config).Mode != gproto.FileMode { + t.Errorf("expected FileMode via cfg.Config, got %v", gproto.GetProtoConfig(cfg.Config).Mode) + } +} diff --git a/pkg/language/protobuf/generate.go b/pkg/language/protobuf/generate.go index e3e96381f..e1095830e 100644 --- a/pkg/language/protobuf/generate.go +++ b/pkg/language/protobuf/generate.go @@ -3,6 +3,7 @@ package protobuf import ( "log" "path" + "strings" "github.com/bazelbuild/bazel-gazelle/config" "github.com/bazelbuild/bazel-gazelle/label" @@ -104,7 +105,14 @@ func (pl *protobufLang) GenerateRules(args language.GenerateArgs) language.Gener protoc.GlobalRuleIndex().Put(internalLabel, r) switch r.Kind() { case "proto_rust_library": - pl.protoRustLibraryPackages = append(pl.protoRustLibraryPackages, args.Rel) + if protoRustLibraryIsExplicitWorkspaceMember(r.Name()) { + pl.protoRustLibraryPackages = append(pl.protoRustLibraryPackages, args.Rel) + } else { + pl.protoRustPerFilePackageDirs = append( + pl.protoRustPerFilePackageDirs, + path.Join(args.Rel, "_rust"), + ) + } // The proto_rust_library macro's underlying _proto_rust_lib rule // (named "_lib") is what provides ProtoCompileInfo for the // wrapper lib.rs + Cargo.toml; that's the label that belongs in @@ -136,6 +144,14 @@ func (pl *protobufLang) GenerateRules(args language.GenerateArgs) language.Gener } } +// protoRustLibraryIsExplicitWorkspaceMember reports whether the generated +// crate belongs in the root Cargo.toml marker section. Per-file crates are +// path dependencies of these package-level roots, so Cargo enrolls them in +// the workspace transitively after their manifests are vendored. +func protoRustLibraryIsExplicitWorkspaceMember(name string) bool { + return !strings.Contains(name, "__") +} + func matchingFiles(files map[string]*protoc.File, srcs []label.Label) []*protoc.File { matching := make([]*protoc.File, 0) for _, src := range srcs { diff --git a/pkg/language/protobuf/generate_test.go b/pkg/language/protobuf/generate_test.go index 2fd5b23ad..19a90e231 100644 --- a/pkg/language/protobuf/generate_test.go +++ b/pkg/language/protobuf/generate_test.go @@ -151,6 +151,28 @@ import "google/protobuf/any.proto"; } } +func TestProtoRustLibraryIsExplicitWorkspaceMember(t *testing.T) { + for name, tc := range map[string]struct { + ruleName string + want bool + }{ + "package-level crate": { + ruleName: "example_proto_rs", + want: true, + }, + "per-file crate": { + ruleName: "example_proto__message_proto_rs", + want: false, + }, + } { + t.Run(name, func(t *testing.T) { + if got := protoRustLibraryIsExplicitWorkspaceMember(tc.ruleName); got != tc.want { + t.Errorf("protoRustLibraryIsExplicitWorkspaceMember(%q) = %t, want %t", tc.ruleName, got, tc.want) + } + }) + } +} + type testGenerateRulesState struct { t *testing.T tmpdir string diff --git a/pkg/language/protobuf/lang.go b/pkg/language/protobuf/lang.go index eeb9b78f2..343c3d1b9 100644 --- a/pkg/language/protobuf/lang.go +++ b/pkg/language/protobuf/lang.go @@ -40,11 +40,17 @@ type protobufLang struct { starlarkRules arrayFlags // starlarkPlugins stores custom starlark proto plugin names in the form filename%pluginname starlarkPlugins arrayFlags - // protoRustLibraryPackages collects the workspace-relative path of every - // package that emits a proto_rust_library rule. Populated in + // protoRustLibraryPackages collects the workspace-relative path of each + // package-level proto_rust_library. Per-file crates remain standalone path + // dependencies under excluded _rust directories. Populated in // GenerateRules and consumed in DoneGeneratingRules to update the root // Cargo.toml [workspace] members list. protoRustLibraryPackages []string + // protoRustPerFilePackageDirs collects the workspace-relative _rust + // directories that contain standalone per-file crates. These directories + // are excluded from the root Cargo workspace so each generated manifest can + // be used directly or as a path dependency. + protoRustPerFilePackageDirs []string // vendorAssetLabels collects bazel labels of every generated rule that // provides ProtoCompileInfo and should appear in the root // `proto_compile_assets` aggregator. Populated in GenerateRules and diff --git a/pkg/language/protobuf/lifecycle.go b/pkg/language/protobuf/lifecycle.go index 019b79d7f..6ac4c3940 100644 --- a/pkg/language/protobuf/lifecycle.go +++ b/pkg/language/protobuf/lifecycle.go @@ -16,19 +16,23 @@ func (pl *protobufLang) Before(context.Context) { // DoneGeneratingRules implements part of the language.LifecycleManager interface. // -// Performs two cross-package syncs that need every GenerateRules call to +// Performs three cross-package syncs that need every GenerateRules call to // have completed first: // // 1. Root Cargo.toml [workspace] members list — the lines between the // `# gazelle:proto_rust_members start/end` markers are replaced with -// one entry per package that emitted a proto_rust_library. +// one entry per package-level proto_rust_library. // // 2. Root BUILD.bazel proto_compile_assets aggregator deps — the lines // between the `# gazelle:vendor_proto_sources_deps start/end` markers // are replaced with one entry per generated proto_compiled_sources rule // and one entry per proto_rust_library's underlying _lib target. // -// Both syncs are no-ops when the corresponding markers are absent (or the +// 3. Root Cargo.toml [workspace] exclude list — the lines between the +// `# gazelle:proto_rust_excludes start/end` markers are replaced with +// one entry per Bazel package containing standalone per-file crates. +// +// All syncs are no-ops when the corresponding markers are absent (or the // target file does not exist). func (pl *protobufLang) DoneGeneratingRules() { if pl.repoRoot == "" { @@ -37,6 +41,9 @@ func (pl *protobufLang) DoneGeneratingRules() { if err := updateRootCargoMembers(pl.repoRoot, pl.protoRustLibraryPackages); err != nil { log.Printf("warning: could not update root Cargo.toml proto_rust_members: %v", err) } + if err := updateRootCargoExcludes(pl.repoRoot, pl.protoRustPerFilePackageDirs); err != nil { + log.Printf("warning: could not update root Cargo.toml proto_rust_excludes: %v", err) + } if err := updateRootVendorAssetsDeps(pl.repoRoot, pl.vendorAssetLabels); err != nil { log.Printf("warning: could not update root BUILD.bazel vendor_proto_sources_deps: %v", err) } @@ -47,10 +54,12 @@ func (pl *protobufLang) AfterResolvingDeps(context.Context) { } const ( - cargoMembersStartMarker = "# gazelle:proto_rust_members start" - cargoMembersEndMarker = "# gazelle:proto_rust_members end" - vendorAssetsDepsStartMarker = "# gazelle:vendor_proto_sources_deps start" - vendorAssetsDepsEndMarker = "# gazelle:vendor_proto_sources_deps end" + cargoMembersStartMarker = "# gazelle:proto_rust_members start" + cargoMembersEndMarker = "# gazelle:proto_rust_members end" + cargoExcludesStartMarker = "# gazelle:proto_rust_excludes start" + cargoExcludesEndMarker = "# gazelle:proto_rust_excludes end" + vendorAssetsDepsStartMarker = "# gazelle:vendor_proto_sources_deps start" + vendorAssetsDepsEndMarker = "# gazelle:vendor_proto_sources_deps end" ) // updateRootCargoMembers rewrites the gazelle:proto_rust_members marker @@ -67,6 +76,19 @@ func updateRootCargoMembers(repoRoot string, packages []string) error { ) } +// updateRootCargoExcludes rewrites the gazelle:proto_rust_excludes marker +// section in the root Cargo.toml with directories containing standalone +// per-file crates. +func updateRootCargoExcludes(repoRoot string, packages []string) error { + return rewriteMarkerSection( + filepath.Join(repoRoot, "Cargo.toml"), + cargoExcludesStartMarker, + cargoExcludesEndMarker, + packages, + "[workspace] exclude list", + ) +} + // updateRootVendorAssetsDeps rewrites the gazelle:vendor_proto_sources_deps // marker section in the root BUILD.bazel with a sorted, deduplicated list // of `"