From 99459cd2cc205bb18d8df3c1b4ddf551ed7098a5 Mon Sep 17 00:00:00 2001 From: Edward McFarlane Date: Tue, 10 Sep 2024 14:50:20 -0400 Subject: [PATCH] Run bufcheck with Plugins Moves the bufconfig.PluginConfig to be provided with the plugin runner as a Plugin. This allows for setting the pluginrpc.Runner based on the config. --- private/buf/bufmigrate/migrator.go | 3 +- private/buf/cmd/buf/buf_test.go | 4 +- .../buf/cmd/buf/command/breaking/breaking.go | 9 +- .../buf/command/config/internal/internal.go | 15 +++- private/buf/cmd/buf/command/lint/lint.go | 9 +- .../cmd/buf/command/mod/internal/internal.go | 4 +- .../cmd/protoc-gen-buf-breaking/breaking.go | 4 +- private/buf/cmd/protoc-gen-buf-lint/lint.go | 4 +- private/bufpkg/bufcheck/breaking_test.go | 7 +- private/bufpkg/bufcheck/bufcheck.go | 47 +++++++--- private/bufpkg/bufcheck/client.go | 90 +++++++++---------- private/bufpkg/bufcheck/lint_test.go | 7 +- private/bufpkg/bufcheck/multi_client_test.go | 17 ++-- private/bufpkg/bufcheck/plugin.go | 40 +++++++++ private/pkg/pluginrpcutil/pluginrpcutil.go | 26 ------ 15 files changed, 157 insertions(+), 129 deletions(-) create mode 100644 private/bufpkg/bufcheck/plugin.go diff --git a/private/buf/bufmigrate/migrator.go b/private/buf/bufmigrate/migrator.go index afe251297c..e87bf4578f 100644 --- a/private/buf/bufmigrate/migrator.go +++ b/private/buf/bufmigrate/migrator.go @@ -28,7 +28,6 @@ import ( "github.com/bufbuild/buf/private/bufpkg/bufmodule" "github.com/bufbuild/buf/private/pkg/command" "github.com/bufbuild/buf/private/pkg/normalpath" - "github.com/bufbuild/buf/private/pkg/pluginrpcutil" "github.com/bufbuild/buf/private/pkg/slicesext" "github.com/bufbuild/buf/private/pkg/storage" "github.com/bufbuild/buf/private/pkg/storage/storagemem" @@ -713,7 +712,7 @@ func equivalentCheckConfigInV2( ) (bufconfig.CheckConfig, error) { // No need for custom lint/breaking plugins since there's no plugins to migrate from <=v1. // TODO: If we ever need v3, then we will have to deal with this. - client, err := bufcheck.NewClient(logger, tracer, pluginrpcutil.NewRunnerProvider(runner)) + client, err := bufcheck.NewClient(logger, tracer) if err != nil { return nil, err } diff --git a/private/buf/cmd/buf/buf_test.go b/private/buf/cmd/buf/buf_test.go index a2e155825b..853a7332a9 100644 --- a/private/buf/cmd/buf/buf_test.go +++ b/private/buf/cmd/buf/buf_test.go @@ -38,9 +38,7 @@ import ( imagev1 "github.com/bufbuild/buf/private/gen/proto/go/buf/alpha/image/v1" "github.com/bufbuild/buf/private/pkg/app/appcmd" "github.com/bufbuild/buf/private/pkg/app/appcmd/appcmdtesting" - "github.com/bufbuild/buf/private/pkg/command" "github.com/bufbuild/buf/private/pkg/osext" - "github.com/bufbuild/buf/private/pkg/pluginrpcutil" "github.com/bufbuild/buf/private/pkg/slicesext" "github.com/bufbuild/buf/private/pkg/storage/storageos" "github.com/bufbuild/buf/private/pkg/storage/storagetesting" @@ -1350,7 +1348,7 @@ func TestCheckLsBreakingRulesFromConfigExceptDeprecated(t *testing.T) { t.Run(version.String(), func(t *testing.T) { t.Parallel() // Do not need any custom lint/breaking plugins here. - client, err := bufcheck.NewClient(zap.NewNop(), tracing.NopTracer, pluginrpcutil.NewRunnerProvider(command.NewRunner())) + client, err := bufcheck.NewClient(zap.NewNop(), tracing.NopTracer) require.NoError(t, err) allRules, err := client.AllRules(context.Background(), check.RuleTypeBreaking, version) require.NoError(t, err) diff --git a/private/buf/cmd/buf/command/breaking/breaking.go b/private/buf/cmd/buf/command/breaking/breaking.go index 97e60339d7..5239fc2e22 100644 --- a/private/buf/cmd/buf/command/breaking/breaking.go +++ b/private/buf/cmd/buf/command/breaking/breaking.go @@ -28,7 +28,6 @@ import ( "github.com/bufbuild/buf/private/pkg/app/appcmd" "github.com/bufbuild/buf/private/pkg/app/appext" "github.com/bufbuild/buf/private/pkg/command" - "github.com/bufbuild/buf/private/pkg/pluginrpcutil" "github.com/bufbuild/buf/private/pkg/slicesext" "github.com/bufbuild/buf/private/pkg/stringutil" "github.com/bufbuild/buf/private/pkg/tracing" @@ -210,12 +209,16 @@ func run( tracer := tracing.NewTracer(container.Tracer()) var allFileAnnotations []bufanalysis.FileAnnotation for i, imageWithConfig := range imageWithConfigs { - client, err := bufcheck.NewClient(container.Logger(), tracer, pluginrpcutil.NewRunnerProvider(command.NewRunner()), bufcheck.ClientWithStderr(container.Stderr())) + client, err := bufcheck.NewClient(container.Logger(), tracer, bufcheck.ClientWithStderr(container.Stderr())) + if err != nil { + return err + } + plugins, err := bufcheck.NewPluginsForRunner(command.NewRunner(), imageWithConfig.PluginConfigs()...) if err != nil { return err } breakingOptions := []bufcheck.BreakingOption{ - bufcheck.WithPluginConfigs(imageWithConfig.PluginConfigs()...), + bufcheck.WithPlugins(plugins...), } if flags.ExcludeImports { breakingOptions = append(breakingOptions, bufcheck.BreakingWithExcludeImports()) diff --git a/private/buf/cmd/buf/command/config/internal/internal.go b/private/buf/cmd/buf/command/config/internal/internal.go index ba0e0db520..67107ffd7b 100644 --- a/private/buf/cmd/buf/command/config/internal/internal.go +++ b/private/buf/cmd/buf/command/config/internal/internal.go @@ -28,7 +28,6 @@ import ( "github.com/bufbuild/buf/private/pkg/app/appext" "github.com/bufbuild/buf/private/pkg/command" "github.com/bufbuild/buf/private/pkg/normalpath" - "github.com/bufbuild/buf/private/pkg/pluginrpcutil" "github.com/bufbuild/buf/private/pkg/slicesext" "github.com/bufbuild/buf/private/pkg/stringutil" "github.com/bufbuild/buf/private/pkg/syserror" @@ -186,7 +185,7 @@ func lsRun( } } tracer := tracing.NewTracer(container.Tracer()) - client, err := bufcheck.NewClient(container.Logger(), tracer, pluginrpcutil.NewRunnerProvider(command.NewRunner()), bufcheck.ClientWithStderr(container.Stderr())) + client, err := bufcheck.NewClient(container.Logger(), tracer, bufcheck.ClientWithStderr(container.Stderr())) if err != nil { return err } @@ -228,8 +227,12 @@ func lsRun( default: return fmt.Errorf("unknown check.RuleType: %v", ruleType) } + plugins, err := bufcheck.NewPluginsForRunner(command.NewRunner(), bufYAMLFile.PluginConfigs()...) + if err != nil { + return err + } configuredRuleOptions := []bufcheck.ConfiguredRulesOption{ - bufcheck.WithPluginConfigs(bufYAMLFile.PluginConfigs()...), + bufcheck.WithPlugins(plugins...), } if bufcli.IsPluginEnabled(container) { configuredRuleOptions = append(configuredRuleOptions, bufcheck.WithPluginsEnabled()) @@ -244,8 +247,12 @@ func lsRun( return err } } else { + plugins, err := bufcheck.NewPluginsForRunner(command.NewRunner(), bufYAMLFile.PluginConfigs()...) + if err != nil { + return err + } allRulesOptions := []bufcheck.AllRulesOption{ - bufcheck.WithPluginConfigs(bufYAMLFile.PluginConfigs()...), + bufcheck.WithPlugins(plugins...), } if bufcli.IsPluginEnabled(container) { allRulesOptions = append(allRulesOptions, bufcheck.WithPluginsEnabled()) diff --git a/private/buf/cmd/buf/command/lint/lint.go b/private/buf/cmd/buf/command/lint/lint.go index fb117a0bcf..7aa16159f7 100644 --- a/private/buf/cmd/buf/command/lint/lint.go +++ b/private/buf/cmd/buf/command/lint/lint.go @@ -26,7 +26,6 @@ import ( "github.com/bufbuild/buf/private/pkg/app/appcmd" "github.com/bufbuild/buf/private/pkg/app/appext" "github.com/bufbuild/buf/private/pkg/command" - "github.com/bufbuild/buf/private/pkg/pluginrpcutil" "github.com/bufbuild/buf/private/pkg/stringutil" "github.com/bufbuild/buf/private/pkg/tracing" "github.com/spf13/pflag" @@ -135,12 +134,16 @@ func run( tracer := tracing.NewTracer(container.Tracer()) var allFileAnnotations []bufanalysis.FileAnnotation for _, imageWithConfig := range imageWithConfigs { - client, err := bufcheck.NewClient(container.Logger(), tracer, pluginrpcutil.NewRunnerProvider(command.NewRunner()), bufcheck.ClientWithStderr(container.Stderr())) + client, err := bufcheck.NewClient(container.Logger(), tracer, bufcheck.ClientWithStderr(container.Stderr())) + if err != nil { + return err + } + plugins, err := bufcheck.NewPluginsForRunner(command.NewRunner(), imageWithConfig.PluginConfigs()...) if err != nil { return err } lintOptions := []bufcheck.LintOption{ - bufcheck.WithPluginConfigs(imageWithConfig.PluginConfigs()...), + bufcheck.WithPlugins(plugins...), } if bufcli.IsPluginEnabled(container) { lintOptions = append(lintOptions, bufcheck.WithPluginsEnabled()) diff --git a/private/buf/cmd/buf/command/mod/internal/internal.go b/private/buf/cmd/buf/command/mod/internal/internal.go index 64a777c60a..6283bf9c6f 100644 --- a/private/buf/cmd/buf/command/mod/internal/internal.go +++ b/private/buf/cmd/buf/command/mod/internal/internal.go @@ -26,8 +26,6 @@ import ( "github.com/bufbuild/buf/private/bufpkg/bufconfig" "github.com/bufbuild/buf/private/pkg/app/appcmd" "github.com/bufbuild/buf/private/pkg/app/appext" - "github.com/bufbuild/buf/private/pkg/command" - "github.com/bufbuild/buf/private/pkg/pluginrpcutil" "github.com/bufbuild/buf/private/pkg/slicesext" "github.com/bufbuild/buf/private/pkg/stringutil" "github.com/bufbuild/buf/private/pkg/syserror" @@ -176,7 +174,7 @@ func lsRun( } // BufYAMLFiles <=v1 never had plugins. tracer := tracing.NewTracer(container.Tracer()) - client, err := bufcheck.NewClient(container.Logger(), tracer, pluginrpcutil.NewRunnerProvider(command.NewRunner()), bufcheck.ClientWithStderr(container.Stderr())) + client, err := bufcheck.NewClient(container.Logger(), tracer, bufcheck.ClientWithStderr(container.Stderr())) if err != nil { return err } diff --git a/private/buf/cmd/protoc-gen-buf-breaking/breaking.go b/private/buf/cmd/protoc-gen-buf-breaking/breaking.go index c0ee1420b8..de6082576c 100644 --- a/private/buf/cmd/protoc-gen-buf-breaking/breaking.go +++ b/private/buf/cmd/protoc-gen-buf-breaking/breaking.go @@ -28,9 +28,7 @@ import ( "github.com/bufbuild/buf/private/bufpkg/bufanalysis" "github.com/bufbuild/buf/private/bufpkg/bufcheck" "github.com/bufbuild/buf/private/bufpkg/bufimage" - "github.com/bufbuild/buf/private/pkg/command" "github.com/bufbuild/buf/private/pkg/encoding" - "github.com/bufbuild/buf/private/pkg/pluginrpcutil" "github.com/bufbuild/buf/private/pkg/protodescriptor" "github.com/bufbuild/buf/private/pkg/protoencoding" "github.com/bufbuild/buf/private/pkg/tracing" @@ -126,7 +124,7 @@ func handle( } // The protoc plugins do not support custom lint/breaking change plugins for now. tracer := tracing.NewTracer(container.Tracer()) - client, err := bufcheck.NewClient(container.Logger(), tracer, pluginrpcutil.NewRunnerProvider(command.NewRunner()), bufcheck.ClientWithStderr(pluginEnv.Stderr)) + client, err := bufcheck.NewClient(container.Logger(), tracer, bufcheck.ClientWithStderr(pluginEnv.Stderr)) if err != nil { return err } diff --git a/private/buf/cmd/protoc-gen-buf-lint/lint.go b/private/buf/cmd/protoc-gen-buf-lint/lint.go index 37bdd9d1f0..67facac4f6 100644 --- a/private/buf/cmd/protoc-gen-buf-lint/lint.go +++ b/private/buf/cmd/protoc-gen-buf-lint/lint.go @@ -27,9 +27,7 @@ import ( "github.com/bufbuild/buf/private/bufpkg/bufanalysis" "github.com/bufbuild/buf/private/bufpkg/bufcheck" "github.com/bufbuild/buf/private/bufpkg/bufimage" - "github.com/bufbuild/buf/private/pkg/command" "github.com/bufbuild/buf/private/pkg/encoding" - "github.com/bufbuild/buf/private/pkg/pluginrpcutil" "github.com/bufbuild/buf/private/pkg/protodescriptor" "github.com/bufbuild/buf/private/pkg/protoencoding" "github.com/bufbuild/buf/private/pkg/tracing" @@ -101,7 +99,7 @@ func handle( } // The protoc plugins do not support custom lint/breaking change plugins for now. tracer := tracing.NewTracer(container.Tracer()) - client, err := bufcheck.NewClient(container.Logger(), tracer, pluginrpcutil.NewRunnerProvider(command.NewRunner()), bufcheck.ClientWithStderr(pluginEnv.Stderr)) + client, err := bufcheck.NewClient(container.Logger(), tracer, bufcheck.ClientWithStderr(pluginEnv.Stderr)) if err != nil { return err } diff --git a/private/bufpkg/bufcheck/breaking_test.go b/private/bufpkg/bufcheck/breaking_test.go index 58231d569c..a9e6eaed4d 100644 --- a/private/bufpkg/bufcheck/breaking_test.go +++ b/private/bufpkg/bufcheck/breaking_test.go @@ -31,7 +31,6 @@ import ( "github.com/bufbuild/buf/private/bufpkg/bufimage" "github.com/bufbuild/buf/private/bufpkg/bufmodule" "github.com/bufbuild/buf/private/pkg/command" - "github.com/bufbuild/buf/private/pkg/pluginrpcutil" "github.com/bufbuild/buf/private/pkg/storage/storageos" "github.com/bufbuild/buf/private/pkg/tracing" "github.com/stretchr/testify/assert" @@ -1348,7 +1347,9 @@ func testBreaking( require.NoError(t, err) breakingConfig := workspace.GetBreakingConfigForOpaqueID(opaqueID) require.NotNil(t, breakingConfig) - client, err := bufcheck.NewClient(zap.NewNop(), tracing.NopTracer, pluginrpcutil.NewRunnerProvider(command.NewRunner())) + plugins, err := bufcheck.NewPluginsForRunner(command.NewRunner(), workspace.PluginConfigs()...) + require.NoError(t, err) + client, err := bufcheck.NewClient(zap.NewNop(), tracing.NopTracer) require.NoError(t, err) err = client.Breaking( ctx, @@ -1356,7 +1357,7 @@ func testBreaking( image, previousImage, bufcheck.BreakingWithExcludeImports(), - bufcheck.WithPluginConfigs(workspace.PluginConfigs()...), + bufcheck.WithPlugins(plugins...), bufcheck.WithPluginsEnabled(), ) if len(expectedFileAnnotations) == 0 { diff --git a/private/bufpkg/bufcheck/bufcheck.go b/private/bufpkg/bufcheck/bufcheck.go index 7e418a61e1..8e8afac10d 100644 --- a/private/bufpkg/bufcheck/bufcheck.go +++ b/private/bufpkg/bufcheck/bufcheck.go @@ -21,6 +21,8 @@ import ( "buf.build/go/bufplugin/check" "github.com/bufbuild/buf/private/bufpkg/bufconfig" "github.com/bufbuild/buf/private/bufpkg/bufimage" + "github.com/bufbuild/buf/private/pkg/command" + "github.com/bufbuild/buf/private/pkg/pluginrpcutil" "github.com/bufbuild/buf/private/pkg/slicesext" "github.com/bufbuild/buf/private/pkg/syserror" "github.com/bufbuild/buf/private/pkg/tracing" @@ -145,12 +147,12 @@ type ClientFunctionOption interface { AllCategoriesOption } -// WithPluginConfigs returns a new ClientFunctionOption that says to also use the given plugins. +// WithPlugin returns a new ClientFunctionOption that says to also use the given plugins. // // The default is to only use the builtin Rules and Categories. -func WithPluginConfigs(pluginConfigs ...bufconfig.PluginConfig) ClientFunctionOption { - return &pluginConfigsOption{ - pluginConfigs: pluginConfigs, +func WithPlugins(plugins ...Plugin) ClientFunctionOption { + return &pluginsOption{ + plugins: plugins, } } @@ -162,27 +164,44 @@ func WithPluginsEnabled() ClientFunctionOption { return pluginsEnabledOption{} } -// RunnerProvider provides pluginrpc.Runners for program names and args. -type RunnerProvider interface { - NewRunner(programName string, programArgs ...string) pluginrpc.Runner +// Plugin is a configured lint or breaking plugin that wraps a pluginrpc.Runner. +type Plugin interface { + Config() bufconfig.PluginConfig + pluginrpc.Runner } -// RunnerProviderFunc is a function that implements RunnerProvider. -type RunnerProviderFunc func(programName string, programArgs ...string) pluginrpc.Runner +// NewPlugin returns a new Plugin. +func NewPlugin( + pluginConfig bufconfig.PluginConfig, + runner pluginrpc.Runner, +) Plugin { + return newPlugin(pluginConfig, runner) +} -// NewRunner implements RunnerProvider. -func (r RunnerProviderFunc) NewRunner(programName string, programArgs ...string) pluginrpc.Runner { - return r(programName, programArgs...) +// NewPluginsForRunner returns new Plugins for the command.Runner and configs. +func NewPluginsForRunner(delegate command.Runner, pluginConfigs ...bufconfig.PluginConfig) ([]Plugin, error) { + return slicesext.MapError( + pluginConfigs, + func(pluginConfig bufconfig.PluginConfig) (Plugin, error) { + if pluginConfig.Type() != bufconfig.PluginConfigTypeLocal { + return nil, syserror.New("we only handle local plugins for now with lint and breaking") + } + path := pluginConfig.Path() + return NewPlugin( + pluginConfig, + pluginrpcutil.NewRunner(delegate, path[0], path[1:]...), + ), nil + }, + ) } // NewClient returns a new Client. func NewClient( logger *zap.Logger, tracer tracing.Tracer, - runnerProvider RunnerProvider, options ...ClientOption, ) (Client, error) { - return newClient(logger, tracer, runnerProvider, options...) + return newClient(logger, tracer, options...) } // ClientOption is an option for a new Client. diff --git a/private/bufpkg/bufcheck/client.go b/private/bufpkg/bufcheck/client.go index 361ac9aa43..fe64cb8fd4 100644 --- a/private/bufpkg/bufcheck/client.go +++ b/private/bufpkg/bufcheck/client.go @@ -39,7 +39,6 @@ import ( type client struct { logger *zap.Logger tracer tracing.Tracer - runnerProvider RunnerProvider stderr io.Writer fileVersionToDefaultCheckClient map[bufconfig.FileVersion]check.Client } @@ -47,7 +46,6 @@ type client struct { func newClient( logger *zap.Logger, tracer tracing.Tracer, - runnerProvider RunnerProvider, options ...ClientOption, ) (*client, error) { clientOptions := newClientOptions() @@ -69,10 +67,9 @@ func newClient( } return &client{ - logger: logger, - tracer: tracer, - runnerProvider: runnerProvider, - stderr: clientOptions.stderr, + logger: logger, + tracer: tracer, + stderr: clientOptions.stderr, fileVersionToDefaultCheckClient: map[bufconfig.FileVersion]check.Client{ bufconfig.FileVersionV1Beta1: v1beta1DefaultCheckClient, bufconfig.FileVersionV1: v1DefaultCheckClient, @@ -97,13 +94,13 @@ func (c *client) Lint( for _, option := range options { option.applyToLint(lintOptions) } - if err := validatePluginConfigs(lintOptions.pluginConfigs, lintOptions.pluginEnabled); err != nil { + if err := validatePlugins(lintOptions.plugins, lintOptions.pluginEnabled); err != nil { return err } allRules, allCategories, err := c.allRulesAndCategories( ctx, lintConfig.FileVersion(), - lintOptions.pluginConfigs, + lintOptions.plugins, lintConfig.DisableBuiltin(), ) if err != nil { @@ -129,7 +126,7 @@ func (c *client) Lint( } multiClient, err := c.getMultiClient( lintConfig.FileVersion(), - lintOptions.pluginConfigs, + lintOptions.plugins, lintConfig.DisableBuiltin(), config.DefaultOptions, ) @@ -160,13 +157,13 @@ func (c *client) Breaking( for _, option := range options { option.applyToBreaking(breakingOptions) } - if err := validatePluginConfigs(breakingOptions.pluginConfigs, breakingOptions.pluginEnabled); err != nil { + if err := validatePlugins(breakingOptions.plugins, breakingOptions.pluginEnabled); err != nil { return err } allRules, allCategories, err := c.allRulesAndCategories( ctx, breakingConfig.FileVersion(), - breakingOptions.pluginConfigs, + breakingOptions.plugins, breakingConfig.DisableBuiltin(), ) if err != nil { @@ -203,7 +200,7 @@ func (c *client) Breaking( } multiClient, err := c.getMultiClient( breakingConfig.FileVersion(), - breakingOptions.pluginConfigs, + breakingOptions.plugins, breakingConfig.DisableBuiltin(), config.DefaultOptions, ) @@ -230,13 +227,13 @@ func (c *client) ConfiguredRules( for _, option := range options { option.applyToConfiguredRules(configuredRulesOptions) } - if err := validatePluginConfigs(configuredRulesOptions.pluginConfigs, configuredRulesOptions.pluginEnabled); err != nil { + if err := validatePlugins(configuredRulesOptions.plugins, configuredRulesOptions.pluginEnabled); err != nil { return nil, err } allRules, allCategories, err := c.allRulesAndCategories( ctx, checkConfig.FileVersion(), - configuredRulesOptions.pluginConfigs, + configuredRulesOptions.plugins, checkConfig.DisableBuiltin(), ) if err != nil { @@ -263,10 +260,10 @@ func (c *client) AllRules( for _, option := range options { option.applyToAllRules(allRulesOptions) } - if err := validatePluginConfigs(allRulesOptions.pluginConfigs, allRulesOptions.pluginEnabled); err != nil { + if err := validatePlugins(allRulesOptions.plugins, allRulesOptions.pluginEnabled); err != nil { return nil, err } - rules, _, err := c.allRulesAndCategories(ctx, fileVersion, allRulesOptions.pluginConfigs, false) + rules, _, err := c.allRulesAndCategories(ctx, fileVersion, allRulesOptions.plugins, false) if err != nil { return nil, err } @@ -285,17 +282,17 @@ func (c *client) AllCategories( for _, option := range options { option.applyToAllCategories(allCategoriesOptions) } - if err := validatePluginConfigs(allCategoriesOptions.pluginConfigs, allCategoriesOptions.pluginEnabled); err != nil { + if err := validatePlugins(allCategoriesOptions.plugins, allCategoriesOptions.pluginEnabled); err != nil { return nil, err } - _, categories, err := c.allRulesAndCategories(ctx, fileVersion, allCategoriesOptions.pluginConfigs, false) + _, categories, err := c.allRulesAndCategories(ctx, fileVersion, allCategoriesOptions.plugins, false) return categories, err } func (c *client) allRulesAndCategories( ctx context.Context, fileVersion bufconfig.FileVersion, - pluginConfigs []bufconfig.PluginConfig, + plugins []Plugin, disableBuiltin bool, ) ([]Rule, []Category, error) { // Just passing through to fulfill all contracts, ie checkClientSpec has non-nil Options. @@ -305,7 +302,7 @@ func (c *client) allRulesAndCategories( if err != nil { return nil, nil, err } - multiClient, err := c.getMultiClient(fileVersion, pluginConfigs, disableBuiltin, emptyOptions) + multiClient, err := c.getMultiClient(fileVersion, plugins, disableBuiltin, emptyOptions) if err != nil { return nil, nil, err } @@ -314,7 +311,7 @@ func (c *client) allRulesAndCategories( func (c *client) getMultiClient( fileVersion bufconfig.FileVersion, - pluginConfigs []bufconfig.PluginConfig, + plugins []Plugin, disableBuiltin bool, defaultOptions check.Options, ) (*multiClient, error) { @@ -330,22 +327,15 @@ func (c *client) getMultiClient( newCheckClientSpec("", defaultCheckClient, defaultOptions), ) } - for _, pluginConfig := range pluginConfigs { - if pluginConfig.Type() != bufconfig.PluginConfigTypeLocal { - return nil, syserror.New("we only handle local plugins for now with lint and breaking") - } + for _, plugin := range plugins { + pluginConfig := plugin.Config() options, err := check.NewOptions(pluginConfig.Options()) if err != nil { return nil, fmt.Errorf("could not parse options for plugin %q: %w", pluginConfig.Name(), err) } - pluginPath := pluginConfig.Path() checkClient := check.NewClient( pluginrpc.NewClient( - c.runnerProvider.NewRunner( - // We know that Path is of at least length 1. - pluginPath[0], - pluginPath[1:]..., - ), + plugin, pluginrpc.ClientWithStderr(c.stderr), // We have to set binary as some things cannot be encoded as JSON. // Example: google.protobuf.Timestamps with positive seconds and negative nanos. @@ -365,8 +355,8 @@ func (c *client) getMultiClient( } // TODO: remove this as part of publicly releasing lint/breaking plugins -func validatePluginConfigs(pluginConfigs []bufconfig.PluginConfig, isPluginEnabled bool) error { - if len(pluginConfigs) > 0 && !isPluginEnabled { +func validatePlugins(plugins []Plugin, isPluginEnabled bool) error { + if len(plugins) > 0 && !isPluginEnabled { return fmt.Errorf("custom plugins are not yet supported. For more information, please contact us at https://buf.build/docs/contact") } return nil @@ -513,7 +503,7 @@ func checkCommentLineForCheckIgnore( } type lintOptions struct { - pluginConfigs []bufconfig.PluginConfig + plugins []Plugin // TODO: remove this as part of publicly releasing lint/breaking plugins pluginEnabled bool } @@ -523,7 +513,7 @@ func newLintOptions() *lintOptions { } type breakingOptions struct { - pluginConfigs []bufconfig.PluginConfig + plugins []Plugin // TODO: remove this as part of publicly releasing lint/breaking plugins pluginEnabled bool excludeImports bool @@ -534,7 +524,7 @@ func newBreakingOptions() *breakingOptions { } type configuredRulesOptions struct { - pluginConfigs []bufconfig.PluginConfig + plugins []Plugin // TODO: remove this as part of publicly releasing lint/breaking plugins pluginEnabled bool } @@ -544,7 +534,7 @@ func newConfiguredRulesOptions() *configuredRulesOptions { } type allRulesOptions struct { - pluginConfigs []bufconfig.PluginConfig + plugins []Plugin // TODO: remove this as part of publicly releasing lint/breaking plugins pluginEnabled bool } @@ -554,7 +544,7 @@ func newAllRulesOptions() *allRulesOptions { } type allCategoriesOptions struct { - pluginConfigs []bufconfig.PluginConfig + plugins []Plugin // TODO: remove this as part of publicly releasing lint/breaking plugins pluginEnabled bool } @@ -577,28 +567,28 @@ func (e *excludeImportsOption) applyToBreaking(breakingOptions *breakingOptions) breakingOptions.excludeImports = true } -type pluginConfigsOption struct { - pluginConfigs []bufconfig.PluginConfig +type pluginsOption struct { + plugins []Plugin } -func (p *pluginConfigsOption) applyToLint(lintOptions *lintOptions) { - lintOptions.pluginConfigs = append(lintOptions.pluginConfigs, p.pluginConfigs...) +func (p *pluginsOption) applyToLint(lintOptions *lintOptions) { + lintOptions.plugins = append(lintOptions.plugins, p.plugins...) } -func (p *pluginConfigsOption) applyToBreaking(breakingOptions *breakingOptions) { - breakingOptions.pluginConfigs = append(breakingOptions.pluginConfigs, p.pluginConfigs...) +func (p *pluginsOption) applyToBreaking(breakingOptions *breakingOptions) { + breakingOptions.plugins = append(breakingOptions.plugins, p.plugins...) } -func (p *pluginConfigsOption) applyToConfiguredRules(configuredRulesOptions *configuredRulesOptions) { - configuredRulesOptions.pluginConfigs = append(configuredRulesOptions.pluginConfigs, p.pluginConfigs...) +func (p *pluginsOption) applyToConfiguredRules(configuredRulesOptions *configuredRulesOptions) { + configuredRulesOptions.plugins = append(configuredRulesOptions.plugins, p.plugins...) } -func (p *pluginConfigsOption) applyToAllRules(allRulesOptions *allRulesOptions) { - allRulesOptions.pluginConfigs = append(allRulesOptions.pluginConfigs, p.pluginConfigs...) +func (p *pluginsOption) applyToAllRules(allRulesOptions *allRulesOptions) { + allRulesOptions.plugins = append(allRulesOptions.plugins, p.plugins...) } -func (p *pluginConfigsOption) applyToAllCategories(allCategoriesOptions *allCategoriesOptions) { - allCategoriesOptions.pluginConfigs = append(allCategoriesOptions.pluginConfigs, p.pluginConfigs...) +func (p *pluginsOption) applyToAllCategories(allCategoriesOptions *allCategoriesOptions) { + allCategoriesOptions.plugins = append(allCategoriesOptions.plugins, p.plugins...) } type pluginsEnabledOption struct{} diff --git a/private/bufpkg/bufcheck/lint_test.go b/private/bufpkg/bufcheck/lint_test.go index e00d4f0609..43427e9713 100644 --- a/private/bufpkg/bufcheck/lint_test.go +++ b/private/bufpkg/bufcheck/lint_test.go @@ -28,7 +28,6 @@ import ( "github.com/bufbuild/buf/private/bufpkg/bufimage" "github.com/bufbuild/buf/private/bufpkg/bufmodule" "github.com/bufbuild/buf/private/pkg/command" - "github.com/bufbuild/buf/private/pkg/pluginrpcutil" "github.com/bufbuild/buf/private/pkg/storage/storageos" "github.com/bufbuild/buf/private/pkg/tracing" "github.com/stretchr/testify/assert" @@ -1294,13 +1293,15 @@ func testLintWithOptions( lintConfig := workspace.GetLintConfigForOpaqueID(opaqueID) require.NotNil(t, lintConfig) - client, err := bufcheck.NewClient(zap.NewNop(), tracing.NopTracer, pluginrpcutil.NewRunnerProvider(command.NewRunner())) + plugins, err := bufcheck.NewPluginsForRunner(command.NewRunner(), workspace.PluginConfigs()...) + require.NoError(t, err) + client, err := bufcheck.NewClient(zap.NewNop(), tracing.NopTracer) require.NoError(t, err) err = client.Lint( ctx, lintConfig, image, - bufcheck.WithPluginConfigs(workspace.PluginConfigs()...), + bufcheck.WithPlugins(plugins...), bufcheck.WithPluginsEnabled(), ) if len(expectedFileAnnotations) == 0 { diff --git a/private/bufpkg/bufcheck/multi_client_test.go b/private/bufpkg/bufcheck/multi_client_test.go index c81c8da7e9..87b6ff880a 100644 --- a/private/bufpkg/bufcheck/multi_client_test.go +++ b/private/bufpkg/bufcheck/multi_client_test.go @@ -24,7 +24,6 @@ import ( "buf.build/go/bufplugin/check/checkutil" "github.com/bufbuild/buf/private/bufpkg/bufconfig" "github.com/bufbuild/buf/private/pkg/command" - "github.com/bufbuild/buf/private/pkg/pluginrpcutil" "github.com/bufbuild/buf/private/pkg/slicesext" "github.com/bufbuild/buf/private/pkg/stringutil" "github.com/bufbuild/buf/private/pkg/tracing" @@ -185,7 +184,6 @@ func TestMultiClientCannotHaveOverlappingRulesWithBuiltIn(t *testing.T) { client, err := newClient( zaptest.NewLogger(t), tracing.NopTracer, - pluginrpcutil.NewRunnerProvider(command.NewRunner()), ) require.NoError(t, err) duplicateBuiltInRulePluginConfig, err := bufconfig.NewLocalPluginConfig( @@ -197,11 +195,12 @@ func TestMultiClientCannotHaveOverlappingRulesWithBuiltIn(t *testing.T) { emptyOptions, err := check.NewOptions(nil) require.NoError(t, err) + plugins, err := NewPluginsForRunner(command.NewRunner(), duplicateBuiltInRulePluginConfig) + require.NoError(t, err) + multiClient, err := client.getMultiClient( bufconfig.FileVersionV2, - []bufconfig.PluginConfig{ - duplicateBuiltInRulePluginConfig, - }, + plugins, false, emptyOptions, ) @@ -279,7 +278,6 @@ func TestMultiClientCannotHaveOverlappingCategoriesWithBuiltIn(t *testing.T) { client, err := newClient( zaptest.NewLogger(t), tracing.NopTracer, - pluginrpcutil.NewRunnerProvider(command.NewRunner()), ) require.NoError(t, err) duplicateBuiltInRulePluginConfig, err := bufconfig.NewLocalPluginConfig( @@ -291,11 +289,12 @@ func TestMultiClientCannotHaveOverlappingCategoriesWithBuiltIn(t *testing.T) { emptyOptions, err := check.NewOptions(nil) require.NoError(t, err) + plugins, err := NewPluginsForRunner(command.NewRunner(), duplicateBuiltInRulePluginConfig) + require.NoError(t, err) + multiClient, err := client.getMultiClient( bufconfig.FileVersionV2, - []bufconfig.PluginConfig{ - duplicateBuiltInRulePluginConfig, - }, + plugins, false, emptyOptions, ) diff --git a/private/bufpkg/bufcheck/plugin.go b/private/bufpkg/bufcheck/plugin.go new file mode 100644 index 0000000000..d5d69143b2 --- /dev/null +++ b/private/bufpkg/bufcheck/plugin.go @@ -0,0 +1,40 @@ +// Copyright 2020-2024 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 bufcheck + +import ( + "github.com/bufbuild/buf/private/bufpkg/bufconfig" + "pluginrpc.com/pluginrpc" +) + +type plugin struct { + pluginrpc.Runner + + pluginConfig bufconfig.PluginConfig +} + +func newPlugin( + pluginConfig bufconfig.PluginConfig, + runner pluginrpc.Runner, +) *plugin { + return &plugin{ + Runner: runner, + pluginConfig: pluginConfig, + } +} + +func (p *plugin) Config() bufconfig.PluginConfig { + return p.pluginConfig +} diff --git a/private/pkg/pluginrpcutil/pluginrpcutil.go b/private/pkg/pluginrpcutil/pluginrpcutil.go index e533e8509a..f984f8b8a9 100644 --- a/private/pkg/pluginrpcutil/pluginrpcutil.go +++ b/private/pkg/pluginrpcutil/pluginrpcutil.go @@ -23,29 +23,3 @@ import ( func NewRunner(delegate command.Runner, programName string, programArgs ...string) pluginrpc.Runner { return newRunner(delegate, programName, programArgs...) } - -// RunnerProvider provides pluginrpc.Runners for program names and args. -type RunnerProvider interface { - NewRunner(programName string, programArgs ...string) pluginrpc.Runner -} - -// RunnerProviderFunc is a function that implements RunnerProvider. -type RunnerProviderFunc func(programName string, programArgs ...string) pluginrpc.Runner - -// NewRunner implements RunnerProvider. -func (r RunnerProviderFunc) NewRunner(programName string, programArgs ...string) pluginrpc.Runner { - return r(programName, programArgs...) -} - -// NewRunnerProvider returns a new RunnerProvider for the command.Runner. -func NewRunnerProvider(delegate command.Runner) RunnerProvider { - return RunnerProviderFunc( - func(programName string, programArgs ...string) pluginrpc.Runner { - return NewRunner( - delegate, - programName, - programArgs..., - ) - }, - ) -}