Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove command package and command.Runner in favor of simpler and stateless execext package #3439

Merged
merged 9 commits into from
Nov 1, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions .golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ linters-settings:
forbid:
# Use private/pkg/thread.Parallelize
- '^errgroup\.'
# Use private/pkg/command.Runner
# Use private/pkg/execext
- '^exec\.Cmd$'
- '^exec\.Command$'
- '^exec\.CommandContext$'
Expand Down Expand Up @@ -213,6 +213,10 @@ issues:
- containedctx
# we actually want to embed a context here
path: private/bufpkg/bufmodule/module_set_builder.go
- linters:
- containedctx
# we actually want to embed a context here
path: private/pkg/execext/process.go
- linters:
- gochecknoinits
# we actually want to use init here
Expand All @@ -225,15 +229,15 @@ issues:
- linters:
- forbidigo
# this is one of two files we want to allow exec.Cmd functions in
path: private/pkg/command/process.go
path: private/pkg/execext/execext.go
- linters:
- forbidigo
# this is one of two files we want to allow exec.Cmd functions in
path: private/pkg/command/runner.go
path: private/pkg/execext/process.go
- linters:
- gosec
# G204 checks that exec.Command is not called with non-constants.
path: private/pkg/command/runner.go
path: private/pkg/execext/execext.go
text: "G204:"
- linters:
- gosec
Expand Down
2 changes: 0 additions & 2 deletions private/buf/bufcli/cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ import (
"github.com/bufbuild/buf/private/bufpkg/bufregistryapi/bufregistryapimodule"
"github.com/bufbuild/buf/private/bufpkg/bufregistryapi/bufregistryapiowner"
"github.com/bufbuild/buf/private/pkg/app/appext"
"github.com/bufbuild/buf/private/pkg/command"
"github.com/bufbuild/buf/private/pkg/filelock"
"github.com/bufbuild/buf/private/pkg/normalpath"
"github.com/bufbuild/buf/private/pkg/storage/storageos"
Expand Down Expand Up @@ -174,7 +173,6 @@ func NewWKTStore(container appext.Container) (bufwktstore.Store, error) {
}
return bufwktstore.NewStore(
container.Logger(),
command.NewRunner(),
cacheBucket,
), nil
}
Expand Down
4 changes: 0 additions & 4 deletions private/buf/bufctl/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ import (
imagev1 "github.com/bufbuild/buf/private/gen/proto/go/buf/alpha/image/v1"
"github.com/bufbuild/buf/private/pkg/app"
"github.com/bufbuild/buf/private/pkg/app/appcmd"
"github.com/bufbuild/buf/private/pkg/command"
"github.com/bufbuild/buf/private/pkg/git"
"github.com/bufbuild/buf/private/pkg/httpauth"
"github.com/bufbuild/buf/private/pkg/ioext"
Expand Down Expand Up @@ -178,7 +177,6 @@ type controller struct {
fileAnnotationsToStdout bool
copyToInMemory bool

commandRunner command.Runner
storageosProvider storageos.Provider
buffetchRefParser buffetch.RefParser
buffetchReader buffetch.Reader
Expand Down Expand Up @@ -214,7 +212,6 @@ func newController(
if err := validateFileAnnotationErrorFormat(controller.fileAnnotationErrorFormat); err != nil {
return nil, err
}
controller.commandRunner = command.NewRunner()
controller.storageosProvider = newStorageosProvider(controller.disableSymlinks)
controller.buffetchRefParser = buffetch.NewRefParser(logger)
controller.buffetchReader = buffetch.NewReader(
Expand All @@ -225,7 +222,6 @@ func newController(
git.NewCloner(
logger,
controller.storageosProvider,
controller.commandRunner,
gitClonerOptions,
),
moduleKeyProvider,
Expand Down
4 changes: 1 addition & 3 deletions private/buf/bufformat/formatter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import (
"testing"

"github.com/bufbuild/buf/private/bufpkg/bufmodule"
"github.com/bufbuild/buf/private/pkg/command"
"github.com/bufbuild/buf/private/pkg/diff"
"github.com/bufbuild/buf/private/pkg/slogtestext"
"github.com/bufbuild/buf/private/pkg/storage"
Expand Down Expand Up @@ -72,7 +71,6 @@ func testFormatProto3(t *testing.T) {
func testFormatNoDiff(t *testing.T, path string) {
t.Run(path, func(t *testing.T) {
ctx := context.Background()
runner := command.NewRunner()
bucket, err := storageos.NewProvider().NewReadWriteBucket(path)
require.NoError(t, err)
moduleSetBuilder := bufmodule.NewModuleSetBuilder(ctx, slogtestext.NewLogger(t), bufmodule.NopModuleDataProvider, bufmodule.NopCommitProvider)
Expand Down Expand Up @@ -102,7 +100,7 @@ func testFormatNoDiff(t *testing.T, path string) {
require.NoError(t, err)
expectedData, err := io.ReadAll(expectedFile)
require.NoError(t, err)
fileDiff, err := diff.Diff(ctx, runner, expectedData, formattedData, expectedPath, formattedFile.Path()+" (formatted)")
fileDiff, err := diff.Diff(ctx, expectedData, formattedData, expectedPath, formattedFile.Path()+" (formatted)")
require.NoError(t, err)
require.Empty(t, string(fileDiff))
})
Expand Down
3 changes: 0 additions & 3 deletions private/buf/bufgen/bufgen.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import (
"github.com/bufbuild/buf/private/bufpkg/bufconfig"
"github.com/bufbuild/buf/private/bufpkg/bufimage"
"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/storage/storageos"
)
Expand Down Expand Up @@ -88,7 +87,6 @@ type Generator interface {
func NewGenerator(
logger *slog.Logger,
storageosProvider storageos.Provider,
runner command.Runner,
// Pass a clientConfig instead of a CodeGenerationServiceClient because the
// plugins' remotes/registries is not known at this time, and remotes/registries
// may be different for different plugins.
Expand All @@ -97,7 +95,6 @@ func NewGenerator(
return newGenerator(
logger,
storageosProvider,
runner,
clientConfig,
)
}
Expand Down
4 changes: 1 addition & 3 deletions private/buf/bufgen/generator.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ import (
"github.com/bufbuild/buf/private/gen/proto/connect/buf/alpha/registry/v1alpha1/registryv1alpha1connect"
registryv1alpha1 "github.com/bufbuild/buf/private/gen/proto/go/buf/alpha/registry/v1alpha1"
"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"
Expand All @@ -52,13 +51,12 @@ type generator struct {
func newGenerator(
logger *slog.Logger,
storageosProvider storageos.Provider,
runner command.Runner,
clientConfig *connectclient.Config,
) *generator {
return &generator{
logger: logger,
storageosProvider: storageosProvider,
pluginexecGenerator: bufprotopluginexec.NewGenerator(logger, storageosProvider, runner),
pluginexecGenerator: bufprotopluginexec.NewGenerator(logger, storageosProvider),
clientConfig: clientConfig,
}
}
Expand Down
3 changes: 0 additions & 3 deletions private/buf/bufmigrate/bufmigrate.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import (

"github.com/bufbuild/buf/private/bufpkg/bufconfig"
"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/storage"
"github.com/bufbuild/buf/private/pkg/syserror"
Expand Down Expand Up @@ -77,13 +76,11 @@ type Migrator interface {
// NewMigrator returns a new Migrator.
func NewMigrator(
logger *slog.Logger,
runner command.Runner,
moduleKeyProvider bufmodule.ModuleKeyProvider,
commitProvider bufmodule.CommitProvider,
) Migrator {
return newMigrator(
logger,
runner,
moduleKeyProvider,
commitProvider,
)
Expand Down
12 changes: 4 additions & 8 deletions private/buf/bufmigrate/migrate_builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import (

"github.com/bufbuild/buf/private/bufpkg/bufconfig"
"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/slicesext"
"github.com/bufbuild/buf/private/pkg/storage"
Expand All @@ -35,7 +34,6 @@ import (

type migrateBuilder struct {
logger *slog.Logger
runner command.Runner
commitProvider bufmodule.CommitProvider
bucket storage.ReadBucket
destinationDirPath string
Expand All @@ -55,14 +53,12 @@ type migrateBuilder struct {

func newMigrateBuilder(
logger *slog.Logger,
runner command.Runner,
commitProvider bufmodule.CommitProvider,
bucket storage.ReadBucket,
destinationDirPath string,
) *migrateBuilder {
return &migrateBuilder{
logger: logger,
runner: runner,
commitProvider: commitProvider,
bucket: bucket,
destinationDirPath: destinationDirPath,
Expand Down Expand Up @@ -268,11 +264,11 @@ func (m *migrateBuilder) addModule(ctx context.Context, moduleDirPath string) (r
if err != nil {
return err
}
lintConfigForRoot, err := equivalentLintConfigInV2(ctx, m.logger, m.runner, moduleConfig.LintConfig())
lintConfigForRoot, err := equivalentLintConfigInV2(ctx, m.logger, moduleConfig.LintConfig())
if err != nil {
return err
}
breakingConfigForRoot, err := equivalentBreakingConfigInV2(ctx, m.logger, m.runner, moduleConfig.BreakingConfig())
breakingConfigForRoot, err := equivalentBreakingConfigInV2(ctx, m.logger, moduleConfig.BreakingConfig())
if err != nil {
return err
}
Expand Down Expand Up @@ -304,11 +300,11 @@ func (m *migrateBuilder) addModule(ctx context.Context, moduleDirPath string) (r
if err != nil {
return err
}
lintConfig, err := equivalentLintConfigInV2(ctx, m.logger, m.runner, moduleConfig.LintConfig())
lintConfig, err := equivalentLintConfigInV2(ctx, m.logger, moduleConfig.LintConfig())
if err != nil {
return err
}
breakingConfig, err := equivalentBreakingConfigInV2(ctx, m.logger, m.runner, moduleConfig.BreakingConfig())
breakingConfig, err := equivalentBreakingConfigInV2(ctx, m.logger, moduleConfig.BreakingConfig())
if err != nil {
return err
}
Expand Down
13 changes: 1 addition & 12 deletions private/buf/bufmigrate/migrator.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ import (
"github.com/bufbuild/buf/private/bufpkg/bufcheck"
"github.com/bufbuild/buf/private/bufpkg/bufconfig"
"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/slicesext"
"github.com/bufbuild/buf/private/pkg/storage"
Expand All @@ -40,20 +39,17 @@ import (

type migrator struct {
logger *slog.Logger
runner command.Runner
moduleKeyProvider bufmodule.ModuleKeyProvider
commitProvider bufmodule.CommitProvider
}

func newMigrator(
logger *slog.Logger,
runner command.Runner,
moduleKeyProvider bufmodule.ModuleKeyProvider,
commitProvider bufmodule.CommitProvider,
) *migrator {
return &migrator{
logger: logger,
runner: runner,
moduleKeyProvider: moduleKeyProvider,
commitProvider: commitProvider,
}
Expand Down Expand Up @@ -140,7 +136,6 @@ func (m *migrator) getMigrateBuilder(
}
migrateBuilder := newMigrateBuilder(
m.logger,
m.runner,
m.commitProvider,
bucket,
destinationDirPath,
Expand Down Expand Up @@ -209,7 +204,6 @@ func (m *migrator) diff(
}
return storage.Diff(
ctx,
m.runner,
writer,
originalFileBucket,
addedFileBucket,
Expand Down Expand Up @@ -647,13 +641,11 @@ func resolvedDeclaredAndLockedDependencies(
func equivalentLintConfigInV2(
ctx context.Context,
logger *slog.Logger,
runner command.Runner,
lintConfig bufconfig.LintConfig,
) (bufconfig.LintConfig, error) {
equivalentCheckConfigV2, err := equivalentCheckConfigInV2(
ctx,
logger,
runner,
check.RuleTypeLint,
lintConfig,
)
Expand All @@ -674,13 +666,11 @@ func equivalentLintConfigInV2(
func equivalentBreakingConfigInV2(
ctx context.Context,
logger *slog.Logger,
runner command.Runner,
breakingConfig bufconfig.BreakingConfig,
) (bufconfig.BreakingConfig, error) {
equivalentCheckConfigV2, err := equivalentCheckConfigInV2(
ctx,
logger,
runner,
check.RuleTypeBreaking,
breakingConfig,
)
Expand All @@ -698,13 +688,12 @@ func equivalentBreakingConfigInV2(
func equivalentCheckConfigInV2(
ctx context.Context,
logger *slog.Logger,
runner command.Runner,
ruleType check.RuleType,
checkConfig bufconfig.CheckConfig,
) (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, bufcheck.NewRunnerProvider(runner, wasm.UnimplementedRuntime))
client, err := bufcheck.NewClient(logger, bufcheck.NewRunnerProvider(wasm.UnimplementedRuntime))
if err != nil {
return nil, err
}
Expand Down
19 changes: 8 additions & 11 deletions private/buf/bufprotopluginexec/binary_handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import (
"log/slog"
"path/filepath"

"github.com/bufbuild/buf/private/pkg/command"
"github.com/bufbuild/buf/private/pkg/execext"
"github.com/bufbuild/buf/private/pkg/ioext"
"github.com/bufbuild/buf/private/pkg/protoencoding"
"github.com/bufbuild/buf/private/pkg/slogext"
Expand All @@ -31,20 +31,17 @@ import (

type binaryHandler struct {
logger *slog.Logger
runner command.Runner
pluginPath string
pluginArgs []string
}

func newBinaryHandler(
logger *slog.Logger,
runner command.Runner,
pluginPath string,
pluginArgs []string,
) *binaryHandler {
return &binaryHandler{
logger: logger,
runner: runner,
pluginPath: pluginPath,
pluginArgs: pluginArgs,
}
Expand All @@ -64,16 +61,16 @@ func (h *binaryHandler) Handle(
}
responseBuffer := bytes.NewBuffer(nil)
stderrWriteCloser := newStderrWriteCloser(pluginEnv.Stderr, h.pluginPath)
runOptions := []command.RunOption{
command.RunWithEnviron(pluginEnv.Environ),
command.RunWithStdin(bytes.NewReader(requestData)),
command.RunWithStdout(responseBuffer),
command.RunWithStderr(stderrWriteCloser),
runOptions := []execext.RunOption{
execext.WithEnv(pluginEnv.Environ),
execext.WithStdin(bytes.NewReader(requestData)),
execext.WithStdout(responseBuffer),
execext.WithStderr(stderrWriteCloser),
}
if len(h.pluginArgs) > 0 {
runOptions = append(runOptions, command.RunWithArgs(h.pluginArgs...))
runOptions = append(runOptions, execext.WithArgs(h.pluginArgs...))
}
if err := h.runner.Run(
if err := execext.Run(
ctx,
h.pluginPath,
runOptions...,
Expand Down
Loading