diff --git a/.golangci.yml b/.golangci.yml index c5977d6182..5b44a56eb7 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -233,3 +233,8 @@ issues: - paralleltest text: "missing the call to method parallel" path: private/pkg/git/repository_test.go + # This greatly simplifies creation of descriptors, and it's safe enough since + # it's just test code. + - linters: + - forcetypeassert + path: private/bufpkg/bufimage/source_retention_options_test\.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 156a8de6fe..6337178b40 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,12 @@ ## [Unreleased] -- No changes yet. +- Update `buf generate` so it populates the recently-added + [`source_file_descriptors`](https://github.com/protocolbuffers/protobuf/blob/v24.0/src/google/protobuf/compiler/plugin.proto#L96-L99) + field of the `CodeGeneratorRequest` message. This provides the plugin with access to options + that are configured to only be retained in source and not at runtime (via + [field option](https://github.com/protocolbuffers/protobuf/blob/v24.0/src/google/protobuf/descriptor.proto#L693-L702)). + Descriptors in the `proto_file` field will not include any options configured this way. ## [v1.29.0] - 2024-01-24 diff --git a/go.mod b/go.mod index 83eafdf882..4f0ad77798 100644 --- a/go.mod +++ b/go.mod @@ -6,7 +6,7 @@ require ( buf.build/gen/go/bufbuild/protovalidate/protocolbuffers/go v1.32.0-20240221180331-f05a6f4403ce.1 connectrpc.com/connect v1.15.0 connectrpc.com/otelconnect v0.7.0 - github.com/bufbuild/protocompile v0.8.0 + github.com/bufbuild/protocompile v0.9.0 github.com/bufbuild/protovalidate-go v0.6.0 github.com/bufbuild/protoyaml-go v0.1.8 github.com/docker/docker v25.0.3+incompatible diff --git a/go.sum b/go.sum index 13dc24630b..e471e83ca4 100644 --- a/go.sum +++ b/go.sum @@ -10,8 +10,8 @@ github.com/Microsoft/go-winio v0.6.1 h1:9/kr64B9VUZrLm5YYwbGtUJnMgqWVOdUAXu6Migc github.com/Microsoft/go-winio v0.6.1/go.mod h1:LRdKpFKfdobln8UmuiYcKPot9D2v6svN5+sAH+4kjUM= github.com/antlr4-go/antlr/v4 v4.13.0 h1:lxCg3LAv+EUK6t1i0y1V6/SLeUi0eKEKdhQAlS8TVTI= github.com/antlr4-go/antlr/v4 v4.13.0/go.mod h1:pfChB/xh/Unjila75QW7+VU4TSnWnnk9UTnmpPaOR2g= -github.com/bufbuild/protocompile v0.8.0 h1:9Kp1q6OkS9L4nM3FYbr8vlJnEwtbpDPQlQOVXfR+78s= -github.com/bufbuild/protocompile v0.8.0/go.mod h1:+Etjg4guZoAqzVk2czwEQP12yaxLJ8DxuqCJ9qHdH94= +github.com/bufbuild/protocompile v0.9.0 h1:DI8qLG5PEO0Mu1Oj51YFPqtx6I3qYXUAhJVJ/IzAVl0= +github.com/bufbuild/protocompile v0.9.0/go.mod h1:s89m1O8CqSYpyE/YaSGtg1r1YFMF5nLTwh4vlj6O444= github.com/bufbuild/protovalidate-go v0.6.0 h1:Jgs1kFuZ2LHvvdj8SpCLA1W/+pXS8QSM3F/E2l3InPY= github.com/bufbuild/protovalidate-go v0.6.0/go.mod h1:1LamgoYHZ2NdIQH0XGczGTc6Z8YrTHjcJVmiBaar4t4= github.com/bufbuild/protoyaml-go v0.1.8 h1:X9QDLfl9uEllh4gsXUGqPanZYCOKzd92uniRtW2OnAQ= diff --git a/private/buf/bufgen/generator.go b/private/buf/bufgen/generator.go index d8c6f0f4ac..57df1a65fb 100644 --- a/private/buf/bufgen/generator.go +++ b/private/buf/bufgen/generator.go @@ -272,22 +272,23 @@ func (g *generator) execLocalPlugin( if err != nil { return nil, err } - generateOptions := []bufpluginexec.GenerateOption{ - bufpluginexec.GenerateWithPluginPath(pluginConfig.Path...), - bufpluginexec.GenerateWithProtocPath(pluginConfig.ProtocPath), + requests, err := bufimage.ImagesToCodeGeneratorRequests( + pluginImages, + pluginConfig.Opt, + nil, + includeImports, + includeWellKnownTypes, + ) + if err != nil { + return nil, err } response, err := g.pluginexecGenerator.Generate( ctx, container, pluginConfig.PluginName(), - bufimage.ImagesToCodeGeneratorRequests( - pluginImages, - pluginConfig.Opt, - nil, - includeImports, - includeWellKnownTypes, - ), - generateOptions..., + requests, + bufpluginexec.GenerateWithPluginPath(pluginConfig.Path...), + bufpluginexec.GenerateWithProtocPath(pluginConfig.ProtocPath), ) if err != nil { return nil, fmt.Errorf("plugin %s: %v", pluginConfig.PluginName(), err) diff --git a/private/buf/cmd/buf/command/alpha/protoc/plugin.go b/private/buf/cmd/buf/command/alpha/protoc/plugin.go index b372094aa9..9bc3890379 100644 --- a/private/buf/cmd/buf/command/alpha/protoc/plugin.go +++ b/private/buf/cmd/buf/command/alpha/protoc/plugin.go @@ -56,6 +56,16 @@ func executePlugin( storageosProvider, runner, ) + requests, err := bufimage.ImagesToCodeGeneratorRequests( + images, + strings.Join(pluginInfo.Opt, ","), + bufpluginexec.DefaultVersion, + false, + false, + ) + if err != nil { + return nil, err + } var options []bufpluginexec.GenerateOption if pluginInfo.Path != "" { options = append(options, bufpluginexec.GenerateWithPluginPath(pluginInfo.Path)) @@ -64,13 +74,7 @@ func executePlugin( ctx, container, pluginName, - bufimage.ImagesToCodeGeneratorRequests( - images, - strings.Join(pluginInfo.Opt, ","), - bufpluginexec.DefaultVersion, - false, - false, - ), + requests, options..., ) if err != nil { diff --git a/private/buf/cmd/protoc-gen-buf-lint/lint_test.go b/private/buf/cmd/protoc-gen-buf-lint/lint_test.go index 5b48fc2e87..a9b3d23140 100644 --- a/private/buf/cmd/protoc-gen-buf-lint/lint_test.go +++ b/private/buf/cmd/protoc-gen-buf-lint/lint_test.go @@ -342,5 +342,7 @@ func testBuildCodeGeneratorRequest( } image, err := bufimage.NewImage(imageFiles) require.NoError(t, err) - return bufimage.ImageToCodeGeneratorRequest(image, parameter, nil, false, false) + codeGenReq, err := bufimage.ImageToCodeGeneratorRequest(image, parameter, nil, false, false) + require.NoError(t, err) + return codeGenReq } diff --git a/private/bufpkg/bufimage/bufimage.go b/private/bufpkg/bufimage/bufimage.go index eac7619c94..c9d96ef478 100644 --- a/private/bufpkg/bufimage/bufimage.go +++ b/private/bufpkg/bufimage/bufimage.go @@ -440,7 +440,7 @@ func ImageToCodeGeneratorRequest( compilerVersion *pluginpb.Version, includeImports bool, includeWellKnownTypes bool, -) *pluginpb.CodeGeneratorRequest { +) (*pluginpb.CodeGeneratorRequest, error) { return imageToCodeGeneratorRequest( image, parameter, @@ -465,7 +465,7 @@ func ImagesToCodeGeneratorRequests( compilerVersion *pluginpb.Version, includeImports bool, includeWellKnownTypes bool, -) []*pluginpb.CodeGeneratorRequest { +) ([]*pluginpb.CodeGeneratorRequest, error) { requests := make([]*pluginpb.CodeGeneratorRequest, len(images)) // alreadyUsedPaths is a map of paths that have already been added to an image. // @@ -499,7 +499,8 @@ func ImagesToCodeGeneratorRequests( } } for i, image := range images { - requests[i] = imageToCodeGeneratorRequest( + var err error + requests[i], err = imageToCodeGeneratorRequest( image, parameter, compilerVersion, @@ -508,8 +509,11 @@ func ImagesToCodeGeneratorRequests( alreadyUsedPaths, nonImportPaths, ) + if err != nil { + return nil, err + } } - return requests + return requests, nil } // ImageModuleDependency is a dependency of an image. diff --git a/private/bufpkg/bufimage/bufimagetesting/bufimagetesting_test.go b/private/bufpkg/bufimage/bufimagetesting/bufimagetesting_test.go index b4e02999d0..9345f19882 100644 --- a/private/bufpkg/bufimage/bufimagetesting/bufimagetesting_test.go +++ b/private/bufpkg/bufimage/bufimagetesting/bufimagetesting_test.go @@ -454,7 +454,7 @@ func TestBasic(t *testing.T) { newImage.Files(), ) diff := cmp.Diff(protoImage, bufimage.ImageToProtoImage(newImage), protocmp.Transform()) - require.Equal(t, "", diff) + require.Empty(t, diff) fileDescriptorSet := &descriptorpb.FileDescriptorSet{ File: []*descriptorpb.FileDescriptorProto{ testProtoImageFileToFileDescriptorProto(protoImageFileAA), @@ -467,7 +467,7 @@ func TestBasic(t *testing.T) { }, } diff = cmp.Diff(fileDescriptorSet, bufimage.ImageToFileDescriptorSet(image), protocmp.Transform()) - require.Equal(t, "", diff) + require.Empty(t, diff) codeGeneratorRequest := &pluginpb.CodeGeneratorRequest{ ProtoFile: []*descriptorpb.FileDescriptorProto{ testProtoImageFileToFileDescriptorProto(protoImageFileAA), @@ -486,21 +486,32 @@ func TestBasic(t *testing.T) { "b/b.proto", "d/d.proto/d.proto", }, + SourceFileDescriptors: []*descriptorpb.FileDescriptorProto{ + testProtoImageFileToFileDescriptorProto(protoImageFileAA), + testProtoImageFileToFileDescriptorProto(protoImageFileAB), + testProtoImageFileToFileDescriptorProto(protoImageFileBA), + testProtoImageFileToFileDescriptorProto(protoImageFileBB), + testProtoImageFileToFileDescriptorProto(protoImageFileOutlandishDirectoryName), + }, } + actualRequest, err := bufimage.ImageToCodeGeneratorRequest(image, "foo", nil, false, false) + require.NoError(t, err) diff = cmp.Diff( codeGeneratorRequest, - bufimage.ImageToCodeGeneratorRequest(image, "foo", nil, false, false), + actualRequest, protocmp.Transform(), ) - require.Equal(t, "", diff) + require.Empty(t, diff) // verify that includeWellKnownTypes is a no-op if includeImports is false + actualRequest, err = bufimage.ImageToCodeGeneratorRequest(image, "foo", nil, false, true) + require.NoError(t, err) diff = cmp.Diff( codeGeneratorRequest, - bufimage.ImageToCodeGeneratorRequest(image, "foo", nil, false, true), + actualRequest, protocmp.Transform(), ) - require.Equal(t, "", diff) + require.Empty(t, diff) codeGeneratorRequestIncludeImports := &pluginpb.CodeGeneratorRequest{ ProtoFile: []*descriptorpb.FileDescriptorProto{ @@ -522,13 +533,23 @@ func TestBasic(t *testing.T) { "b/b.proto", "d/d.proto/d.proto", }, + SourceFileDescriptors: []*descriptorpb.FileDescriptorProto{ + testProtoImageFileToFileDescriptorProto(protoImageFileAA), + testProtoImageFileToFileDescriptorProto(protoImageFileImport), + testProtoImageFileToFileDescriptorProto(protoImageFileAB), + testProtoImageFileToFileDescriptorProto(protoImageFileBA), + testProtoImageFileToFileDescriptorProto(protoImageFileBB), + testProtoImageFileToFileDescriptorProto(protoImageFileOutlandishDirectoryName), + }, } + actualRequest, err = bufimage.ImageToCodeGeneratorRequest(image, "foo", nil, true, false) + require.NoError(t, err) diff = cmp.Diff( codeGeneratorRequestIncludeImports, - bufimage.ImageToCodeGeneratorRequest(image, "foo", nil, true, false), + actualRequest, protocmp.Transform(), ) - require.Equal(t, "", diff) + require.Empty(t, diff) newImage, err = bufimage.NewImageForCodeGeneratorRequest(codeGeneratorRequest) require.NoError(t, err) AssertImageFilesEqual( @@ -564,13 +585,24 @@ func TestBasic(t *testing.T) { "b/b.proto", "d/d.proto/d.proto", }, + SourceFileDescriptors: []*descriptorpb.FileDescriptorProto{ + testProtoImageFileToFileDescriptorProto(protoImageFileAA), + testProtoImageFileToFileDescriptorProto(protoImageFileImport), + testProtoImageFileToFileDescriptorProto(protoImageFileWellKnownTypeImport), + testProtoImageFileToFileDescriptorProto(protoImageFileAB), + testProtoImageFileToFileDescriptorProto(protoImageFileBA), + testProtoImageFileToFileDescriptorProto(protoImageFileBB), + testProtoImageFileToFileDescriptorProto(protoImageFileOutlandishDirectoryName), + }, } + actualRequest, err = bufimage.ImageToCodeGeneratorRequest(image, "foo", nil, true, true) + require.NoError(t, err) diff = cmp.Diff( codeGeneratorRequestIncludeImportsAndWellKnownTypes, - bufimage.ImageToCodeGeneratorRequest(image, "foo", nil, true, true), + actualRequest, protocmp.Transform(), ) - require.Equal(t, "", diff) + require.Empty(t, diff) // imagesByDir and multiple Image tests imagesByDir, err := bufimage.ImageByDir(image) require.NoError(t, err) @@ -618,6 +650,10 @@ func TestBasic(t *testing.T) { "a/a.proto", "a/b.proto", }, + SourceFileDescriptors: []*descriptorpb.FileDescriptorProto{ + testProtoImageFileToFileDescriptorProto(protoImageFileAA), + testProtoImageFileToFileDescriptorProto(protoImageFileAB), + }, }, { ProtoFile: []*descriptorpb.FileDescriptorProto{ @@ -633,6 +669,10 @@ func TestBasic(t *testing.T) { "b/a.proto", "b/b.proto", }, + SourceFileDescriptors: []*descriptorpb.FileDescriptorProto{ + testProtoImageFileToFileDescriptorProto(protoImageFileBA), + testProtoImageFileToFileDescriptorProto(protoImageFileBB), + }, }, { ProtoFile: []*descriptorpb.FileDescriptorProto{ @@ -643,13 +683,17 @@ func TestBasic(t *testing.T) { FileToGenerate: []string{ "d/d.proto/d.proto", }, + SourceFileDescriptors: []*descriptorpb.FileDescriptorProto{ + testProtoImageFileToFileDescriptorProto(protoImageFileOutlandishDirectoryName), + }, }, } - requestsFromImages := bufimage.ImagesToCodeGeneratorRequests(imagesByDir, "foo", nil, false, false) + requestsFromImages, err := bufimage.ImagesToCodeGeneratorRequests(imagesByDir, "foo", nil, false, false) + require.NoError(t, err) require.Equal(t, len(codeGeneratorRequests), len(requestsFromImages)) for i := range codeGeneratorRequests { diff = cmp.Diff(codeGeneratorRequests[i], requestsFromImages[i], protocmp.Transform()) - require.Equal(t, "", diff) + require.Empty(t, diff) } codeGeneratorRequestsIncludeImports := []*pluginpb.CodeGeneratorRequest{ { @@ -665,6 +709,11 @@ func TestBasic(t *testing.T) { "import.proto", "a/b.proto", }, + SourceFileDescriptors: []*descriptorpb.FileDescriptorProto{ + testProtoImageFileToFileDescriptorProto(protoImageFileAA), + testProtoImageFileToFileDescriptorProto(protoImageFileImport), + testProtoImageFileToFileDescriptorProto(protoImageFileAB), + }, }, { ProtoFile: []*descriptorpb.FileDescriptorProto{ @@ -680,6 +729,10 @@ func TestBasic(t *testing.T) { "b/a.proto", "b/b.proto", }, + SourceFileDescriptors: []*descriptorpb.FileDescriptorProto{ + testProtoImageFileToFileDescriptorProto(protoImageFileBA), + testProtoImageFileToFileDescriptorProto(protoImageFileBB), + }, }, { ProtoFile: []*descriptorpb.FileDescriptorProto{ @@ -690,13 +743,17 @@ func TestBasic(t *testing.T) { FileToGenerate: []string{ "d/d.proto/d.proto", }, + SourceFileDescriptors: []*descriptorpb.FileDescriptorProto{ + testProtoImageFileToFileDescriptorProto(protoImageFileOutlandishDirectoryName), + }, }, } - requestsFromImages = bufimage.ImagesToCodeGeneratorRequests(imagesByDir, "foo", nil, true, false) + requestsFromImages, err = bufimage.ImagesToCodeGeneratorRequests(imagesByDir, "foo", nil, true, false) + require.NoError(t, err) require.Equal(t, len(codeGeneratorRequestsIncludeImports), len(requestsFromImages)) for i := range codeGeneratorRequestsIncludeImports { diff = cmp.Diff(codeGeneratorRequestsIncludeImports[i], requestsFromImages[i], protocmp.Transform()) - require.Equal(t, "", diff) + require.Empty(t, diff) } } diff --git a/private/bufpkg/bufimage/util.go b/private/bufpkg/bufimage/util.go index baa78eb13e..61e6047dc6 100644 --- a/private/bufpkg/bufimage/util.go +++ b/private/bufpkg/bufimage/util.go @@ -24,6 +24,7 @@ import ( imagev1 "github.com/bufbuild/buf/private/gen/proto/go/buf/alpha/image/v1" "github.com/bufbuild/buf/private/pkg/normalpath" "github.com/bufbuild/buf/private/pkg/slicesext" + "github.com/bufbuild/protocompile/options" "google.golang.org/protobuf/encoding/protowire" "google.golang.org/protobuf/proto" "google.golang.org/protobuf/reflect/protoreflect" @@ -414,7 +415,7 @@ func imageToCodeGeneratorRequest( includeWellKnownTypes bool, alreadyUsedPaths map[string]struct{}, nonImportPaths map[string]struct{}, -) *pluginpb.CodeGeneratorRequest { +) (*pluginpb.CodeGeneratorRequest, error) { imageFiles := image.Files() request := &pluginpb.CodeGeneratorRequest{ ProtoFile: make([]*descriptorpb.FileDescriptorProto, len(imageFiles)), @@ -424,7 +425,8 @@ func imageToCodeGeneratorRequest( request.Parameter = proto.String(parameter) } for i, imageFile := range imageFiles { - request.ProtoFile[i] = imageFile.FileDescriptorProto() + fileDescriptorProto := imageFile.FileDescriptorProto() + // ProtoFile should include only runtime-retained options for files to generate. if isFileToGenerate( imageFile, alreadyUsedPaths, @@ -433,9 +435,18 @@ func imageToCodeGeneratorRequest( includeWellKnownTypes, ) { request.FileToGenerate = append(request.FileToGenerate, imageFile.Path()) + // Source-retention options for items in FileToGenerate are provided in SourceFileDescriptors. + request.SourceFileDescriptors = append(request.SourceFileDescriptors, fileDescriptorProto) + // And the corresponding descriptor in ProtoFile will have source-retention options stripped. + var err error + fileDescriptorProto, err = options.StripSourceRetentionOptionsFromFile(fileDescriptorProto) + if err != nil { + return nil, fmt.Errorf("failed to strip source-retention options for file %q when constructing a CodeGeneratorRequest: %w", imageFile.Path(), err) + } } + request.ProtoFile[i] = fileDescriptorProto } - return request + return request, nil } func isFileToGenerate( diff --git a/private/pkg/protodescriptor/protodescriptor.go b/private/pkg/protodescriptor/protodescriptor.go index 4aac104b9a..1c69c8c8be 100644 --- a/private/pkg/protodescriptor/protodescriptor.go +++ b/private/pkg/protodescriptor/protodescriptor.go @@ -138,9 +138,14 @@ func ValidateCodeGeneratorRequest(request *pluginpb.CodeGeneratorRequest) error if err := ValidateCodeGeneratorRequestExceptFileDescriptorProtos(request); err != nil { return err } - for _, fileDescriptorProto := range request.ProtoFile { + for i, fileDescriptorProto := range request.ProtoFile { if err := ValidateFileDescriptor(fileDescriptorProto); err != nil { - return err + return fmt.Errorf("CodeGeneratorRequest.ProtoFile[%d]: %w", i, err) + } + } + for i, fileDescriptorProto := range request.SourceFileDescriptors { + if err := ValidateFileDescriptor(fileDescriptorProto); err != nil { + return fmt.Errorf("CodeGeneratorRequest.SourceFileDescriptors[%d]: %w", i, err) } } return nil @@ -161,6 +166,9 @@ func ValidateCodeGeneratorRequestExceptFileDescriptorProtos(request *pluginpb.Co if err := ValidateProtoPaths("CodeGeneratorRequest.FileToGenerate", request.FileToGenerate); err != nil { return err } + // TODO: validate that there are no duplicates in file_to_generate, proto_file, and source_file_descriptors + // validate that proto_file is in topological order and contains everything from file_to_generate + // validate that source_file_descriptors, if present, matches file_to_generate return nil }