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/pkg/plugin/neoeinstein/prost/extern_paths.go b/pkg/plugin/neoeinstein/prost/extern_paths.go index d014575e0..eb4e561e2 100644 --- a/pkg/plugin/neoeinstein/prost/extern_paths.go +++ b/pkg/plugin/neoeinstein/prost/extern_paths.go @@ -108,28 +108,39 @@ func ResolveExternPathOptionsForReferences(cfg *protoc.PluginConfiguration, r *r // package. Self-extern overrides are NOT included — see // ResolveExternPathOptionsForReferences for the variant that adds them. func ResolveTransitiveExternPaths(r *rule.Rule, from label.Label) []string { - lib := r.PrivateAttr(protoc.ProtoLibraryKey) - if lib == nil { + libraries := mergedLibraries(r) + if len(libraries) == 0 { return nil } - library := lib.(protoc.ProtoLibrary) - libRule := library.Rule() - - if cached, ok := libRule.PrivateAttr(TransitiveExternPathsKey).([]string); ok { + // Cache off the first library's underlying rule — merge can occur but the + // post-merge PrivateAttrs travel with the first library's rule for back- + // compat with the existing cache placement. + cacheRule := libraries[0].Rule() + if cached, ok := cacheRule.PrivateAttr(TransitiveExternPathsKey).([]string); ok { return cached } resolver := protoc.GlobalResolver() ownFiles := make(map[string]bool) - for _, src := range library.Srcs() { - ownFiles[path.Join(from.Pkg, src)] = true + for _, library := range libraries { + for _, src := range library.Srcs() { + ownFiles[path.Join(from.Pkg, src)] = true + } } + // Also treat any proto file whose registered prost_extern crate matches + // one of our own as own. This is a belt-and-suspenders guard for cases + // where ownFiles can't identify a merged-in file by path alone (the + // caller's `from.Pkg` is the rule's bazel package, but a merged proto_- + // library may sit in a different bazel package). + ownCrates := ownCrateNames(libraries, resolver, from) seen := make(map[string]bool) stack := list.New() - for _, src := range library.Srcs() { - stack.PushBack(path.Join(from.Pkg, src)) + for _, library := range libraries { + for _, src := range library.Srcs() { + stack.PushBack(path.Join(from.Pkg, src)) + } } externPathsByPackage := make(map[string]string) @@ -154,11 +165,6 @@ func ResolveTransitiveExternPaths(r *rule.Rule, from label.Label) []string { continue } - // Skip well-known types — prost ships these built-in. - if strings.HasPrefix(protofile, "google/protobuf/") { - continue - } - results := resolver.Resolve("proto", "prost_extern", protofile) if len(results) == 0 { continue @@ -170,13 +176,20 @@ func ResolveTransitiveExternPaths(r *rule.Rule, from label.Label) []string { if protoPackage == "" { continue } + // Skip files whose crate is one of ours — they're part of the + // merged library set, just couldn't be matched by file path. + if ownCrates[crateName] { + continue + } if _, exists := externPathsByPackage[protoPackage]; exists { continue } - // extern_path=.{proto_package}=::{crate_name}::{rust_module_path} - rustModulePath := strings.ReplaceAll(protoPackage, ".", "::") - externPathsByPackage[protoPackage] = "extern_path=." + protoPackage + "=::" + crateName + "::" + rustModulePath + // extern_path=.{proto_package}=::{crate_name} + // The crate exposes all generated types at its root (see the + // proto_rust_library Starlark macro), so no nested module path is + // appended after the crate name. + externPathsByPackage[protoPackage] = "extern_path=." + protoPackage + "=::" + crateName } result := make([]string, 0, len(externPathsByPackage)) @@ -185,7 +198,7 @@ func ResolveTransitiveExternPaths(r *rule.Rule, from label.Label) []string { } sort.Strings(result) - libRule.SetPrivateAttr(TransitiveExternPathsKey, result) + cacheRule.SetPrivateAttr(TransitiveExternPathsKey, result) return result } @@ -205,31 +218,68 @@ func mergeExternPathOptions(cfg *protoc.PluginConfiguration, externPaths []strin // ownProtoPackages returns the set of proto packages the library itself // contributes, computed from prost_extern resolver entries for each own -// proto file. Cached on the library rule. +// proto file across all merged proto_libraries. Cached on the rule. func ownProtoPackages(r *rule.Rule, from label.Label) map[string]bool { - lib := r.PrivateAttr(protoc.ProtoLibraryKey) - if lib == nil { + libraries := mergedLibraries(r) + if len(libraries) == 0 { return nil } - library := lib.(protoc.ProtoLibrary) - libRule := library.Rule() - - if cached, ok := libRule.PrivateAttr(OwnProtoPackagesKey).(map[string]bool); ok { + cacheRule := libraries[0].Rule() + if cached, ok := cacheRule.PrivateAttr(OwnProtoPackagesKey).(map[string]bool); ok { return cached } resolver := protoc.GlobalResolver() out := make(map[string]bool) - for _, src := range library.Srcs() { - ownFile := path.Join(from.Pkg, src) - for _, ext := range resolver.Resolve("proto", "prost_extern", ownFile) { - if ext.Label.Pkg != "" { - out[ext.Label.Pkg] = true + for _, library := range libraries { + for _, src := range library.Srcs() { + ownFile := path.Join(from.Pkg, src) + for _, ext := range resolver.Resolve("proto", "prost_extern", ownFile) { + if ext.Label.Pkg != "" { + out[ext.Label.Pkg] = true + } } } } - libRule.SetPrivateAttr(OwnProtoPackagesKey, out) + cacheRule.SetPrivateAttr(OwnProtoPackagesKey, out) + return out +} + +// mergedLibraries returns the full set of proto_libraries backing a proto_- +// compile / proto_compiled_sources rule. Prefers MergedProtoLibrariesKey +// (set by proto_compile.Rule for every merge — see protoc.MergedProtoLibrariesKey) +// and falls back to wrapping the single ProtoLibraryKey for callers that +// haven't migrated (e.g. proto_rust_library, hand-constructed test rules). +// Returns nil when the rule carries neither. +func mergedLibraries(r *rule.Rule) []protoc.ProtoLibrary { + if libs, ok := r.PrivateAttr(protoc.MergedProtoLibrariesKey).([]protoc.ProtoLibrary); ok && len(libs) > 0 { + return libs + } + if lib, ok := r.PrivateAttr(protoc.ProtoLibraryKey).(protoc.ProtoLibrary); ok && lib != nil { + return []protoc.ProtoLibrary{lib} + } + return nil +} + +// ownCrateNames returns the set of rust crate names registered (via +// prost_extern) for files belonging to any of the library's merged proto_- +// libraries. Used to recognise own-merged files in the dep walk even when +// their on-disk path doesn't share a prefix with from.Pkg (e.g. a +// proto_compiled_sources at //a:foo that merges in //b:bar_proto would +// otherwise see b/bar.proto as external). +func ownCrateNames(libraries []protoc.ProtoLibrary, resolver protoc.ImportResolver, from label.Label) map[string]bool { + out := make(map[string]bool) + for _, library := range libraries { + for _, src := range library.Srcs() { + ownFile := path.Join(from.Pkg, src) + for _, ext := range resolver.Resolve("proto", "prost_extern", ownFile) { + if ext.Label.Name != "" { + out[ext.Label.Name] = true + } + } + } + } return out } @@ -247,8 +297,9 @@ func selfExternPathsForOverride(ownPackages map[string]bool, parents []string) [ if !hasParentInImports(ownPkg, parentPkgs) { continue } - rustModulePath := strings.ReplaceAll(ownPkg, ".", "::") - out = append(out, "extern_path=."+ownPkg+"=crate::"+rustModulePath) + // All own types live at the crate root (flat convention), so the + // self-extern override maps the proto sub-package to bare `crate`. + out = append(out, "extern_path=."+ownPkg+"=crate") } return out } diff --git a/pkg/plugin/neoeinstein/prost/extern_paths_test.go b/pkg/plugin/neoeinstein/prost/extern_paths_test.go index fad87ee0a..d3bad4198 100644 --- a/pkg/plugin/neoeinstein/prost/extern_paths_test.go +++ b/pkg/plugin/neoeinstein/prost/extern_paths_test.go @@ -56,8 +56,8 @@ func TestResolveTransitiveExternPaths(t *testing.T) { sort.Strings(got) want := []string{ - "extern_path=.extern.dep_a=::depA_rs::extern::dep_a", - "extern_path=.extern.dep_b=::depB_rs::extern::dep_b", + "extern_path=.extern.dep_a=::depA_rs", + "extern_path=.extern.dep_b=::depB_rs", } if !reflect.DeepEqual(got, want) { t.Errorf("ResolveTransitiveExternPaths:\n got: %v\nwant: %v", got, want) @@ -113,7 +113,7 @@ func TestResolveTransitiveExternPaths_SubpackageOfImport(t *testing.T) { got := prost.ResolveTransitiveExternPaths(r, from) want := []string{ - "extern_path=.subpkg.parent=::parent_rs::subpkg::parent", + "extern_path=.subpkg.parent=::parent_rs", } if !reflect.DeepEqual(got, want) { t.Errorf("ResolveTransitiveExternPaths:\n got: %v\nwant: %v", got, want) @@ -146,8 +146,8 @@ func TestResolveExternPathOptionsForReferences_SubpackageOfImport(t *testing.T) cfg := &protoc.PluginConfiguration{Options: nil} got := prost.ResolveExternPathOptionsForReferences(cfg, r, from) want := []string{ - "extern_path=.refs.parent.child=crate::refs::parent::child", - "extern_path=.refs.parent=::parent_rs::refs::parent", + "extern_path=.refs.parent.child=crate", + "extern_path=.refs.parent=::parent_rs", } sort.Strings(want) sort.Strings(got) @@ -182,7 +182,7 @@ func TestResolveTransitiveExternPaths_SiblingNotFiltered(t *testing.T) { from := label.New("", "sibling/a/y", "y_proto") got := prost.ResolveTransitiveExternPaths(r, from) - want := []string{"extern_path=.sibling.a.x=::x_rs::sibling::a::x"} + want := []string{"extern_path=.sibling.a.x=::x_rs"} if !reflect.DeepEqual(got, want) { t.Errorf("ResolveTransitiveExternPaths:\n got: %v\nwant: %v", got, want) } @@ -197,13 +197,13 @@ func TestResolveExternPathOptions_FiltersExisting(t *testing.T) { cfg := &protoc.PluginConfiguration{ Options: []string{ "compile_well_known_types=true", - "extern_path=.stale.pkg=::stale_rs::stale::pkg", + "extern_path=.stale.pkg=::stale_rs", }, } got := prost.ResolveExternPathOptions(cfg, r, from) for _, opt := range got { - if opt == "extern_path=.stale.pkg=::stale_rs::stale::pkg" { + if opt == "extern_path=.stale.pkg=::stale_rs" { t.Errorf("stale extern_path option was not filtered: %v", got) } } @@ -213,3 +213,171 @@ func TestResolveExternPathOptions_FiltersExisting(t *testing.T) { t.Errorf("ResolveExternPathOptions:\n got: %v\nwant: %v", got, want) } } + +// TestResolveTransitiveExternPaths_GoogleProtobufNotSkipped guards against the +// historical hard-coded skip of google/protobuf/* in the dep walk. When a +// downstream repo registers a rust target for google.protobuf (via +// proto_language rust enable true on that package), the extern_path entry +// must flow through so consumers reference ::google_protobuf instead of the +// prost-build default ::prost_types — which carries no serde impls. +func TestResolveTransitiveExternPaths_GoogleProtobufNotSkipped(t *testing.T) { + resolver := protoc.GlobalResolver() + + resolver.Provide("proto", "prost_extern", + "google/protobuf/duration.proto", + label.New("", "google.protobuf", "google_protobuf")) + resolver.Provide("proto", "prost_extern", + "wkttest/consumer/c.proto", + label.New("", "wkttest.consumer", "wkttest_consumer")) + + resolver.Provide("proto", "depends", + "wkttest/consumer/c.proto", + label.New("", "google/protobuf", "duration.proto")) + + r := makeLibraryRule("c_proto", "wkttest/consumer", []string{"c.proto"}) + from := label.New("", "wkttest/consumer", "c_proto") + + got := prost.ResolveTransitiveExternPaths(r, from) + want := []string{"extern_path=.google.protobuf=::google_protobuf"} + if !reflect.DeepEqual(got, want) { + t.Errorf("ResolveTransitiveExternPaths:\n got: %v\nwant: %v", got, want) + } +} + +// TestResolveProstOptions_AddsCompileWellKnownTypes_OwnPackage verifies that +// when the library compiles google.protobuf protos itself, the prost plugin +// prepends compile_well_known_types=true so prost-build emits well-known +// type definitions locally (default behaviour is to skip them, which leaves +// the matching prost-serde impls referencing undefined structs). +func TestResolveProstOptions_AddsCompileWellKnownTypes_OwnPackage(t *testing.T) { + resolver := protoc.GlobalResolver() + + resolver.Provide("proto", "prost_extern", + "wktown/google/protobuf/any.proto", + label.New("", "google.protobuf", "google_protobuf")) + + r := makeLibraryRule("google_protobuf_proto", "wktown/google/protobuf", []string{"any.proto"}) + from := label.New("", "wktown/google/protobuf", "google_protobuf_proto") + + plugin := &prost.ProtocGenProstPlugin{} + got := plugin.ResolvePluginOptions(&protoc.PluginConfiguration{}, r, from) + + if len(got) == 0 || got[0] != "compile_well_known_types=true" { + t.Errorf("expected compile_well_known_types=true at head of options for own-google.protobuf library, got: %v", got) + } +} + +// TestResolveProstOptions_AddsCompileWellKnownTypes_ExternPath verifies that +// when a library *consumes* google.protobuf via a foreign crate, the prost +// plugin emits compile_well_known_types=true alongside the extern_path. +// Without the flag, prost-build registers its default +// .google.protobuf=::prost_types extern path and ExternPaths::insert errors +// out with "duplicate extern Protobuf path: .google.protobuf". +func TestResolveProstOptions_AddsCompileWellKnownTypes_ExternPath(t *testing.T) { + resolver := protoc.GlobalResolver() + + resolver.Provide("proto", "prost_extern", + "wktdep/google/protobuf/duration.proto", + label.New("", "google.protobuf", "google_protobuf")) + resolver.Provide("proto", "prost_extern", + "wktdep/consumer/c.proto", + label.New("", "wktdep.consumer", "wktdep_consumer")) + resolver.Provide("proto", "depends", + "wktdep/consumer/c.proto", + label.New("", "wktdep/google/protobuf", "duration.proto")) + + r := makeLibraryRule("c_proto", "wktdep/consumer", []string{"c.proto"}) + from := label.New("", "wktdep/consumer", "c_proto") + + plugin := &prost.ProtocGenProstPlugin{} + got := plugin.ResolvePluginOptions(&protoc.PluginConfiguration{}, r, from) + + wantHead := "compile_well_known_types=true" + wantExtern := "extern_path=.google.protobuf=::google_protobuf" + if len(got) == 0 || got[0] != wantHead { + t.Errorf("expected %q as first option, got: %v", wantHead, got) + } + found := false + for _, opt := range got { + if opt == wantExtern { + found = true + break + } + } + if !found { + t.Errorf("expected %q in options, got: %v", wantExtern, got) + } +} + +// TestResolveProstOptions_NoCompileWellKnownTypes_WhenIrrelevant verifies +// that the flag is NOT emitted when the library neither compiles nor +// consumes google.protobuf. Guards against accidental over-emission. +func TestResolveProstOptions_NoCompileWellKnownTypes_WhenIrrelevant(t *testing.T) { + resolver := protoc.GlobalResolver() + + resolver.Provide("proto", "prost_extern", + "wktneg/leaf/l.proto", + label.New("", "wktneg.leaf", "wktneg_leaf")) + + r := makeLibraryRule("l_proto", "wktneg/leaf", []string{"l.proto"}) + from := label.New("", "wktneg/leaf", "l_proto") + + plugin := &prost.ProtocGenProstPlugin{} + got := plugin.ResolvePluginOptions(&protoc.PluginConfiguration{}, r, from) + + for _, opt := range got { + if opt == "compile_well_known_types=true" { + t.Errorf("did not expect compile_well_known_types=true for irrelevant library, got: %v", got) + } + } +} + +// TestResolveTransitiveExternPaths_MergedLibrariesOwn covers the merged-library +// case: proto_compile/proto_compiled_sources collapses two proto_libraries into +// one rule via the protos= attribute. Before the fix, only the first library's +// srcs went into ownFiles, so files from the second library were treated as +// external in the dep walk and produced a self-referential extern_path entry +// like .google.api=::google_api. The fix is for the rule to carry the full +// []ProtoLibrary set under MergedProtoLibrariesKey, and ResolveTransitive- +// ExternPaths to iterate all of them. +func TestResolveTransitiveExternPaths_MergedLibrariesOwn(t *testing.T) { + resolver := protoc.GlobalResolver() + + // Two libraries sharing one proto package (mergetest.merged) — emulates + // e.g. google/api's annotations_proto + http_proto pair, both declaring + // `package google.api;`. + resolver.Provide("proto", "prost_extern", + "mergetest/merged/a.proto", + label.New("", "mergetest.merged", "mergetest_merged")) + resolver.Provide("proto", "prost_extern", + "mergetest/merged/b.proto", + label.New("", "mergetest.merged", "mergetest_merged")) + + // a.proto imports b.proto — the dep walk would reach b.proto from a.proto. + resolver.Provide("proto", "depends", + "mergetest/merged/a.proto", + label.New("", "mergetest/merged", "b.proto")) + + // Build a single merged rule directly: instead of ProtoLibraryKey only, + // set MergedProtoLibrariesKey with both backing libraries. + r := rule.NewRule("proto_library", "merged_proto") + libA := protoc.NewOtherProtoLibrary(nil, + makeLibraryRule("a_proto", "mergetest/merged", []string{"a.proto"}), + protoc.NewFile("mergetest/merged", "a.proto")) + libB := protoc.NewOtherProtoLibrary(nil, + makeLibraryRule("b_proto", "mergetest/merged", []string{"b.proto"}), + protoc.NewFile("mergetest/merged", "b.proto")) + r.SetPrivateAttr(protoc.MergedProtoLibrariesKey, []protoc.ProtoLibrary{libA, libB}) + + from := label.New("", "mergetest/merged", "merged_proto") + + got := prost.ResolveTransitiveExternPaths(r, from) + for _, opt := range got { + if opt == "extern_path=.mergetest.merged=::mergetest_merged" { + t.Errorf("got self-referential extern_path for merged-in own library, all options: %v", got) + } + } + if len(got) != 0 { + t.Errorf("expected no extern_path entries (all files are own), got: %v", got) + } +} diff --git a/pkg/plugin/neoeinstein/prost/protoc-gen-prost.go b/pkg/plugin/neoeinstein/prost/protoc-gen-prost.go index 72b260cc4..78e61637e 100644 --- a/pkg/plugin/neoeinstein/prost/protoc-gen-prost.go +++ b/pkg/plugin/neoeinstein/prost/protoc-gen-prost.go @@ -46,10 +46,50 @@ func (p *ProtocGenProstPlugin) Configure(ctx *protoc.PluginContext) *protoc.Plug } } -// ResolvePluginOptions implements the PluginOptionsResolver interface. -// It computes extern_path options based on transitive proto file dependencies. +// ResolvePluginOptions implements the PluginOptionsResolver interface. It +// computes extern_path options based on transitive proto file dependencies, +// and prepends compile_well_known_types=true whenever: +// +// - one of the library's own proto packages is google.protobuf (the library +// is itself compiling the well-known types — without this flag prost +// skips them and emits a stub), or +// - the resolved extern_path set references .google.protobuf (the library +// consumes well-known types via a foreign crate; prost-build registers a +// default extern path to ::prost_types unless this flag clears it, which +// would collide with our extern_path entry and error out as "duplicate +// extern Protobuf path"). +// +// Only the prost plugin needs this — protoc-gen-prost-serde (pbjson-build) +// doesn't consult the flag and its codegen doesn't hit the collision path. func (p *ProtocGenProstPlugin) ResolvePluginOptions(cfg *protoc.PluginConfiguration, r *rule.Rule, from label.Label) []string { - return ResolveExternPathOptions(cfg, r, from) + opts := ResolveExternPathOptions(cfg, r, from) + if needsCompileWellKnownTypes(opts, ownProtoPackages(r, from)) { + opts = append([]string{"compile_well_known_types=true"}, opts...) + } + return opts +} + +const wellKnownTypesProtoPackage = "google.protobuf" + +// needsCompileWellKnownTypes reports whether the prost plugin should emit +// compile_well_known_types=true for a library based on its computed options +// and own-package set. See ResolvePluginOptions for the rationale. +func needsCompileWellKnownTypes(opts []string, ownPackages map[string]bool) bool { + if ownPackages[wellKnownTypesProtoPackage] { + return true + } + for _, opt := range opts { + if opt == "extern_path=."+wellKnownTypesProtoPackage+"=::"+protoc.RustCrateName(wellKnownTypesProtoPackage) { + return true + } + // More general guard: any extern_path mapping for .google.protobuf + // suppresses prost-build's default registration to avoid collision. + const prefix = "extern_path=." + wellKnownTypesProtoPackage + "=" + if len(opt) > len(prefix) && opt[:len(prefix)] == prefix { + return true + } + } + return false } // shouldApply returns true if the library has files with messages or enums. diff --git a/pkg/protoc/proto_compile.go b/pkg/protoc/proto_compile.go index bb0b7bcd3..f38b81499 100644 --- a/pkg/protoc/proto_compile.go +++ b/pkg/protoc/proto_compile.go @@ -14,8 +14,19 @@ import ( ) const ( - // ProtoLibraryKey stores the ProtoLibrary implementation for a rule. + // ProtoLibraryKey stores the ProtoLibrary implementation for a rule. When a + // proto_compile / proto_compiled_sources rule merges multiple proto_libraries + // into its `protos` attribute, this key still references only the first + // library — preserved for compatibility with callers that index a single + // ProtoLibrary. See MergedProtoLibrariesKey for the full set. ProtoLibraryKey = "_proto_library" + // MergedProtoLibrariesKey stores the full []ProtoLibrary backing a merged + // proto_compile / proto_compiled_sources rule. Always contains at least one + // library; equal to []{ProtoLibraryKey} when no merge occurred. Plugin option + // resolvers must consult this — relying on ProtoLibraryKey alone misses the + // post-merge libraries and lets their .proto files leak into the dep-walk's + // "external" classification. + MergedProtoLibrariesKey = "_merged_proto_libraries" ) func init() { @@ -132,6 +143,11 @@ func (s *protoCompileRule) Rule(otherGen ...*rule.Rule) *rule.Rule { existingProtos = append(existingProtos, s.config.Library.Name()) other.SetAttr("protos", DeduplicateAndSort(existingProtos)) + // Track the full backing-library set for downstream plugin option + // resolution. See MergedProtoLibrariesKey docs. + existingMerged, _ := other.PrivateAttr(MergedProtoLibrariesKey).([]ProtoLibrary) + other.SetPrivateAttr(MergedProtoLibrariesKey, append(existingMerged, s.config.Library)) + // Merge plugins otherPlugins := other.AttrStrings("plugins") otherPlugins = append(otherPlugins, GetPluginLabels(s.config.Plugins)...) @@ -162,6 +178,7 @@ func (s *protoCompileRule) Rule(otherGen ...*rule.Rule) *rule.Rule { newRule.SetAttr(s.outputsAttrName, outputs) newRule.SetAttr("plugins", GetPluginLabels(s.config.Plugins)) newRule.SetAttr("proto", s.config.Library.Name()) + newRule.SetPrivateAttr(MergedProtoLibrariesKey, []ProtoLibrary{s.config.Library}) if s.config.LanguageConfig.Protoc != "" { newRule.SetAttr("protoc", s.config.LanguageConfig.Protoc) diff --git a/pkg/protoc/rust_keywords.go b/pkg/protoc/rust_keywords.go index f6202952f..1a6f2a24a 100644 --- a/pkg/protoc/rust_keywords.go +++ b/pkg/protoc/rust_keywords.go @@ -109,14 +109,15 @@ func RustKeywordEscapeMappings(pkg string, outputs []string) map[string]string { } // RustCrateName returns the canonical Rust crate name for a proto package. -// The proto package's dots are replaced with underscores and an "_rs" suffix -// is appended so the resulting identifier is unambiguously a Rust target, -// not a proto_library (e.g. "trumid.common.utils.state.snapshot.proto" → -// "trumid_common_utils_state_snapshot_proto_rs"). Returns the empty string -// for an empty input. +// The proto package's dots are replaced with underscores; the crate name IS +// the namespace, so no language suffix is appended (e.g. +// "trumid.common.utils.state.snapshot.proto" → +// "trumid_common_utils_state_snapshot_proto"). The proto_rust_library macro +// is expected to expose all generated types at the crate root, matching this +// flat convention. Returns the empty string for an empty input. func RustCrateName(protoPackage string) string { if protoPackage == "" { return "" } - return strings.ReplaceAll(protoPackage, ".", "_") + "_rs" + return strings.ReplaceAll(protoPackage, ".", "_") } diff --git a/pkg/protoc/rust_keywords_test.go b/pkg/protoc/rust_keywords_test.go index 8b3b396c4..1c06328e2 100644 --- a/pkg/protoc/rust_keywords_test.go +++ b/pkg/protoc/rust_keywords_test.go @@ -85,11 +85,11 @@ func TestRustCrateName(t *testing.T) { want string }{ "empty": {pkg: "", want: ""}, - "single segment": {pkg: "foo", want: "foo_rs"}, - "trailing proto": {pkg: "trumid.common.utils.state.snapshot.proto", want: "trumid_common_utils_state_snapshot_proto_rs"}, + "single segment": {pkg: "foo", want: "foo"}, + "trailing proto": {pkg: "trumid.common.utils.state.snapshot.proto", want: "trumid_common_utils_state_snapshot_proto"}, "sub-package": {pkg: "trumid.common.utils.state.snapshot.proto.example", - want: "trumid_common_utils_state_snapshot_proto_example_rs"}, - "keywords are not escaped here": {pkg: "google.type", want: "google_type_rs"}, + want: "trumid_common_utils_state_snapshot_proto_example"}, + "keywords are not escaped here": {pkg: "google.type", want: "google_type"}, } { t.Run(name, func(t *testing.T) { if got := RustCrateName(tc.pkg); got != tc.want { diff --git a/pkg/rule/rules_rust/proto_rust_library_test.go b/pkg/rule/rules_rust/proto_rust_library_test.go index 0798992c9..520410889 100644 --- a/pkg/rule/rules_rust/proto_rust_library_test.go +++ b/pkg/rule/rules_rust/proto_rust_library_test.go @@ -51,9 +51,8 @@ func TestProtoRustLibraryRule(t *testing.T) { }, want: ` proto_rust_library( - name = "google_api_rs", + name = "google_api", srcs = ["google.api.rs"], - pkg = "google.api", deps = [ "@crates//:pbjson", "@crates//:prost", @@ -77,12 +76,11 @@ proto_rust_library( }, want: ` proto_rust_library( - name = "trumid_common_proto_rs", + name = "trumid_common_proto", srcs = [ "trumid.common.proto.rs", "trumid.common.proto.serde.rs", ], - pkg = "trumid.common.proto", deps = [ "@crates//:pbjson", "@crates//:prost", @@ -107,9 +105,8 @@ proto_rust_library( }, want: ` proto_rust_library( - name = "example_wkt_rs", + name = "example_wkt", srcs = ["example.wkt.rs"], - pkg = "example.wkt", deps = [ "@crates//:pbjson", "@crates//:prost", @@ -134,12 +131,11 @@ proto_rust_library( }, want: ` proto_rust_library( - name = "example_grpc_rs", + name = "example_grpc", srcs = [ "example.grpc.rs", "example.grpc.tonic.rs", ], - pkg = "example.grpc", deps = [ "@crates//:pbjson", "@crates//:prost", diff --git a/pkg/rule/rules_rust/rust_library.go b/pkg/rule/rules_rust/rust_library.go index 6335b73db..3cc41239a 100644 --- a/pkg/rule/rules_rust/rust_library.go +++ b/pkg/rule/rules_rust/rust_library.go @@ -51,7 +51,10 @@ func (s *RustLibrary) Name() string { return s.Config.Library.BaseName() + s.RuleNameSuffix } -// Pkg returns the proto package name from the first file in the library. +// Pkg returns the proto package name of the first file in the library, or "". +// Internal helper: drives the rule's crate name (via RustCrateName) and the +// reexport computation. Not propagated to the generated rule as an attribute — +// the consuming Starlark rule no longer accepts a `pkg` attr. func (s *RustLibrary) Pkg() string { files := s.Config.Library.Files() if len(files) == 0 { @@ -156,9 +159,6 @@ func (s *RustLibrary) Rule(otherGen ...*rule.Rule) *rule.Rule { newRule.SetPrivateAttr(config.GazelleImportsKey, imports) s.protoLibrariesByRule[s.id] = []protoc.ProtoLibrary{s.Config.Library} - if pkg := s.Pkg(); pkg != "" { - newRule.SetAttr("pkg", pkg) - } if len(deps) > 0 { newRule.SetAttr("deps", deps) } diff --git a/rules/proto_compile.bzl b/rules/proto_compile.bzl index 793341ed4..675bbd232 100644 --- a/rules/proto_compile.bzl +++ b/rules/proto_compile.bzl @@ -195,17 +195,15 @@ def _proto_compile_impl(ctx): for plugin in plugins: ### Part 2.1: build protos list - # When using protos (plural), all ProtoInfo providers share the - # same package (that's why their outputs overlap). Only pass files - # from the first provider to protoc as file_to_generate — the - # descriptor sets from ALL providers are already included, giving - # the plugin full type information to generate the complete - # package output. This avoids duplicate CodeGeneratorResponse.File - # entries from package-level plugins like protoc-gen-prost. - gen_infos = [proto_infos[0]] if len(proto_infos) > 1 else proto_infos - - # add all protos unless excluded - for pi in gen_infos: + # All ProtoInfo providers (either the single `proto` attr or every + # entry in `protos`) contribute their direct_sources to protoc's + # file_to_generate list. protoc plugins emit code only for files + # explicitly listed there, so dropping providers here would silently + # truncate the generated output for package-level plugins like + # protoc-gen-prost (which generates per-file, not per-package). + # The inner `proto in protos` check deduplicates if a .proto somehow + # appears in multiple providers' direct_sources. + for pi in proto_infos: for proto in pi.direct_sources: if any([ proto.dirname.endswith(exclusion) or proto.path.endswith(exclusion)