From c5ab3c144f3d5e681356b3dc3d8fa3bc9f521813 Mon Sep 17 00:00:00 2001 From: Alex McKinney Date: Fri, 24 Sep 2021 18:22:05 -0700 Subject: [PATCH 01/14] Simplify 'buf generate' --- go.mod | 1 - go.sum | 1 - private/buf/bufgen/bufgen.go | 21 +- private/buf/bufgen/generator.go | 521 +++++++----------- private/buf/cmd/buf/buf_test.go | 85 +++ private/buf/cmd/buf/command/protoc/plugin.go | 16 +- private/buf/cmd/buf/command/protoc/protoc.go | 31 +- .../generate/insertion_point/test.txt | 15 + private/bufpkg/bufplugin/bufplugin.go | 98 ---- private/bufpkg/bufplugin/bufplugin_test.go | 40 -- private/pkg/app/appproto/appproto.go | 156 +++--- private/pkg/app/appproto/appproto_test.go | 14 +- .../pkg/app/appproto/appprotoos/appprotoos.go | 48 +- .../appprotoos/bucket_response_writer.go | 269 +++++++++ .../pkg/app/appproto/appprotoos/generator.go | 123 +---- private/pkg/app/appproto/generator.go | 71 +-- private/pkg/app/appproto/response_handler.go | 154 ++++++ private/pkg/app/appproto/response_writer.go | 4 +- private/pkg/thread/thread.go | 17 +- 19 files changed, 927 insertions(+), 758 deletions(-) create mode 100644 private/buf/cmd/buf/testdata/generate/insertion_point/test.txt create mode 100644 private/pkg/app/appproto/appprotoos/bucket_response_writer.go create mode 100644 private/pkg/app/appproto/response_handler.go diff --git a/go.mod b/go.mod index de273664ec..35d0a032ec 100644 --- a/go.mod +++ b/go.mod @@ -19,7 +19,6 @@ require ( go.uber.org/multierr v1.7.0 go.uber.org/zap v1.19.1 golang.org/x/net v0.0.0-20210929161516-d455829e376d // indirect - golang.org/x/sync v0.0.0-20210220032951-036812b2e83c golang.org/x/sys v0.0.0-20210927094055-39ccf1dd6fa6 // indirect golang.org/x/term v0.0.0-20210927222741-03fcf44c2211 golang.org/x/text v0.3.7 // indirect diff --git a/go.sum b/go.sum index e07204974f..b9d907d5c9 100644 --- a/go.sum +++ b/go.sum @@ -383,7 +383,6 @@ golang.org/x/sync v0.0.0-20200317015054-43a5402ce75a/go.mod h1:RxMgew5VJxzue5/jJ golang.org/x/sync v0.0.0-20200625203802-6e8e738ad208/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20201020160332-67f06af15bc9/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20201207232520-09787c993a3a/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= -golang.org/x/sync v0.0.0-20210220032951-036812b2e83c h1:5KslGYwFpkhGh+Q16bwMP3cOontH8FOep7tGV86Y7SQ= golang.org/x/sync v0.0.0-20210220032951-036812b2e83c/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sys v0.0.0-20180823144017-11551d06cbcc/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20180830151530-49385e6e1522/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= diff --git a/private/buf/bufgen/bufgen.go b/private/buf/bufgen/bufgen.go index 0bba382a57..2e937a1124 100644 --- a/private/buf/bufgen/bufgen.go +++ b/private/buf/bufgen/bufgen.go @@ -113,7 +113,11 @@ func NewGenerator( storageosProvider storageos.Provider, registryProvider registryv1alpha1apiclient.Provider, ) Generator { - return newGenerator(logger, storageosProvider, registryProvider) + return newGenerator( + logger, + storageosProvider, + registryProvider, + ) } // GenerateOption is an option for Generate. @@ -161,6 +165,21 @@ type PluginConfig struct { Strategy Strategy } +// PluginName returns this PluginConfig's plugin name. +// Only one of Name or Remote will be set. +func (p *PluginConfig) PluginName() string { + if p == nil { + return "" + } + if p.Name != "" { + return p.Name + } + if p.Remote != "" { + return p.Remote + } + return "" +} + // ManagedConfig is the Managed Mode configuration. type ManagedConfig struct { CcEnableArenas *bool diff --git a/private/buf/bufgen/generator.go b/private/buf/bufgen/generator.go index 6678fc94d1..c34c78702f 100644 --- a/private/buf/bufgen/generator.go +++ b/private/buf/bufgen/generator.go @@ -17,7 +17,6 @@ package bufgen import ( "context" "fmt" - "os" "path/filepath" "github.com/bufbuild/buf/private/bufpkg/bufimage" @@ -29,20 +28,18 @@ import ( "github.com/bufbuild/buf/private/pkg/app/appproto" "github.com/bufbuild/buf/private/pkg/app/appproto/appprotoexec" "github.com/bufbuild/buf/private/pkg/app/appproto/appprotoos" - "github.com/bufbuild/buf/private/pkg/storage" - "github.com/bufbuild/buf/private/pkg/storage/storagearchive" - "github.com/bufbuild/buf/private/pkg/storage/storagemem" "github.com/bufbuild/buf/private/pkg/storage/storageos" "github.com/bufbuild/buf/private/pkg/thread" - "go.uber.org/multierr" "go.uber.org/zap" - "golang.org/x/sync/errgroup" + "google.golang.org/protobuf/types/pluginpb" ) type generator struct { - logger *zap.Logger - storageosProvider storageos.Provider - registryProvider registryv1alpha1apiclient.Provider + logger *zap.Logger + storageosProvider storageos.Provider + appprotoosGenerator appprotoos.Generator + responseHandler appproto.ResponseHandler + registryProvider registryv1alpha1apiclient.Provider } func newGenerator( @@ -51,9 +48,11 @@ func newGenerator( registryProvider registryv1alpha1apiclient.Provider, ) *generator { return &generator{ - logger: logger, - storageosProvider: storageosProvider, - registryProvider: registryProvider, + logger: logger, + storageosProvider: storageosProvider, + appprotoosGenerator: appprotoos.NewGenerator(logger, storageosProvider), + responseHandler: appproto.NewResponseHandler(logger), + registryProvider: registryProvider, } } @@ -78,6 +77,10 @@ func (g *generator) Generate( ) } +// generate executes all of the plugins specified by the given Config, and +// consolidates the results in the same order that the plugins are listed. +// Order is particularly important for insertion points, which are used to +// modify the generted output from other plugins executed earlier in the chain. func (g *generator) generate( ctx context.Context, container app.EnvStdioContainer, @@ -89,9 +92,10 @@ func (g *generator) generate( if err := modifyImage(ctx, g.logger, config, image); err != nil { return err } - pluginResponses, err := g.generateConcurrently( + responses, err := g.execPlugins( ctx, container, + g.appprotoosGenerator, config, image, includeImports, @@ -99,63 +103,43 @@ func (g *generator) generate( if err != nil { return err } - if len(pluginResponses) != len(config.PluginConfigs) { - return fmt.Errorf("unexpected number of responses, got %d, wanted %d", len(pluginResponses), len(config.PluginConfigs)) - } - mergedPluginResults, err := bufplugin.MergeInsertionPoints(pluginResponses) - if err != nil { - return err - } - if len(mergedPluginResults) != len(config.PluginConfigs) { - return fmt.Errorf("unexpected number of merged results, got %d, wanted %d", len(mergedPluginResults), len(config.PluginConfigs)) - } - for i, mergedPluginResult := range mergedPluginResults { - pluginOut := config.PluginConfigs[i].Out + // Apply the CodeGeneratorResponses in the order they were specified. + bucketResponseWriter := appprotoos.NewBucketResponseWriter( + g.logger, + g.storageosProvider, + appprotoos.BucketResponseWriterWithCreateOutDirIfNotExists(), + ) + for i, pluginConfig := range config.PluginConfigs { + out := pluginConfig.Out if baseOutDirPath != "" && baseOutDirPath != "." { - pluginOut = filepath.Join(baseOutDirPath, pluginOut) + out = filepath.Join(baseOutDirPath, out) + } + response := responses[i] + if response == nil { + return fmt.Errorf("failed to get plugin response for %s", pluginConfig.PluginName()) } - switch filepath.Ext(pluginOut) { - case ".jar": - if err := g.generateZip( - ctx, - pluginOut, - mergedPluginResult.Files, - true, - ); err != nil { - return err - } - case ".zip": - if err := g.generateZip( - ctx, - pluginOut, - mergedPluginResult.Files, - false, - ); err != nil { - return err - } - default: - if err := g.generateDirectory( - ctx, - pluginOut, - mergedPluginResult.Files, - ); err != nil { - return err - } + if err := bucketResponseWriter.AddResponse( + ctx, + response, + out, + ); err != nil { + return fmt.Errorf("plugin %s: %v", pluginConfig.PluginName(), err) } } + if err := bucketResponseWriter.Close(); err != nil { + return err + } return nil } -// generateConcurrently starts a separate goroutine for each per-image-directory -// invocation of local plugins, and single grouped invocation of remote plugins, -// to run concurrently, with different and independent concurrency restrictions. -func (g *generator) generateConcurrently( +func (g *generator) execPlugins( ctx context.Context, container app.EnvStdioContainer, + appprotoosGenerator appprotoos.Generator, config *Config, image bufimage.Image, includeImports bool, -) ([]*bufplugin.PluginResponse, error) { +) ([]*pluginpb.CodeGeneratorResponse, error) { // Cache imagesByDir up-front if at least one plugin requires it var imagesByDir []bufimage.Image for _, pluginConfig := range config.PluginConfigs { @@ -173,239 +157,148 @@ func (g *generator) generateConcurrently( } break } - remoteToPlugins := make(map[string][]*remotePlugin) - var localPluginResponses []*localPluginResponse - // Parallelize local and remote plugin execution with an errgroup. - // Limit concurrent execution of local plugins using semaphore. - localSemaphore := newSemaphore(thread.Parallelism()) - defer localSemaphore.Discard() - // Note: this new context will be cancelled after eg.Wait() returns. - eg, ctx := errgroup.WithContext(ctx) - // pluginResponses contains the response for each plugin, in the same order - // as they are specified in the plugin configs. We need to store each response - // for processing insertion points after all plugins have finished. - pluginResponses := make([]*bufplugin.PluginResponse, len(config.PluginConfigs)) + // Collect all of the plugin jobs so that they can be executed in parallel. + jobs := make([]func(context.Context) error, 0, len(config.PluginConfigs)) + responses := make([]*pluginpb.CodeGeneratorResponse, len(config.PluginConfigs)) for i, pluginConfig := range config.PluginConfigs { - switch { - case pluginConfig.Name != "": // Local plugin - var pluginImages []bufimage.Image - switch pluginConfig.Strategy { - case StrategyAll: - pluginImages = []bufimage.Image{image} - case StrategyDirectory: - pluginImages = imagesByDir - default: - return nil, fmt.Errorf("unknown strategy: %v", pluginConfig.Strategy) - } - var handlerOptions []appprotoexec.HandlerOption - if pluginConfig.Path != "" { - handlerOptions = append(handlerOptions, appprotoexec.HandlerWithPluginPath(pluginConfig.Path)) - } - handler, err := appprotoexec.NewHandler( - g.logger, - g.storageosProvider, - pluginConfig.Name, - handlerOptions..., - ) - if err != nil { - return nil, err - } - requests := bufimage.ImagesToCodeGeneratorRequests( - pluginImages, - pluginConfig.Opt, - appprotoexec.DefaultVersion, - includeImports, - ) - responseWriter := appproto.NewResponseWriter(container) - for _, request := range requests { - // prevent issues with asynchronously executed closures - request := request - // Queue up a parallel job for each split of the image, writing to the same response writer. - eg.Go(func() error { - // Limit concurrent invocations using semaphore channel, since - // most of this work is CPU/MEM intensive. - if err := localSemaphore.Acquire(ctx); err != nil { - return fmt.Errorf("failed to acquire semaphore: %w", err) - } - defer localSemaphore.Release(ctx) - if err := handler.Handle(ctx, container, responseWriter, request); err != nil { - return err - } - return nil - }) - } - // Responses are not valid until parallelized jobs have finished - localPluginResponses = append( - localPluginResponses, - newLocalPluginResponse(responseWriter, pluginConfig.Name, i), - ) - case pluginConfig.Remote != "": // Remote plugin - remote, owner, name, version, err := bufplugin.ParsePluginVersionPath(pluginConfig.Remote) - if err != nil { - return nil, fmt.Errorf("invalid plugin path: %w", err) - } - var parameters []string - if len(pluginConfig.Opt) > 0 { - // Only include parameters if they're not empty - parameters = []string{pluginConfig.Opt} - } - // Group remote plugins by remote to execute together. - remoteToPlugins[remote] = append( - remoteToPlugins[remote], - newRemotePlugin( - ®istryv1alpha1.PluginReference{ - Owner: owner, - Name: name, - Version: version, - Parameters: parameters, - }, - pluginConfig.Remote, - i, // So we know the order this plugins response should slot in. - ), - ) - default: - // Already guaranteed by config validation, but best to be safe. - return nil, fmt.Errorf("either remote or name must be specified") + index := i + currentPluginConfig := pluginConfig + if pluginConfig.Remote != "" { + jobs = append(jobs, func(ctx context.Context) error { + response, err := g.execRemotePlugin( + ctx, + container, + image, + currentPluginConfig, + // TODO: We need to support the includeImports option here. + ) + if err != nil { + return err + } + responses[index] = response + return nil + }) } - } - // Limit concurrent execution of remote plugins using separate semaphore channel. - // This time we can use a higher concurrency limit since the work is almost - // exclusively I/O bound. - remoteSemaphore := newSemaphore(thread.Parallelism() * 10) - defer remoteSemaphore.Discard() - for remote, plugins := range remoteToPlugins { - // prevent issues with asynchronously executed closures - remote := remote - plugins := plugins - // Add a job for each remote. - eg.Go(func() error { - if err := remoteSemaphore.Acquire(ctx); err != nil { - return fmt.Errorf("failed to acquire semaphore: %w", err) - } - defer remoteSemaphore.Release(ctx) - generateService, err := g.registryProvider.NewGenerateService(ctx, remote) - if err != nil { - return fmt.Errorf("failed to create generate service for remote %q: %w", remote, err) - } - references := make([]*registryv1alpha1.PluginReference, 0, len(plugins)) - for _, pluginOption := range plugins { - references = append(references, pluginOption.reference) - } - responses, _, err := generateService.GeneratePlugins(ctx, bufimage.ImageToProtoImage(image), references) - if err != nil { - return err - } - if len(responses) != len(references) { - return fmt.Errorf("unexpected number of responses received, got %d, wanted %d", len(responses), len(references)) - } - for i, response := range responses { - // Map each plugin response back to the right place. - // Note: does not require a lock, since each response is - // assigned to its own index in the slice. - pluginResponses[plugins[i].index] = bufplugin.NewPluginResponse( - response, - plugins[i].pluginRemote, + if pluginConfig.Name != "" { + jobs = append(jobs, func(ctx context.Context) error { + response, err := g.execLocalPlugin( + ctx, + container, + g.appprotoosGenerator, + image, + imagesByDir, + currentPluginConfig, + includeImports, ) - } - return nil - }) + if err != nil { + return err + } + responses[index] = response + return nil + }) + } } - if err := eg.Wait(); err != nil { + // We execute all of the jobs in parallel, but apply them in order so that any + // insertion points are handled correctly. + // + // For example, + // + // # buf.gen.yaml + // version: v1 + // plugins: + // - remote: buf.build/org/plugins/insertion-point-receiver + // out: gen/proto + // - name: insertion-point-writer + // out: gen/proto + ctx, cancel := context.WithCancel(ctx) + defer cancel() + if err := thread.Parallelize( + ctx, + jobs, + thread.ParallelizeWithCancel(cancel), + thread.ParallelizeWithFastFail(), // We want error messages to be concise here. + ); err != nil { return nil, err } - for _, localResponse := range localPluginResponses { - pluginResponses[localResponse.index] = bufplugin.NewPluginResponse( - localResponse.responseWriter.ToResponse(), - localResponse.pluginName, - ) - } - for i, response := range pluginResponses { - if response == nil { - // The size of pluginResponses and config.PluginConfigs are guaranteed - // to be the same, so it's safe to access the config.PluginConfigs with - // this index. However, we check the length again and return a separate - // error just in case the initialization changes above. - if len(pluginResponses) != len(config.PluginConfigs) { - return nil, fmt.Errorf("failed for response %d", i) - } - var pluginName string - if pluginConfig := config.PluginConfigs[i]; pluginConfig.Name != "" { - pluginName = pluginConfig.Name - } else { - pluginName = pluginConfig.Remote - } - return nil, fmt.Errorf("failed for plugin %s", pluginName) - } + if err := validateResponses(responses, config.PluginConfigs); err != nil { + return nil, err } - return pluginResponses, nil + return responses, nil } -func (g *generator) generateZip( +func (g *generator) execLocalPlugin( ctx context.Context, - outFilePath string, - files []*bufplugin.File, - includeManifest bool, -) (retErr error) { - outDirPath := filepath.Dir(outFilePath) - // OK to use os.Stat instead of os.Lstat here - fileInfo, err := os.Stat(outDirPath) + container app.EnvStdioContainer, + appprotoosGenerator appprotoos.Generator, + image bufimage.Image, + imagesByDir []bufimage.Image, + pluginConfig *PluginConfig, + includeImports bool, +) (*pluginpb.CodeGeneratorResponse, error) { + var pluginImages []bufimage.Image + switch pluginConfig.Strategy { + case StrategyAll: + pluginImages = []bufimage.Image{image} + case StrategyDirectory: + pluginImages = imagesByDir + default: + return nil, fmt.Errorf("unknown strategy: %v", pluginConfig.Strategy) + } + response, err := appprotoosGenerator.Generate( + ctx, + container, + pluginConfig.Name, + bufimage.ImagesToCodeGeneratorRequests( + pluginImages, + pluginConfig.Opt, + appprotoexec.DefaultVersion, + includeImports, + ), + appprotoos.GenerateWithPluginPath(pluginConfig.Path), + ) if err != nil { - if os.IsNotExist(err) { - if err := os.MkdirAll(outDirPath, 0755); err != nil { - return fmt.Errorf("failed to create output directory: %w", err) - } - } - return err - } else if !fileInfo.IsDir() { - return fmt.Errorf("not a directory: %s", outDirPath) - } - readWriteBucket := storagemem.NewReadWriteBucket() - for _, file := range files { - if err := storage.PutPath(ctx, readWriteBucket, file.Name, file.Content); err != nil { - return fmt.Errorf("failed to write generated file %s: %w", file.Name, err) - } + return nil, fmt.Errorf("plugin %s: %v", pluginConfig.PluginName(), err) } - if includeManifest { - if err := storage.PutPath(ctx, readWriteBucket, appprotoos.ManifestPath, appprotoos.ManifestContent); err != nil { - return fmt.Errorf("failed to write manifest file: %w", err) - } + return response, nil +} + +func (g *generator) execRemotePlugin( + ctx context.Context, + container app.EnvStdioContainer, + image bufimage.Image, + pluginConfig *PluginConfig, +) (*pluginpb.CodeGeneratorResponse, error) { + remote, owner, name, version, err := bufplugin.ParsePluginVersionPath(pluginConfig.Remote) + if err != nil { + return nil, fmt.Errorf("invalid plugin path: %w", err) } - file, err := os.Create(outFilePath) + generateService, err := g.registryProvider.NewGenerateService(ctx, remote) if err != nil { - return fmt.Errorf("failed to create output file %s: %w", outFilePath, err) + return nil, fmt.Errorf("failed to create generate service for remote %q: %w", remote, err) } - defer func() { - retErr = multierr.Append(retErr, file.Close()) - }() - // protoc does not compress - if err := storagearchive.Zip(ctx, readWriteBucket, file, false); err != nil { - return fmt.Errorf("failed to zip results: %w", err) + var parameters []string + if len(pluginConfig.Opt) > 0 { + // Only include parameters if they're not empty. + parameters = []string{pluginConfig.Opt} } - return nil -} - -func (g *generator) generateDirectory( - ctx context.Context, - outDirPath string, - files []*bufplugin.File, -) error { - if err := os.MkdirAll(outDirPath, 0755); err != nil { - return err + pluginReference := ®istryv1alpha1.PluginReference{ + Owner: owner, + Name: name, + Version: version, + Parameters: parameters, } - // this checks that the directory exists - readWriteBucket, err := g.storageosProvider.NewReadWriteBucket( - outDirPath, - storageos.ReadWriteBucketWithSymlinksIfSupported(), + responses, _, err := generateService.GeneratePlugins( + ctx, + bufimage.ImageToProtoImage(image), + []*registryv1alpha1.PluginReference{pluginReference}, ) if err != nil { - return err + return nil, err } - for _, file := range files { - if err := storage.PutPath(ctx, readWriteBucket, file.Name, file.Content); err != nil { - return fmt.Errorf("failed to write generated file %s: %w", file.Name, err) - } + if len(responses) != 1 { + return nil, fmt.Errorf("unexpected number of responses received, got %d, wanted %d", len(responses), 1) } - return nil + return responses[0], nil } // modifyImage modifies the image according to the given configuration (i.e. Managed Mode). @@ -529,69 +422,41 @@ func newModifier( return modifier, nil } -type generateOptions struct { - baseOutDirPath string - includeImports bool -} - -func newGenerateOptions() *generateOptions { - return &generateOptions{} -} - -type localPluginResponse struct { - responseWriter appproto.ResponseWriter - pluginName string - index int -} - -func newLocalPluginResponse( - responseWriter appproto.ResponseWriter, - pluginName string, - index int, -) *localPluginResponse { - return &localPluginResponse{ - responseWriter: responseWriter, - pluginName: pluginName, - index: index, +// validateResponses verifies that a response is set for each of the +// pluginConfigs, and that each generated file is generated by a single +// plugin. +func validateResponses( + responses []*pluginpb.CodeGeneratorResponse, + pluginConfigs []*PluginConfig, +) error { + if len(responses) != len(pluginConfigs) { + return fmt.Errorf("unexpected number of responses: expected %d but got %d", len(pluginConfigs), len(responses)) } -} - -type remotePlugin struct { - reference *registryv1alpha1.PluginReference - pluginRemote string - index int -} - -func newRemotePlugin(reference *registryv1alpha1.PluginReference, pluginRemote string, index int) *remotePlugin { - return &remotePlugin{ - reference: reference, - pluginRemote: pluginRemote, - index: index, + pluginResponses := make([]*appproto.PluginResponse, 0, len(responses)) + for i, response := range responses { + pluginConfig := pluginConfigs[i] + if response == nil { + return fmt.Errorf("failed to create a response for %q", pluginConfig.PluginName()) + } + pluginResponses = append( + pluginResponses, + appproto.NewPluginResponse( + response, + pluginConfig.PluginName(), + ), + ) } -} - -type semaphore chan struct{} - -func newSemaphore(limit int) semaphore { - return make(semaphore, limit) -} - -func (s semaphore) Acquire(ctx context.Context) error { - select { - case s <- struct{}{}: - return nil - case <-ctx.Done(): - return ctx.Err() + if err := appproto.ValidatePluginResponses(pluginResponses); err != nil { + return err } + return nil } -func (s semaphore) Release(ctx context.Context) { - select { - case <-s: - case <-ctx.Done(): - } +type generateOptions struct { + baseOutDirPath string + includeImports bool } -func (s semaphore) Discard() { - close(s) +func newGenerateOptions() *generateOptions { + return &generateOptions{} } diff --git a/private/buf/cmd/buf/buf_test.go b/private/buf/cmd/buf/buf_test.go index f010f10e21..b78f0dc239 100644 --- a/private/buf/cmd/buf/buf_test.go +++ b/private/buf/cmd/buf/buf_test.go @@ -29,6 +29,7 @@ import ( "github.com/bufbuild/buf/private/pkg/app/appcmd" "github.com/bufbuild/buf/private/pkg/app/appcmd/appcmdtesting" "github.com/bufbuild/buf/private/pkg/storage" + "github.com/bufbuild/buf/private/pkg/storage/storagemem" "github.com/bufbuild/buf/private/pkg/storage/storageos" "github.com/bufbuild/buf/private/pkg/storage/storagetesting" "github.com/stretchr/testify/assert" @@ -1167,6 +1168,89 @@ Successfully migrated your buf.yaml and buf.gen.yaml to v1.`, }) } +func TestGenerate(t *testing.T) { + successTemplate := ` +version: v1 +plugins: + - name: insertion-point-receiver + out: . + - name: insertion-point-writer + out: . +` + storageosProvider := storageos.NewProvider() + tempDir, readWriteBucket := testCopyReadBucketToTempDir( + context.Background(), + t, + storageosProvider, + storagemem.NewReadWriteBucket(), + ) + testRunStdout( + t, + nil, + 0, + ``, + "generate", + filepath.Join("testdata", "success"), // The input directory is irrelevant for these insertion points. + "--template", + successTemplate, + "-o", + tempDir, + ) + expectedOutput, err := storageosProvider.NewReadWriteBucket(filepath.Join("testdata", "generate", "insertion_point")) + require.NoError(t, err) + diff, err := storage.DiffBytes(context.Background(), expectedOutput, readWriteBucket) + require.NoError(t, err) + require.Empty(t, string(diff)) +} + +func TestGenerateInsertionPointFail(t *testing.T) { + successTemplate := ` +version: v1 +plugins: + - name: insertion-point-receiver + out: gen/proto/insertion + - name: insertion-point-writer + out: . +` + testRunStdoutStderr( + t, + nil, + 1, + ``, + `Failure: plugin insertion-point-writer: test.txt: does not exist`, + "generate", + filepath.Join("testdata", "success"), // The input directory is irrelevant for these insertion points. + "--template", + successTemplate, + "-o", + t.TempDir(), + ) +} + +func TestGenerateDuplicateFileFail(t *testing.T) { + successTemplate := ` +version: v1 +plugins: + - name: insertion-point-receiver + out: . + - name: insertion-point-receiver + out: . +` + testRunStdoutStderr( + t, + nil, + 1, + ``, + `Failure: file "test.txt" was generated multiple times: once by plugin "insertion-point-receiver" and again by plugin "insertion-point-receiver"`, + "generate", + filepath.Join("testdata", "success"), // The input directory is irrelevant for these insertion points. + "--template", + successTemplate, + "-o", + t.TempDir(), + ) +} + func testMigrateV1Beta1Diff(t *testing.T, storageosProvider storageos.Provider, scenario string, expectedStderr string) { // Copy test setup to temporary directory to avoid writing to filesystem inputBucket, err := storageosProvider.NewReadWriteBucket(filepath.Join("testdata", "migrate-v1beta1", "success", scenario, "input")) @@ -1316,6 +1400,7 @@ func testRun( return map[string]string{ useEnvVar(use, "CONFIG_DIR"): "testdata/config", useEnvVar(use, "CACHE_DIR"): "cache", + useEnvVar(use, "PATH"): os.Getenv("PATH"), } }, stdin, diff --git a/private/buf/cmd/buf/command/protoc/plugin.go b/private/buf/cmd/buf/command/protoc/plugin.go index 9a807e8bc1..b451ce4797 100644 --- a/private/buf/cmd/buf/command/protoc/plugin.go +++ b/private/buf/cmd/buf/command/protoc/plugin.go @@ -25,6 +25,7 @@ import ( "github.com/bufbuild/buf/private/pkg/app/appproto/appprotoos" "github.com/bufbuild/buf/private/pkg/storage/storageos" "go.uber.org/zap" + "google.golang.org/protobuf/types/pluginpb" ) type pluginInfo struct { @@ -48,12 +49,14 @@ func executePlugin( images []bufimage.Image, pluginName string, pluginInfo *pluginInfo, -) error { - if err := appprotoos.NewGenerator(logger, storageosProvider).Generate( +) (*pluginpb.CodeGeneratorResponse, error) { + response, err := appprotoos.NewGenerator( + logger, + storageosProvider, + ).Generate( ctx, container, pluginName, - pluginInfo.Out, bufimage.ImagesToCodeGeneratorRequests( images, strings.Join(pluginInfo.Opt, ","), @@ -61,8 +64,9 @@ func executePlugin( false, ), appprotoos.GenerateWithPluginPath(pluginInfo.Path), - ); err != nil { - return fmt.Errorf("--%s_out: %v", pluginName, err) + ) + if err != nil { + return nil, fmt.Errorf("--%s_out: %v", pluginName, err) } - return nil + return response, nil } diff --git a/private/buf/cmd/buf/command/protoc/protoc.go b/private/buf/cmd/buf/command/protoc/protoc.go index c1afc59bd7..0992aef8c4 100644 --- a/private/buf/cmd/buf/command/protoc/protoc.go +++ b/private/buf/cmd/buf/command/protoc/protoc.go @@ -29,6 +29,8 @@ import ( "github.com/bufbuild/buf/private/pkg/app" "github.com/bufbuild/buf/private/pkg/app/appcmd" "github.com/bufbuild/buf/private/pkg/app/appflag" + "github.com/bufbuild/buf/private/pkg/app/appproto" + "github.com/bufbuild/buf/private/pkg/app/appproto/appprotoos" "github.com/bufbuild/buf/private/pkg/storage/storageos" "go.opencensus.io/trace" "go.uber.org/zap" @@ -176,13 +178,13 @@ func run( } span.End() } - // we need to run these in the order they appear for insertion points to work + pluginResponses := make([]*appproto.PluginResponse, 0, len(env.PluginNamesSortedByOutIndex)) for _, pluginName := range env.PluginNamesSortedByOutIndex { pluginInfo, ok := env.PluginNameToPluginInfo[pluginName] if !ok { return fmt.Errorf("no value in PluginNamesToPluginInfo for %q", pluginName) } - if err := executePlugin( + response, err := executePlugin( ctx, container.Logger(), storageosProvider, @@ -190,10 +192,35 @@ func run( images, pluginName, pluginInfo, + ) + if err != nil { + return err + } + pluginResponses = append(pluginResponses, appproto.NewPluginResponse(response, pluginName)) + } + if err := appproto.ValidatePluginResponses(pluginResponses); err != nil { + return err + } + bucketResponseWriter := appprotoos.NewBucketResponseWriter( + container.Logger(), + storageosProvider, + ) + for _, pluginResponse := range pluginResponses { + pluginInfo, ok := env.PluginNameToPluginInfo[pluginResponse.PluginName] + if !ok { + return fmt.Errorf("no value in PluginNamesToPluginInfo for %q", pluginResponse.PluginName) + } + if err := bucketResponseWriter.AddResponse( + ctx, + pluginResponse.Response, + pluginInfo.Out, ); err != nil { return err } } + if err := bucketResponseWriter.Close(); err != nil { + return err + } return nil } if env.Output == "" { diff --git a/private/buf/cmd/buf/testdata/generate/insertion_point/test.txt b/private/buf/cmd/buf/testdata/generate/insertion_point/test.txt new file mode 100644 index 0000000000..ed6c6095c4 --- /dev/null +++ b/private/buf/cmd/buf/testdata/generate/insertion_point/test.txt @@ -0,0 +1,15 @@ + + // The following line represents an insertion point named 'example'. + // We include a few indentation to verify the whitespace is preserved + // in the inserted content. + // + + // Include this comment on the 'example' insertion point. + // This is another example where whitespaces are preserved. + // And this demonstrates a newline literal (\n). + // And don't forget the windows newline literal (\r\n). + + // @@protoc_insertion_point(example) + // + // Note that all text should be added above the insertion point. + \ No newline at end of file diff --git a/private/bufpkg/bufplugin/bufplugin.go b/private/bufpkg/bufplugin/bufplugin.go index ead1fd0a3d..fffce69d99 100644 --- a/private/bufpkg/bufplugin/bufplugin.go +++ b/private/bufpkg/bufplugin/bufplugin.go @@ -15,8 +15,6 @@ package bufplugin import ( - "bytes" - "context" "errors" "fmt" "os" @@ -25,9 +23,7 @@ import ( registryv1alpha1 "github.com/bufbuild/buf/private/gen/proto/go/buf/alpha/registry/v1alpha1" "github.com/bufbuild/buf/private/pkg/app/appcmd" - "github.com/bufbuild/buf/private/pkg/app/appproto" "github.com/bufbuild/buf/private/pkg/encoding" - "google.golang.org/protobuf/types/pluginpb" ) const ( @@ -245,100 +241,6 @@ func ParseTemplateVersionConfig(config string) (*TemplateVersionConfig, error) { return templateVersionConfig, nil } -// File represents a generated file, and tracks the plugin -// that originally generated it (i.e. if insertion points -// are applied to this File, the PluginName will be unchanged). -type File struct { - Name string - Content []byte - PluginName string -} - -// PluginResponse encapsulates a CodeGeneratorResponse, -// along with the name of the plugin that created it. -type PluginResponse struct { - Response *pluginpb.CodeGeneratorResponse - PluginName string -} - -// NewPluginResponse retruns a new *PluginResponse. -func NewPluginResponse(response *pluginpb.CodeGeneratorResponse, pluginName string) *PluginResponse { - return &PluginResponse{ - Response: response, - PluginName: pluginName, - } -} - -// MergedPluginResult holds the files for a plugin result. -type MergedPluginResult struct { - Files []*File -} - -// MergeInsertionPoints traverses the plugin results and merges any insertion points present in any responses. -// The order of inserts depends on the order of the input plugins. The returned results are in the same order -// as the input plugin results. -func MergeInsertionPoints(pluginResponses []*PluginResponse) ([]*MergedPluginResult, error) { - allFiles := make(map[string]*File) - results := make([]*MergedPluginResult, len(pluginResponses)) - for i, pluginResponse := range pluginResponses { - response := pluginResponse.Response - pluginName := pluginResponse.PluginName - if response.Error != nil { - return nil, fmt.Errorf("unexpected response error: %s", *response.Error) - } - var files []*File - for _, generatedFile := range response.File { - fileName := generatedFile.GetName() - insertionPoint := generatedFile.GetInsertionPoint() - if insertionPoint != "" { - parentFile, ok := allFiles[fileName] - if !ok { - return nil, fmt.Errorf( - "plugin %q requested insertion point in file %q which does not exist", - pluginName, - fileName, - ) - } - newFileContents, err := appproto.ApplyInsertionPoint( - context.Background(), - generatedFile, - bytes.NewBuffer(parentFile.Content), - ) - if err != nil { - return nil, fmt.Errorf( - "failed to apply insertion point %q in file %q for plugin %q: %w", - insertionPoint, - fileName, - pluginName, - err, - ) - } - allFiles[fileName].Content = newFileContents - continue - } - if previousFile, ok := allFiles[fileName]; ok { - return nil, fmt.Errorf( - "file %q was generated multiple times: once by plugin %q and again by plugin %q", - fileName, - previousFile.PluginName, - pluginName, - ) - } - file := &File{ - Name: fileName, - Content: []byte(generatedFile.GetContent()), - PluginName: pluginName, - } - files = append(files, file) - allFiles[fileName] = file - } - results[i] = &MergedPluginResult{ - Files: files, - } - } - return results, nil -} - type externalTemplateConfig struct { Version string `json:"version,omitempty" yaml:"version,omitempty"` Plugins []externalPluginConfig `json:"plugins,omitempty" yaml:"plugins,omitempty"` diff --git a/private/bufpkg/bufplugin/bufplugin_test.go b/private/bufpkg/bufplugin/bufplugin_test.go index 4acceb65e5..1824d514ad 100644 --- a/private/bufpkg/bufplugin/bufplugin_test.go +++ b/private/bufpkg/bufplugin/bufplugin_test.go @@ -19,7 +19,6 @@ import ( "testing" "github.com/stretchr/testify/require" - "google.golang.org/protobuf/types/pluginpb" ) func TestParseTemplateConfigJSONFile(t *testing.T) { @@ -412,42 +411,3 @@ plugin_versions: `) require.Error(t, err) } - -func TestMergeInsertionPoints(t *testing.T) { - results := []*PluginResponse{ - { - PluginName: "protoc-gen-java", - Response: &pluginpb.CodeGeneratorResponse{ - File: []*pluginpb.CodeGeneratorResponse_File{ - { - Name: testStringPointer("file1.java"), - Content: testStringPointer("// @@protoc_insertion_point(insertionPoint1)"), - }, - }, - }, - }, - { - PluginName: "protoc-gen-java-insertion", - Response: &pluginpb.CodeGeneratorResponse{ - File: []*pluginpb.CodeGeneratorResponse_File{ - { - Name: testStringPointer("file1.java"), - Content: testStringPointer("!! this was inserted !!"), - InsertionPoint: testStringPointer("insertionPoint1"), - }, - }, - }, - }, - } - mergedResults, err := MergeInsertionPoints(results) - require.NoError(t, err) - require.Len(t, mergedResults, 2) - require.Len(t, mergedResults[0].Files, 1) - require.EqualValues(t, "file1.java", mergedResults[0].Files[0].Name) - require.EqualValues(t, "protoc-gen-java", mergedResults[0].Files[0].PluginName) - require.EqualValues(t, "!! this was inserted !!\n// @@protoc_insertion_point(insertionPoint1)", string(mergedResults[0].Files[0].Content)) -} - -func testStringPointer(s string) *string { - return &s -} diff --git a/private/pkg/app/appproto/appproto.go b/private/pkg/app/appproto/appproto.go index 32d7f099c1..6ff5d208e3 100644 --- a/private/pkg/app/appproto/appproto.go +++ b/private/pkg/app/appproto/appproto.go @@ -22,6 +22,7 @@ import ( "bufio" "bytes" "context" + "fmt" "io" "unicode" "unicode/utf8" @@ -58,9 +59,9 @@ type ResponseWriter interface { AddError(message string) // SetFeatureProto3Optional sets the proto3 optional feature. SetFeatureProto3Optional() - // ToResponse returns the resulting CodeGeneratorResponse. This must + // toResponse returns the resulting CodeGeneratorResponse. This must // only be called after all writing has been completed. - ToResponse() *pluginpb.CodeGeneratorResponse + toResponse() *pluginpb.CodeGeneratorResponse } // Handler is a protoc plugin handler @@ -111,43 +112,95 @@ func Run(ctx context.Context, container app.Container, handler Handler) error { // Generator executes the Handler using protoc's plugin execution logic. // -// This invokes a Handler and writes out the response to the output location, -// additionally accounting for insertion point logic. -// // If multiple requests are specified, these are executed in parallel and the // result is combined into one response that is written. type Generator interface { - // Generate generates to the bucket. + // Generate generates a CodeGeneratorResponse for the given CodeGeneratorRequests. Generate( ctx context.Context, container app.EnvStderrContainer, - writeBucket storage.WriteBucket, requests []*pluginpb.CodeGeneratorRequest, - options ...GenerateOption, + ) (*pluginpb.CodeGeneratorResponse, error) +} + +// NewGenerator returns a new Generator. +func NewGenerator( + logger *zap.Logger, + handler Handler, +) Generator { + return newGenerator(logger, handler) +} + +// ResponseHandler handles the response and writes it to the given storage.WriteBucket +// without executing any plugins and handles insertion points as needed. +type ResponseHandler interface { + // HandleResponse writes to the bucket with the given response. + HandleResponse( + ctx context.Context, + writeBucket storage.WriteBucket, + response *pluginpb.CodeGeneratorResponse, + options ...HandleResponseOption, ) error } -// GenerateOption is an option for Generate. -type GenerateOption func(*generateOptions) +// NewResponseHandler returns a new ResponseHandler. +func NewResponseHandler(logger *zap.Logger) ResponseHandler { + return newResponseHandler(logger) +} + +// HandleResponseOption is an option for HandleResponse. +type HandleResponseOption func(*handleResponseOptions) -// GenerateWithInsertionPointReadBucket returns a new GenerateOption that uses the given +// HandleResponseWithInsertionPointReadBucket returns a new HandleResponseOption that uses the given // ReadBucket to read from for insertion points. // // If this is not specified, insertion points are not supported. -func GenerateWithInsertionPointReadBucket( +func HandleResponseWithInsertionPointReadBucket( insertionPointReadBucket storage.ReadBucket, -) GenerateOption { - return func(generateOptions *generateOptions) { - generateOptions.insertionPointReadBucket = insertionPointReadBucket +) HandleResponseOption { + return func(handleResponseOptions *handleResponseOptions) { + handleResponseOptions.insertionPointReadBucket = insertionPointReadBucket } } -// NewGenerator returns a new Generator. -func NewGenerator( - logger *zap.Logger, - handler Handler, -) Generator { - return newGenerator(logger, handler) +// PluginResponse encapsulates a CodeGeneratorResponse, +// along with the name of the plugin that created it. +type PluginResponse struct { + Response *pluginpb.CodeGeneratorResponse + PluginName string +} + +// NewPluginResponse retruns a new *PluginResponse. +func NewPluginResponse(response *pluginpb.CodeGeneratorResponse, pluginName string) *PluginResponse { + return &PluginResponse{ + Response: response, + PluginName: pluginName, + } +} + +// ValidatePluginResponses validates that each file is only defined by a single *PluginResponse. +func ValidatePluginResponses(pluginResponses []*PluginResponse) error { + seen := make(map[string]string) + for _, pluginResponse := range pluginResponses { + for _, file := range pluginResponse.Response.File { + if file.GetInsertionPoint() != "" { + // We expect insertion points to write + // to files that already exist. + continue + } + fileName := file.GetName() + if pluginName, ok := seen[fileName]; ok { + return fmt.Errorf( + "file %q was generated multiple times: once by plugin %q and again by plugin %q", + fileName, + pluginName, + pluginResponse.PluginName, + ) + } + seen[fileName] = pluginResponse.PluginName + } + } + return nil } // newRunFunc returns a new RunFunc for app.Main and app.Run. @@ -169,7 +222,7 @@ func newRunFunc(handler Handler) func(context.Context, app.Container) error { if err := handler.Handle(ctx, container, responseWriter, request); err != nil { return err } - response := responseWriter.ToResponse() + response := responseWriter.toResponse() if err := protodescriptor.ValidateCodeGeneratorResponse(response); err != nil { return err } @@ -182,68 +235,11 @@ func newRunFunc(handler Handler) func(context.Context, app.Container) error { } } -// NewReponseWriter returns a new ResponseWriter. +// NewResponseWriter returns a new ResponseWriter. func NewResponseWriter(container app.StderrContainer) ResponseWriter { return newResponseWriter(container) } -// ApplyInsertionPoint applies the insertion point defined in insertionPointFile -// to the targetFile and returns the result as []byte. The caller must ensure the -// provided targetFile matches the file requested in insertionPointFile.Name. -func ApplyInsertionPoint( - ctx context.Context, - insertionPointFile *pluginpb.CodeGeneratorResponse_File, - targetFile io.Reader, -) (_ []byte, retErr error) { - targetScanner := bufio.NewScanner(targetFile) - match := []byte("@@protoc_insertion_point(" + insertionPointFile.GetInsertionPoint() + ")") - postInsertionContent := bytes.NewBuffer(nil) - postInsertionContent.Grow(averageGeneratedFileSize) - // TODO: We should respect the line endings in the generated file. This would - // require either targetFile being an io.ReadSeeker and in the worst case - // doing 2 full scans of the file (if it is a single line), or implementing - // bufio.Scanner.Scan() inline - newline := []byte{'\n'} - for targetScanner.Scan() { - targetLine := targetScanner.Bytes() - if !bytes.Contains(targetLine, match) { - // these writes cannot fail, they will panic if they cannot - // allocate - _, _ = postInsertionContent.Write(targetLine) - _, _ = postInsertionContent.Write(newline) - continue - } - // For each line in then new content, apply the - // same amount of whitespace. This is important - // for specific languages, e.g. Python. - whitespace := leadingWhitespace(targetLine) - - // Create another scanner so that we can seamlessly handle - // newlines in a platform-agnostic manner. - insertedContentScanner := bufio.NewScanner(bytes.NewBufferString(insertionPointFile.GetContent())) - insertedContent := scanWithPrefixAndLineEnding(insertedContentScanner, whitespace, newline) - // This write cannot fail, it will panic if it cannot - // allocate - _, _ = postInsertionContent.Write(insertedContent) - - // Code inserted at this point is placed immediately - // above the line containing the insertion point, so - // we include it last. - // These writes cannot fail, they will panic if they cannot - // allocate - _, _ = postInsertionContent.Write(targetLine) - _, _ = postInsertionContent.Write(newline) - } - - if err := targetScanner.Err(); err != nil { - return nil, err - } - - // trim the trailing newline - postInsertionBytes := postInsertionContent.Bytes() - return postInsertionBytes[:len(postInsertionBytes)-1], nil -} - // leadingWhitespace iterates through the given string, // and returns the leading whitespace substring, if any, // respecting utf-8 encoding. diff --git a/private/pkg/app/appproto/appproto_test.go b/private/pkg/app/appproto/appproto_test.go index a3825c5083..1e7c4c49fa 100644 --- a/private/pkg/app/appproto/appproto_test.go +++ b/private/pkg/app/appproto/appproto_test.go @@ -24,7 +24,7 @@ import ( "google.golang.org/protobuf/types/pluginpb" ) -func TestApplyInsertionPoint(t *testing.T) { +func TestWriteInsertionPoint(t *testing.T) { // \u205F is "Medium Mathematical Space" whitespacePrefix := "\u205F\t\t\t" targetFileContent := `This is a test file @@ -46,7 +46,7 @@ at varied indentation levels Content: &insertionPointContent, } - postInsertionContent, err := ApplyInsertionPoint( + postInsertionContent, err := writeInsertionPoint( context.Background(), insertionPointConsumer, strings.NewReader(targetFileContent), @@ -72,7 +72,7 @@ at varied indentation levels Content: &insertionPointContent, } - postInsertionContent, err := ApplyInsertionPoint( + postInsertionContent, err := writeInsertionPoint( context.Background(), insertionPointConsumer, strings.NewReader(targetFileContent), @@ -98,7 +98,7 @@ at varied indentation levels Content: &insertionPointContent, } - postInsertionContent, err := ApplyInsertionPoint( + postInsertionContent, err := writeInsertionPoint( context.Background(), insertionPointConsumer, strings.NewReader(targetFileContent), @@ -121,7 +121,7 @@ at varied indentation levels }) } -func BenchmarkApplyInsertionPoint(b *testing.B) { +func BenchmarkWriteInsertionPoint(b *testing.B) { // \u205F is "Medium Mathematical Space" whitespacePrefix := "\u205F\t\t\t" targetFileContent := `This is a test file @@ -163,7 +163,7 @@ at varied indentation levels` + whitespacePrefix + "// @@protoc_insertion_point( for i := 0; i < b.N; i++ { b.ReportAllocs() - postInsertionContent, _ = ApplyInsertionPoint( + postInsertionContent, _ = writeInsertionPoint( context.Background(), insertionPointConsumer, strings.NewReader(targetFileContent), @@ -197,7 +197,7 @@ at varied indentation levels` + whitespacePrefix + "// @@protoc_insertion_point( for i := 0; i < b.N; i++ { b.ReportAllocs() - postInsertionContent, _ = ApplyInsertionPoint( + postInsertionContent, _ = writeInsertionPoint( context.Background(), insertionPointConsumer, strings.NewReader(inflatedTargetFileContent), diff --git a/private/pkg/app/appproto/appprotoos/appprotoos.go b/private/pkg/app/appproto/appprotoos/appprotoos.go index 6665d09080..d825e661ba 100644 --- a/private/pkg/app/appproto/appprotoos/appprotoos.go +++ b/private/pkg/app/appproto/appprotoos/appprotoos.go @@ -17,6 +17,7 @@ package appprotoos import ( "context" + "io" "github.com/bufbuild/buf/private/pkg/app" "github.com/bufbuild/buf/private/pkg/storage/storageos" @@ -24,19 +25,19 @@ import ( "google.golang.org/protobuf/types/pluginpb" ) +// Generator is used to generate code to the OS filesystem. type Generator interface { // Generate generates to the os filesystem, switching on the file extension. // If there is a .jar extension, this generates a jar. If there is a .zip // extension, this generates a zip. If there is no extension, this outputs - // to the directory. + // to the directory. The corresponding CodeGeneratorResponse written is returned. Generate( ctx context.Context, container app.EnvStderrContainer, pluginName string, - pluginOut string, requests []*pluginpb.CodeGeneratorRequest, options ...GenerateOption, - ) error + ) (*pluginpb.CodeGeneratorResponse, error) } // NewGenerator returns a new Generator. @@ -58,10 +59,43 @@ func GenerateWithPluginPath(pluginPath string) GenerateOption { } } -// GenerateWithCreateOutDirIfNotExists returns a new GenerateOption that creates +// BucketResponseWriter writes CodeGeneratorResponses to the OS filesystem. +type BucketResponseWriter interface { + // Close writes all of the responses to disk. No further calls can be + // made to the BucketResponseWriter after this call. + io.Closer + + // AddResponse adds the response to the writer, switching on the file extension. + // If there is a .jar extension, this generates a jar. If there is a .zip + // extension, this generates a zip. If there is no extension, this outputs + // to the directory. + AddResponse( + ctx context.Context, + response *pluginpb.CodeGeneratorResponse, + pluginOut string, + ) error +} + +// NewBucketResponseWriter returns a new BucketResponseWriter. +func NewBucketResponseWriter( + logger *zap.Logger, + storageosProvider storageos.Provider, + options ...BucketResponseWriterOption, +) BucketResponseWriter { + return newBucketResponseWriter( + logger, + storageosProvider, + options..., + ) +} + +// BucketResponseWriterOption is an option for the BucketResponseWriter. +type BucketResponseWriterOption func(*bucketResponseWriterOptions) + +// BucketResponseWriterWithCreateOutDirIfNotExists returns a new BucketResponseWriterOption that creates // the directory if it does not exist. -func GenerateWithCreateOutDirIfNotExists() GenerateOption { - return func(generateOptions *generateOptions) { - generateOptions.createOutDirIfNotExists = true +func BucketResponseWriterWithCreateOutDirIfNotExists() BucketResponseWriterOption { + return func(bucketResponseWriterOptions *bucketResponseWriterOptions) { + bucketResponseWriterOptions.createOutDirIfNotExists = true } } diff --git a/private/pkg/app/appproto/appprotoos/bucket_response_writer.go b/private/pkg/app/appproto/appprotoos/bucket_response_writer.go new file mode 100644 index 0000000000..8dafdb9b0b --- /dev/null +++ b/private/pkg/app/appproto/appprotoos/bucket_response_writer.go @@ -0,0 +1,269 @@ +// Copyright 2020-2021 Buf Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package appprotoos + +import ( + "context" + "fmt" + "os" + "path/filepath" + "sync" + + "github.com/bufbuild/buf/private/pkg/app/appproto" + "github.com/bufbuild/buf/private/pkg/storage" + "github.com/bufbuild/buf/private/pkg/storage/storagearchive" + "github.com/bufbuild/buf/private/pkg/storage/storagemem" + "github.com/bufbuild/buf/private/pkg/storage/storageos" + "go.uber.org/multierr" + "go.uber.org/zap" + "google.golang.org/protobuf/types/pluginpb" +) + +type bucketResponseWriter struct { + lock sync.RWMutex + logger *zap.Logger + storageosProvider storageos.Provider + responseHandler appproto.ResponseHandler + + // If set, create directories if they don't already exist. + createOutDirIfNotExists bool + + // Cache the readWriteBuckets by their respective output paths. + // These builders are transformed to storage.ReadBuckets and written + // to disk once the bucketResponseWriter is flushed. + readWriteBuckets map[string]storage.ReadWriteBucket + + // Cache the functions used to flush all of the responses to disk. + // This holds all of the buckets in-memory so that we only write + // the results to disk if all of the responses are successful. + closers []func() error +} + +func newBucketResponseWriter( + logger *zap.Logger, + storageosProvider storageos.Provider, + options ...BucketResponseWriterOption, +) *bucketResponseWriter { + bucketResponseWriterOptions := newBucketResponseWriterOptions() + for _, option := range options { + option(bucketResponseWriterOptions) + } + return &bucketResponseWriter{ + logger: logger, + storageosProvider: storageosProvider, + responseHandler: appproto.NewResponseHandler(logger), + createOutDirIfNotExists: bucketResponseWriterOptions.createOutDirIfNotExists, + readWriteBuckets: make(map[string]storage.ReadWriteBucket), + } +} + +func (w *bucketResponseWriter) AddResponse( + ctx context.Context, + response *pluginpb.CodeGeneratorResponse, + pluginOut string, +) error { + w.lock.Lock() + defer w.lock.Unlock() + return w.addResponse( + ctx, + response, + pluginOut, + w.createOutDirIfNotExists, + ) +} + +func (w *bucketResponseWriter) Close() error { + w.lock.Lock() + defer w.lock.Unlock() + for _, closeFunc := range w.closers { + if err := closeFunc(); err != nil { + // Although unlikely, if an error happens here, + // some generated files could be written to disk, + // whereas others aren't. + // + // Regardless, we stop at the first error so that + // we don't unncessarily write more results. + return err + } + } + // Re-initialize the cached values to be safe. + w.readWriteBuckets = make(map[string]storage.ReadWriteBucket) + w.closers = nil + return nil +} + +func (w *bucketResponseWriter) addResponse( + ctx context.Context, + response *pluginpb.CodeGeneratorResponse, + pluginOut string, + createOutDirIfNotExists bool, +) error { + switch filepath.Ext(pluginOut) { + case ".jar": + return w.writeZip( + ctx, + response, + pluginOut, + true, + createOutDirIfNotExists, + ) + case ".zip": + return w.writeZip( + ctx, + response, + pluginOut, + false, + createOutDirIfNotExists, + ) + default: + return w.writeDirectory( + ctx, + response, + pluginOut, + createOutDirIfNotExists, + ) + } +} + +func (w *bucketResponseWriter) writeZip( + ctx context.Context, + response *pluginpb.CodeGeneratorResponse, + outFilePath string, + includeManifest bool, + createOutDirIfNotExists bool, +) (retErr error) { + outDirPath := filepath.Dir(outFilePath) + if readWriteBucket, ok := w.readWriteBuckets[outFilePath]; ok { + // We already have a readWriteBucket for this outFilePath, so + // we can write to the same bucket. + if err := w.responseHandler.HandleResponse( + ctx, + readWriteBucket, + response, + appproto.HandleResponseWithInsertionPointReadBucket(readWriteBucket), + ); err != nil { + return err + } + return nil + } + // OK to use os.Stat instead of os.Lstat here. + fileInfo, err := os.Stat(outDirPath) + if err != nil { + if os.IsNotExist(err) { + if createOutDirIfNotExists { + if err := os.MkdirAll(outDirPath, 0755); err != nil { + return err + } + } else { + return err + } + } + return err + } else if !fileInfo.IsDir() { + return fmt.Errorf("not a directory: %s", outDirPath) + } + readWriteBucket := storagemem.NewReadWriteBucket() + if includeManifest { + if err := storage.PutPath(ctx, readWriteBucket, ManifestPath, ManifestContent); err != nil { + return err + } + } + if err := w.responseHandler.HandleResponse( + ctx, + readWriteBucket, + response, + appproto.HandleResponseWithInsertionPointReadBucket(readWriteBucket), + ); err != nil { + return err + } + // Add this readWriteBucket to the set so that other plugins + // can write to the same files (re: insertion points). + w.readWriteBuckets[outFilePath] = readWriteBucket + w.closers = append(w.closers, func() (retErr error) { + // We're done writing all of the content into this + // readWriteBucket, so we zip it when we flush. + file, err := os.Create(outFilePath) + if err != nil { + return err + } + defer func() { + retErr = multierr.Append(retErr, file.Close()) + }() + // protoc does not compress. + return storagearchive.Zip(ctx, readWriteBucket, file, false) + }) + return nil +} + +func (w *bucketResponseWriter) writeDirectory( + ctx context.Context, + response *pluginpb.CodeGeneratorResponse, + outDirPath string, + createOutDirIfNotExists bool, +) error { + if readWriteBucket, ok := w.readWriteBuckets[outDirPath]; ok { + // We already have a readWriteBucket for this outDirPath, so + // we can write to the same bucket. + if err := w.responseHandler.HandleResponse( + ctx, + readWriteBucket, + response, + appproto.HandleResponseWithInsertionPointReadBucket(readWriteBucket), + ); err != nil { + return err + } + return nil + } + readWriteBucket := storagemem.NewReadWriteBucket() + if err := w.responseHandler.HandleResponse( + ctx, + readWriteBucket, + response, + appproto.HandleResponseWithInsertionPointReadBucket(readWriteBucket), + ); err != nil { + return err + } + // Add this readWriteBucket to the set so that other plugins + // can write to the same files (re: insertion points). + w.readWriteBuckets[outDirPath] = readWriteBucket + w.closers = append(w.closers, func() error { + if createOutDirIfNotExists { + if err := os.MkdirAll(outDirPath, 0755); err != nil { + return err + } + } + // This checks that the directory exists. + osReadWriteBucket, err := w.storageosProvider.NewReadWriteBucket( + outDirPath, + storageos.ReadWriteBucketWithSymlinksIfSupported(), + ) + if err != nil { + return err + } + if _, err := storage.Copy(ctx, readWriteBucket, osReadWriteBucket); err != nil { + return err + } + return nil + }) + return nil +} + +type bucketResponseWriterOptions struct { + createOutDirIfNotExists bool +} + +func newBucketResponseWriterOptions() *bucketResponseWriterOptions { + return &bucketResponseWriterOptions{} +} diff --git a/private/pkg/app/appproto/appprotoos/generator.go b/private/pkg/app/appproto/appprotoos/generator.go index 6590128b1c..51da11af87 100644 --- a/private/pkg/app/appproto/appprotoos/generator.go +++ b/private/pkg/app/appproto/appprotoos/generator.go @@ -16,19 +16,12 @@ package appprotoos import ( "context" - "fmt" - "os" - "path/filepath" "github.com/bufbuild/buf/private/pkg/app" "github.com/bufbuild/buf/private/pkg/app/appproto" "github.com/bufbuild/buf/private/pkg/app/appproto/appprotoexec" "github.com/bufbuild/buf/private/pkg/normalpath" - "github.com/bufbuild/buf/private/pkg/storage" - "github.com/bufbuild/buf/private/pkg/storage/storagearchive" - "github.com/bufbuild/buf/private/pkg/storage/storagemem" "github.com/bufbuild/buf/private/pkg/storage/storageos" - "go.uber.org/multierr" "go.uber.org/zap" "google.golang.org/protobuf/types/pluginpb" ) @@ -61,10 +54,9 @@ func (g *generator) Generate( ctx context.Context, container app.EnvStderrContainer, pluginName string, - pluginOut string, requests []*pluginpb.CodeGeneratorRequest, options ...GenerateOption, -) (retErr error) { +) (_ *pluginpb.CodeGeneratorResponse, retErr error) { generateOptions := newGenerateOptions() for _, option := range options { option(generateOptions) @@ -76,121 +68,20 @@ func (g *generator) Generate( appprotoexec.HandlerWithPluginPath(generateOptions.pluginPath), ) if err != nil { - return err + return nil, err } - appprotoGenerator := appproto.NewGenerator(g.logger, handler) - switch filepath.Ext(pluginOut) { - case ".jar": - return g.generateZip( - ctx, - container, - appprotoGenerator, - pluginOut, - requests, - true, - generateOptions.createOutDirIfNotExists, - ) - case ".zip": - return g.generateZip( - ctx, - container, - appprotoGenerator, - pluginOut, - requests, - false, - generateOptions.createOutDirIfNotExists, - ) - default: - return g.generateDirectory( - ctx, - container, - appprotoGenerator, - pluginOut, - requests, - generateOptions.createOutDirIfNotExists, - ) - } -} - -func (g *generator) generateZip( - ctx context.Context, - container app.EnvStderrContainer, - appprotoGenerator appproto.Generator, - outFilePath string, - requests []*pluginpb.CodeGeneratorRequest, - includeManifest bool, - createOutDirIfNotExists bool, -) (retErr error) { - outDirPath := filepath.Dir(outFilePath) - // OK to use os.Stat instead of os.Lstat here - fileInfo, err := os.Stat(outDirPath) - if err != nil { - if os.IsNotExist(err) { - if createOutDirIfNotExists { - if err := os.MkdirAll(outDirPath, 0755); err != nil { - return err - } - } else { - return err - } - } - return err - } else if !fileInfo.IsDir() { - return fmt.Errorf("not a directory: %s", outDirPath) - } - readWriteBucket := storagemem.NewReadWriteBucket() - if err := appprotoGenerator.Generate(ctx, container, readWriteBucket, requests); err != nil { - return err - } - if includeManifest { - if err := storage.PutPath(ctx, readWriteBucket, ManifestPath, ManifestContent); err != nil { - return err - } - } - file, err := os.Create(outFilePath) - if err != nil { - return err - } - defer func() { - retErr = multierr.Append(retErr, file.Close()) - }() - // protoc does not compress - return storagearchive.Zip(ctx, readWriteBucket, file, false) -} - -func (g *generator) generateDirectory( - ctx context.Context, - container app.EnvStderrContainer, - appprotoGenerator appproto.Generator, - outDirPath string, - requests []*pluginpb.CodeGeneratorRequest, - createOutDirIfNotExists bool, -) error { - if createOutDirIfNotExists { - if err := os.MkdirAll(outDirPath, 0755); err != nil { - return err - } - } - // this checks that the directory exists - readWriteBucket, err := g.storageosProvider.NewReadWriteBucket( - outDirPath, - storageos.ReadWriteBucketWithSymlinksIfSupported(), - ) - if err != nil { - return err - } - return appprotoGenerator.Generate( + return appproto.NewGenerator( + g.logger, + handler, + ).Generate( ctx, container, - readWriteBucket, requests, - appproto.GenerateWithInsertionPointReadBucket(readWriteBucket), ) } type generateOptions struct { - pluginPath string - createOutDirIfNotExists bool + pluginPath string } func newGenerateOptions() *generateOptions { diff --git a/private/pkg/app/appproto/generator.go b/private/pkg/app/appproto/generator.go index 9e75f5bfad..8f63188efd 100644 --- a/private/pkg/app/appproto/generator.go +++ b/private/pkg/app/appproto/generator.go @@ -20,9 +20,7 @@ import ( "github.com/bufbuild/buf/private/pkg/app" "github.com/bufbuild/buf/private/pkg/protodescriptor" - "github.com/bufbuild/buf/private/pkg/storage" "github.com/bufbuild/buf/private/pkg/thread" - "go.uber.org/multierr" "go.uber.org/zap" "google.golang.org/protobuf/types/pluginpb" ) @@ -45,38 +43,8 @@ func newGenerator( func (g *generator) Generate( ctx context.Context, container app.EnvStderrContainer, - writeBucket storage.WriteBucket, requests []*pluginpb.CodeGeneratorRequest, - options ...GenerateOption, -) error { - generateOptions := newGenerateOptions() - for _, option := range options { - option(generateOptions) - } - files, err := g.getResponseFiles(ctx, container, requests) - if err != nil { - return err - } - for _, file := range files { - if file.GetInsertionPoint() != "" { - if generateOptions.insertionPointReadBucket == nil { - return storage.NewErrNotExist(file.GetName()) - } - if err := applyInsertionPoint(ctx, file, generateOptions.insertionPointReadBucket, writeBucket); err != nil { - return err - } - } else if err := storage.PutPath(ctx, writeBucket, file.GetName(), []byte(file.GetContent())); err != nil { - return err - } - } - return nil -} - -func (g *generator) getResponseFiles( - ctx context.Context, - container app.EnvStderrContainer, - requests []*pluginpb.CodeGeneratorRequest, -) ([]*pluginpb.CodeGeneratorResponse_File, error) { +) (*pluginpb.CodeGeneratorResponse, error) { responseWriter := newResponseWriter(container) jobs := make([]func(context.Context) error, len(requests)) for i, request := range requests { @@ -93,45 +61,12 @@ func (g *generator) getResponseFiles( if err := thread.Parallelize(ctx, jobs, thread.ParallelizeWithCancel(cancel)); err != nil { return nil, err } - response := responseWriter.ToResponse() + response := responseWriter.toResponse() if err := protodescriptor.ValidateCodeGeneratorResponse(response); err != nil { return nil, err } if errString := response.GetError(); errString != "" { return nil, errors.New(errString) } - return response.File, nil -} - -// applyInsertionPoint inserts the content of the given file at the insertion point that it specfiies. -// For more details on insertion points, see the following: -// -// https://github.com/protocolbuffers/protobuf/blob/f5bdd7cd56aa86612e166706ed8ef139db06edf2/src/google/protobuf/compiler/plugin.proto#L135-L171 -func applyInsertionPoint( - ctx context.Context, - file *pluginpb.CodeGeneratorResponse_File, - readBucket storage.ReadBucket, - writeBucket storage.WriteBucket, -) (retErr error) { - targetReadObjectCloser, err := readBucket.Get(ctx, file.GetName()) - if err != nil { - return err - } - defer func() { - retErr = multierr.Append(retErr, targetReadObjectCloser.Close()) - }() - resultData, err := ApplyInsertionPoint(ctx, file, targetReadObjectCloser) - if err != nil { - return err - } - // This relies on storageos buckets maintaining existing file permissions - return storage.PutPath(ctx, writeBucket, file.GetName(), resultData) -} - -type generateOptions struct { - insertionPointReadBucket storage.ReadBucket -} - -func newGenerateOptions() *generateOptions { - return &generateOptions{} + return response, nil } diff --git a/private/pkg/app/appproto/response_handler.go b/private/pkg/app/appproto/response_handler.go new file mode 100644 index 0000000000..4c49ecadce --- /dev/null +++ b/private/pkg/app/appproto/response_handler.go @@ -0,0 +1,154 @@ +// Copyright 2020-2021 Buf Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package appproto + +import ( + "bufio" + "bytes" + "context" + "io" + + "github.com/bufbuild/buf/private/pkg/storage" + "go.uber.org/multierr" + "go.uber.org/zap" + "google.golang.org/protobuf/types/pluginpb" +) + +type responseHandler struct { + logger *zap.Logger +} + +func newResponseHandler( + logger *zap.Logger, +) *responseHandler { + return &responseHandler{ + logger: logger, + } +} + +func (h *responseHandler) HandleResponse( + ctx context.Context, + writeBucket storage.WriteBucket, + response *pluginpb.CodeGeneratorResponse, + options ...HandleResponseOption, +) error { + handleResponseOptions := newHandleResponseOptions() + for _, option := range options { + option(handleResponseOptions) + } + for _, file := range response.File { + if file.GetInsertionPoint() != "" { + if handleResponseOptions.insertionPointReadBucket == nil { + return storage.NewErrNotExist(file.GetName()) + } + if err := applyInsertionPoint(ctx, file, handleResponseOptions.insertionPointReadBucket, writeBucket); err != nil { + return err + } + } else if err := storage.PutPath(ctx, writeBucket, file.GetName(), []byte(file.GetContent())); err != nil { + return err + } + } + return nil +} + +// applyInsertionPoint inserts the content of the given file at the insertion point that it specfiies. +// For more details on insertion points, see the following: +// +// https://github.com/protocolbuffers/protobuf/blob/f5bdd7cd56aa86612e166706ed8ef139db06edf2/src/google/protobuf/compiler/plugin.proto#L135-L171 +func applyInsertionPoint( + ctx context.Context, + file *pluginpb.CodeGeneratorResponse_File, + readBucket storage.ReadBucket, + writeBucket storage.WriteBucket, +) (retErr error) { + targetReadObjectCloser, err := readBucket.Get(ctx, file.GetName()) + if err != nil { + return err + } + defer func() { + retErr = multierr.Append(retErr, targetReadObjectCloser.Close()) + }() + resultData, err := writeInsertionPoint(ctx, file, targetReadObjectCloser) + if err != nil { + return err + } + // This relies on storageos buckets maintaining existing file permissions + return storage.PutPath(ctx, writeBucket, file.GetName(), resultData) +} + +// writeInsertionPoint writes the insertion point defined in insertionPointFile +// to the targetFile and returns the result as []byte. The caller must ensure the +// provided targetFile matches the file requested in insertionPointFile.Name. +func writeInsertionPoint( + ctx context.Context, + insertionPointFile *pluginpb.CodeGeneratorResponse_File, + targetFile io.Reader, +) (_ []byte, retErr error) { + targetScanner := bufio.NewScanner(targetFile) + match := []byte("@@protoc_insertion_point(" + insertionPointFile.GetInsertionPoint() + ")") + postInsertionContent := bytes.NewBuffer(nil) + postInsertionContent.Grow(averageGeneratedFileSize) + // TODO: We should respect the line endings in the generated file. This would + // require either targetFile being an io.ReadSeeker and in the worst case + // doing 2 full scans of the file (if it is a single line), or implementing + // bufio.Scanner.Scan() inline + newline := []byte{'\n'} + for targetScanner.Scan() { + targetLine := targetScanner.Bytes() + if !bytes.Contains(targetLine, match) { + // these writes cannot fail, they will panic if they cannot + // allocate + _, _ = postInsertionContent.Write(targetLine) + _, _ = postInsertionContent.Write(newline) + continue + } + // For each line in then new content, apply the + // same amount of whitespace. This is important + // for specific languages, e.g. Python. + whitespace := leadingWhitespace(targetLine) + + // Create another scanner so that we can seamlessly handle + // newlines in a platform-agnostic manner. + insertedContentScanner := bufio.NewScanner(bytes.NewBufferString(insertionPointFile.GetContent())) + insertedContent := scanWithPrefixAndLineEnding(insertedContentScanner, whitespace, newline) + // This write cannot fail, it will panic if it cannot + // allocate + _, _ = postInsertionContent.Write(insertedContent) + + // Code inserted at this point is placed immediately + // above the line containing the insertion point, so + // we include it last. + // These writes cannot fail, they will panic if they cannot + // allocate + _, _ = postInsertionContent.Write(targetLine) + _, _ = postInsertionContent.Write(newline) + } + + if err := targetScanner.Err(); err != nil { + return nil, err + } + + // trim the trailing newline + postInsertionBytes := postInsertionContent.Bytes() + return postInsertionBytes[:len(postInsertionBytes)-1], nil +} + +type handleResponseOptions struct { + insertionPointReadBucket storage.ReadBucket +} + +func newHandleResponseOptions() *handleResponseOptions { + return &handleResponseOptions{} +} diff --git a/private/pkg/app/appproto/response_writer.go b/private/pkg/app/appproto/response_writer.go index fd977be5c4..ebde1f7b11 100644 --- a/private/pkg/app/appproto/response_writer.go +++ b/private/pkg/app/appproto/response_writer.go @@ -101,9 +101,9 @@ func (r *responseWriter) SetFeatureProto3Optional() { r.featureProto3Optional = true } -// ToResponse turns the response writer into a Protobuf CodeGeneratorResponse. +// toResponse turns the response writer into a Protobuf CodeGeneratorResponse. // It should be run after all writing to the response has finished. -func (r *responseWriter) ToResponse() *pluginpb.CodeGeneratorResponse { +func (r *responseWriter) toResponse() *pluginpb.CodeGeneratorResponse { r.lock.RLock() defer r.lock.RUnlock() response := &pluginpb.CodeGeneratorResponse{ diff --git a/private/pkg/thread/thread.go b/private/pkg/thread/thread.go index 949d4ade4b..9187156c85 100644 --- a/private/pkg/thread/thread.go +++ b/private/pkg/thread/thread.go @@ -102,7 +102,13 @@ func Parallelize(ctx context.Context, jobs []func(context.Context) error, option go func() { if err := job(ctx); err != nil { lock.Lock() - retErr = multierr.Append(retErr, err) + if parallelizeOptions.fastFail { + if retErr == nil { + retErr = err + } + } else { + retErr = multierr.Append(retErr, err) + } lock.Unlock() if parallelizeOptions.cancel != nil { parallelizeOptions.cancel() @@ -141,9 +147,18 @@ func ParallelizeWithCancel(cancel context.CancelFunc) ParallelizeOption { } } +// ParallelizeWithFastFail returns a new ParallelizeOption that will return +// the first error encountered from any job that fails. +func ParallelizeWithFastFail() ParallelizeOption { + return func(parallelizeOptions *parallelizeOptions) { + parallelizeOptions.fastFail = true + } +} + type parallelizeOptions struct { multiplier int cancel context.CancelFunc + fastFail bool } func newParallelizeOptions() *parallelizeOptions { From bc32363647113680e90c03d2d9814acf0f73714c Mon Sep 17 00:00:00 2001 From: bufdev Date: Thu, 30 Sep 2021 16:48:47 -0400 Subject: [PATCH 02/14] Move generate tests to command/generate and cleanup --- .dockerignore | 1 - .gitignore | 1 - make/buf/all.mk | 1 - private/buf/cmd/buf/buf_test.go | 139 +------------ .../cmd/buf/command/generate/generate_test.go | 191 +++++++++++++----- .../testdata}/insertion_point/test.txt | 0 .../internaltesting/internaltesting.go | 70 +++++++ .../buf/internal/internaltesting/usage.gen.go | 19 ++ .../buf/cmd/buf/testdata/config/config.yaml | 0 9 files changed, 234 insertions(+), 188 deletions(-) rename private/buf/cmd/buf/{testdata/generate => command/generate/testdata}/insertion_point/test.txt (100%) create mode 100644 private/buf/cmd/buf/internal/internaltesting/internaltesting.go create mode 100644 private/buf/cmd/buf/internal/internaltesting/usage.gen.go delete mode 100644 private/buf/cmd/buf/testdata/config/config.yaml diff --git a/.dockerignore b/.dockerignore index bc7e22f7e0..59bffcdc9b 100644 --- a/.dockerignore +++ b/.dockerignore @@ -7,7 +7,6 @@ cmd/buf/buf cmd/protoc-gen-buf-breaking/protoc-gen-buf-breaking cmd/protoc-gen-buf-lint/protoc-gen-buf-lint -private/buf/cmd/buf/cache/ private/buf/cmd/buf/command/protoc/internal/protoc-gen-insertion-point-receiver/protoc-gen-insertion-point-receiver private/buf/cmd/buf/command/protoc/internal/protoc-gen-insertion-point-writer/protoc-gen-insertion-point-writer private/buf/cmd/buf/workspacetests/other/proto/workspacetest/cache/ diff --git a/.gitignore b/.gitignore index beb4ab36a9..2af697fb13 100644 --- a/.gitignore +++ b/.gitignore @@ -7,7 +7,6 @@ /cmd/buf/buf /cmd/protoc-gen-buf-breaking/protoc-gen-buf-breaking /cmd/protoc-gen-buf-lint/protoc-gen-buf-lint -/private/buf/cmd/buf/cache/ /private/buf/cmd/buf/command/protoc/internal/protoc-gen-insertion-point-receiver/protoc-gen-insertion-point-receiver /private/buf/cmd/buf/command/protoc/internal/protoc-gen-insertion-point-writer/protoc-gen-insertion-point-writer /private/buf/cmd/buf/workspacetests/other/proto/workspacetest/cache/ diff --git a/make/buf/all.mk b/make/buf/all.mk index 61416f664a..093bd8811e 100644 --- a/make/buf/all.mk +++ b/make/buf/all.mk @@ -24,7 +24,6 @@ FILE_IGNORES := $(FILE_IGNORES) \ .build/ \ .ctrlp \ .vscode/ \ - private/buf/cmd/buf/cache/ \ private/buf/cmd/buf/workspacetests/other/proto/workspacetest/cache/ \ private/bufpkg/buftesting/cache/ \ private/pkg/storage/storageos/tmp/ diff --git a/private/buf/cmd/buf/buf_test.go b/private/buf/cmd/buf/buf_test.go index b78f0dc239..9b7aa940da 100644 --- a/private/buf/cmd/buf/buf_test.go +++ b/private/buf/cmd/buf/buf_test.go @@ -21,18 +21,16 @@ import ( "io" "os" "path/filepath" - "strings" "testing" "github.com/bufbuild/buf/private/buf/bufcli" "github.com/bufbuild/buf/private/buf/bufconfig" + "github.com/bufbuild/buf/private/buf/cmd/buf/internal/internaltesting" "github.com/bufbuild/buf/private/pkg/app/appcmd" "github.com/bufbuild/buf/private/pkg/app/appcmd/appcmdtesting" "github.com/bufbuild/buf/private/pkg/storage" - "github.com/bufbuild/buf/private/pkg/storage/storagemem" "github.com/bufbuild/buf/private/pkg/storage/storageos" "github.com/bufbuild/buf/private/pkg/storage/storagetesting" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -1168,94 +1166,11 @@ Successfully migrated your buf.yaml and buf.gen.yaml to v1.`, }) } -func TestGenerate(t *testing.T) { - successTemplate := ` -version: v1 -plugins: - - name: insertion-point-receiver - out: . - - name: insertion-point-writer - out: . -` - storageosProvider := storageos.NewProvider() - tempDir, readWriteBucket := testCopyReadBucketToTempDir( - context.Background(), - t, - storageosProvider, - storagemem.NewReadWriteBucket(), - ) - testRunStdout( - t, - nil, - 0, - ``, - "generate", - filepath.Join("testdata", "success"), // The input directory is irrelevant for these insertion points. - "--template", - successTemplate, - "-o", - tempDir, - ) - expectedOutput, err := storageosProvider.NewReadWriteBucket(filepath.Join("testdata", "generate", "insertion_point")) - require.NoError(t, err) - diff, err := storage.DiffBytes(context.Background(), expectedOutput, readWriteBucket) - require.NoError(t, err) - require.Empty(t, string(diff)) -} - -func TestGenerateInsertionPointFail(t *testing.T) { - successTemplate := ` -version: v1 -plugins: - - name: insertion-point-receiver - out: gen/proto/insertion - - name: insertion-point-writer - out: . -` - testRunStdoutStderr( - t, - nil, - 1, - ``, - `Failure: plugin insertion-point-writer: test.txt: does not exist`, - "generate", - filepath.Join("testdata", "success"), // The input directory is irrelevant for these insertion points. - "--template", - successTemplate, - "-o", - t.TempDir(), - ) -} - -func TestGenerateDuplicateFileFail(t *testing.T) { - successTemplate := ` -version: v1 -plugins: - - name: insertion-point-receiver - out: . - - name: insertion-point-receiver - out: . -` - testRunStdoutStderr( - t, - nil, - 1, - ``, - `Failure: file "test.txt" was generated multiple times: once by plugin "insertion-point-receiver" and again by plugin "insertion-point-receiver"`, - "generate", - filepath.Join("testdata", "success"), // The input directory is irrelevant for these insertion points. - "--template", - successTemplate, - "-o", - t.TempDir(), - ) -} - func testMigrateV1Beta1Diff(t *testing.T, storageosProvider storageos.Provider, scenario string, expectedStderr string) { // Copy test setup to temporary directory to avoid writing to filesystem inputBucket, err := storageosProvider.NewReadWriteBucket(filepath.Join("testdata", "migrate-v1beta1", "success", scenario, "input")) require.NoError(t, err) - tempDir, readWriteBucket := testCopyReadBucketToTempDir(context.Background(), t, storageosProvider, inputBucket) + tempDir, readWriteBucket := internaltesting.CopyReadBucketToTempDir(context.Background(), t, storageosProvider, inputBucket) testRunStdoutStderr( t, @@ -1280,7 +1195,7 @@ func testMigrateV1Beta1Failure(t *testing.T, storageosProvider storageos.Provide // Copy test setup to temporary directory to avoid writing to filesystem inputBucket, err := storageosProvider.NewReadWriteBucket(filepath.Join("testdata", "migrate-v1beta1", "failure", scenario)) require.NoError(t, err) - tempDir, _ := testCopyReadBucketToTempDir(context.Background(), t, storageosProvider, inputBucket) + tempDir, _ := internaltesting.CopyReadBucketToTempDir(context.Background(), t, storageosProvider, inputBucket) testRunStdoutStderr( t, @@ -1294,22 +1209,6 @@ func testMigrateV1Beta1Failure(t *testing.T, storageosProvider storageos.Provide ) } -func testCopyReadBucketToTempDir( - ctx context.Context, - tb testing.TB, - storageosProvider storageos.Provider, - readBucket storage.ReadBucket, -) (string, storage.ReadWriteBucket) { - tb.Helper() - // Copy to temporary directory to avoid writing to filesystem - tempDir := tb.TempDir() - readWriteBucket, err := storageosProvider.NewReadWriteBucket(tempDir) - require.NoError(tb, err) - _, err = storage.Copy(ctx, readBucket, readWriteBucket) - require.NoError(tb, err) - return tempDir, readWriteBucket -} - func testConfigInit(t *testing.T, expectedData string, document bool, name string, deps ...string) { tempDir := t.TempDir() baseArgs := []string{"config", "init"} @@ -1332,12 +1231,7 @@ func testRunStdout(t *testing.T, stdin io.Reader, expectedExitCode int, expected func(use string) *appcmd.Command { return NewRootCommand(use) }, expectedExitCode, expectedStdout, - func(use string) map[string]string { - return map[string]string{ - useEnvVar(use, "CONFIG_DIR"): "testdata/config", - useEnvVar(use, "CACHE_DIR"): "cache", - } - }, + internaltesting.NewEnvFunc(t), stdin, args..., ) @@ -1350,12 +1244,7 @@ func testRunStdoutStderr(t *testing.T, stdin io.Reader, expectedExitCode int, ex expectedExitCode, expectedStdout, expectedStderr, - func(use string) map[string]string { - return map[string]string{ - useEnvVar(use, "CONFIG_DIR"): "testdata/config", - useEnvVar(use, "CACHE_DIR"): "cache", - } - }, + internaltesting.NewEnvFunc(t), stdin, // we do not want warnings to be part of our stderr test calculation append( @@ -1366,9 +1255,7 @@ func testRunStdoutStderr(t *testing.T, stdin io.Reader, expectedExitCode int, ex } func testRunStdoutProfile(t *testing.T, stdin io.Reader, expectedExitCode int, expectedStdout string, args ...string) { - profileDirPath, err := os.MkdirTemp("", "") - require.NoError(t, err) - defer func() { assert.NoError(t, os.RemoveAll(profileDirPath)) }() + tempDirPath := t.TempDir() testRunStdout( t, stdin, @@ -1377,7 +1264,7 @@ func testRunStdoutProfile(t *testing.T, stdin io.Reader, expectedExitCode int, e append( args, "--profile", - fmt.Sprintf("--profile-path=%s", profileDirPath), + fmt.Sprintf("--profile-path=%s", tempDirPath), "--profile-loops=1", "--profile-type=cpu", )..., @@ -1396,20 +1283,10 @@ func testRun( t, func(use string) *appcmd.Command { return NewRootCommand(use) }, expectedExitCode, - func(use string) map[string]string { - return map[string]string{ - useEnvVar(use, "CONFIG_DIR"): "testdata/config", - useEnvVar(use, "CACHE_DIR"): "cache", - useEnvVar(use, "PATH"): os.Getenv("PATH"), - } - }, + internaltesting.NewEnvFunc(t), stdin, stdout, stderr, args..., ) } - -func useEnvVar(use string, suffix string) string { - return strings.ToUpper(use) + "_" + suffix -} diff --git a/private/buf/cmd/buf/command/generate/generate_test.go b/private/buf/cmd/buf/command/generate/generate_test.go index f1c308c777..0b1cc2328f 100644 --- a/private/buf/cmd/buf/command/generate/generate_test.go +++ b/private/buf/cmd/buf/command/generate/generate_test.go @@ -19,11 +19,13 @@ import ( "context" "encoding/json" "fmt" + "io" "os" "path/filepath" "testing" "github.com/bufbuild/buf/private/buf/bufgen" + "github.com/bufbuild/buf/private/buf/cmd/buf/internal/internaltesting" "github.com/bufbuild/buf/private/bufpkg/buftesting" "github.com/bufbuild/buf/private/pkg/app/appcmd" "github.com/bufbuild/buf/private/pkg/app/appcmd/appcmdtesting" @@ -56,7 +58,7 @@ func TestCompareGeneratedStubsGoogleapisGo(t *testing.T) { googleapisDirPath := buftesting.GetGoogleapisDirPath(t, buftestingDirPath) testCompareGeneratedStubs(t, googleapisDirPath, - []testPluginInfo{ + []*testPluginInfo{ {name: "go", opt: "Mgoogle/api/auth.proto=foo"}, }, ) @@ -68,7 +70,7 @@ func TestCompareGeneratedStubsGoogleapisGoZip(t *testing.T) { googleapisDirPath := buftesting.GetGoogleapisDirPath(t, buftestingDirPath) testCompareGeneratedStubsArchive(t, googleapisDirPath, - []testPluginInfo{ + []*testPluginInfo{ {name: "go", opt: "Mgoogle/api/auth.proto=foo"}, }, false, @@ -81,7 +83,7 @@ func TestCompareGeneratedStubsGoogleapisGoJar(t *testing.T) { googleapisDirPath := buftesting.GetGoogleapisDirPath(t, buftestingDirPath) testCompareGeneratedStubsArchive(t, googleapisDirPath, - []testPluginInfo{ + []*testPluginInfo{ {name: "go", opt: "Mgoogle/api/auth.proto=foo"}, }, true, @@ -94,7 +96,7 @@ func TestCompareGeneratedStubsGoogleapisObjc(t *testing.T) { googleapisDirPath := buftesting.GetGoogleapisDirPath(t, buftestingDirPath) testCompareGeneratedStubs(t, googleapisDirPath, - []testPluginInfo{{name: "objc"}}, + []*testPluginInfo{{name: "objc"}}, ) } @@ -104,7 +106,7 @@ func TestCompareInsertionPointOutput(t *testing.T) { insertionTestdataDirPath := filepath.Join("testdata", "insertion") testCompareGeneratedStubs(t, insertionTestdataDirPath, - []testPluginInfo{ + []*testPluginInfo{ {name: "insertion-point-receiver"}, {name: "insertion-point-writer"}, }, @@ -113,21 +115,8 @@ func TestCompareInsertionPointOutput(t *testing.T) { func TestOutputFlag(t *testing.T) { tempDirPath := t.TempDir() - appcmdtesting.RunCommandSuccess( + testRunSuccess( t, - func(name string) *appcmd.Command { - return NewCommand( - name, - appflag.NewBuilder(name), - ) - }, - func(string) map[string]string { - return map[string]string{ - "PATH": os.Getenv("PATH"), - } - }, - nil, - nil, "--output", tempDirPath, "--template", @@ -138,19 +127,96 @@ func TestOutputFlag(t *testing.T) { require.NoError(t, err) } +func TestGenerateInsertionPoints(t *testing.T) { + successTemplate := ` +version: v1 +plugins: + - name: insertion-point-receiver + out: . + - name: insertion-point-writer + out: . +` + storageosProvider := storageos.NewProvider() + tempDir, readWriteBucket := internaltesting.CopyReadBucketToTempDir( + context.Background(), + t, + storageosProvider, + storagemem.NewReadWriteBucket(), + ) + testRunSuccess( + t, + filepath.Join("testdata", "simple"), // The input directory is irrelevant for these insertion points. + "--template", + successTemplate, + "-o", + tempDir, + ) + expectedOutput, err := storageosProvider.NewReadWriteBucket(filepath.Join("testdata", "insertion_point")) + require.NoError(t, err) + diff, err := storage.DiffBytes(context.Background(), expectedOutput, readWriteBucket) + require.NoError(t, err) + require.Empty(t, string(diff)) +} + +func TestGenerateInsertionPointFail(t *testing.T) { + successTemplate := ` +version: v1 +plugins: + - name: insertion-point-receiver + out: gen/proto/insertion + - name: insertion-point-writer + out: . +` + testRunStdoutStderr( + t, + nil, + 1, + ``, + `Failure: plugin insertion-point-writer: test.txt: does not exist`, + filepath.Join("testdata", "simple"), // The input directory is irrelevant for these insertion points. + "--template", + successTemplate, + "-o", + t.TempDir(), + ) +} + +func TestGenerateDuplicateFileFail(t *testing.T) { + successTemplate := ` +version: v1 +plugins: + - name: insertion-point-receiver + out: . + - name: insertion-point-receiver + out: . +` + testRunStdoutStderr( + t, + nil, + 1, + ``, + `Failure: file "test.txt" was generated multiple times: once by plugin "insertion-point-receiver" and again by plugin "insertion-point-receiver"`, + filepath.Join("testdata", "simple"), // The input directory is irrelevant for these insertion points. + "--template", + successTemplate, + "-o", + t.TempDir(), + ) +} + func testCompareGeneratedStubs( t *testing.T, dirPath string, - plugins []testPluginInfo, + testPluginInfos []*testPluginInfo, ) { filePaths := buftesting.GetProtocFilePaths(t, dirPath, 100) actualProtocDir := t.TempDir() bufGenDir := t.TempDir() var actualProtocPluginFlags []string - for _, plugin := range plugins { - actualProtocPluginFlags = append(actualProtocPluginFlags, fmt.Sprintf("--%s_out=%s", plugin.name, actualProtocDir)) - if plugin.opt != "" { - actualProtocPluginFlags = append(actualProtocPluginFlags, fmt.Sprintf("--%s_opt=%s", plugin.name, plugin.opt)) + for _, testPluginInfo := range testPluginInfos { + actualProtocPluginFlags = append(actualProtocPluginFlags, fmt.Sprintf("--%s_out=%s", testPluginInfo.name, actualProtocDir)) + if testPluginInfo.opt != "" { + actualProtocPluginFlags = append(actualProtocPluginFlags, fmt.Sprintf("--%s_opt=%s", testPluginInfo.name, testPluginInfo.opt)) } } buftesting.RunActualProtoc( @@ -168,7 +234,7 @@ func testCompareGeneratedStubs( genFlags := []string{ dirPath, "--template", - newExternalConfigV1String(t, plugins, bufGenDir), + newExternalConfigV1String(t, testPluginInfos, bufGenDir), } for _, filePath := range filePaths { genFlags = append( @@ -185,11 +251,7 @@ func testCompareGeneratedStubs( appflag.NewBuilder(name), ) }, - func(string) map[string]string { - return map[string]string{ - "PATH": os.Getenv("PATH"), - } - }, + internaltesting.NewEnvFunc(t), nil, nil, genFlags..., @@ -217,7 +279,7 @@ func testCompareGeneratedStubs( func testCompareGeneratedStubsArchive( t *testing.T, dirPath string, - plugins []testPluginInfo, + testPluginInfos []*testPluginInfo, useJar bool, ) { fileExt := ".zip" @@ -229,10 +291,10 @@ func testCompareGeneratedStubsArchive( actualProtocFile := filepath.Join(tempDir, "actual-protoc"+fileExt) bufGenFile := filepath.Join(tempDir, "buf-generate"+fileExt) var actualProtocPluginFlags []string - for _, plugin := range plugins { - actualProtocPluginFlags = append(actualProtocPluginFlags, fmt.Sprintf("--%s_out=%s", plugin.name, actualProtocFile)) - if plugin.opt != "" { - actualProtocPluginFlags = append(actualProtocPluginFlags, fmt.Sprintf("--%s_opt=%s", plugin.name, plugin.opt)) + for _, testPluginInfo := range testPluginInfos { + actualProtocPluginFlags = append(actualProtocPluginFlags, fmt.Sprintf("--%s_out=%s", testPluginInfo.name, actualProtocFile)) + if testPluginInfo.opt != "" { + actualProtocPluginFlags = append(actualProtocPluginFlags, fmt.Sprintf("--%s_opt=%s", testPluginInfo.name, testPluginInfo.opt)) } } buftesting.RunActualProtoc( @@ -250,7 +312,7 @@ func testCompareGeneratedStubsArchive( genFlags := []string{ dirPath, "--template", - newExternalConfigV1String(t, plugins, bufGenFile), + newExternalConfigV1String(t, testPluginInfos, bufGenFile), } for _, filePath := range filePaths { genFlags = append( @@ -259,21 +321,8 @@ func testCompareGeneratedStubsArchive( filePath, ) } - appcmdtesting.RunCommandSuccess( + testRunSuccess( t, - func(name string) *appcmd.Command { - return NewCommand( - name, - appflag.NewBuilder(name), - ) - }, - func(string) map[string]string { - return map[string]string{ - "PATH": os.Getenv("PATH"), - } - }, - nil, - nil, genFlags..., ) actualData, err := os.ReadFile(actualProtocFile) @@ -309,12 +358,41 @@ func testCompareGeneratedStubsArchive( assert.Empty(t, string(diff)) } -type testPluginInfo struct { - name string - opt string +func testRunSuccess(t *testing.T, args ...string) { + appcmdtesting.RunCommandSuccess( + t, + func(name string) *appcmd.Command { + return NewCommand( + name, + appflag.NewBuilder(name), + ) + }, + internaltesting.NewEnvFunc(t), + nil, + nil, + args..., + ) +} + +func testRunStdoutStderr(t *testing.T, stdin io.Reader, expectedExitCode int, expectedStdout string, expectedStderr string, args ...string) { + appcmdtesting.RunCommandExitCodeStdoutStderr( + t, + func(name string) *appcmd.Command { + return NewCommand( + name, + appflag.NewBuilder(name), + ) + }, + expectedExitCode, + expectedStdout, + expectedStderr, + internaltesting.NewEnvFunc(t), + stdin, + args..., + ) } -func newExternalConfigV1String(t *testing.T, plugins []testPluginInfo, out string) string { +func newExternalConfigV1String(t *testing.T, plugins []*testPluginInfo, out string) string { externalConfig := bufgen.ExternalConfigV1{ Version: "v1", } @@ -332,3 +410,8 @@ func newExternalConfigV1String(t *testing.T, plugins []testPluginInfo, out strin require.NoError(t, err) return string(data) } + +type testPluginInfo struct { + name string + opt string +} diff --git a/private/buf/cmd/buf/testdata/generate/insertion_point/test.txt b/private/buf/cmd/buf/command/generate/testdata/insertion_point/test.txt similarity index 100% rename from private/buf/cmd/buf/testdata/generate/insertion_point/test.txt rename to private/buf/cmd/buf/command/generate/testdata/insertion_point/test.txt diff --git a/private/buf/cmd/buf/internal/internaltesting/internaltesting.go b/private/buf/cmd/buf/internal/internaltesting/internaltesting.go new file mode 100644 index 0000000000..d1d6243911 --- /dev/null +++ b/private/buf/cmd/buf/internal/internaltesting/internaltesting.go @@ -0,0 +1,70 @@ +// Copyright 2020-2021 Buf Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package internaltesting + +import ( + "context" + "os" + "strings" + "testing" + + "github.com/bufbuild/buf/private/pkg/storage" + "github.com/bufbuild/buf/private/pkg/storage/storageos" + "github.com/stretchr/testify/require" +) + +// CopyReadBucketToTempDir copies the ReadBucket to a new temporary directory. +// +// Returns the temporary directory path and a ReadWriteBucket of the temporary directory. +func CopyReadBucketToTempDir( + ctx context.Context, + tb testing.TB, + storageosProvider storageos.Provider, + readBucket storage.ReadBucket, +) (string, storage.ReadWriteBucket) { + // Copy to temporary directory to avoid writing to filesystem + tempDirPath := tb.TempDir() + readWriteBucket, err := storageosProvider.NewReadWriteBucket(tempDirPath) + require.NoError(tb, err) + _, err = storage.Copy(ctx, readBucket, readWriteBucket) + require.NoError(tb, err) + return tempDirPath, readWriteBucket +} + +// NewEnv returns a new env map for testing. +func NewEnv(tb testing.TB, use string) func(string) map[string]string { + tempDirPath := tb.TempDir() + return func(use string) map[string]string { + return map[string]string{ + useEnvVar(use, "CACHE_DIR"): tempDirPath, + "PATH": os.Getenv("PATH"), + } + } +} + +// NewEnvFunc returns a new env func for testing. +func NewEnvFunc(tb testing.TB) func(string) map[string]string { + tempDirPath := tb.TempDir() + return func(use string) map[string]string { + return map[string]string{ + useEnvVar(use, "CACHE_DIR"): tempDirPath, + "PATH": os.Getenv("PATH"), + } + } +} + +func useEnvVar(use string, suffix string) string { + return strings.ToUpper(use) + "_" + suffix +} diff --git a/private/buf/cmd/buf/internal/internaltesting/usage.gen.go b/private/buf/cmd/buf/internal/internaltesting/usage.gen.go new file mode 100644 index 0000000000..8417cfc575 --- /dev/null +++ b/private/buf/cmd/buf/internal/internaltesting/usage.gen.go @@ -0,0 +1,19 @@ +// Copyright 2020-2021 Buf Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Generated. DO NOT EDIT. + +package internaltesting + +import _ "github.com/bufbuild/buf/private/usage" diff --git a/private/buf/cmd/buf/testdata/config/config.yaml b/private/buf/cmd/buf/testdata/config/config.yaml deleted file mode 100644 index e69de29bb2..0000000000 From 655f854e4bb4b91cf0ff5daa546906cbb4daf930 Mon Sep 17 00:00:00 2001 From: bufdev Date: Thu, 30 Sep 2021 17:08:02 -0400 Subject: [PATCH 03/14] Add imageProvider --- private/buf/bufgen/generator.go | 35 +++------------- private/buf/bufgen/image_provider.go | 40 +++++++++++++++++++ .../appprotoos/bucket_response_writer.go | 5 +-- 3 files changed, 47 insertions(+), 33 deletions(-) create mode 100644 private/buf/bufgen/image_provider.go diff --git a/private/buf/bufgen/generator.go b/private/buf/bufgen/generator.go index c34c78702f..6839bb46d4 100644 --- a/private/buf/bufgen/generator.go +++ b/private/buf/bufgen/generator.go @@ -140,23 +140,7 @@ func (g *generator) execPlugins( image bufimage.Image, includeImports bool, ) ([]*pluginpb.CodeGeneratorResponse, error) { - // Cache imagesByDir up-front if at least one plugin requires it - var imagesByDir []bufimage.Image - for _, pluginConfig := range config.PluginConfigs { - if pluginConfig.Strategy != StrategyDirectory { - continue - } - // Already guaranteed by config validation, but best to be safe. - if pluginConfig.Remote != "" { - return nil, fmt.Errorf("remote plugin %q cannot set strategy directory", pluginConfig.Remote) - } - var err error - imagesByDir, err = bufimage.ImageByDir(image) - if err != nil { - return nil, err - } - break - } + imageProvider := newImageProvider(image) // Collect all of the plugin jobs so that they can be executed in parallel. jobs := make([]func(context.Context) error, 0, len(config.PluginConfigs)) responses := make([]*pluginpb.CodeGeneratorResponse, len(config.PluginConfigs)) @@ -185,8 +169,7 @@ func (g *generator) execPlugins( ctx, container, g.appprotoosGenerator, - image, - imagesByDir, + imageProvider, currentPluginConfig, includeImports, ) @@ -230,19 +213,13 @@ func (g *generator) execLocalPlugin( ctx context.Context, container app.EnvStdioContainer, appprotoosGenerator appprotoos.Generator, - image bufimage.Image, - imagesByDir []bufimage.Image, + imageProvider *imageProvider, pluginConfig *PluginConfig, includeImports bool, ) (*pluginpb.CodeGeneratorResponse, error) { - var pluginImages []bufimage.Image - switch pluginConfig.Strategy { - case StrategyAll: - pluginImages = []bufimage.Image{image} - case StrategyDirectory: - pluginImages = imagesByDir - default: - return nil, fmt.Errorf("unknown strategy: %v", pluginConfig.Strategy) + pluginImages, err := imageProvider.GetImages(pluginConfig.Strategy) + if err != nil { + return nil, err } response, err := appprotoosGenerator.Generate( ctx, diff --git a/private/buf/bufgen/image_provider.go b/private/buf/bufgen/image_provider.go new file mode 100644 index 0000000000..b9773df960 --- /dev/null +++ b/private/buf/bufgen/image_provider.go @@ -0,0 +1,40 @@ +package bufgen + +import ( + "fmt" + "sync" + + "github.com/bufbuild/buf/private/bufpkg/bufimage" +) + +type imageProvider struct { + image bufimage.Image + imagesByDir []bufimage.Image + lock sync.Mutex +} + +func newImageProvider(image bufimage.Image) *imageProvider { + return &imageProvider{ + image: image, + } +} + +func (p *imageProvider) GetImages(strategy Strategy) ([]bufimage.Image, error) { + switch strategy { + case StrategyAll: + return []bufimage.Image{p.image}, nil + case StrategyDirectory: + p.lock.Lock() + defer p.lock.Unlock() + if p.imagesByDir == nil { + var err error + p.imagesByDir, err = bufimage.ImageByDir(p.image) + if err != nil { + return nil, err + } + } + return p.imagesByDir, nil + default: + return nil, fmt.Errorf("unknown strategy: %v", strategy) + } +} diff --git a/private/pkg/app/appproto/appprotoos/bucket_response_writer.go b/private/pkg/app/appproto/appprotoos/bucket_response_writer.go index 8dafdb9b0b..bed13c8c61 100644 --- a/private/pkg/app/appproto/appprotoos/bucket_response_writer.go +++ b/private/pkg/app/appproto/appprotoos/bucket_response_writer.go @@ -32,23 +32,20 @@ import ( ) type bucketResponseWriter struct { - lock sync.RWMutex logger *zap.Logger storageosProvider storageos.Provider responseHandler appproto.ResponseHandler - // If set, create directories if they don't already exist. createOutDirIfNotExists bool - // Cache the readWriteBuckets by their respective output paths. // These builders are transformed to storage.ReadBuckets and written // to disk once the bucketResponseWriter is flushed. readWriteBuckets map[string]storage.ReadWriteBucket - // Cache the functions used to flush all of the responses to disk. // This holds all of the buckets in-memory so that we only write // the results to disk if all of the responses are successful. closers []func() error + lock sync.RWMutex } func newBucketResponseWriter( From b713f42b17622546c3e8afeb8d2b429f885d2dcb Mon Sep 17 00:00:00 2001 From: bufdev Date: Thu, 30 Sep 2021 17:11:03 -0400 Subject: [PATCH 04/14] make generate --- private/buf/bufgen/image_provider.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/private/buf/bufgen/image_provider.go b/private/buf/bufgen/image_provider.go index b9773df960..d9da096762 100644 --- a/private/buf/bufgen/image_provider.go +++ b/private/buf/bufgen/image_provider.go @@ -1,3 +1,17 @@ +// Copyright 2020-2021 Buf Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + package bufgen import ( From 3b6104257dfd642a6076f2379c11bdff5fc20576 Mon Sep 17 00:00:00 2001 From: Alex McKinney Date: Thu, 30 Sep 2021 18:39:15 -0400 Subject: [PATCH 05/14] Rename types, add tests, and include more comments --- private/buf/bufgen/generator.go | 59 +++-- private/buf/bufgen/image_provider.go | 6 + .../cmd/buf/command/generate/generate_test.go | 33 +++ .../main.go | 2 +- .../protoc-gen-insertion-point-writer/main.go | 2 +- private/buf/cmd/buf/command/protoc/plugin.go | 5 +- private/buf/cmd/buf/command/protoc/protoc.go | 6 +- .../buf/cmd/buf/command/protoc/protoc_test.go | 62 ++++++ .../cmd/protoc-gen-buf-breaking/breaking.go | 4 +- private/buf/cmd/protoc-gen-buf-lint/lint.go | 4 +- .../buf/cmd/protoc-gen-buf-lint/lint_test.go | 2 +- private/pkg/app/appproto/appproto.go | 62 +++--- .../app/appproto/appprotoexec/appprotoexec.go | 98 +++------ .../appproto/appprotoexec/binary_handler.go | 2 +- .../{appprotoos => appprotoexec}/generator.go | 17 +- .../pkg/app/appproto/appprotoexec/handler.go | 61 ++++++ .../appprotoexec/protoc_proxy_handler.go | 2 +- .../pkg/app/appproto/appprotoos/appprotoos.go | 63 ++---- ..._response_writer.go => response_writer.go} | 66 +++--- private/pkg/app/appproto/generator.go | 6 +- private/pkg/app/appproto/response_builder.go | 145 +++++++++++++ private/pkg/app/appproto/response_handler.go | 154 ------------- private/pkg/app/appproto/response_writer.go | 205 +++++++++--------- private/pkg/protogenutil/protogenutil.go | 2 +- 24 files changed, 587 insertions(+), 481 deletions(-) rename private/pkg/app/appproto/{appprotoos => appprotoexec}/generator.go (80%) create mode 100644 private/pkg/app/appproto/appprotoexec/handler.go rename private/pkg/app/appproto/appprotoos/{bucket_response_writer.go => response_writer.go} (79%) create mode 100644 private/pkg/app/appproto/response_builder.go delete mode 100644 private/pkg/app/appproto/response_handler.go diff --git a/private/buf/bufgen/generator.go b/private/buf/bufgen/generator.go index 6839bb46d4..3b8716c40b 100644 --- a/private/buf/bufgen/generator.go +++ b/private/buf/bufgen/generator.go @@ -35,11 +35,10 @@ import ( ) type generator struct { - logger *zap.Logger - storageosProvider storageos.Provider - appprotoosGenerator appprotoos.Generator - responseHandler appproto.ResponseHandler - registryProvider registryv1alpha1apiclient.Provider + logger *zap.Logger + storageosProvider storageos.Provider + appprotoexecGenerator appprotoexec.Generator + registryProvider registryv1alpha1apiclient.Provider } func newGenerator( @@ -48,14 +47,30 @@ func newGenerator( registryProvider registryv1alpha1apiclient.Provider, ) *generator { return &generator{ - logger: logger, - storageosProvider: storageosProvider, - appprotoosGenerator: appprotoos.NewGenerator(logger, storageosProvider), - responseHandler: appproto.NewResponseHandler(logger), - registryProvider: registryProvider, + logger: logger, + storageosProvider: storageosProvider, + appprotoexecGenerator: appprotoexec.NewGenerator(logger, storageosProvider), + registryProvider: registryProvider, } } +// Generate executes all of the plugins specified by the given Config, and +// consolidates the results in the same order that the plugins are listed. +// Order is particularly important for insertion points, which are used to +// modify the generted output from other plugins executed earlier in the chain. +// +// Note that insertion points will only have access to files that are written +// in the same protoc invocation; plugins will not be able to insert code into +// other files that already exist on disk (just like protoc). +// +// All of the plugins, both local and remote, are called concurrently. Each +// plugin returns a single CodeGeneratorResponse, which are cached in-memory in +// the appprotoos.ResponseWriter. Once all of the CodeGeneratorResponses +// are written in-memory, we flush them to the OS filesystem by closing the +// appprotoos.ResponseWriter. +// +// This behavior is equivalent to protoc, which only writes out the content +// for each of the plugins if all of the plugins are successful. func (g *generator) Generate( ctx context.Context, container app.EnvStdioContainer, @@ -77,10 +92,6 @@ func (g *generator) Generate( ) } -// generate executes all of the plugins specified by the given Config, and -// consolidates the results in the same order that the plugins are listed. -// Order is particularly important for insertion points, which are used to -// modify the generted output from other plugins executed earlier in the chain. func (g *generator) generate( ctx context.Context, container app.EnvStdioContainer, @@ -95,7 +106,7 @@ func (g *generator) generate( responses, err := g.execPlugins( ctx, container, - g.appprotoosGenerator, + g.appprotoexecGenerator, config, image, includeImports, @@ -104,10 +115,10 @@ func (g *generator) generate( return err } // Apply the CodeGeneratorResponses in the order they were specified. - bucketResponseWriter := appprotoos.NewBucketResponseWriter( + responseWriter := appprotoos.NewResponseWriter( g.logger, g.storageosProvider, - appprotoos.BucketResponseWriterWithCreateOutDirIfNotExists(), + appprotoos.ResponseWriterWithCreateOutDirIfNotExists(), ) for i, pluginConfig := range config.PluginConfigs { out := pluginConfig.Out @@ -118,7 +129,7 @@ func (g *generator) generate( if response == nil { return fmt.Errorf("failed to get plugin response for %s", pluginConfig.PluginName()) } - if err := bucketResponseWriter.AddResponse( + if err := responseWriter.AddResponse( ctx, response, out, @@ -126,7 +137,7 @@ func (g *generator) generate( return fmt.Errorf("plugin %s: %v", pluginConfig.PluginName(), err) } } - if err := bucketResponseWriter.Close(); err != nil { + if err := responseWriter.Close(); err != nil { return err } return nil @@ -135,7 +146,7 @@ func (g *generator) generate( func (g *generator) execPlugins( ctx context.Context, container app.EnvStdioContainer, - appprotoosGenerator appprotoos.Generator, + appprotoexecGenerator appprotoexec.Generator, config *Config, image bufimage.Image, includeImports bool, @@ -168,7 +179,7 @@ func (g *generator) execPlugins( response, err := g.execLocalPlugin( ctx, container, - g.appprotoosGenerator, + g.appprotoexecGenerator, imageProvider, currentPluginConfig, includeImports, @@ -212,7 +223,7 @@ func (g *generator) execPlugins( func (g *generator) execLocalPlugin( ctx context.Context, container app.EnvStdioContainer, - appprotoosGenerator appprotoos.Generator, + appprotoexecGenerator appprotoexec.Generator, imageProvider *imageProvider, pluginConfig *PluginConfig, includeImports bool, @@ -221,7 +232,7 @@ func (g *generator) execLocalPlugin( if err != nil { return nil, err } - response, err := appprotoosGenerator.Generate( + response, err := appprotoexecGenerator.Generate( ctx, container, pluginConfig.Name, @@ -231,7 +242,7 @@ func (g *generator) execLocalPlugin( appprotoexec.DefaultVersion, includeImports, ), - appprotoos.GenerateWithPluginPath(pluginConfig.Path), + appprotoexec.GenerateWithPluginPath(pluginConfig.Path), ) if err != nil { return nil, fmt.Errorf("plugin %s: %v", pluginConfig.PluginName(), err) diff --git a/private/buf/bufgen/image_provider.go b/private/buf/bufgen/image_provider.go index d9da096762..46849204c6 100644 --- a/private/buf/bufgen/image_provider.go +++ b/private/buf/bufgen/image_provider.go @@ -21,6 +21,12 @@ import ( "github.com/bufbuild/buf/private/bufpkg/bufimage" ) +// imageProvider is used to provide the images used +// when generating with a local plugin. Each plugin is +// in control of its own Strategy - we cache the +// imagesByDir so that we only have to build it once for +// all of the plugins that configure the the Directory +// strategy. type imageProvider struct { image bufimage.Image imagesByDir []bufimage.Image diff --git a/private/buf/cmd/buf/command/generate/generate_test.go b/private/buf/cmd/buf/command/generate/generate_test.go index 0b1cc2328f..ad91bfeb40 100644 --- a/private/buf/cmd/buf/command/generate/generate_test.go +++ b/private/buf/cmd/buf/command/generate/generate_test.go @@ -204,6 +204,39 @@ plugins: ) } +func TestGenerateInsertionPointMixedPathsFail(t *testing.T) { + wd, err := os.Getwd() + require.NoError(t, err) + testGenerateInsertionPointMixedPathsFail(t, ".", wd) + testGenerateInsertionPointMixedPathsFail(t, wd, ".") +} + +// testGenerateInsertionPointMixedPathsFail demonstrates that insertion points are only +// able to generate to the same output directory, even if the absolute path points to the +// same place. This is equivalent to protoc's behavior. +func testGenerateInsertionPointMixedPathsFail(t *testing.T, receiverOut string, writerOut string) { + successTemplate := ` +version: v1 +plugins: + - name: insertion-point-receiver + out: %s + - name: insertion-point-writer + out: %s +` + testRunStdoutStderr( + t, + nil, + 1, + ``, + `Failure: plugin insertion-point-writer: test.txt: does not exist`, + filepath.Join("testdata", "simple"), // The input directory is irrelevant for these insertion points. + "--template", + fmt.Sprintf(successTemplate, receiverOut, writerOut), + "-o", + t.TempDir(), + ) +} + func testCompareGeneratedStubs( t *testing.T, dirPath string, diff --git a/private/buf/cmd/buf/command/protoc/internal/protoc-gen-insertion-point-receiver/main.go b/private/buf/cmd/buf/command/protoc/internal/protoc-gen-insertion-point-receiver/main.go index 97ccd44b9b..c154c6a375 100644 --- a/private/buf/cmd/buf/command/protoc/internal/protoc-gen-insertion-point-receiver/main.go +++ b/private/buf/cmd/buf/command/protoc/internal/protoc-gen-insertion-point-receiver/main.go @@ -30,7 +30,7 @@ func main() { func handle( ctx context.Context, container app.EnvStderrContainer, - responseWriter appproto.ResponseWriter, + responseWriter appproto.ResponseBuilder, request *pluginpb.CodeGeneratorRequest, ) error { return responseWriter.AddFile( diff --git a/private/buf/cmd/buf/command/protoc/internal/protoc-gen-insertion-point-writer/main.go b/private/buf/cmd/buf/command/protoc/internal/protoc-gen-insertion-point-writer/main.go index 947ae62998..d95e84f7cd 100644 --- a/private/buf/cmd/buf/command/protoc/internal/protoc-gen-insertion-point-writer/main.go +++ b/private/buf/cmd/buf/command/protoc/internal/protoc-gen-insertion-point-writer/main.go @@ -30,7 +30,7 @@ func main() { func handle( ctx context.Context, container app.EnvStderrContainer, - responseWriter appproto.ResponseWriter, + responseWriter appproto.ResponseBuilder, request *pluginpb.CodeGeneratorRequest, ) error { return responseWriter.AddFile( diff --git a/private/buf/cmd/buf/command/protoc/plugin.go b/private/buf/cmd/buf/command/protoc/plugin.go index b451ce4797..5ddf412c92 100644 --- a/private/buf/cmd/buf/command/protoc/plugin.go +++ b/private/buf/cmd/buf/command/protoc/plugin.go @@ -22,7 +22,6 @@ import ( "github.com/bufbuild/buf/private/bufpkg/bufimage" "github.com/bufbuild/buf/private/pkg/app" "github.com/bufbuild/buf/private/pkg/app/appproto/appprotoexec" - "github.com/bufbuild/buf/private/pkg/app/appproto/appprotoos" "github.com/bufbuild/buf/private/pkg/storage/storageos" "go.uber.org/zap" "google.golang.org/protobuf/types/pluginpb" @@ -50,7 +49,7 @@ func executePlugin( pluginName string, pluginInfo *pluginInfo, ) (*pluginpb.CodeGeneratorResponse, error) { - response, err := appprotoos.NewGenerator( + response, err := appprotoexec.NewGenerator( logger, storageosProvider, ).Generate( @@ -63,7 +62,7 @@ func executePlugin( appprotoexec.DefaultVersion, false, ), - appprotoos.GenerateWithPluginPath(pluginInfo.Path), + appprotoexec.GenerateWithPluginPath(pluginInfo.Path), ) if err != nil { return nil, fmt.Errorf("--%s_out: %v", pluginName, err) diff --git a/private/buf/cmd/buf/command/protoc/protoc.go b/private/buf/cmd/buf/command/protoc/protoc.go index 0992aef8c4..f65e9719c4 100644 --- a/private/buf/cmd/buf/command/protoc/protoc.go +++ b/private/buf/cmd/buf/command/protoc/protoc.go @@ -201,7 +201,7 @@ func run( if err := appproto.ValidatePluginResponses(pluginResponses); err != nil { return err } - bucketResponseWriter := appprotoos.NewBucketResponseWriter( + responseWriter := appprotoos.NewResponseWriter( container.Logger(), storageosProvider, ) @@ -210,7 +210,7 @@ func run( if !ok { return fmt.Errorf("no value in PluginNamesToPluginInfo for %q", pluginResponse.PluginName) } - if err := bucketResponseWriter.AddResponse( + if err := responseWriter.AddResponse( ctx, pluginResponse.Response, pluginInfo.Out, @@ -218,7 +218,7 @@ func run( return err } } - if err := bucketResponseWriter.Close(); err != nil { + if err := responseWriter.Close(); err != nil { return err } return nil diff --git a/private/buf/cmd/buf/command/protoc/protoc_test.go b/private/buf/cmd/buf/command/protoc/protoc_test.go index bd96a2435b..d87c2bb0ac 100644 --- a/private/buf/cmd/buf/command/protoc/protoc_test.go +++ b/private/buf/cmd/buf/command/protoc/protoc_test.go @@ -18,6 +18,7 @@ import ( "bytes" "context" "fmt" + "io" "os" "path/filepath" "testing" @@ -195,6 +196,67 @@ func TestCompareInsertionPointOutput(t *testing.T) { ) } +func TestInsertionPointMixedPathsFail(t *testing.T) { + testingextended.SkipIfShort(t) + t.Parallel() + wd, err := os.Getwd() + require.NoError(t, err) + testInsertionPointMixedPathsFail(t, ".", wd) + testInsertionPointMixedPathsFail(t, wd, ".") +} + +// testInsertionPointMixedPathsFail demonstrates that insertion points are only +// able to generate to the same output directory, even if the absolute path points +// to the same place. +func testInsertionPointMixedPathsFail(t *testing.T, receiverOut string, writerOut string) { + dirPath := filepath.Join("testdata", "insertion") + filePaths := buftesting.GetProtocFilePaths(t, dirPath, 100) + protocFlags := []string{ + fmt.Sprintf("--%s_out=%s", "insertion-point-receiver", receiverOut), + fmt.Sprintf("--%s_out=%s", "insertion-point-writer", writerOut), + } + err := prototesting.RunProtoc( + context.Background(), + []string{dirPath}, + filePaths, + false, + false, + map[string]string{ + "PATH": os.Getenv("PATH"), + }, + nil, + protocFlags..., + ) + require.Error(t, err) + appcmdtesting.RunCommandExitCode( + t, + func(name string) *appcmd.Command { + return NewCommand( + name, + appflag.NewBuilder(name), + ) + }, + 1, + func(string) map[string]string { + return map[string]string{ + "PATH": os.Getenv("PATH"), + } + }, + nil, + nil, + io.Discard, + append( + append( + protocFlags, + "-I", + dirPath, + "--by-dir", + ), + filePaths..., + )..., + ) +} + func testCompareGeneratedStubs( t *testing.T, dirPath string, diff --git a/private/buf/cmd/protoc-gen-buf-breaking/breaking.go b/private/buf/cmd/protoc-gen-buf-breaking/breaking.go index 32d279b321..dcb3460c5a 100644 --- a/private/buf/cmd/protoc-gen-buf-breaking/breaking.go +++ b/private/buf/cmd/protoc-gen-buf-breaking/breaking.go @@ -47,7 +47,7 @@ func Main() { func( ctx context.Context, container app.EnvStderrContainer, - responseWriter appproto.ResponseWriter, + responseWriter appproto.ResponseBuilder, request *pluginpb.CodeGeneratorRequest, ) error { return handle( @@ -64,7 +64,7 @@ func Main() { func handle( ctx context.Context, container app.EnvStderrContainer, - responseWriter appproto.ResponseWriter, + responseWriter appproto.ResponseBuilder, request *pluginpb.CodeGeneratorRequest, ) error { responseWriter.SetFeatureProto3Optional() diff --git a/private/buf/cmd/protoc-gen-buf-lint/lint.go b/private/buf/cmd/protoc-gen-buf-lint/lint.go index dd32e0d85b..cbc6c7d24d 100644 --- a/private/buf/cmd/protoc-gen-buf-lint/lint.go +++ b/private/buf/cmd/protoc-gen-buf-lint/lint.go @@ -43,7 +43,7 @@ func Main() { func( ctx context.Context, container app.EnvStderrContainer, - responseWriter appproto.ResponseWriter, + responseWriter appproto.ResponseBuilder, request *pluginpb.CodeGeneratorRequest, ) error { return handle( @@ -60,7 +60,7 @@ func Main() { func handle( ctx context.Context, container app.EnvStderrContainer, - responseWriter appproto.ResponseWriter, + responseWriter appproto.ResponseBuilder, request *pluginpb.CodeGeneratorRequest, ) error { responseWriter.SetFeatureProto3Optional() 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 f47e2ead95..0c85e45717 100644 --- a/private/buf/cmd/protoc-gen-buf-lint/lint_test.go +++ b/private/buf/cmd/protoc-gen-buf-lint/lint_test.go @@ -190,7 +190,7 @@ func testRunLint( func( ctx context.Context, container app.EnvStderrContainer, - responseWriter appproto.ResponseWriter, + responseWriter appproto.ResponseBuilder, request *pluginpb.CodeGeneratorRequest, ) error { return handle( diff --git a/private/pkg/app/appproto/appproto.go b/private/pkg/app/appproto/appproto.go index 6ff5d208e3..d6dd38da65 100644 --- a/private/pkg/app/appproto/appproto.go +++ b/private/pkg/app/appproto/appproto.go @@ -15,7 +15,8 @@ // Package appproto contains helper functionality for protoc plugins. // // Note this is currently implicitly tested through buf's protoc command. -// If this were split out into a separate package, testing would need to be moved to this package. +// If this were split out into a separate package, testing would need to be +// moved to this package. package appproto import ( @@ -45,8 +46,8 @@ const ( averageInsertionPointSize = 1024 ) -// ResponseWriter handles CodeGeneratorResponses. -type ResponseWriter interface { +// ResponseBuilder builds CodeGeneratorResponses. +type ResponseBuilder interface { // Add adds the file to the response. // // Returns error if nil or the name is empty. @@ -64,7 +65,7 @@ type ResponseWriter interface { toResponse() *pluginpb.CodeGeneratorResponse } -// Handler is a protoc plugin handler +// Handler is a protoc plugin handler. type Handler interface { // Handle handles the plugin. // @@ -75,7 +76,7 @@ type Handler interface { Handle( ctx context.Context, container app.EnvStderrContainer, - responseWriter ResponseWriter, + responseWriter ResponseBuilder, request *pluginpb.CodeGeneratorRequest, ) error } @@ -84,7 +85,7 @@ type Handler interface { type HandlerFunc func( context.Context, app.EnvStderrContainer, - ResponseWriter, + ResponseBuilder, *pluginpb.CodeGeneratorRequest, ) error @@ -92,7 +93,7 @@ type HandlerFunc func( func (h HandlerFunc) Handle( ctx context.Context, container app.EnvStderrContainer, - responseWriter ResponseWriter, + responseWriter ResponseBuilder, request *pluginpb.CodeGeneratorRequest, ) error { return h(ctx, container, responseWriter, request) @@ -116,6 +117,10 @@ func Run(ctx context.Context, container app.Container, handler Handler) error { // result is combined into one response that is written. type Generator interface { // Generate generates a CodeGeneratorResponse for the given CodeGeneratorRequests. + // + // A new ResponseBuilder is constructed for every invocation of Generate and is + // used to consolidate all of the CodeGeneratorResponse_Files returned from a single + // plugin into a single CodeGeneratorResponse. Generate( ctx context.Context, container app.EnvStderrContainer, @@ -131,35 +136,40 @@ func NewGenerator( return newGenerator(logger, handler) } -// ResponseHandler handles the response and writes it to the given storage.WriteBucket +// ResponseWriter handles the response and writes it to the given storage.WriteBucket // without executing any plugins and handles insertion points as needed. -type ResponseHandler interface { - // HandleResponse writes to the bucket with the given response. - HandleResponse( +type ResponseWriter interface { + // WriteResponse writes to the bucket with the given response. In practice, the + // WriteBucket is most often an in-memory bucket. + // + // CodeGeneratorResponses are consolidated into the bucket, and insertion points + // are applied in-place so that they can only access the files created in a single + // generation invocation (just like protoc). + WriteResponse( ctx context.Context, writeBucket storage.WriteBucket, response *pluginpb.CodeGeneratorResponse, - options ...HandleResponseOption, + options ...WriteResponseOption, ) error } -// NewResponseHandler returns a new ResponseHandler. -func NewResponseHandler(logger *zap.Logger) ResponseHandler { - return newResponseHandler(logger) +// NewResponseWriter returns a new ResponseWriter. +func NewResponseWriter(logger *zap.Logger) ResponseWriter { + return newResponseWriter(logger) } -// HandleResponseOption is an option for HandleResponse. -type HandleResponseOption func(*handleResponseOptions) +// WriteResponseOption is an option for WriteResponse. +type WriteResponseOption func(*writeResponseOptions) -// HandleResponseWithInsertionPointReadBucket returns a new HandleResponseOption that uses the given +// WriteResponseWithInsertionPointReadBucket returns a new WriteResponseOption that uses the given // ReadBucket to read from for insertion points. // // If this is not specified, insertion points are not supported. -func HandleResponseWithInsertionPointReadBucket( +func WriteResponseWithInsertionPointReadBucket( insertionPointReadBucket storage.ReadBucket, -) HandleResponseOption { - return func(handleResponseOptions *handleResponseOptions) { - handleResponseOptions.insertionPointReadBucket = insertionPointReadBucket +) WriteResponseOption { + return func(writeResponseOptions *writeResponseOptions) { + writeResponseOptions.insertionPointReadBucket = insertionPointReadBucket } } @@ -218,7 +228,7 @@ func newRunFunc(handler Handler) func(context.Context, app.Container) error { if err := protodescriptor.ValidateCodeGeneratorRequest(request); err != nil { return err } - responseWriter := newResponseWriter(container) + responseWriter := newResponseBuilder(container) if err := handler.Handle(ctx, container, responseWriter, request); err != nil { return err } @@ -235,9 +245,9 @@ func newRunFunc(handler Handler) func(context.Context, app.Container) error { } } -// NewResponseWriter returns a new ResponseWriter. -func NewResponseWriter(container app.StderrContainer) ResponseWriter { - return newResponseWriter(container) +// NewResponseBuilder returns a new ResponseBuilder. +func NewResponseBuilder(container app.StderrContainer) ResponseBuilder { + return newResponseBuilder(container) } // leadingWhitespace iterates through the given string, diff --git a/private/pkg/app/appproto/appprotoexec/appprotoexec.go b/private/pkg/app/appproto/appprotoexec/appprotoexec.go index 5b14962887..6672c9a2bb 100644 --- a/private/pkg/app/appproto/appprotoexec/appprotoexec.go +++ b/private/pkg/app/appproto/appprotoexec/appprotoexec.go @@ -19,12 +19,12 @@ package appprotoexec import ( - "fmt" - "os/exec" + "context" - "github.com/bufbuild/buf/private/pkg/app/appproto" + "github.com/bufbuild/buf/private/pkg/app" "github.com/bufbuild/buf/private/pkg/storage/storageos" "go.uber.org/zap" + "google.golang.org/protobuf/types/pluginpb" ) const ( @@ -62,76 +62,36 @@ var ( ) ) -// NewHandler returns a new Handler based on the plugin name and optional path. -// -// protocPath and pluginPath are optional. -// -// - If the plugin path is set, this returns a new binary handler for that path. -// - If the plugin path is unset, this does exec.LookPath for a binary named protoc-gen-pluginName, -// and if one is found, a new binary handler is returned for this. -// - Else, if the name is in ProtocProxyPluginNames, this returns a new protoc proxy handler. -// - Else, this returns error. -func NewHandler( +// Generator is used to generate code with plugins found on the local filesystem. +type Generator interface { + // Generate generates a CodeGeneratorResponse for the given pluginName. The + // pluginName must be available on the system's PATH or one of the plugins + // built-in to protoc. The plugin path can be overridden via the + // GenerateWithPluginPath option. + Generate( + ctx context.Context, + container app.EnvStderrContainer, + pluginName string, + requests []*pluginpb.CodeGeneratorRequest, + options ...GenerateOption, + ) (*pluginpb.CodeGeneratorResponse, error) +} + +// NewGenerator returns a new Generator. +func NewGenerator( logger *zap.Logger, storageosProvider storageos.Provider, - pluginName string, - options ...HandlerOption, -) (appproto.Handler, error) { - handlerOptions := newHandlerOptions() - for _, option := range options { - option(handlerOptions) - } - if handlerOptions.pluginPath != "" { - pluginPath, err := exec.LookPath(handlerOptions.pluginPath) - if err != nil { - return nil, err - } - return newBinaryHandler(logger, pluginPath), nil - } - pluginPath, err := exec.LookPath("protoc-gen-" + pluginName) - if err == nil { - return newBinaryHandler(logger, pluginPath), nil - } - // we always look for protoc-gen-X first, but if not, check the builtins - if _, ok := ProtocProxyPluginNames[pluginName]; ok { - if handlerOptions.protocPath == "" { - handlerOptions.protocPath = "protoc" - } - protocPath, err := exec.LookPath(handlerOptions.protocPath) - if err != nil { - return nil, err - } - return newProtocProxyHandler(logger, storageosProvider, protocPath, pluginName), nil - } - return nil, fmt.Errorf("could not find protoc plugin for name %s", pluginName) +) Generator { + return newGenerator(logger, storageosProvider) } -// HandlerOption is an option for a new Handler. -type HandlerOption func(*handlerOptions) +// GenerateOption is an option for Generate. +type GenerateOption func(*generateOptions) -// HandlerWithProtocPath returns a new HandlerOption that sets the path to the protoc binary. -// -// The default is to do exec.LookPath on "protoc". -func HandlerWithProtocPath(protocPath string) HandlerOption { - return func(handlerOptions *handlerOptions) { - handlerOptions.protocPath = protocPath +// GenerateWithPluginPath returns a new GenerateOption that uses the given +// path to the plugin. +func GenerateWithPluginPath(pluginPath string) GenerateOption { + return func(generateOptions *generateOptions) { + generateOptions.pluginPath = pluginPath } } - -// HandlerWithPluginPath returns a new HandlerOption that sets the path to the plugin binary. -// -// The default is to do exec.LookPath on "protoc-gen-" + pluginName. -func HandlerWithPluginPath(pluginPath string) HandlerOption { - return func(handlerOptions *handlerOptions) { - handlerOptions.pluginPath = pluginPath - } -} - -type handlerOptions struct { - protocPath string - pluginPath string -} - -func newHandlerOptions() *handlerOptions { - return &handlerOptions{} -} diff --git a/private/pkg/app/appproto/appprotoexec/binary_handler.go b/private/pkg/app/appproto/appprotoexec/binary_handler.go index 08c9235866..c700d3a999 100644 --- a/private/pkg/app/appproto/appprotoexec/binary_handler.go +++ b/private/pkg/app/appproto/appprotoexec/binary_handler.go @@ -46,7 +46,7 @@ func newBinaryHandler( func (h *binaryHandler) Handle( ctx context.Context, container app.EnvStderrContainer, - responseWriter appproto.ResponseWriter, + responseWriter appproto.ResponseBuilder, request *pluginpb.CodeGeneratorRequest, ) error { ctx, span := trace.StartSpan(ctx, "plugin_proxy") diff --git a/private/pkg/app/appproto/appprotoos/generator.go b/private/pkg/app/appproto/appprotoexec/generator.go similarity index 80% rename from private/pkg/app/appproto/appprotoos/generator.go rename to private/pkg/app/appproto/appprotoexec/generator.go index 51da11af87..9795d85ffc 100644 --- a/private/pkg/app/appproto/appprotoos/generator.go +++ b/private/pkg/app/appproto/appprotoexec/generator.go @@ -12,29 +12,18 @@ // See the License for the specific language governing permissions and // limitations under the License. -package appprotoos +package appprotoexec import ( "context" "github.com/bufbuild/buf/private/pkg/app" "github.com/bufbuild/buf/private/pkg/app/appproto" - "github.com/bufbuild/buf/private/pkg/app/appproto/appprotoexec" - "github.com/bufbuild/buf/private/pkg/normalpath" "github.com/bufbuild/buf/private/pkg/storage/storageos" "go.uber.org/zap" "google.golang.org/protobuf/types/pluginpb" ) -// Constants used to create .jar files -var ( - ManifestPath = normalpath.Join("META-INF", "MANIFEST.MF") - ManifestContent = []byte(`Manifest-Version: 1.0 -Created-By: 1.6.0 (protoc) - -`) -) - type generator struct { logger *zap.Logger storageosProvider storageos.Provider @@ -61,11 +50,11 @@ func (g *generator) Generate( for _, option := range options { option(generateOptions) } - handler, err := appprotoexec.NewHandler( + handler, err := newHandler( g.logger, g.storageosProvider, pluginName, - appprotoexec.HandlerWithPluginPath(generateOptions.pluginPath), + generateOptions.pluginPath, ) if err != nil { return nil, err diff --git a/private/pkg/app/appproto/appprotoexec/handler.go b/private/pkg/app/appproto/appprotoexec/handler.go new file mode 100644 index 0000000000..a58f5243e1 --- /dev/null +++ b/private/pkg/app/appproto/appprotoexec/handler.go @@ -0,0 +1,61 @@ +// Copyright 2020-2021 Buf Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package appprotoexec + +import ( + "fmt" + "os/exec" + + "github.com/bufbuild/buf/private/pkg/app/appproto" + "github.com/bufbuild/buf/private/pkg/storage/storageos" + "go.uber.org/zap" +) + +// newHandler returns a new appproto.Handler based on the plugin name and optional path. +// +// pluginPath is optional. +// +// - If the plugin path is set, this returns a new binary handler for that path. +// - If the plugin path is unset, this does exec.LookPath for a binary named protoc-gen-pluginName, +// and if one is found, a new binary handler is returned for this. +// - Else, if the name is in ProtocProxyPluginNames, this returns a new protoc proxy handler. +// - Else, this returns error. +func newHandler( + logger *zap.Logger, + storageosProvider storageos.Provider, + pluginName string, + pluginPath string, +) (appproto.Handler, error) { + if pluginPath != "" { + pluginPath, err := exec.LookPath(pluginPath) + if err != nil { + return nil, err + } + return newBinaryHandler(logger, pluginPath), nil + } + pluginPath, err := exec.LookPath("protoc-gen-" + pluginName) + if err == nil { + return newBinaryHandler(logger, pluginPath), nil + } + // we always look for protoc-gen-X first, but if not, check the builtins + if _, ok := ProtocProxyPluginNames[pluginName]; ok { + protocPath, err := exec.LookPath("protoc") + if err != nil { + return nil, err + } + return newProtocProxyHandler(logger, storageosProvider, protocPath, pluginName), nil + } + return nil, fmt.Errorf("could not find protoc plugin for name %s", pluginName) +} diff --git a/private/pkg/app/appproto/appprotoexec/protoc_proxy_handler.go b/private/pkg/app/appproto/appprotoexec/protoc_proxy_handler.go index cb1b897102..8f66a110e3 100644 --- a/private/pkg/app/appproto/appprotoexec/protoc_proxy_handler.go +++ b/private/pkg/app/appproto/appprotoexec/protoc_proxy_handler.go @@ -62,7 +62,7 @@ func newProtocProxyHandler( func (h *protocProxyHandler) Handle( ctx context.Context, container app.EnvStderrContainer, - responseWriter appproto.ResponseWriter, + responseWriter appproto.ResponseBuilder, request *pluginpb.CodeGeneratorRequest, ) (retErr error) { ctx, span := trace.StartSpan(ctx, "protoc_proxy") diff --git a/private/pkg/app/appproto/appprotoos/appprotoos.go b/private/pkg/app/appproto/appprotoos/appprotoos.go index d825e661ba..e2e46d3559 100644 --- a/private/pkg/app/appproto/appprotoos/appprotoos.go +++ b/private/pkg/app/appproto/appprotoos/appprotoos.go @@ -19,50 +19,15 @@ import ( "context" "io" - "github.com/bufbuild/buf/private/pkg/app" "github.com/bufbuild/buf/private/pkg/storage/storageos" "go.uber.org/zap" "google.golang.org/protobuf/types/pluginpb" ) -// Generator is used to generate code to the OS filesystem. -type Generator interface { - // Generate generates to the os filesystem, switching on the file extension. - // If there is a .jar extension, this generates a jar. If there is a .zip - // extension, this generates a zip. If there is no extension, this outputs - // to the directory. The corresponding CodeGeneratorResponse written is returned. - Generate( - ctx context.Context, - container app.EnvStderrContainer, - pluginName string, - requests []*pluginpb.CodeGeneratorRequest, - options ...GenerateOption, - ) (*pluginpb.CodeGeneratorResponse, error) -} - -// NewGenerator returns a new Generator. -func NewGenerator( - logger *zap.Logger, - storageosProvider storageos.Provider, -) Generator { - return newGenerator(logger, storageosProvider) -} - -// GenerateOption is an option for Generate. -type GenerateOption func(*generateOptions) - -// GenerateWithPluginPath returns a new GenerateOption that uses the given -// path to the plugin. -func GenerateWithPluginPath(pluginPath string) GenerateOption { - return func(generateOptions *generateOptions) { - generateOptions.pluginPath = pluginPath - } -} - -// BucketResponseWriter writes CodeGeneratorResponses to the OS filesystem. -type BucketResponseWriter interface { +// ResponseWriter writes CodeGeneratorResponses to the OS filesystem. +type ResponseWriter interface { // Close writes all of the responses to disk. No further calls can be - // made to the BucketResponseWriter after this call. + // made to the ResponseWriter after this call. io.Closer // AddResponse adds the response to the writer, switching on the file extension. @@ -76,26 +41,26 @@ type BucketResponseWriter interface { ) error } -// NewBucketResponseWriter returns a new BucketResponseWriter. -func NewBucketResponseWriter( +// NewResponseWriter returns a new ResponseWriter. +func NewResponseWriter( logger *zap.Logger, storageosProvider storageos.Provider, - options ...BucketResponseWriterOption, -) BucketResponseWriter { - return newBucketResponseWriter( + options ...ResponseWriterOption, +) ResponseWriter { + return newResponseWriter( logger, storageosProvider, options..., ) } -// BucketResponseWriterOption is an option for the BucketResponseWriter. -type BucketResponseWriterOption func(*bucketResponseWriterOptions) +// ResponseWriterOption is an option for the ResponseWriter. +type ResponseWriterOption func(*responseWriterOptions) -// BucketResponseWriterWithCreateOutDirIfNotExists returns a new BucketResponseWriterOption that creates +// ResponseWriterWithCreateOutDirIfNotExists returns a new ResponseWriterOption that creates // the directory if it does not exist. -func BucketResponseWriterWithCreateOutDirIfNotExists() BucketResponseWriterOption { - return func(bucketResponseWriterOptions *bucketResponseWriterOptions) { - bucketResponseWriterOptions.createOutDirIfNotExists = true +func ResponseWriterWithCreateOutDirIfNotExists() ResponseWriterOption { + return func(responseWriterOptions *responseWriterOptions) { + responseWriterOptions.createOutDirIfNotExists = true } } diff --git a/private/pkg/app/appproto/appprotoos/bucket_response_writer.go b/private/pkg/app/appproto/appprotoos/response_writer.go similarity index 79% rename from private/pkg/app/appproto/appprotoos/bucket_response_writer.go rename to private/pkg/app/appproto/appprotoos/response_writer.go index bed13c8c61..807363d712 100644 --- a/private/pkg/app/appproto/appprotoos/bucket_response_writer.go +++ b/private/pkg/app/appproto/appprotoos/response_writer.go @@ -22,6 +22,7 @@ import ( "sync" "github.com/bufbuild/buf/private/pkg/app/appproto" + "github.com/bufbuild/buf/private/pkg/normalpath" "github.com/bufbuild/buf/private/pkg/storage" "github.com/bufbuild/buf/private/pkg/storage/storagearchive" "github.com/bufbuild/buf/private/pkg/storage/storagemem" @@ -31,15 +32,24 @@ import ( "google.golang.org/protobuf/types/pluginpb" ) -type bucketResponseWriter struct { +// Constants used to create .jar files. +var ( + manifestPath = normalpath.Join("META-INF", "MANIFEST.MF") + manifestContent = []byte(`Manifest-Version: 1.0 +Created-By: 1.6.0 (protoc) + +`) +) + +type responseWriter struct { logger *zap.Logger storageosProvider storageos.Provider - responseHandler appproto.ResponseHandler + responseWriter appproto.ResponseWriter // If set, create directories if they don't already exist. createOutDirIfNotExists bool // Cache the readWriteBuckets by their respective output paths. // These builders are transformed to storage.ReadBuckets and written - // to disk once the bucketResponseWriter is flushed. + // to disk once the responseWriter is flushed. readWriteBuckets map[string]storage.ReadWriteBucket // Cache the functions used to flush all of the responses to disk. // This holds all of the buckets in-memory so that we only write @@ -48,25 +58,25 @@ type bucketResponseWriter struct { lock sync.RWMutex } -func newBucketResponseWriter( +func newResponseWriter( logger *zap.Logger, storageosProvider storageos.Provider, - options ...BucketResponseWriterOption, -) *bucketResponseWriter { - bucketResponseWriterOptions := newBucketResponseWriterOptions() + options ...ResponseWriterOption, +) *responseWriter { + responseWriterOptions := newResponseWriterOptions() for _, option := range options { - option(bucketResponseWriterOptions) + option(responseWriterOptions) } - return &bucketResponseWriter{ + return &responseWriter{ logger: logger, storageosProvider: storageosProvider, - responseHandler: appproto.NewResponseHandler(logger), - createOutDirIfNotExists: bucketResponseWriterOptions.createOutDirIfNotExists, + responseWriter: appproto.NewResponseWriter(logger), + createOutDirIfNotExists: responseWriterOptions.createOutDirIfNotExists, readWriteBuckets: make(map[string]storage.ReadWriteBucket), } } -func (w *bucketResponseWriter) AddResponse( +func (w *responseWriter) AddResponse( ctx context.Context, response *pluginpb.CodeGeneratorResponse, pluginOut string, @@ -81,7 +91,7 @@ func (w *bucketResponseWriter) AddResponse( ) } -func (w *bucketResponseWriter) Close() error { +func (w *responseWriter) Close() error { w.lock.Lock() defer w.lock.Unlock() for _, closeFunc := range w.closers { @@ -101,7 +111,7 @@ func (w *bucketResponseWriter) Close() error { return nil } -func (w *bucketResponseWriter) addResponse( +func (w *responseWriter) addResponse( ctx context.Context, response *pluginpb.CodeGeneratorResponse, pluginOut string, @@ -134,7 +144,7 @@ func (w *bucketResponseWriter) addResponse( } } -func (w *bucketResponseWriter) writeZip( +func (w *responseWriter) writeZip( ctx context.Context, response *pluginpb.CodeGeneratorResponse, outFilePath string, @@ -145,11 +155,11 @@ func (w *bucketResponseWriter) writeZip( if readWriteBucket, ok := w.readWriteBuckets[outFilePath]; ok { // We already have a readWriteBucket for this outFilePath, so // we can write to the same bucket. - if err := w.responseHandler.HandleResponse( + if err := w.responseWriter.WriteResponse( ctx, readWriteBucket, response, - appproto.HandleResponseWithInsertionPointReadBucket(readWriteBucket), + appproto.WriteResponseWithInsertionPointReadBucket(readWriteBucket), ); err != nil { return err } @@ -173,15 +183,15 @@ func (w *bucketResponseWriter) writeZip( } readWriteBucket := storagemem.NewReadWriteBucket() if includeManifest { - if err := storage.PutPath(ctx, readWriteBucket, ManifestPath, ManifestContent); err != nil { + if err := storage.PutPath(ctx, readWriteBucket, manifestPath, manifestContent); err != nil { return err } } - if err := w.responseHandler.HandleResponse( + if err := w.responseWriter.WriteResponse( ctx, readWriteBucket, response, - appproto.HandleResponseWithInsertionPointReadBucket(readWriteBucket), + appproto.WriteResponseWithInsertionPointReadBucket(readWriteBucket), ); err != nil { return err } @@ -204,7 +214,7 @@ func (w *bucketResponseWriter) writeZip( return nil } -func (w *bucketResponseWriter) writeDirectory( +func (w *responseWriter) writeDirectory( ctx context.Context, response *pluginpb.CodeGeneratorResponse, outDirPath string, @@ -213,22 +223,22 @@ func (w *bucketResponseWriter) writeDirectory( if readWriteBucket, ok := w.readWriteBuckets[outDirPath]; ok { // We already have a readWriteBucket for this outDirPath, so // we can write to the same bucket. - if err := w.responseHandler.HandleResponse( + if err := w.responseWriter.WriteResponse( ctx, readWriteBucket, response, - appproto.HandleResponseWithInsertionPointReadBucket(readWriteBucket), + appproto.WriteResponseWithInsertionPointReadBucket(readWriteBucket), ); err != nil { return err } return nil } readWriteBucket := storagemem.NewReadWriteBucket() - if err := w.responseHandler.HandleResponse( + if err := w.responseWriter.WriteResponse( ctx, readWriteBucket, response, - appproto.HandleResponseWithInsertionPointReadBucket(readWriteBucket), + appproto.WriteResponseWithInsertionPointReadBucket(readWriteBucket), ); err != nil { return err } @@ -257,10 +267,10 @@ func (w *bucketResponseWriter) writeDirectory( return nil } -type bucketResponseWriterOptions struct { +type responseWriterOptions struct { createOutDirIfNotExists bool } -func newBucketResponseWriterOptions() *bucketResponseWriterOptions { - return &bucketResponseWriterOptions{} +func newResponseWriterOptions() *responseWriterOptions { + return &responseWriterOptions{} } diff --git a/private/pkg/app/appproto/generator.go b/private/pkg/app/appproto/generator.go index 8f63188efd..3d97a53197 100644 --- a/private/pkg/app/appproto/generator.go +++ b/private/pkg/app/appproto/generator.go @@ -45,7 +45,7 @@ func (g *generator) Generate( container app.EnvStderrContainer, requests []*pluginpb.CodeGeneratorRequest, ) (*pluginpb.CodeGeneratorResponse, error) { - responseWriter := newResponseWriter(container) + responseBuilder := newResponseBuilder(container) jobs := make([]func(context.Context) error, len(requests)) for i, request := range requests { request := request @@ -53,7 +53,7 @@ func (g *generator) Generate( if err := protodescriptor.ValidateCodeGeneratorRequest(request); err != nil { return err } - return g.handler.Handle(ctx, container, responseWriter, request) + return g.handler.Handle(ctx, container, responseBuilder, request) } } ctx, cancel := context.WithCancel(ctx) @@ -61,7 +61,7 @@ func (g *generator) Generate( if err := thread.Parallelize(ctx, jobs, thread.ParallelizeWithCancel(cancel)); err != nil { return nil, err } - response := responseWriter.toResponse() + response := responseBuilder.toResponse() if err := protodescriptor.ValidateCodeGeneratorResponse(response); err != nil { return nil, err } diff --git a/private/pkg/app/appproto/response_builder.go b/private/pkg/app/appproto/response_builder.go new file mode 100644 index 0000000000..8b5ef6f5e1 --- /dev/null +++ b/private/pkg/app/appproto/response_builder.go @@ -0,0 +1,145 @@ +// Copyright 2020-2021 Buf Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package appproto + +import ( + "errors" + "fmt" + "strings" + "sync" + + "github.com/bufbuild/buf/private/pkg/app" + "github.com/bufbuild/buf/private/pkg/normalpath" + "google.golang.org/protobuf/proto" + "google.golang.org/protobuf/types/pluginpb" +) + +type responseBuilder struct { + container app.StderrContainer + fileNames map[string]struct{} + files []*pluginpb.CodeGeneratorResponse_File + errorMessages []string + featureProto3Optional bool + lock sync.RWMutex +} + +func newResponseBuilder(container app.StderrContainer) *responseBuilder { + return &responseBuilder{ + container: container, + fileNames: make(map[string]struct{}), + } +} + +func (r *responseBuilder) AddFile(file *pluginpb.CodeGeneratorResponse_File) error { + r.lock.Lock() + defer r.lock.Unlock() + if file == nil { + return errors.New("add CodeGeneratorResponse.File is nil") + } + name := file.GetName() + if name == "" { + return errors.New("add CodeGeneratorResponse.File.Name is empty") + } + // name must be relative, to-slashed, and not contain "." or ".." per the documentation + // this is what normalize does + normalizedName, err := normalpath.NormalizeAndValidate(name) + if err != nil { + // we need names to be normalized for the appproto.Generator to properly put them in buckets + // so we have to error here if it is not validated + return newUnvalidatedNameError(name) + } + if normalizedName != name { + if err := r.warnUnnormalizedName(name); err != nil { + return err + } + // we need names to be normalized for the appproto.Generator to properly put + // them in buckets, so we will coerce this into a normalized name if it is + // validated, ie if it does not container ".." and is absolute, we can still + // continue, assuming we validate here + name = normalizedName + file.Name = proto.String(name) + } + if _, ok := r.fileNames[name]; ok { + if err := r.warnDuplicateName(name); err != nil { + return err + } + } else { + // we drop the file if it is duplicated, and only put in the map and files slice + // if it does not exist + r.fileNames[name] = struct{}{} + r.files = append(r.files, file) + } + return nil +} + +func (r *responseBuilder) AddError(message string) { + r.lock.Lock() + defer r.lock.Unlock() + if message == "" { + // default to an error message to make sure we pass an error + // if this function was called + message = "error" + } + r.errorMessages = append(r.errorMessages, message) +} + +func (r *responseBuilder) SetFeatureProto3Optional() { + r.lock.Lock() + defer r.lock.Unlock() + r.featureProto3Optional = true +} + +// toResponse turns the response writer into a Protobuf CodeGeneratorResponse. +// It should be run after all writing to the response has finished. +func (r *responseBuilder) toResponse() *pluginpb.CodeGeneratorResponse { + r.lock.RLock() + defer r.lock.RUnlock() + response := &pluginpb.CodeGeneratorResponse{ + File: r.files, + } + if len(r.errorMessages) > 0 { + response.Error = proto.String(strings.Join(r.errorMessages, "\n")) + } + if r.featureProto3Optional { + response.SupportedFeatures = proto.Uint64(uint64(pluginpb.CodeGeneratorResponse_FEATURE_PROTO3_OPTIONAL)) + } + return response +} + +func (r *responseBuilder) warnUnnormalizedName(name string) error { + _, err := r.container.Stderr().Write([]byte(fmt.Sprintf( + `Warning: Generated file name %q does not conform to the Protobuf generation specification. Note that the file name must be relative, use "/" instead of "\", and not use "." or ".." as part of the file name. Buf will continue without error here, but please raise an issue with the maintainer of the plugin and reference https://github.com/protocolbuffers/protobuf/blob/95e6c5b4746dd7474d540ce4fb375e3f79a086f8/src/google/protobuf/compiler/plugin.proto#L122 +`, + name, + ))) + return err +} + +func (r *responseBuilder) warnDuplicateName(name string) error { + _, err := r.container.Stderr().Write([]byte(fmt.Sprintf( + `Warning: Duplicate generated file name %q. Buf will continue without error here and drop the second occurrence of this file, but please raise an issue with the maintainer of the plugin. +`, + name, + ))) + return err +} + +func newUnvalidatedNameError(name string) error { + return fmt.Errorf( + `Generated file name %q does not conform to the Protobuf generation specification. Note that the file name must be relative, and not use "..". Please raise an issue with the maintainer of the plugin and reference https://github.com/protocolbuffers/protobuf/blob/95e6c5b4746dd7474d540ce4fb375e3f79a086f8/src/google/protobuf/compiler/plugin.proto#L122 +`, + name, + ) +} diff --git a/private/pkg/app/appproto/response_handler.go b/private/pkg/app/appproto/response_handler.go deleted file mode 100644 index 4c49ecadce..0000000000 --- a/private/pkg/app/appproto/response_handler.go +++ /dev/null @@ -1,154 +0,0 @@ -// Copyright 2020-2021 Buf Technologies, Inc. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package appproto - -import ( - "bufio" - "bytes" - "context" - "io" - - "github.com/bufbuild/buf/private/pkg/storage" - "go.uber.org/multierr" - "go.uber.org/zap" - "google.golang.org/protobuf/types/pluginpb" -) - -type responseHandler struct { - logger *zap.Logger -} - -func newResponseHandler( - logger *zap.Logger, -) *responseHandler { - return &responseHandler{ - logger: logger, - } -} - -func (h *responseHandler) HandleResponse( - ctx context.Context, - writeBucket storage.WriteBucket, - response *pluginpb.CodeGeneratorResponse, - options ...HandleResponseOption, -) error { - handleResponseOptions := newHandleResponseOptions() - for _, option := range options { - option(handleResponseOptions) - } - for _, file := range response.File { - if file.GetInsertionPoint() != "" { - if handleResponseOptions.insertionPointReadBucket == nil { - return storage.NewErrNotExist(file.GetName()) - } - if err := applyInsertionPoint(ctx, file, handleResponseOptions.insertionPointReadBucket, writeBucket); err != nil { - return err - } - } else if err := storage.PutPath(ctx, writeBucket, file.GetName(), []byte(file.GetContent())); err != nil { - return err - } - } - return nil -} - -// applyInsertionPoint inserts the content of the given file at the insertion point that it specfiies. -// For more details on insertion points, see the following: -// -// https://github.com/protocolbuffers/protobuf/blob/f5bdd7cd56aa86612e166706ed8ef139db06edf2/src/google/protobuf/compiler/plugin.proto#L135-L171 -func applyInsertionPoint( - ctx context.Context, - file *pluginpb.CodeGeneratorResponse_File, - readBucket storage.ReadBucket, - writeBucket storage.WriteBucket, -) (retErr error) { - targetReadObjectCloser, err := readBucket.Get(ctx, file.GetName()) - if err != nil { - return err - } - defer func() { - retErr = multierr.Append(retErr, targetReadObjectCloser.Close()) - }() - resultData, err := writeInsertionPoint(ctx, file, targetReadObjectCloser) - if err != nil { - return err - } - // This relies on storageos buckets maintaining existing file permissions - return storage.PutPath(ctx, writeBucket, file.GetName(), resultData) -} - -// writeInsertionPoint writes the insertion point defined in insertionPointFile -// to the targetFile and returns the result as []byte. The caller must ensure the -// provided targetFile matches the file requested in insertionPointFile.Name. -func writeInsertionPoint( - ctx context.Context, - insertionPointFile *pluginpb.CodeGeneratorResponse_File, - targetFile io.Reader, -) (_ []byte, retErr error) { - targetScanner := bufio.NewScanner(targetFile) - match := []byte("@@protoc_insertion_point(" + insertionPointFile.GetInsertionPoint() + ")") - postInsertionContent := bytes.NewBuffer(nil) - postInsertionContent.Grow(averageGeneratedFileSize) - // TODO: We should respect the line endings in the generated file. This would - // require either targetFile being an io.ReadSeeker and in the worst case - // doing 2 full scans of the file (if it is a single line), or implementing - // bufio.Scanner.Scan() inline - newline := []byte{'\n'} - for targetScanner.Scan() { - targetLine := targetScanner.Bytes() - if !bytes.Contains(targetLine, match) { - // these writes cannot fail, they will panic if they cannot - // allocate - _, _ = postInsertionContent.Write(targetLine) - _, _ = postInsertionContent.Write(newline) - continue - } - // For each line in then new content, apply the - // same amount of whitespace. This is important - // for specific languages, e.g. Python. - whitespace := leadingWhitespace(targetLine) - - // Create another scanner so that we can seamlessly handle - // newlines in a platform-agnostic manner. - insertedContentScanner := bufio.NewScanner(bytes.NewBufferString(insertionPointFile.GetContent())) - insertedContent := scanWithPrefixAndLineEnding(insertedContentScanner, whitespace, newline) - // This write cannot fail, it will panic if it cannot - // allocate - _, _ = postInsertionContent.Write(insertedContent) - - // Code inserted at this point is placed immediately - // above the line containing the insertion point, so - // we include it last. - // These writes cannot fail, they will panic if they cannot - // allocate - _, _ = postInsertionContent.Write(targetLine) - _, _ = postInsertionContent.Write(newline) - } - - if err := targetScanner.Err(); err != nil { - return nil, err - } - - // trim the trailing newline - postInsertionBytes := postInsertionContent.Bytes() - return postInsertionBytes[:len(postInsertionBytes)-1], nil -} - -type handleResponseOptions struct { - insertionPointReadBucket storage.ReadBucket -} - -func newHandleResponseOptions() *handleResponseOptions { - return &handleResponseOptions{} -} diff --git a/private/pkg/app/appproto/response_writer.go b/private/pkg/app/appproto/response_writer.go index ebde1f7b11..5f0a3a785a 100644 --- a/private/pkg/app/appproto/response_writer.go +++ b/private/pkg/app/appproto/response_writer.go @@ -15,131 +15,140 @@ package appproto import ( - "errors" - "fmt" - "strings" - "sync" + "bufio" + "bytes" + "context" + "io" - "github.com/bufbuild/buf/private/pkg/app" - "github.com/bufbuild/buf/private/pkg/normalpath" - "google.golang.org/protobuf/proto" + "github.com/bufbuild/buf/private/pkg/storage" + "go.uber.org/multierr" + "go.uber.org/zap" "google.golang.org/protobuf/types/pluginpb" ) type responseWriter struct { - container app.StderrContainer - fileNames map[string]struct{} - files []*pluginpb.CodeGeneratorResponse_File - errorMessages []string - featureProto3Optional bool - lock sync.RWMutex + logger *zap.Logger } -func newResponseWriter(container app.StderrContainer) *responseWriter { +func newResponseWriter( + logger *zap.Logger, +) *responseWriter { return &responseWriter{ - container: container, - fileNames: make(map[string]struct{}), + logger: logger, } } -func (r *responseWriter) AddFile(file *pluginpb.CodeGeneratorResponse_File) error { - r.lock.Lock() - defer r.lock.Unlock() - if file == nil { - return errors.New("add CodeGeneratorResponse.File is nil") +func (h *responseWriter) WriteResponse( + ctx context.Context, + writeBucket storage.WriteBucket, + response *pluginpb.CodeGeneratorResponse, + options ...WriteResponseOption, +) error { + writeResponseOptions := newWriteResponseOptions() + for _, option := range options { + option(writeResponseOptions) } - name := file.GetName() - if name == "" { - return errors.New("add CodeGeneratorResponse.File.Name is empty") - } - // name must be relative, to-slashed, and not contain "." or ".." per the documentation - // this is what normalize does - normalizedName, err := normalpath.NormalizeAndValidate(name) - if err != nil { - // we need names to be normalized for the appproto.Generator to properly put them in buckets - // so we have to error here if it is not validated - return newUnvalidatedNameError(name) - } - if normalizedName != name { - if err := r.warnUnnormalizedName(name); err != nil { - return err - } - // we need names to be normalized for the appproto.Generator to properly put - // them in buckets, so we will coerce this into a normalized name if it is - // validated, ie if it does not container ".." and is absolute, we can still - // continue, assuming we validate here - name = normalizedName - file.Name = proto.String(name) - } - if _, ok := r.fileNames[name]; ok { - if err := r.warnDuplicateName(name); err != nil { + for _, file := range response.File { + if file.GetInsertionPoint() != "" { + if writeResponseOptions.insertionPointReadBucket == nil { + return storage.NewErrNotExist(file.GetName()) + } + if err := applyInsertionPoint(ctx, file, writeResponseOptions.insertionPointReadBucket, writeBucket); err != nil { + return err + } + } else if err := storage.PutPath(ctx, writeBucket, file.GetName(), []byte(file.GetContent())); err != nil { return err } - } else { - // we drop the file if it is duplicated, and only put in the map and files slice - // if it does not exist - r.fileNames[name] = struct{}{} - r.files = append(r.files, file) } return nil } -func (r *responseWriter) AddError(message string) { - r.lock.Lock() - defer r.lock.Unlock() - if message == "" { - // default to an error message to make sure we pass an error - // if this function was called - message = "error" +// applyInsertionPoint inserts the content of the given file at the insertion point that it specfiies. +// For more details on insertion points, see the following: +// +// https://github.com/protocolbuffers/protobuf/blob/f5bdd7cd56aa86612e166706ed8ef139db06edf2/src/google/protobuf/compiler/plugin.proto#L135-L171 +func applyInsertionPoint( + ctx context.Context, + file *pluginpb.CodeGeneratorResponse_File, + readBucket storage.ReadBucket, + writeBucket storage.WriteBucket, +) (retErr error) { + targetReadObjectCloser, err := readBucket.Get(ctx, file.GetName()) + if err != nil { + return err } - r.errorMessages = append(r.errorMessages, message) + defer func() { + retErr = multierr.Append(retErr, targetReadObjectCloser.Close()) + }() + resultData, err := writeInsertionPoint(ctx, file, targetReadObjectCloser) + if err != nil { + return err + } + // This relies on storageos buckets maintaining existing file permissions + return storage.PutPath(ctx, writeBucket, file.GetName(), resultData) } -func (r *responseWriter) SetFeatureProto3Optional() { - r.lock.Lock() - defer r.lock.Unlock() - r.featureProto3Optional = true -} +// writeInsertionPoint writes the insertion point defined in insertionPointFile +// to the targetFile and returns the result as []byte. The caller must ensure the +// provided targetFile matches the file requested in insertionPointFile.Name. +func writeInsertionPoint( + ctx context.Context, + insertionPointFile *pluginpb.CodeGeneratorResponse_File, + targetFile io.Reader, +) (_ []byte, retErr error) { + targetScanner := bufio.NewScanner(targetFile) + match := []byte("@@protoc_insertion_point(" + insertionPointFile.GetInsertionPoint() + ")") + postInsertionContent := bytes.NewBuffer(nil) + postInsertionContent.Grow(averageGeneratedFileSize) + // TODO: We should respect the line endings in the generated file. This would + // require either targetFile being an io.ReadSeeker and in the worst case + // doing 2 full scans of the file (if it is a single line), or implementing + // bufio.Scanner.Scan() inline + newline := []byte{'\n'} + for targetScanner.Scan() { + targetLine := targetScanner.Bytes() + if !bytes.Contains(targetLine, match) { + // these writes cannot fail, they will panic if they cannot + // allocate + _, _ = postInsertionContent.Write(targetLine) + _, _ = postInsertionContent.Write(newline) + continue + } + // For each line in then new content, apply the + // same amount of whitespace. This is important + // for specific languages, e.g. Python. + whitespace := leadingWhitespace(targetLine) -// toResponse turns the response writer into a Protobuf CodeGeneratorResponse. -// It should be run after all writing to the response has finished. -func (r *responseWriter) toResponse() *pluginpb.CodeGeneratorResponse { - r.lock.RLock() - defer r.lock.RUnlock() - response := &pluginpb.CodeGeneratorResponse{ - File: r.files, - } - if len(r.errorMessages) > 0 { - response.Error = proto.String(strings.Join(r.errorMessages, "\n")) + // Create another scanner so that we can seamlessly handle + // newlines in a platform-agnostic manner. + insertedContentScanner := bufio.NewScanner(bytes.NewBufferString(insertionPointFile.GetContent())) + insertedContent := scanWithPrefixAndLineEnding(insertedContentScanner, whitespace, newline) + // This write cannot fail, it will panic if it cannot + // allocate + _, _ = postInsertionContent.Write(insertedContent) + + // Code inserted at this point is placed immediately + // above the line containing the insertion point, so + // we include it last. + // These writes cannot fail, they will panic if they cannot + // allocate + _, _ = postInsertionContent.Write(targetLine) + _, _ = postInsertionContent.Write(newline) } - if r.featureProto3Optional { - response.SupportedFeatures = proto.Uint64(uint64(pluginpb.CodeGeneratorResponse_FEATURE_PROTO3_OPTIONAL)) + + if err := targetScanner.Err(); err != nil { + return nil, err } - return response -} -func (r *responseWriter) warnUnnormalizedName(name string) error { - _, err := r.container.Stderr().Write([]byte(fmt.Sprintf( - `Warning: Generated file name %q does not conform to the Protobuf generation specification. Note that the file name must be relative, use "/" instead of "\", and not use "." or ".." as part of the file name. Buf will continue without error here, but please raise an issue with the maintainer of the plugin and reference https://github.com/protocolbuffers/protobuf/blob/95e6c5b4746dd7474d540ce4fb375e3f79a086f8/src/google/protobuf/compiler/plugin.proto#L122 -`, - name, - ))) - return err + // trim the trailing newline + postInsertionBytes := postInsertionContent.Bytes() + return postInsertionBytes[:len(postInsertionBytes)-1], nil } -func (r *responseWriter) warnDuplicateName(name string) error { - _, err := r.container.Stderr().Write([]byte(fmt.Sprintf( - `Warning: Duplicate generated file name %q. Buf will continue without error here and drop the second occurrence of this file, but please raise an issue with the maintainer of the plugin. -`, - name, - ))) - return err +type writeResponseOptions struct { + insertionPointReadBucket storage.ReadBucket } -func newUnvalidatedNameError(name string) error { - return fmt.Errorf( - `Generated file name %q does not conform to the Protobuf generation specification. Note that the file name must be relative, and not use "..". Please raise an issue with the maintainer of the plugin and reference https://github.com/protocolbuffers/protobuf/blob/95e6c5b4746dd7474d540ce4fb375e3f79a086f8/src/google/protobuf/compiler/plugin.proto#L122 -`, - name, - ) +func newWriteResponseOptions() *writeResponseOptions { + return &writeResponseOptions{} } diff --git a/private/pkg/protogenutil/protogenutil.go b/private/pkg/protogenutil/protogenutil.go index 64291c1ca4..f17ea877b4 100644 --- a/private/pkg/protogenutil/protogenutil.go +++ b/private/pkg/protogenutil/protogenutil.go @@ -40,7 +40,7 @@ func NewHandler(f func(*protogen.Plugin) error, options ...HandlerOption) apppro func( ctx context.Context, container app.EnvStderrContainer, - responseWriter appproto.ResponseWriter, + responseWriter appproto.ResponseBuilder, request *pluginpb.CodeGeneratorRequest, ) error { plugin, err := protogen.Options{ From 622bf3df3291e77701346af5a634c995ce5021ed Mon Sep 17 00:00:00 2001 From: Alex McKinney Date: Fri, 1 Oct 2021 13:36:10 -0400 Subject: [PATCH 06/14] Fix typo --- private/buf/bufgen/generator.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/private/buf/bufgen/generator.go b/private/buf/bufgen/generator.go index 3b8716c40b..8a55a06a7e 100644 --- a/private/buf/bufgen/generator.go +++ b/private/buf/bufgen/generator.go @@ -57,7 +57,7 @@ func newGenerator( // Generate executes all of the plugins specified by the given Config, and // consolidates the results in the same order that the plugins are listed. // Order is particularly important for insertion points, which are used to -// modify the generted output from other plugins executed earlier in the chain. +// modify the generated output from other plugins executed earlier in the chain. // // Note that insertion points will only have access to files that are written // in the same protoc invocation; plugins will not be able to insert code into From 31e58e789f08ee1787e5b022712cec747ecd98cd Mon Sep 17 00:00:00 2001 From: bufdev Date: Fri, 1 Oct 2021 13:49:46 -0400 Subject: [PATCH 07/14] Delete ParallelizeWithFastFail --- private/buf/bufgen/generator.go | 4 ++++ private/pkg/thread/thread.go | 17 +---------------- 2 files changed, 5 insertions(+), 16 deletions(-) diff --git a/private/buf/bufgen/generator.go b/private/buf/bufgen/generator.go index 8a55a06a7e..a0d54e30b3 100644 --- a/private/buf/bufgen/generator.go +++ b/private/buf/bufgen/generator.go @@ -30,6 +30,7 @@ import ( "github.com/bufbuild/buf/private/pkg/app/appproto/appprotoos" "github.com/bufbuild/buf/private/pkg/storage/storageos" "github.com/bufbuild/buf/private/pkg/thread" + "go.uber.org/multierr" "go.uber.org/zap" "google.golang.org/protobuf/types/pluginpb" ) @@ -212,6 +213,9 @@ func (g *generator) execPlugins( thread.ParallelizeWithCancel(cancel), thread.ParallelizeWithFastFail(), // We want error messages to be concise here. ); err != nil { + if errs := multierr.Errors(err); len(errs) > 0 { + return nil, errs[0] + } return nil, err } if err := validateResponses(responses, config.PluginConfigs); err != nil { diff --git a/private/pkg/thread/thread.go b/private/pkg/thread/thread.go index 9187156c85..949d4ade4b 100644 --- a/private/pkg/thread/thread.go +++ b/private/pkg/thread/thread.go @@ -102,13 +102,7 @@ func Parallelize(ctx context.Context, jobs []func(context.Context) error, option go func() { if err := job(ctx); err != nil { lock.Lock() - if parallelizeOptions.fastFail { - if retErr == nil { - retErr = err - } - } else { - retErr = multierr.Append(retErr, err) - } + retErr = multierr.Append(retErr, err) lock.Unlock() if parallelizeOptions.cancel != nil { parallelizeOptions.cancel() @@ -147,18 +141,9 @@ func ParallelizeWithCancel(cancel context.CancelFunc) ParallelizeOption { } } -// ParallelizeWithFastFail returns a new ParallelizeOption that will return -// the first error encountered from any job that fails. -func ParallelizeWithFastFail() ParallelizeOption { - return func(parallelizeOptions *parallelizeOptions) { - parallelizeOptions.fastFail = true - } -} - type parallelizeOptions struct { multiplier int cancel context.CancelFunc - fastFail bool } func newParallelizeOptions() *parallelizeOptions { From f93525369b8341ee4a1200e240ec912342acc52a Mon Sep 17 00:00:00 2001 From: bufdev Date: Fri, 1 Oct 2021 13:54:49 -0400 Subject: [PATCH 08/14] cleanup --- private/buf/bufgen/generator.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/private/buf/bufgen/generator.go b/private/buf/bufgen/generator.go index a0d54e30b3..71ed4b9172 100644 --- a/private/buf/bufgen/generator.go +++ b/private/buf/bufgen/generator.go @@ -211,7 +211,6 @@ func (g *generator) execPlugins( ctx, jobs, thread.ParallelizeWithCancel(cancel), - thread.ParallelizeWithFastFail(), // We want error messages to be concise here. ); err != nil { if errs := multierr.Errors(err); len(errs) > 0 { return nil, errs[0] @@ -273,16 +272,17 @@ func (g *generator) execRemotePlugin( // Only include parameters if they're not empty. parameters = []string{pluginConfig.Opt} } - pluginReference := ®istryv1alpha1.PluginReference{ - Owner: owner, - Name: name, - Version: version, - Parameters: parameters, - } responses, _, err := generateService.GeneratePlugins( ctx, bufimage.ImageToProtoImage(image), - []*registryv1alpha1.PluginReference{pluginReference}, + []*registryv1alpha1.PluginReference{ + ®istryv1alpha1.PluginReference{ + Owner: owner, + Name: name, + Version: version, + Parameters: parameters, + }, + }, ) if err != nil { return nil, err From cc22ebca83c7e0197f0be906306f8cbe7aa52820 Mon Sep 17 00:00:00 2001 From: Alex McKinney Date: Fri, 1 Oct 2021 14:32:24 -0400 Subject: [PATCH 09/14] gofmt --- private/buf/bufgen/generator.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/private/buf/bufgen/generator.go b/private/buf/bufgen/generator.go index 71ed4b9172..90aa699d46 100644 --- a/private/buf/bufgen/generator.go +++ b/private/buf/bufgen/generator.go @@ -276,7 +276,7 @@ func (g *generator) execRemotePlugin( ctx, bufimage.ImageToProtoImage(image), []*registryv1alpha1.PluginReference{ - ®istryv1alpha1.PluginReference{ + { Owner: owner, Name: name, Version: version, From 65030b91aadf5adf79928c4ec77668e01e79e9ab Mon Sep 17 00:00:00 2001 From: Alex McKinney Date: Fri, 1 Oct 2021 14:43:45 -0400 Subject: [PATCH 10/14] Add comment justifying ResponseWriter bucketing --- .../pkg/app/appproto/appprotoos/response_writer.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/private/pkg/app/appproto/appprotoos/response_writer.go b/private/pkg/app/appproto/appprotoos/response_writer.go index 807363d712..01b7928e1d 100644 --- a/private/pkg/app/appproto/appprotoos/response_writer.go +++ b/private/pkg/app/appproto/appprotoos/response_writer.go @@ -50,6 +50,18 @@ type responseWriter struct { // Cache the readWriteBuckets by their respective output paths. // These builders are transformed to storage.ReadBuckets and written // to disk once the responseWriter is flushed. + // + // Note that output paths are used as-is with respect to the + // caller's configuration. It's possible that a single invocation + // will specify the same filepath in multiple ways, e.g. "." and + // "$(pwd)". However, we intentionally treat these as distinct paths + // to mirror protoc's insertion point behavior. + // + // For example, the following command will fail because protoc treats + // "." and "$(pwd)" as distinct paths: + // + // $ protoc example.proto --insertion-point-receiver_out=. --insertion-point-writer_out=$(pwd) + // readWriteBuckets map[string]storage.ReadWriteBucket // Cache the functions used to flush all of the responses to disk. // This holds all of the buckets in-memory so that we only write From 1d3a743cd05bb53b54fff8c0a9f14dcfb10bd26b Mon Sep 17 00:00:00 2001 From: Alex McKinney Date: Fri, 1 Oct 2021 14:44:56 -0400 Subject: [PATCH 11/14] Add CHANGELOG.md entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f5b03eea60..97118ff0f4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ - Add `--as-import-paths` flag to `ls-files` that strips local directory paths and prints file paths as they are imported. - Fix issue where groups used in custom options did not result in the same behavior as `protoc`. +- Fix issue where insertion points were not applied with respect to the configured output directory. ## [v1.0.0-rc2] - 2021-09-23 From d655b707d7cf909528dda1298a14ee4e0eac3595 Mon Sep 17 00:00:00 2001 From: Alex McKinney Date: Fri, 1 Oct 2021 15:05:52 -0400 Subject: [PATCH 12/14] Re-export appprotoexec.NewHandler --- .../app/appproto/appprotoexec/appprotoexec.go | 77 +++++++++++++++++++ .../app/appproto/appprotoexec/generator.go | 4 +- .../pkg/app/appproto/appprotoexec/handler.go | 61 --------------- 3 files changed, 79 insertions(+), 63 deletions(-) delete mode 100644 private/pkg/app/appproto/appprotoexec/handler.go diff --git a/private/pkg/app/appproto/appprotoexec/appprotoexec.go b/private/pkg/app/appproto/appprotoexec/appprotoexec.go index 6672c9a2bb..251b74c01d 100644 --- a/private/pkg/app/appproto/appprotoexec/appprotoexec.go +++ b/private/pkg/app/appproto/appprotoexec/appprotoexec.go @@ -20,8 +20,11 @@ package appprotoexec import ( "context" + "fmt" + "os/exec" "github.com/bufbuild/buf/private/pkg/app" + "github.com/bufbuild/buf/private/pkg/app/appproto" "github.com/bufbuild/buf/private/pkg/storage/storageos" "go.uber.org/zap" "google.golang.org/protobuf/types/pluginpb" @@ -95,3 +98,77 @@ func GenerateWithPluginPath(pluginPath string) GenerateOption { generateOptions.pluginPath = pluginPath } } + +// NewHandler returns a new Handler based on the plugin name and optional path. +// +// protocPath and pluginPath are optional. +// +// - If the plugin path is set, this returns a new binary handler for that path. +// - If the plugin path is unset, this does exec.LookPath for a binary named protoc-gen-pluginName, +// and if one is found, a new binary handler is returned for this. +// - Else, if the name is in ProtocProxyPluginNames, this returns a new protoc proxy handler. +// - Else, this returns error. +func NewHandler( + logger *zap.Logger, + storageosProvider storageos.Provider, + pluginName string, + options ...HandlerOption, +) (appproto.Handler, error) { + handlerOptions := newHandlerOptions() + for _, option := range options { + option(handlerOptions) + } + if handlerOptions.pluginPath != "" { + pluginPath, err := exec.LookPath(handlerOptions.pluginPath) + if err != nil { + return nil, err + } + return newBinaryHandler(logger, pluginPath), nil + } + pluginPath, err := exec.LookPath("protoc-gen-" + pluginName) + if err == nil { + return newBinaryHandler(logger, pluginPath), nil + } + // we always look for protoc-gen-X first, but if not, check the builtins + if _, ok := ProtocProxyPluginNames[pluginName]; ok { + if handlerOptions.protocPath == "" { + handlerOptions.protocPath = "protoc" + } + protocPath, err := exec.LookPath(handlerOptions.protocPath) + if err != nil { + return nil, err + } + return newProtocProxyHandler(logger, storageosProvider, protocPath, pluginName), nil + } + return nil, fmt.Errorf("could not find protoc plugin for name %s", pluginName) +} + +// HandlerOption is an option for a new Handler. +type HandlerOption func(*handlerOptions) + +// HandlerWithProtocPath returns a new HandlerOption that sets the path to the protoc binary. +// +// The default is to do exec.LookPath on "protoc". +func HandlerWithProtocPath(protocPath string) HandlerOption { + return func(handlerOptions *handlerOptions) { + handlerOptions.protocPath = protocPath + } +} + +// HandlerWithPluginPath returns a new HandlerOption that sets the path to the plugin binary. +// +// The default is to do exec.LookPath on "protoc-gen-" + pluginName. +func HandlerWithPluginPath(pluginPath string) HandlerOption { + return func(handlerOptions *handlerOptions) { + handlerOptions.pluginPath = pluginPath + } +} + +type handlerOptions struct { + protocPath string + pluginPath string +} + +func newHandlerOptions() *handlerOptions { + return &handlerOptions{} +} diff --git a/private/pkg/app/appproto/appprotoexec/generator.go b/private/pkg/app/appproto/appprotoexec/generator.go index 9795d85ffc..c7db910c45 100644 --- a/private/pkg/app/appproto/appprotoexec/generator.go +++ b/private/pkg/app/appproto/appprotoexec/generator.go @@ -50,11 +50,11 @@ func (g *generator) Generate( for _, option := range options { option(generateOptions) } - handler, err := newHandler( + handler, err := NewHandler( g.logger, g.storageosProvider, pluginName, - generateOptions.pluginPath, + HandlerWithPluginPath(generateOptions.pluginPath), ) if err != nil { return nil, err diff --git a/private/pkg/app/appproto/appprotoexec/handler.go b/private/pkg/app/appproto/appprotoexec/handler.go deleted file mode 100644 index a58f5243e1..0000000000 --- a/private/pkg/app/appproto/appprotoexec/handler.go +++ /dev/null @@ -1,61 +0,0 @@ -// Copyright 2020-2021 Buf Technologies, Inc. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package appprotoexec - -import ( - "fmt" - "os/exec" - - "github.com/bufbuild/buf/private/pkg/app/appproto" - "github.com/bufbuild/buf/private/pkg/storage/storageos" - "go.uber.org/zap" -) - -// newHandler returns a new appproto.Handler based on the plugin name and optional path. -// -// pluginPath is optional. -// -// - If the plugin path is set, this returns a new binary handler for that path. -// - If the plugin path is unset, this does exec.LookPath for a binary named protoc-gen-pluginName, -// and if one is found, a new binary handler is returned for this. -// - Else, if the name is in ProtocProxyPluginNames, this returns a new protoc proxy handler. -// - Else, this returns error. -func newHandler( - logger *zap.Logger, - storageosProvider storageos.Provider, - pluginName string, - pluginPath string, -) (appproto.Handler, error) { - if pluginPath != "" { - pluginPath, err := exec.LookPath(pluginPath) - if err != nil { - return nil, err - } - return newBinaryHandler(logger, pluginPath), nil - } - pluginPath, err := exec.LookPath("protoc-gen-" + pluginName) - if err == nil { - return newBinaryHandler(logger, pluginPath), nil - } - // we always look for protoc-gen-X first, but if not, check the builtins - if _, ok := ProtocProxyPluginNames[pluginName]; ok { - protocPath, err := exec.LookPath("protoc") - if err != nil { - return nil, err - } - return newProtocProxyHandler(logger, storageosProvider, protocPath, pluginName), nil - } - return nil, fmt.Errorf("could not find protoc plugin for name %s", pluginName) -} From 37d64e427fb183edfa45b7e68908d38a1706882b Mon Sep 17 00:00:00 2001 From: Alex McKinney Date: Fri, 1 Oct 2021 15:55:11 -0400 Subject: [PATCH 13/14] Clean the plugin output path --- private/pkg/app/appproto/appprotoos/response_writer.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/private/pkg/app/appproto/appprotoos/response_writer.go b/private/pkg/app/appproto/appprotoos/response_writer.go index 01b7928e1d..f008bd7909 100644 --- a/private/pkg/app/appproto/appprotoos/response_writer.go +++ b/private/pkg/app/appproto/appprotoos/response_writer.go @@ -98,7 +98,14 @@ func (w *responseWriter) AddResponse( return w.addResponse( ctx, response, - pluginOut, + // It's important that we clean the plugin output path + // so that we use the same in-memory bucket for paths + // set to the same directory. + // + // For example, + // + // --insertion-point-receiver_out=insertion --insertion-point-writer_out=./insertion/ + filepath.Clean(pluginOut), w.createOutDirIfNotExists, ) } From f3e9f28770f20d5c1a9f4da76d17bf19cc7ff920 Mon Sep 17 00:00:00 2001 From: Alex McKinney Date: Fri, 1 Oct 2021 16:31:49 -0400 Subject: [PATCH 14/14] Add tests for cleaned paths --- .../cmd/buf/command/generate/generate_test.go | 64 ++++++++++--------- .../gen/proto/insertion/test.txt | 15 +++++ 2 files changed, 50 insertions(+), 29 deletions(-) create mode 100644 private/buf/cmd/buf/command/generate/testdata/nested_insertion_point/gen/proto/insertion/test.txt diff --git a/private/buf/cmd/buf/command/generate/generate_test.go b/private/buf/cmd/buf/command/generate/generate_test.go index ad91bfeb40..cb9bfb212f 100644 --- a/private/buf/cmd/buf/command/generate/generate_test.go +++ b/private/buf/cmd/buf/command/generate/generate_test.go @@ -127,35 +127,10 @@ func TestOutputFlag(t *testing.T) { require.NoError(t, err) } -func TestGenerateInsertionPoints(t *testing.T) { - successTemplate := ` -version: v1 -plugins: - - name: insertion-point-receiver - out: . - - name: insertion-point-writer - out: . -` - storageosProvider := storageos.NewProvider() - tempDir, readWriteBucket := internaltesting.CopyReadBucketToTempDir( - context.Background(), - t, - storageosProvider, - storagemem.NewReadWriteBucket(), - ) - testRunSuccess( - t, - filepath.Join("testdata", "simple"), // The input directory is irrelevant for these insertion points. - "--template", - successTemplate, - "-o", - tempDir, - ) - expectedOutput, err := storageosProvider.NewReadWriteBucket(filepath.Join("testdata", "insertion_point")) - require.NoError(t, err) - diff, err := storage.DiffBytes(context.Background(), expectedOutput, readWriteBucket) - require.NoError(t, err) - require.Empty(t, string(diff)) +func TestGenerateInsertionPoint(t *testing.T) { + testGenerateInsertionPoint(t, ".", ".", filepath.Join("testdata", "insertion_point")) + testGenerateInsertionPoint(t, "gen/proto/insertion", "gen/proto/insertion", filepath.Join("testdata", "nested_insertion_point")) + testGenerateInsertionPoint(t, "gen/proto/insertion/", "./gen/proto/insertion", filepath.Join("testdata", "nested_insertion_point")) } func TestGenerateInsertionPointFail(t *testing.T) { @@ -211,6 +186,37 @@ func TestGenerateInsertionPointMixedPathsFail(t *testing.T) { testGenerateInsertionPointMixedPathsFail(t, wd, ".") } +func testGenerateInsertionPoint(t *testing.T, receiverOut string, writerOut string, expectedOutputPath string) { + successTemplate := ` +version: v1 +plugins: + - name: insertion-point-receiver + out: %s + - name: insertion-point-writer + out: %s +` + storageosProvider := storageos.NewProvider() + tempDir, readWriteBucket := internaltesting.CopyReadBucketToTempDir( + context.Background(), + t, + storageosProvider, + storagemem.NewReadWriteBucket(), + ) + testRunSuccess( + t, + filepath.Join("testdata", "simple"), // The input directory is irrelevant for these insertion points. + "--template", + fmt.Sprintf(successTemplate, receiverOut, writerOut), + "-o", + tempDir, + ) + expectedOutput, err := storageosProvider.NewReadWriteBucket(expectedOutputPath) + require.NoError(t, err) + diff, err := storage.DiffBytes(context.Background(), expectedOutput, readWriteBucket) + require.NoError(t, err) + require.Empty(t, string(diff)) +} + // testGenerateInsertionPointMixedPathsFail demonstrates that insertion points are only // able to generate to the same output directory, even if the absolute path points to the // same place. This is equivalent to protoc's behavior. diff --git a/private/buf/cmd/buf/command/generate/testdata/nested_insertion_point/gen/proto/insertion/test.txt b/private/buf/cmd/buf/command/generate/testdata/nested_insertion_point/gen/proto/insertion/test.txt new file mode 100644 index 0000000000..ed6c6095c4 --- /dev/null +++ b/private/buf/cmd/buf/command/generate/testdata/nested_insertion_point/gen/proto/insertion/test.txt @@ -0,0 +1,15 @@ + + // The following line represents an insertion point named 'example'. + // We include a few indentation to verify the whitespace is preserved + // in the inserted content. + // + + // Include this comment on the 'example' insertion point. + // This is another example where whitespaces are preserved. + // And this demonstrates a newline literal (\n). + // And don't forget the windows newline literal (\r\n). + + // @@protoc_insertion_point(example) + // + // Note that all text should be added above the insertion point. + \ No newline at end of file