From 1bf4f0014316b1a9427692301950f4077a7673ff Mon Sep 17 00:00:00 2001 From: Doria Keung Date: Thu, 4 Jul 2024 16:56:00 -0400 Subject: [PATCH 1/3] Add `clean` to plugin configs in `buf.gen.yaml` --- CHANGELOG.md | 2 + private/buf/bufgen/generator.go | 41 +++--- .../cmd/buf/command/generate/generate_test.go | 139 +++++++++++++----- private/bufpkg/bufconfig/buf_gen_yaml_file.go | 3 + .../bufconfig/buf_gen_yaml_file_test.go | 21 +++ .../bufconfig/generate_plugin_config.go | 34 +++++ 6 files changed, 178 insertions(+), 62 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fff5e79144..d8fe364722 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,8 @@ - Add `buf generate --clean` flag that will delete the directories, jar files, or zip files that the plugins will write to, prior to generation. Allows cleaning of existing assets without having to call `rm -rf`. +- Add `clean` as an option for plugin configurations in `buf.gen.yaml`. If set to true, this + will delete the directories, jar files, or zip files set to `out` for the plugin. ## [v1.34.0] - 2024-06-21 diff --git a/private/buf/bufgen/generator.go b/private/buf/bufgen/generator.go index 86273d7921..fc795ab7fc 100644 --- a/private/buf/bufgen/generator.go +++ b/private/buf/bufgen/generator.go @@ -34,7 +34,6 @@ import ( "github.com/bufbuild/buf/private/pkg/app" "github.com/bufbuild/buf/private/pkg/command" "github.com/bufbuild/buf/private/pkg/connectclient" - "github.com/bufbuild/buf/private/pkg/slicesext" "github.com/bufbuild/buf/private/pkg/storage/storageos" "github.com/bufbuild/buf/private/pkg/thread" "github.com/bufbuild/buf/private/pkg/tracing" @@ -105,14 +104,13 @@ func (g *generator) Generate( return err } } - if generateOptions.deleteOuts { - if err := g.deleteOuts( - ctx, - generateOptions.baseOutDirPath, - config.GeneratePluginConfigs(), - ); err != nil { - return err - } + if err := g.deleteOuts( + ctx, + generateOptions.baseOutDirPath, + generateOptions.deleteOuts, + config.GeneratePluginConfigs(), + ); err != nil { + return err } for _, image := range images { if err := g.generateCode( @@ -133,21 +131,20 @@ func (g *generator) Generate( func (g *generator) deleteOuts( ctx context.Context, baseOutDir string, + deleteAllOuts bool, pluginConfigs []bufconfig.GeneratePluginConfig, ) error { - return bufprotopluginos.NewCleaner(g.storageosProvider).DeleteOuts( - ctx, - slicesext.Map( - pluginConfigs, - func(pluginConfig bufconfig.GeneratePluginConfig) string { - out := pluginConfig.Out() - if baseOutDir != "" && baseOutDir != "." { - return filepath.Join(baseOutDir, out) - } - return out - }, - ), - ) + var pluginOuts []string + for _, pluginConfig := range pluginConfigs { + if deleteAllOuts || pluginConfig.Clean() { + if baseOutDir != "" && baseOutDir != "." { + pluginOuts = append(pluginOuts, filepath.Join(baseOutDir, pluginConfig.Out())) + } else { + pluginOuts = append(pluginOuts, pluginConfig.Out()) + } + } + } + return bufprotopluginos.NewCleaner(g.storageosProvider).DeleteOuts(ctx, pluginOuts) } func (g *generator) generateCode( diff --git a/private/buf/cmd/buf/command/generate/generate_test.go b/private/buf/cmd/buf/command/generate/generate_test.go index a8986d930c..2b18b18524 100644 --- a/private/buf/cmd/buf/command/generate/generate_test.go +++ b/private/buf/cmd/buf/command/generate/generate_test.go @@ -550,20 +550,37 @@ func TestGenerateInsertionPointMixedPathsFail(t *testing.T) { func TestGenerateDeleteOutDir(t *testing.T) { t.Parallel() - testGenerateDeleteOuts(t, "", "foo") - testGenerateDeleteOuts(t, "base", "foo") - testGenerateDeleteOuts(t, "", "foo", "bar") - testGenerateDeleteOuts(t, "", "foo", "bar", "foo") - testGenerateDeleteOuts(t, "base", "foo", "bar") - testGenerateDeleteOuts(t, "base", "foo", "bar", "foo") - testGenerateDeleteOuts(t, "", "foo.jar") - testGenerateDeleteOuts(t, "", "foo.zip") - testGenerateDeleteOuts(t, "", "foo/bar.jar") - testGenerateDeleteOuts(t, "", "foo/bar.zip") - testGenerateDeleteOuts(t, "base", "foo.jar") - testGenerateDeleteOuts(t, "base", "foo.zip") - testGenerateDeleteOuts(t, "base", "foo/bar.jar") - testGenerateDeleteOuts(t, "base", "foo/bar.zip") + // Use --clean flag + testGenerateDeleteOuts(t, true, "", "foo") + testGenerateDeleteOuts(t, true, "base", "foo") + testGenerateDeleteOuts(t, true, "", "foo", "bar") + testGenerateDeleteOuts(t, true, "", "foo", "bar", "foo") + testGenerateDeleteOuts(t, true, "base", "foo", "bar") + testGenerateDeleteOuts(t, true, "base", "foo", "bar", "foo") + testGenerateDeleteOuts(t, true, "", "foo.jar") + testGenerateDeleteOuts(t, true, "", "foo.zip") + testGenerateDeleteOuts(t, true, "", "foo/bar.jar") + testGenerateDeleteOuts(t, true, "", "foo/bar.zip") + testGenerateDeleteOuts(t, true, "base", "foo.jar") + testGenerateDeleteOuts(t, true, "base", "foo.zip") + testGenerateDeleteOuts(t, true, "base", "foo/bar.jar") + testGenerateDeleteOuts(t, true, "base", "foo/bar.zip") + + // Only set clean in plugin configs + testGenerateDeleteOuts(t, false, "", "foo") + testGenerateDeleteOuts(t, false, "base", "foo") + testGenerateDeleteOuts(t, false, "", "foo", "bar") + testGenerateDeleteOuts(t, false, "", "foo", "bar", "foo") + testGenerateDeleteOuts(t, false, "base", "foo", "bar") + testGenerateDeleteOuts(t, false, "base", "foo", "bar", "foo") + testGenerateDeleteOuts(t, false, "", "foo.jar") + testGenerateDeleteOuts(t, false, "", "foo.zip") + testGenerateDeleteOuts(t, false, "", "foo/bar.jar") + testGenerateDeleteOuts(t, false, "", "foo/bar.zip") + testGenerateDeleteOuts(t, false, "base", "foo.jar") + testGenerateDeleteOuts(t, false, "base", "foo.zip") + testGenerateDeleteOuts(t, false, "base", "foo/bar.jar") + testGenerateDeleteOuts(t, false, "base", "foo/bar.zip") } func TestBoolPointerFlagTrue(t *testing.T) { @@ -906,11 +923,13 @@ func testRunSuccess(t *testing.T, args ...string) { func testGenerateDeleteOuts( t *testing.T, + useCleanFlag bool, baseOutDirPath string, outputPaths ...string, ) { - // Just add more builtins to the plugins slice below if this goes off - require.True(t, len(outputPaths) < 4, "we want to have unique plugins to work with and this test is only set up for three plugins max right now") + plugins := []string{"java", "cpp", "ruby"} + // Just add more builtins to the plugins slice above if this goes off + require.True(t, len(outputPaths) <= len(plugins), "we want to have unique plugins to work with and this test is only set up for three plugins max right now") fullOutputPaths := outputPaths if baseOutDirPath != "" && baseOutDirPath != "." { fullOutputPaths = slicesext.Map( @@ -955,28 +974,59 @@ func testGenerateDeleteOuts( plugins: `) - plugins := []string{"java", "cpp", "ruby"} - for i, outputPath := range outputPaths { - _, _ = templateBuilder.WriteString(` - protoc_builtin: `) - _, _ = templateBuilder.WriteString(plugins[i]) - _, _ = templateBuilder.WriteString("\n") - _, _ = templateBuilder.WriteString(` out: `) - _, _ = templateBuilder.WriteString(outputPath) - _, _ = templateBuilder.WriteString("\n") + var expectedPluginConfigCleanOut string + if useCleanFlag { + // When using the --clean flag, we do not set clean in the plugin configs at all + for i, outputPath := range outputPaths { + _, _ = templateBuilder.WriteString(` - protoc_builtin: `) + _, _ = templateBuilder.WriteString(plugins[i]) + _, _ = templateBuilder.WriteString("\n") + _, _ = templateBuilder.WriteString(` out: `) + _, _ = templateBuilder.WriteString(outputPath) + _, _ = templateBuilder.WriteString("\n") + } + testRunStdoutStderr( + t, + nil, + 0, + ``, + ``, + filepath.Join("testdata", "simple"), + "--template", + templateBuilder.String(), + "-o", + filepath.Join(tmpDirPath, normalpath.Unnormalize(baseOutDirPath)), + "--clean", + ) + } else { + for i, outputPath := range outputPaths { + _, _ = templateBuilder.WriteString(` - protoc_builtin: `) + _, _ = templateBuilder.WriteString(plugins[i]) + _, _ = templateBuilder.WriteString("\n") + _, _ = templateBuilder.WriteString(` out: `) + _, _ = templateBuilder.WriteString(outputPath) + _, _ = templateBuilder.WriteString("\n") + // Only set "clean: true" for the first plugin (java), and we expect that only the + // output for the first plugin to be cleaned + if i == 0 { // plugins[0] == "java" + _, _ = templateBuilder.WriteString(` clean: true`) + _, _ = templateBuilder.WriteString("\n") + expectedPluginConfigCleanOut = fullOutputPaths[i] + } + } + testRunStdoutStderr( + t, + nil, + 0, + ``, + ``, + filepath.Join("testdata", "simple"), + "--template", + templateBuilder.String(), + "-o", + filepath.Join(tmpDirPath, normalpath.Unnormalize(baseOutDirPath)), + ) } - testRunStdoutStderr( - t, - nil, - 0, - ``, - ``, - filepath.Join("testdata", "simple"), - "--template", - templateBuilder.String(), - "-o", - filepath.Join(tmpDirPath, normalpath.Unnormalize(baseOutDirPath)), - "--clean", - ) for _, fullOutputPath := range fullOutputPaths { switch normalpath.Ext(fullOutputPath) { case ".jar", ".zip": @@ -986,14 +1036,23 @@ plugins: fullOutputPath, ) require.NoError(t, err) - require.True(t, len(data) > 1, "expected non-fake data at %q", fullOutputPath) + if useCleanFlag || fullOutputPath == expectedPluginConfigCleanOut { + require.True(t, len(data) > 1, "expected non-fake data at %q", fullOutputPath) + } else { + require.True(t, len(data) == 1, "expected fake data at %q", fullOutputPath) + } default: - _, err := storage.ReadPath( + data, err := storage.ReadPath( ctx, storageBucket, normalpath.Join(fullOutputPath, "foo.txt"), ) - require.ErrorIs(t, err, fs.ErrNotExist) + if useCleanFlag || fullOutputPath == expectedPluginConfigCleanOut { + require.ErrorIs(t, err, fs.ErrNotExist) + } else { + require.NotNil(t, data, "expected foo.txt at %q", fullOutputPath) + require.NoError(t, err, "expected foo.txt at %q", fullOutputPath) + } } } } diff --git a/private/bufpkg/bufconfig/buf_gen_yaml_file.go b/private/bufpkg/bufconfig/buf_gen_yaml_file.go index a35511689e..6d09e3e667 100644 --- a/private/bufpkg/bufconfig/buf_gen_yaml_file.go +++ b/private/bufpkg/bufconfig/buf_gen_yaml_file.go @@ -504,6 +504,9 @@ type externalGeneratePluginConfigV2 struct { ProtocPath any `json:"protoc_path,omitempty" yaml:"protoc_path,omitempty"` // Out is required. Out string `json:"out,omitempty" yaml:"out,omitempty"` + // Clean, if set to true, will delete the output directories, zip files, or jar files + // before generation is run. + Clean bool `json:"clean,omitempty" yaml:"clean,omitempty"` // Opt can be one string or multiple strings. Opt any `json:"opt,omitempty" yaml:"opt,omitempty"` IncludeImports bool `json:"include_imports,omitempty" yaml:"include_imports,omitempty"` diff --git a/private/bufpkg/bufconfig/buf_gen_yaml_file_test.go b/private/bufpkg/bufconfig/buf_gen_yaml_file_test.go index b8d3b26e37..a110286f82 100644 --- a/private/bufpkg/bufconfig/buf_gen_yaml_file_test.go +++ b/private/bufpkg/bufconfig/buf_gen_yaml_file_test.go @@ -106,6 +106,27 @@ plugins: t, // input `version: v2 +plugins: + - local: custom-gen-go + out: gen/go + clean: true + opt: paths=source_relative + strategy: directory +`, + // expected output + `version: v2 +plugins: + - local: custom-gen-go + out: gen/go + clean: true + opt: paths=source_relative + strategy: directory +`, + ) + testReadWriteBufGenYAMLFileRoundTrip( + t, + // input + `version: v2 managed: disable: - module: buf.build/googleapis/googleapis diff --git a/private/bufpkg/bufconfig/generate_plugin_config.go b/private/bufpkg/bufconfig/generate_plugin_config.go index e2509438b1..c51d3364fa 100644 --- a/private/bufpkg/bufconfig/generate_plugin_config.go +++ b/private/bufpkg/bufconfig/generate_plugin_config.go @@ -83,6 +83,9 @@ type GeneratePluginConfig interface { Name() string // Out returns the output directory for generation. This is never empty. Out() string + // Clean, if true, will delete the output directories, zip files, or jar files before + // generation is run. + Clean() bool // Opt returns the plugin options as a comma separated string. Opt() string // IncludeImports returns whether to generate code for imported files. This @@ -120,6 +123,7 @@ type GeneratePluginConfig interface { func NewRemotePluginConfig( name string, out string, + clean bool, opt []string, includeImports bool, includeWKT bool, @@ -128,6 +132,7 @@ func NewRemotePluginConfig( return newRemotePluginConfig( name, out, + clean, opt, includeImports, includeWKT, @@ -140,6 +145,7 @@ func NewLocalOrProtocBuiltinPluginConfig( name string, out string, opt []string, + clean bool, includeImports bool, includeWKT bool, strategy *GenerateStrategy, @@ -147,6 +153,7 @@ func NewLocalOrProtocBuiltinPluginConfig( return newLocalOrProtocBuiltinPluginConfig( name, out, + clean, opt, includeImports, includeWKT, @@ -158,6 +165,7 @@ func NewLocalOrProtocBuiltinPluginConfig( func NewLocalPluginConfig( name string, out string, + clean bool, opt []string, includeImports bool, includeWKT bool, @@ -167,6 +175,7 @@ func NewLocalPluginConfig( return newLocalPluginConfig( name, out, + clean, opt, includeImports, includeWKT, @@ -180,6 +189,7 @@ func NewLocalPluginConfig( func NewProtocBuiltinPluginConfig( name string, out string, + clean bool, opt []string, includeImports bool, includeWKT bool, @@ -189,6 +199,7 @@ func NewProtocBuiltinPluginConfig( return newProtocBuiltinPluginConfig( name, out, + clean, opt, includeImports, includeWKT, @@ -224,6 +235,7 @@ type pluginConfig struct { pluginConfigType PluginConfigType name string out string + clean bool opts []string includeImports bool includeWKT bool @@ -255,6 +267,7 @@ func newPluginConfigFromExternalV1Beta1( return newLocalPluginConfig( externalConfig.Name, externalConfig.Out, + false, // Clean is not available on Date: Thu, 18 Jul 2024 13:19:38 -0400 Subject: [PATCH 2/3] Move clean config option to the top level of generation config (#3165) This PR targets branch `add-buf-gen-yaml-clean` and moves `clean` to `buf.gen.yaml`'s top level, from the plugin level. --------- Co-authored-by: Doria Keung --- CHANGELOG.md | 4 +- private/buf/bufgen/bufgen.go | 4 +- private/buf/bufgen/generator.go | 47 ++--- .../buf/cmd/buf/command/generate/generate.go | 12 +- .../cmd/buf/command/generate/generate_test.go | 162 ++++++++---------- private/bufpkg/bufconfig/buf_gen_yaml_file.go | 23 +-- .../bufconfig/buf_gen_yaml_file_test.go | 4 +- private/bufpkg/bufconfig/generate_config.go | 27 ++- .../bufconfig/generate_plugin_config.go | 34 ---- 9 files changed, 138 insertions(+), 179 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d8fe364722..c73c46aa06 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,8 +5,8 @@ - Add `buf generate --clean` flag that will delete the directories, jar files, or zip files that the plugins will write to, prior to generation. Allows cleaning of existing assets without having to call `rm -rf`. -- Add `clean` as an option for plugin configurations in `buf.gen.yaml`. If set to true, this - will delete the directories, jar files, or zip files set to `out` for the plugin. +- Add `clean_plugin_outs` as a top-level option in `buf.gen.yaml`. If set to true, this will delete the directories, + jar files, or zip files set to `out` for each plugin. ## [v1.34.0] - 2024-06-21 diff --git a/private/buf/bufgen/bufgen.go b/private/buf/bufgen/bufgen.go index a9d3262176..c349691e18 100644 --- a/private/buf/bufgen/bufgen.go +++ b/private/buf/bufgen/bufgen.go @@ -120,9 +120,9 @@ func GenerateWithBaseOutDirPath(baseOutDirPath string) GenerateOption { // GenerateWithDeleteOuts returns a new GenerateOption that results in the // output directories, zip files, or jar files being deleted before generation is run. -func GenerateWithDeleteOuts() GenerateOption { +func GenerateWithDeleteOuts(deleteOuts bool) GenerateOption { return func(generateOptions *generateOptions) { - generateOptions.deleteOuts = true + generateOptions.deleteOuts = &deleteOuts } } diff --git a/private/buf/bufgen/generator.go b/private/buf/bufgen/generator.go index fc795ab7fc..375420a4f0 100644 --- a/private/buf/bufgen/generator.go +++ b/private/buf/bufgen/generator.go @@ -34,6 +34,7 @@ import ( "github.com/bufbuild/buf/private/pkg/app" "github.com/bufbuild/buf/private/pkg/command" "github.com/bufbuild/buf/private/pkg/connectclient" + "github.com/bufbuild/buf/private/pkg/slicesext" "github.com/bufbuild/buf/private/pkg/storage/storageos" "github.com/bufbuild/buf/private/pkg/thread" "github.com/bufbuild/buf/private/pkg/tracing" @@ -104,13 +105,18 @@ func (g *generator) Generate( return err } } - if err := g.deleteOuts( - ctx, - generateOptions.baseOutDirPath, - generateOptions.deleteOuts, - config.GeneratePluginConfigs(), - ); err != nil { - return err + shouldDeleteOuts := config.CleanPluginOuts() + if generateOptions.deleteOuts != nil { + shouldDeleteOuts = *generateOptions.deleteOuts + } + if shouldDeleteOuts { + if err := g.deleteOuts( + ctx, + generateOptions.baseOutDirPath, + config.GeneratePluginConfigs(), + ); err != nil { + return err + } } for _, image := range images { if err := g.generateCode( @@ -131,20 +137,21 @@ func (g *generator) Generate( func (g *generator) deleteOuts( ctx context.Context, baseOutDir string, - deleteAllOuts bool, pluginConfigs []bufconfig.GeneratePluginConfig, ) error { - var pluginOuts []string - for _, pluginConfig := range pluginConfigs { - if deleteAllOuts || pluginConfig.Clean() { - if baseOutDir != "" && baseOutDir != "." { - pluginOuts = append(pluginOuts, filepath.Join(baseOutDir, pluginConfig.Out())) - } else { - pluginOuts = append(pluginOuts, pluginConfig.Out()) - } - } - } - return bufprotopluginos.NewCleaner(g.storageosProvider).DeleteOuts(ctx, pluginOuts) + return bufprotopluginos.NewCleaner(g.storageosProvider).DeleteOuts( + ctx, + slicesext.Map( + pluginConfigs, + func(pluginConfig bufconfig.GeneratePluginConfig) string { + out := pluginConfig.Out() + if baseOutDir != "" && baseOutDir != "." { + return filepath.Join(baseOutDir, out) + } + return out + }, + ), + ) } func (g *generator) generateCode( @@ -481,7 +488,7 @@ func validateResponses( type generateOptions struct { baseOutDirPath string - deleteOuts bool + deleteOuts *bool includeImportsOverride *bool includeWellKnownTypesOverride *bool } diff --git a/private/buf/cmd/buf/command/generate/generate.go b/private/buf/cmd/buf/command/generate/generate.go index daa8297ead..613cbe8866 100644 --- a/private/buf/cmd/buf/command/generate/generate.go +++ b/private/buf/cmd/buf/command/generate/generate.go @@ -373,7 +373,7 @@ Insertion points are processed in the order the plugins are specified in the tem type flags struct { Template string BaseOutDirPath string - DeleteOuts bool + DeleteOuts *bool ErrorFormat string Files []string Config string @@ -427,10 +427,10 @@ func (f *flags) Bind(flagSet *pflag.FlagSet) { ".", `The base directory to generate to. This is prepended to the out directories in the generation template`, ) - flagSet.BoolVar( - &f.DeleteOuts, + bindBoolPointer( + flagSet, deleteOutsFlagName, - false, + &f.DeleteOuts, `Prior to generation, delete the directories, jar files, or zip files that the plugins will write to. Allows cleaning of existing assets without having to call rm -rf`, ) flagSet.StringVar( @@ -522,10 +522,10 @@ func run( generateOptions := []bufgen.GenerateOption{ bufgen.GenerateWithBaseOutDirPath(flags.BaseOutDirPath), } - if flags.DeleteOuts { + if flags.DeleteOuts != nil { generateOptions = append( generateOptions, - bufgen.GenerateWithDeleteOuts(), + bufgen.GenerateWithDeleteOuts(*flags.DeleteOuts), ) } if flags.IncludeImportsOverride != nil { diff --git a/private/buf/cmd/buf/command/generate/generate_test.go b/private/buf/cmd/buf/command/generate/generate_test.go index 2b18b18524..7ffdd62eca 100644 --- a/private/buf/cmd/buf/command/generate/generate_test.go +++ b/private/buf/cmd/buf/command/generate/generate_test.go @@ -550,37 +550,20 @@ func TestGenerateInsertionPointMixedPathsFail(t *testing.T) { func TestGenerateDeleteOutDir(t *testing.T) { t.Parallel() - // Use --clean flag - testGenerateDeleteOuts(t, true, "", "foo") - testGenerateDeleteOuts(t, true, "base", "foo") - testGenerateDeleteOuts(t, true, "", "foo", "bar") - testGenerateDeleteOuts(t, true, "", "foo", "bar", "foo") - testGenerateDeleteOuts(t, true, "base", "foo", "bar") - testGenerateDeleteOuts(t, true, "base", "foo", "bar", "foo") - testGenerateDeleteOuts(t, true, "", "foo.jar") - testGenerateDeleteOuts(t, true, "", "foo.zip") - testGenerateDeleteOuts(t, true, "", "foo/bar.jar") - testGenerateDeleteOuts(t, true, "", "foo/bar.zip") - testGenerateDeleteOuts(t, true, "base", "foo.jar") - testGenerateDeleteOuts(t, true, "base", "foo.zip") - testGenerateDeleteOuts(t, true, "base", "foo/bar.jar") - testGenerateDeleteOuts(t, true, "base", "foo/bar.zip") - - // Only set clean in plugin configs - testGenerateDeleteOuts(t, false, "", "foo") - testGenerateDeleteOuts(t, false, "base", "foo") - testGenerateDeleteOuts(t, false, "", "foo", "bar") - testGenerateDeleteOuts(t, false, "", "foo", "bar", "foo") - testGenerateDeleteOuts(t, false, "base", "foo", "bar") - testGenerateDeleteOuts(t, false, "base", "foo", "bar", "foo") - testGenerateDeleteOuts(t, false, "", "foo.jar") - testGenerateDeleteOuts(t, false, "", "foo.zip") - testGenerateDeleteOuts(t, false, "", "foo/bar.jar") - testGenerateDeleteOuts(t, false, "", "foo/bar.zip") - testGenerateDeleteOuts(t, false, "base", "foo.jar") - testGenerateDeleteOuts(t, false, "base", "foo.zip") - testGenerateDeleteOuts(t, false, "base", "foo/bar.jar") - testGenerateDeleteOuts(t, false, "base", "foo/bar.zip") + testGenerateDeleteOuts(t, "", "foo") + testGenerateDeleteOuts(t, "base", "foo") + testGenerateDeleteOuts(t, "", "foo", "bar") + testGenerateDeleteOuts(t, "", "foo", "bar", "foo") + testGenerateDeleteOuts(t, "base", "foo", "bar") + testGenerateDeleteOuts(t, "base", "foo", "bar", "foo") + testGenerateDeleteOuts(t, "", "foo.jar") + testGenerateDeleteOuts(t, "", "foo.zip") + testGenerateDeleteOuts(t, "", "foo/bar.jar") + testGenerateDeleteOuts(t, "", "foo/bar.zip") + testGenerateDeleteOuts(t, "base", "foo.jar") + testGenerateDeleteOuts(t, "base", "foo.zip") + testGenerateDeleteOuts(t, "base", "foo/bar.jar") + testGenerateDeleteOuts(t, "base", "foo/bar.zip") } func TestBoolPointerFlagTrue(t *testing.T) { @@ -923,13 +906,29 @@ func testRunSuccess(t *testing.T, args ...string) { func testGenerateDeleteOuts( t *testing.T, - useCleanFlag bool, baseOutDirPath string, outputPaths ...string, ) { - plugins := []string{"java", "cpp", "ruby"} - // Just add more builtins to the plugins slice above if this goes off - require.True(t, len(outputPaths) <= len(plugins), "we want to have unique plugins to work with and this test is only set up for three plugins max right now") + testGenerateDeleteOutsWithArgAndConfig(t, baseOutDirPath, nil, true, true, outputPaths) + testGenerateDeleteOutsWithArgAndConfig(t, baseOutDirPath, nil, false, false, outputPaths) + testGenerateDeleteOutsWithArgAndConfig(t, baseOutDirPath, []string{"--clean"}, false, true, outputPaths) + testGenerateDeleteOutsWithArgAndConfig(t, baseOutDirPath, []string{"--clean"}, true, true, outputPaths) + testGenerateDeleteOutsWithArgAndConfig(t, baseOutDirPath, []string{"--clean=true"}, true, true, outputPaths) + testGenerateDeleteOutsWithArgAndConfig(t, baseOutDirPath, []string{"--clean=true"}, false, true, outputPaths) + testGenerateDeleteOutsWithArgAndConfig(t, baseOutDirPath, []string{"--clean=false"}, true, false, outputPaths) + testGenerateDeleteOutsWithArgAndConfig(t, baseOutDirPath, []string{"--clean=false"}, false, false, outputPaths) +} + +func testGenerateDeleteOutsWithArgAndConfig( + t *testing.T, + baseOutDirPath string, + cleanArgs []string, + cleanOptionInConfig bool, + expectedClean bool, + outputPaths []string, +) { + // Just add more builtins to the plugins slice below if this goes off + require.True(t, len(outputPaths) < 4, "we want to have unique plugins to work with and this test is only set up for three plugins max right now") fullOutputPaths := outputPaths if baseOutDirPath != "" && baseOutDirPath != "." { fullOutputPaths = slicesext.Map( @@ -974,59 +973,35 @@ func testGenerateDeleteOuts( plugins: `) - var expectedPluginConfigCleanOut string - if useCleanFlag { - // When using the --clean flag, we do not set clean in the plugin configs at all - for i, outputPath := range outputPaths { - _, _ = templateBuilder.WriteString(` - protoc_builtin: `) - _, _ = templateBuilder.WriteString(plugins[i]) - _, _ = templateBuilder.WriteString("\n") - _, _ = templateBuilder.WriteString(` out: `) - _, _ = templateBuilder.WriteString(outputPath) - _, _ = templateBuilder.WriteString("\n") - } - testRunStdoutStderr( - t, - nil, - 0, - ``, - ``, - filepath.Join("testdata", "simple"), - "--template", - templateBuilder.String(), - "-o", - filepath.Join(tmpDirPath, normalpath.Unnormalize(baseOutDirPath)), - "--clean", - ) - } else { - for i, outputPath := range outputPaths { - _, _ = templateBuilder.WriteString(` - protoc_builtin: `) - _, _ = templateBuilder.WriteString(plugins[i]) - _, _ = templateBuilder.WriteString("\n") - _, _ = templateBuilder.WriteString(` out: `) - _, _ = templateBuilder.WriteString(outputPath) - _, _ = templateBuilder.WriteString("\n") - // Only set "clean: true" for the first plugin (java), and we expect that only the - // output for the first plugin to be cleaned - if i == 0 { // plugins[0] == "java" - _, _ = templateBuilder.WriteString(` clean: true`) - _, _ = templateBuilder.WriteString("\n") - expectedPluginConfigCleanOut = fullOutputPaths[i] - } - } - testRunStdoutStderr( - t, - nil, - 0, - ``, - ``, - filepath.Join("testdata", "simple"), - "--template", - templateBuilder.String(), - "-o", - filepath.Join(tmpDirPath, normalpath.Unnormalize(baseOutDirPath)), - ) + plugins := []string{"java", "cpp", "ruby"} + for i, outputPath := range outputPaths { + _, _ = templateBuilder.WriteString(` - protoc_builtin: `) + _, _ = templateBuilder.WriteString(plugins[i]) + _, _ = templateBuilder.WriteString("\n") + _, _ = templateBuilder.WriteString(` out: `) + _, _ = templateBuilder.WriteString(outputPath) + _, _ = templateBuilder.WriteString("\n") } + if cleanOptionInConfig { + templateBuilder.WriteString("clean_plugin_outs: true\n") + } + testRunStdoutStderr( + t, + nil, + 0, + ``, + ``, + append( + []string{ + filepath.Join("testdata", "simple"), + "--template", + templateBuilder.String(), + "-o", + filepath.Join(tmpDirPath, normalpath.Unnormalize(baseOutDirPath)), + }, + cleanArgs..., + )..., + ) for _, fullOutputPath := range fullOutputPaths { switch normalpath.Ext(fullOutputPath) { case ".jar", ".zip": @@ -1036,22 +1011,21 @@ plugins: fullOutputPath, ) require.NoError(t, err) - if useCleanFlag || fullOutputPath == expectedPluginConfigCleanOut { - require.True(t, len(data) > 1, "expected non-fake data at %q", fullOutputPath) - } else { - require.True(t, len(data) == 1, "expected fake data at %q", fullOutputPath) - } + // Always expect non-fake data, because the existing ".jar" or ".zip" + // file is always replaced by the output. This is the existing and correct + // behavior. + require.True(t, len(data) > 1, "expected non-fake data at %q", fullOutputPath) default: data, err := storage.ReadPath( ctx, storageBucket, normalpath.Join(fullOutputPath, "foo.txt"), ) - if useCleanFlag || fullOutputPath == expectedPluginConfigCleanOut { + if expectedClean { require.ErrorIs(t, err, fs.ErrNotExist) } else { - require.NotNil(t, data, "expected foo.txt at %q", fullOutputPath) require.NoError(t, err, "expected foo.txt at %q", fullOutputPath) + require.NotNil(t, data, "expected foo.txt at %q", fullOutputPath) } } } diff --git a/private/bufpkg/bufconfig/buf_gen_yaml_file.go b/private/bufpkg/bufconfig/buf_gen_yaml_file.go index 6d09e3e667..394e3814d5 100644 --- a/private/bufpkg/bufconfig/buf_gen_yaml_file.go +++ b/private/bufpkg/bufconfig/buf_gen_yaml_file.go @@ -257,10 +257,11 @@ func writeBufGenYAMLFile(writer io.Writer, bufGenYAMLFile BufGenYAMLFile) error return err } externalBufGenYAMLFileV2 := externalBufGenYAMLFileV2{ - Version: FileVersionV2.String(), - Plugins: externalPluginConfigsV2, - Managed: externalManagedConfigV2, - Inputs: externalInputConfigsV2, + Version: FileVersionV2.String(), + CleanPluginOuts: bufGenYAMLFile.GenerateConfig().CleanPluginOuts(), + Plugins: externalPluginConfigsV2, + Managed: externalManagedConfigV2, + Inputs: externalInputConfigsV2, } data, err := encoding.MarshalYAML(&externalBufGenYAMLFileV2) if err != nil { @@ -481,10 +482,13 @@ type externalTypesConfigV1 struct { // externalBufGenYAMLFileV2 represents the v2 buf.gen.yaml file. type externalBufGenYAMLFileV2 struct { - Version string `json:"version,omitempty" yaml:"version,omitempty"` - Managed externalGenerateManagedConfigV2 `json:"managed,omitempty" yaml:"managed,omitempty"` - Plugins []externalGeneratePluginConfigV2 `json:"plugins,omitempty" yaml:"plugins,omitempty"` - Inputs []externalInputConfigV2 `json:"inputs,omitempty" yaml:"inputs,omitempty"` + Version string `json:"version,omitempty" yaml:"version,omitempty"` + Managed externalGenerateManagedConfigV2 `json:"managed,omitempty" yaml:"managed,omitempty"` + // CleanPluginOuts, if set to true, will delete the output directories, zip files, or jar files + // before generation is run. + CleanPluginOuts bool `json:"clean_plugin_outs,omitempty" yaml:"clean_plugin_outs,omitempty"` + Plugins []externalGeneratePluginConfigV2 `json:"plugins,omitempty" yaml:"plugins,omitempty"` + Inputs []externalInputConfigV2 `json:"inputs,omitempty" yaml:"inputs,omitempty"` } // externalGeneratePluginConfigV2 represents a single plugin config in a v2 buf.gen.yaml file. @@ -504,9 +508,6 @@ type externalGeneratePluginConfigV2 struct { ProtocPath any `json:"protoc_path,omitempty" yaml:"protoc_path,omitempty"` // Out is required. Out string `json:"out,omitempty" yaml:"out,omitempty"` - // Clean, if set to true, will delete the output directories, zip files, or jar files - // before generation is run. - Clean bool `json:"clean,omitempty" yaml:"clean,omitempty"` // Opt can be one string or multiple strings. Opt any `json:"opt,omitempty" yaml:"opt,omitempty"` IncludeImports bool `json:"include_imports,omitempty" yaml:"include_imports,omitempty"` diff --git a/private/bufpkg/bufconfig/buf_gen_yaml_file_test.go b/private/bufpkg/bufconfig/buf_gen_yaml_file_test.go index a110286f82..8ef14b0826 100644 --- a/private/bufpkg/bufconfig/buf_gen_yaml_file_test.go +++ b/private/bufpkg/bufconfig/buf_gen_yaml_file_test.go @@ -106,19 +106,19 @@ plugins: t, // input `version: v2 +clean_plugin_outs: true plugins: - local: custom-gen-go out: gen/go - clean: true opt: paths=source_relative strategy: directory `, // expected output `version: v2 +clean_plugin_outs: true plugins: - local: custom-gen-go out: gen/go - clean: true opt: paths=source_relative strategy: directory `, diff --git a/private/bufpkg/bufconfig/generate_config.go b/private/bufpkg/bufconfig/generate_config.go index 8c2bc51b59..ac243467db 100644 --- a/private/bufpkg/bufconfig/generate_config.go +++ b/private/bufpkg/bufconfig/generate_config.go @@ -22,6 +22,9 @@ import ( // GenerateConfig is a generation configuration. type GenerateConfig interface { + // CleanPluginOuts is whether to delete the output directories, zip files, or jar files before + // generation is run. + CleanPluginOuts() bool // GeneratePluginConfigs returns the plugin configurations. This will always be // non-empty. Zero plugin configs will cause an error at construction time. GeneratePluginConfigs() []GeneratePluginConfig @@ -38,6 +41,7 @@ type GenerateConfig interface { // NewGenerateConfig returns a validated GenerateConfig. func NewGenerateConfig( + cleanPluginOuts bool, pluginConfigs []GeneratePluginConfig, managedConfig GenerateManagedConfig, typeConfig GenerateTypeConfig, @@ -46,18 +50,20 @@ func NewGenerateConfig( return nil, newNoPluginsError() } return &generateConfig{ - pluginConfigs: pluginConfigs, - managedConfig: managedConfig, - typeConfig: typeConfig, + cleanPluginOuts: cleanPluginOuts, + pluginConfigs: pluginConfigs, + managedConfig: managedConfig, + typeConfig: typeConfig, }, nil } // *** PRIVATE *** type generateConfig struct { - pluginConfigs []GeneratePluginConfig - managedConfig GenerateManagedConfig - typeConfig GenerateTypeConfig + cleanPluginOuts bool + pluginConfigs []GeneratePluginConfig + managedConfig GenerateManagedConfig + typeConfig GenerateTypeConfig } func newGenerateConfigFromExternalFileV1Beta1( @@ -122,11 +128,16 @@ func newGenerateConfigFromExternalFileV2( return nil, err } return &generateConfig{ - managedConfig: managedConfig, - pluginConfigs: pluginConfigs, + cleanPluginOuts: externalFile.CleanPluginOuts, + managedConfig: managedConfig, + pluginConfigs: pluginConfigs, }, nil } +func (g *generateConfig) CleanPluginOuts() bool { + return g.cleanPluginOuts +} + func (g *generateConfig) GeneratePluginConfigs() []GeneratePluginConfig { return g.pluginConfigs } diff --git a/private/bufpkg/bufconfig/generate_plugin_config.go b/private/bufpkg/bufconfig/generate_plugin_config.go index c51d3364fa..e2509438b1 100644 --- a/private/bufpkg/bufconfig/generate_plugin_config.go +++ b/private/bufpkg/bufconfig/generate_plugin_config.go @@ -83,9 +83,6 @@ type GeneratePluginConfig interface { Name() string // Out returns the output directory for generation. This is never empty. Out() string - // Clean, if true, will delete the output directories, zip files, or jar files before - // generation is run. - Clean() bool // Opt returns the plugin options as a comma separated string. Opt() string // IncludeImports returns whether to generate code for imported files. This @@ -123,7 +120,6 @@ type GeneratePluginConfig interface { func NewRemotePluginConfig( name string, out string, - clean bool, opt []string, includeImports bool, includeWKT bool, @@ -132,7 +128,6 @@ func NewRemotePluginConfig( return newRemotePluginConfig( name, out, - clean, opt, includeImports, includeWKT, @@ -145,7 +140,6 @@ func NewLocalOrProtocBuiltinPluginConfig( name string, out string, opt []string, - clean bool, includeImports bool, includeWKT bool, strategy *GenerateStrategy, @@ -153,7 +147,6 @@ func NewLocalOrProtocBuiltinPluginConfig( return newLocalOrProtocBuiltinPluginConfig( name, out, - clean, opt, includeImports, includeWKT, @@ -165,7 +158,6 @@ func NewLocalOrProtocBuiltinPluginConfig( func NewLocalPluginConfig( name string, out string, - clean bool, opt []string, includeImports bool, includeWKT bool, @@ -175,7 +167,6 @@ func NewLocalPluginConfig( return newLocalPluginConfig( name, out, - clean, opt, includeImports, includeWKT, @@ -189,7 +180,6 @@ func NewLocalPluginConfig( func NewProtocBuiltinPluginConfig( name string, out string, - clean bool, opt []string, includeImports bool, includeWKT bool, @@ -199,7 +189,6 @@ func NewProtocBuiltinPluginConfig( return newProtocBuiltinPluginConfig( name, out, - clean, opt, includeImports, includeWKT, @@ -235,7 +224,6 @@ type pluginConfig struct { pluginConfigType PluginConfigType name string out string - clean bool opts []string includeImports bool includeWKT bool @@ -267,7 +255,6 @@ func newPluginConfigFromExternalV1Beta1( return newLocalPluginConfig( externalConfig.Name, externalConfig.Out, - false, // Clean is not available on Date: Thu, 1 Aug 2024 18:19:24 -0400 Subject: [PATCH 3/3] s/clean_plugin_outs/clean/ --- CHANGELOG.md | 5 +++-- .../cmd/buf/command/generate/generate_test.go | 2 +- private/bufpkg/bufconfig/buf_gen_yaml_file.go | 18 +++++++++--------- .../bufpkg/bufconfig/buf_gen_yaml_file_test.go | 4 ++-- private/bufpkg/bufconfig/generate_config.go | 2 +- 5 files changed, 16 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 55d39114e7..27378e6c40 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,9 @@ ## [Unreleased] +- Add `clean` as a top-level option in `buf.gen.yaml`, matching the `buf generate --clean` flag. If + set to true, this will delete the directories, jar files, or zip files set to `out` for each + plugin. - Fix git input handling of annotated tags. - Update `buf registry login` to complete the login flow in the browser by default. This allows users to login with their browser and have the token automatically provided to the CLI. @@ -18,8 +21,6 @@ - Add `buf generate --clean` flag that will delete the directories, jar files, or zip files that the plugins will write to, prior to generation. Allows cleaning of existing assets without having to call `rm -rf`. -- Add `clean_plugin_outs` as a top-level option in `buf.gen.yaml`. If set to true, this will delete the directories, - jar files, or zip files set to `out` for each plugin. - Deprecate `--username` flag on and username prompt on `buf registry login`. A username is no longer required to log in. - Add `--list-services` and `--list-methods` flags to `buf curl`, which trigger the command to list diff --git a/private/buf/cmd/buf/command/generate/generate_test.go b/private/buf/cmd/buf/command/generate/generate_test.go index 7ffdd62eca..22758a0b10 100644 --- a/private/buf/cmd/buf/command/generate/generate_test.go +++ b/private/buf/cmd/buf/command/generate/generate_test.go @@ -983,7 +983,7 @@ plugins: _, _ = templateBuilder.WriteString("\n") } if cleanOptionInConfig { - templateBuilder.WriteString("clean_plugin_outs: true\n") + templateBuilder.WriteString("clean: true\n") } testRunStdoutStderr( t, diff --git a/private/bufpkg/bufconfig/buf_gen_yaml_file.go b/private/bufpkg/bufconfig/buf_gen_yaml_file.go index 394e3814d5..fb98bb62a4 100644 --- a/private/bufpkg/bufconfig/buf_gen_yaml_file.go +++ b/private/bufpkg/bufconfig/buf_gen_yaml_file.go @@ -257,11 +257,11 @@ func writeBufGenYAMLFile(writer io.Writer, bufGenYAMLFile BufGenYAMLFile) error return err } externalBufGenYAMLFileV2 := externalBufGenYAMLFileV2{ - Version: FileVersionV2.String(), - CleanPluginOuts: bufGenYAMLFile.GenerateConfig().CleanPluginOuts(), - Plugins: externalPluginConfigsV2, - Managed: externalManagedConfigV2, - Inputs: externalInputConfigsV2, + Version: FileVersionV2.String(), + Clean: bufGenYAMLFile.GenerateConfig().CleanPluginOuts(), + Plugins: externalPluginConfigsV2, + Managed: externalManagedConfigV2, + Inputs: externalInputConfigsV2, } data, err := encoding.MarshalYAML(&externalBufGenYAMLFileV2) if err != nil { @@ -484,11 +484,11 @@ type externalTypesConfigV1 struct { type externalBufGenYAMLFileV2 struct { Version string `json:"version,omitempty" yaml:"version,omitempty"` Managed externalGenerateManagedConfigV2 `json:"managed,omitempty" yaml:"managed,omitempty"` - // CleanPluginOuts, if set to true, will delete the output directories, zip files, or jar files + // Clean, if set to true, will delete the output directories, zip files, or jar files // before generation is run. - CleanPluginOuts bool `json:"clean_plugin_outs,omitempty" yaml:"clean_plugin_outs,omitempty"` - Plugins []externalGeneratePluginConfigV2 `json:"plugins,omitempty" yaml:"plugins,omitempty"` - Inputs []externalInputConfigV2 `json:"inputs,omitempty" yaml:"inputs,omitempty"` + Clean bool `json:"clean,omitempty" yaml:"clean,omitempty"` + Plugins []externalGeneratePluginConfigV2 `json:"plugins,omitempty" yaml:"plugins,omitempty"` + Inputs []externalInputConfigV2 `json:"inputs,omitempty" yaml:"inputs,omitempty"` } // externalGeneratePluginConfigV2 represents a single plugin config in a v2 buf.gen.yaml file. diff --git a/private/bufpkg/bufconfig/buf_gen_yaml_file_test.go b/private/bufpkg/bufconfig/buf_gen_yaml_file_test.go index 8ef14b0826..1ef6c16d96 100644 --- a/private/bufpkg/bufconfig/buf_gen_yaml_file_test.go +++ b/private/bufpkg/bufconfig/buf_gen_yaml_file_test.go @@ -106,7 +106,7 @@ plugins: t, // input `version: v2 -clean_plugin_outs: true +clean: true plugins: - local: custom-gen-go out: gen/go @@ -115,7 +115,7 @@ plugins: `, // expected output `version: v2 -clean_plugin_outs: true +clean: true plugins: - local: custom-gen-go out: gen/go diff --git a/private/bufpkg/bufconfig/generate_config.go b/private/bufpkg/bufconfig/generate_config.go index ac243467db..dad48e8717 100644 --- a/private/bufpkg/bufconfig/generate_config.go +++ b/private/bufpkg/bufconfig/generate_config.go @@ -128,7 +128,7 @@ func newGenerateConfigFromExternalFileV2( return nil, err } return &generateConfig{ - cleanPluginOuts: externalFile.CleanPluginOuts, + cleanPluginOuts: externalFile.Clean, managedConfig: managedConfig, pluginConfigs: pluginConfigs, }, nil