Move clean config option to the top level of generation config

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
 

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
  }
 }
 

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
 }

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 {

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)
  }
  }
  }

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"`