diff --git a/private/buf/bufwire/bufwire.go b/private/buf/bufwire/bufwire.go index a59751d549..003d3a2c76 100644 --- a/private/buf/bufwire/bufwire.go +++ b/private/buf/bufwire/bufwire.go @@ -75,22 +75,28 @@ func NewImageConfigReader( ) } -// ModuleConfig is an module and configuration. +// ModuleConfig is a Module and configuration. type ModuleConfig interface { Module() bufmodule.Module Config() *bufconfig.Config +} + +// ModuleConfigSet is a set of ModuleConfigs with a potentially associated Workspace. +type ModuleConfigSet interface { + ModuleConfigs() []ModuleConfig + // Optional. May be nil. Workspace() bufmodule.Workspace } // ModuleConfigReader is a ModuleConfig reader. type ModuleConfigReader interface { - // GetModuleConfigs gets the ModuleConfig for the fetch value. + // GetModuleConfigSet gets the ModuleConfigSet for the fetch value. // // If externalDirOrFilePaths is empty, this builds all files under Buf control. // - // Note that as opposed to ModuleReader, this will return a Module for either + // Note that as opposed to ModuleReader, this will return Modules for either // a source or module reference, not just a module reference. - GetModuleConfigs( + GetModuleConfigSet( ctx context.Context, container app.EnvStdinContainer, sourceOrModuleRef buffetch.SourceOrModuleRef, @@ -98,7 +104,7 @@ type ModuleConfigReader interface { externalDirOrFilePaths []string, externalExcludeDirOrFilePaths []string, externalDirOrFilePathsAllowNotExist bool, - ) ([]ModuleConfig, error) + ) (ModuleConfigSet, error) } // NewModuleConfigReader returns a new ModuleConfigReader diff --git a/private/buf/bufwire/image_config_reader.go b/private/buf/bufwire/image_config_reader.go index e84c985772..26e6248129 100644 --- a/private/buf/bufwire/image_config_reader.go +++ b/private/buf/bufwire/image_config_reader.go @@ -127,7 +127,7 @@ func (i *imageConfigReader) getSourceOrModuleImageConfigs( externalDirOrFilePathsAllowNotExist bool, excludeSourceCodeInfo bool, ) ([]ImageConfig, []bufanalysis.FileAnnotation, error) { - moduleConfigs, err := i.moduleConfigReader.GetModuleConfigs( + moduleConfigSet, err := i.moduleConfigReader.GetModuleConfigSet( ctx, container, sourceOrModuleRef, @@ -139,6 +139,7 @@ func (i *imageConfigReader) getSourceOrModuleImageConfigs( if err != nil { return nil, nil, err } + moduleConfigs := moduleConfigSet.ModuleConfigs() imageConfigs := make([]ImageConfig, 0, len(moduleConfigs)) var allFileAnnotations []bufanalysis.FileAnnotation for _, moduleConfig := range moduleConfigs { @@ -153,7 +154,7 @@ func (i *imageConfigReader) getSourceOrModuleImageConfigs( } buildOpts := []bufimagebuild.BuildOption{ bufimagebuild.WithExpectedDirectDependencies(moduleConfig.Module().DeclaredDirectDependencies()), - bufimagebuild.WithWorkspace(moduleConfig.Workspace()), + bufimagebuild.WithWorkspace(moduleConfigSet.Workspace()), } if excludeSourceCodeInfo { buildOpts = append(buildOpts, bufimagebuild.WithExcludeSourceCodeInfo()) diff --git a/private/buf/bufwire/module_config.go b/private/buf/bufwire/module_config.go index d9d59c63e0..37cc355769 100644 --- a/private/buf/bufwire/module_config.go +++ b/private/buf/bufwire/module_config.go @@ -20,16 +20,14 @@ import ( ) type moduleConfig struct { - module bufmodule.Module - config *bufconfig.Config - workspace bufmodule.Workspace + module bufmodule.Module + config *bufconfig.Config } -func newModuleConfig(module bufmodule.Module, config *bufconfig.Config, workspace bufmodule.Workspace) *moduleConfig { +func newModuleConfig(module bufmodule.Module, config *bufconfig.Config) *moduleConfig { return &moduleConfig{ - module: module, - config: config, - workspace: workspace, + module: module, + config: config, } } @@ -40,7 +38,3 @@ func (m *moduleConfig) Module() bufmodule.Module { func (m *moduleConfig) Config() *bufconfig.Config { return m.config } - -func (m *moduleConfig) Workspace() bufmodule.Workspace { - return m.workspace -} diff --git a/private/buf/bufwire/module_config_reader.go b/private/buf/bufwire/module_config_reader.go index 65f47ce5c7..89f9cc8dee 100644 --- a/private/buf/bufwire/module_config_reader.go +++ b/private/buf/bufwire/module_config_reader.go @@ -61,7 +61,7 @@ func newModuleConfigReader( } } -func (m *moduleConfigReader) GetModuleConfigs( +func (m *moduleConfigReader) GetModuleConfigSet( ctx context.Context, container app.EnvStdinContainer, sourceOrModuleRef buffetch.SourceOrModuleRef, @@ -69,7 +69,7 @@ func (m *moduleConfigReader) GetModuleConfigs( externalDirOrFilePaths []string, externalExcludeDirOrFilePaths []string, externalDirOrFilePathsAllowNotExist bool, -) (_ []ModuleConfig, retErr error) { +) (_ ModuleConfigSet, retErr error) { ctx, span := m.tracer.Start(ctx, "get_module_config") defer span.End() defer func() { @@ -82,7 +82,7 @@ func (m *moduleConfigReader) GetModuleConfigs( workspaceBuilder := bufwork.NewWorkspaceBuilder() switch t := sourceOrModuleRef.(type) { case buffetch.ProtoFileRef: - return m.getProtoFileModuleSourceConfigs( + return m.getProtoFileModuleSourceConfigSet( ctx, container, t, @@ -93,7 +93,7 @@ func (m *moduleConfigReader) GetModuleConfigs( externalDirOrFilePathsAllowNotExist, ) case buffetch.SourceRef: - return m.getSourceModuleConfigs( + return m.getSourceModuleConfigSet( ctx, container, t, @@ -116,15 +116,18 @@ func (m *moduleConfigReader) GetModuleConfigs( if err != nil { return nil, err } - return []ModuleConfig{ - moduleConfig, - }, nil + return newModuleConfigSet( + []ModuleConfig{ + moduleConfig, + }, + nil, + ), nil default: return nil, fmt.Errorf("invalid ref: %T", sourceOrModuleRef) } } -func (m *moduleConfigReader) getSourceModuleConfigs( +func (m *moduleConfigReader) getSourceModuleConfigSet( ctx context.Context, container app.EnvStdinContainer, sourceRef buffetch.SourceRef, @@ -133,7 +136,7 @@ func (m *moduleConfigReader) getSourceModuleConfigs( externalDirOrFilePaths []string, externalExcludeDirOrFilePaths []string, externalDirOrFilePathsAllowNotExist bool, -) (_ []ModuleConfig, retErr error) { +) (_ ModuleConfigSet, retErr error) { readBucketCloser, err := m.fetchReader.GetSourceBucket(ctx, container, sourceRef) if err != nil { return nil, err @@ -146,7 +149,7 @@ func (m *moduleConfigReader) getSourceModuleConfigs( return nil, err } if existingConfigFilePath != "" { - return m.getWorkspaceModuleConfigs( + return m.getWorkspaceModuleConfigSet( ctx, sourceRef, workspaceBuilder, @@ -176,9 +179,12 @@ func (m *moduleConfigReader) getSourceModuleConfigs( if err != nil { return nil, err } - return []ModuleConfig{ - moduleConfig, - }, nil + return newModuleConfigSet( + []ModuleConfig{ + moduleConfig, + }, + nil, + ), nil } func (m *moduleConfigReader) getModuleModuleConfig( @@ -240,10 +246,10 @@ func (m *moduleConfigReader) getModuleModuleConfig( if err != nil { return nil, err } - return newModuleConfig(module, config, nil /* Workspaces aren't supported for ModuleRefs */), nil + return newModuleConfig(module, config), nil } -func (m *moduleConfigReader) getProtoFileModuleSourceConfigs( +func (m *moduleConfigReader) getProtoFileModuleSourceConfigSet( ctx context.Context, container app.EnvStdinContainer, protoFileRef buffetch.ProtoFileRef, @@ -252,7 +258,7 @@ func (m *moduleConfigReader) getProtoFileModuleSourceConfigs( externalDirOrFilePaths []string, externalExcludeDirOrFilePaths []string, externalDirOrFilePathsAllowNotExist bool, -) (_ []ModuleConfig, retErr error) { +) (_ ModuleConfigSet, retErr error) { readBucketCloser, err := m.fetchReader.GetSourceBucket(ctx, container, protoFileRef) if err != nil { return nil, err @@ -296,7 +302,7 @@ func (m *moduleConfigReader) getProtoFileModuleSourceConfigs( } } } - return m.getWorkspaceModuleConfigs( + return m.getWorkspaceModuleConfigSet( ctx, protoFileRef, workspaceBuilder, @@ -326,12 +332,15 @@ func (m *moduleConfigReader) getProtoFileModuleSourceConfigs( if err != nil { return nil, err } - return []ModuleConfig{ - moduleConfig, - }, nil + return newModuleConfigSet( + []ModuleConfig{ + moduleConfig, + }, + nil, + ), nil } -func (m *moduleConfigReader) getWorkspaceModuleConfigs( +func (m *moduleConfigReader) getWorkspaceModuleConfigSet( ctx context.Context, sourceRef buffetch.SourceRef, workspaceBuilder bufwork.WorkspaceBuilder, @@ -342,28 +351,26 @@ func (m *moduleConfigReader) getWorkspaceModuleConfigs( externalDirOrFilePaths []string, externalExcludeDirOrFilePaths []string, externalDirOrFilePathsAllowNotExist bool, -) ([]ModuleConfig, error) { +) (ModuleConfigSet, error) { workspaceConfig, err := bufwork.GetConfigForBucket(ctx, readBucket, relativeRootPath) if err != nil { return nil, err } + workspace, err := workspaceBuilder.BuildWorkspace( + ctx, + workspaceConfig, + readBucket, + relativeRootPath, + subDirPath, // this is used to only apply the config override to this directory + configOverride, + externalDirOrFilePaths, + externalExcludeDirOrFilePaths, + externalDirOrFilePathsAllowNotExist, + ) + if err != nil { + return nil, err + } if subDirPath != "." { - // There's only a single ModuleConfig based on the subDirPath, - // so we only need to create a single workspace. - workspace, err := workspaceBuilder.BuildWorkspace( - ctx, - workspaceConfig, - readBucket, - relativeRootPath, - subDirPath, - configOverride, - externalDirOrFilePaths, - externalExcludeDirOrFilePaths, - externalDirOrFilePathsAllowNotExist, - ) - if err != nil { - return nil, err - } moduleConfig, err := m.getSourceModuleConfig( ctx, sourceRef, @@ -381,9 +388,12 @@ func (m *moduleConfigReader) getWorkspaceModuleConfigs( if err != nil { return nil, err } - return []ModuleConfig{ - moduleConfig, - }, nil + return newModuleConfigSet( + []ModuleConfig{ + moduleConfig, + }, + workspace, + ), nil } if configOverride != "" { return nil, errors.New("the --config flag is not compatible with workspaces") @@ -427,20 +437,6 @@ func (m *moduleConfigReader) getWorkspaceModuleConfigs( for excludeFileOrDirPath, subDirRelExcludePath := range externalExcludeToSubDirRelExcludePaths { externalExcludePathToRelPaths[excludeFileOrDirPath] = subDirRelExcludePath } - workspace, err := workspaceBuilder.BuildWorkspace( - ctx, - workspaceConfig, - readBucket, - relativeRootPath, - directory, - configOverride, - externalDirOrFilePaths, - externalExcludeDirOrFilePaths, - externalDirOrFilePathsAllowNotExist, - ) - if err != nil { - return nil, err - } moduleConfig, err := m.getSourceModuleConfig( ctx, sourceRef, @@ -473,7 +469,7 @@ func (m *moduleConfigReader) getWorkspaceModuleConfigs( } } } - return moduleConfigs, nil + return newModuleConfigSet(moduleConfigs, workspace), nil } func (m *moduleConfigReader) getSourceModuleConfig( @@ -505,7 +501,7 @@ func (m *moduleConfigReader) getSourceModuleConfig( } } } - return newModuleConfig(module, moduleConfig, workspace), nil + return newModuleConfig(module, moduleConfig), nil } mappedReadBucket := readBucket if subDirPath != "." { @@ -643,7 +639,7 @@ func (m *moduleConfigReader) getSourceModuleConfig( } m.logger.Warn(builder.String()) } - return newModuleConfig(module, moduleConfig, workspace), nil + return newModuleConfig(module, moduleConfig), nil } func workspaceDirectoryEqualsOrContainsSubDirPath(workspaceConfig *bufwork.Config, subDirPath string) bool { diff --git a/private/buf/bufwire/module_config_set.go b/private/buf/bufwire/module_config_set.go new file mode 100644 index 0000000000..bc011348ae --- /dev/null +++ b/private/buf/bufwire/module_config_set.go @@ -0,0 +1,39 @@ +// Copyright 2020-2023 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 bufwire + +import ( + "github.com/bufbuild/buf/private/bufpkg/bufmodule" +) + +type moduleConfigSet struct { + moduleConfigs []ModuleConfig + workspace bufmodule.Workspace +} + +func newModuleConfigSet(moduleConfigs []ModuleConfig, workspace bufmodule.Workspace) *moduleConfigSet { + return &moduleConfigSet{ + moduleConfigs: moduleConfigs, + workspace: workspace, + } +} + +func (m *moduleConfigSet) ModuleConfigs() []ModuleConfig { + return m.moduleConfigs +} + +func (m *moduleConfigSet) Workspace() bufmodule.Workspace { + return m.workspace +} diff --git a/private/buf/bufwork/bufwork.go b/private/buf/bufwork/bufwork.go index f7f58fff1e..2ee498aedb 100644 --- a/private/buf/bufwork/bufwork.go +++ b/private/buf/bufwork/bufwork.go @@ -101,6 +101,12 @@ type WorkspaceBuilder interface { // BuildWorkspace builds a bufmodule.Workspace. // // The given targetSubDirPath is the only path that will have the configOverride applied to it. + // TODO: delete targetSubDirPath entirely. We are building a Workspace, we don't necessarily + // have a specific target directory within it. This would mean doing the config override at + // a higher level for any specific modules within the Workspace. The only thing in the config + // we care about is the build.excludes, so in theory we should be able to figure out a way + // to say "exclude these files from these modules when you are building". Even better, the + // WorkspaceBuilder has nothing to do with building modules. BuildWorkspace( ctx context.Context, workspaceConfig *Config, diff --git a/private/buf/bufwork/workspace_builder.go b/private/buf/bufwork/workspace_builder.go index bab7a62cca..6539ab09bc 100644 --- a/private/buf/bufwork/workspace_builder.go +++ b/private/buf/bufwork/workspace_builder.go @@ -211,6 +211,18 @@ func validateWorkspaceDirectoryNonEmpty( // overlap in either direction. The last argument is only used for // error reporting. // +// This verifies that ie we do not mistakenly target a directory input that is +// within a Workspace, but is not listed as a directory in the Workspace, which +// could cause issues. If we say "we have a workspace with directory proto, but +// we are targeting proto/a", then we will still detect the buf.work.yaml and +// bring in all the directories within it, making them available for import, +// but potentially in two ways. If there was proto/a/a.proto, we could theoretically +// import it as both a/a.proto, and a.proto. +// +// TODO: See if the above explanation is nonsense. It should be nonsense if we +// did our job right here. And regardless, this shouldn't need to be validated +// at this level. +// // validateInputOverlap("foo", "bar", "buf.work.yaml") -> OK // validateInputOverlap("foo/bar", "foo", "buf.work.yaml") -> NOT OK // validateInputOverlap("foo", "foo/bar", "buf.work.yaml") -> NOT OK @@ -219,6 +231,15 @@ func validateInputOverlap( targetSubDirPath string, workspaceID string, ) error { + // If we are targeting the whole workspace and not a specific directory, + // we do not do this check. + // + // TODO: targetSubDirPath needs to be completely removed from WorkspaceBuilder + // and validateInputOverlap needs to be done somewhere else at a higher level, + // as the "target" is just a CLI argument concept, not a pure Workspace concept. + if targetSubDirPath == "." { + return nil + } if normalpath.ContainsPath(workspaceDirectory, targetSubDirPath, normalpath.Relative) { return fmt.Errorf( `failed to build input "%s" because it is contained by directory "%s" listed in %s`, diff --git a/private/buf/cmd/buf/command/beta/price/price.go b/private/buf/cmd/buf/command/beta/price/price.go index 669865c1cd..3eabfa855b 100644 --- a/private/buf/cmd/buf/command/beta/price/price.go +++ b/private/buf/cmd/buf/command/beta/price/price.go @@ -145,7 +145,7 @@ func run( if err != nil { return err } - moduleConfigs, err := moduleConfigReader.GetModuleConfigs( + moduleConfigSet, err := moduleConfigReader.GetModuleConfigSet( ctx, container, sourceOrModuleRef, @@ -157,6 +157,7 @@ func run( if err != nil { return err } + moduleConfigs := moduleConfigSet.ModuleConfigs() statsSlice := make([]*protostat.Stats, len(moduleConfigs)) for i, moduleConfig := range moduleConfigs { stats, err := protostat.GetStats(ctx, bufmodulestat.NewFileWalker(moduleConfig.Module())) diff --git a/private/buf/cmd/buf/command/beta/stats/stats.go b/private/buf/cmd/buf/command/beta/stats/stats.go index 70852eaa9c..8822c29605 100644 --- a/private/buf/cmd/buf/command/beta/stats/stats.go +++ b/private/buf/cmd/buf/command/beta/stats/stats.go @@ -116,7 +116,7 @@ func run( if err != nil { return err } - moduleConfigs, err := moduleConfigReader.GetModuleConfigs( + moduleConfigSet, err := moduleConfigReader.GetModuleConfigSet( ctx, container, sourceOrModuleRef, @@ -128,6 +128,7 @@ func run( if err != nil { return err } + moduleConfigs := moduleConfigSet.ModuleConfigs() statsSlice := make([]*protostat.Stats, len(moduleConfigs)) for i, moduleConfig := range moduleConfigs { stats, err := protostat.GetStats(ctx, bufmodulestat.NewFileWalker(moduleConfig.Module())) diff --git a/private/buf/cmd/buf/command/export/export.go b/private/buf/cmd/buf/command/export/export.go index 771de6977d..c30f8b64a9 100644 --- a/private/buf/cmd/buf/command/export/export.go +++ b/private/buf/cmd/buf/command/export/export.go @@ -162,7 +162,7 @@ func run( if err != nil { return err } - moduleConfigs, err := moduleConfigReader.GetModuleConfigs( + moduleConfigSet, err := moduleConfigReader.GetModuleConfigSet( ctx, container, sourceOrModuleRef, @@ -174,6 +174,7 @@ func run( if err != nil { return err } + moduleConfigs := moduleConfigSet.ModuleConfigs() moduleFileSetBuilder := bufmodulebuild.NewModuleFileSetBuilder( container.Logger(), moduleReader, @@ -184,7 +185,7 @@ func run( moduleFileSet, err := moduleFileSetBuilder.Build( ctx, moduleConfig.Module(), - bufmodulebuild.WithWorkspace(moduleConfig.Workspace()), + bufmodulebuild.WithWorkspace(moduleConfigSet.Workspace()), ) if err != nil { return err diff --git a/private/buf/cmd/buf/command/format/format.go b/private/buf/cmd/buf/command/format/format.go index c6ab28edad..92d0cfe65e 100644 --- a/private/buf/cmd/buf/command/format/format.go +++ b/private/buf/cmd/buf/command/format/format.go @@ -80,16 +80,16 @@ Most people will want to rewrite the files defined in the current directory in-p Display a diff between the original and formatted content with -d Write a diff instead of the formatted file: - + $ buf format simple/simple.proto -d - + $ diff -u simple/simple.proto.orig simple/simple.proto --- simple/simple.proto.orig 2022-03-24 09:44:10.000000000 -0700 +++ simple/simple.proto 2022-03-24 09:44:10.000000000 -0700 @@ -2,8 +2,7 @@ - + package simple; - + - message Object { - string key = 1; @@ -106,13 +106,13 @@ Use the --exit-code flag to exit with a non-zero exit code if there is a diff: Format a file, directory, or module reference by specifying a source e.g. Write the formatted file to stdout: - + $ buf format simple/simple.proto - + syntax = "proto3"; - + package simple; - + message Object { string key = 1; bytes value = 2; @@ -285,7 +285,7 @@ func run( if err != nil { return err } - moduleConfigs, err := moduleConfigReader.GetModuleConfigs( + moduleConfigSet, err := moduleConfigReader.GetModuleConfigSet( ctx, container, sourceOrModuleRef, @@ -297,6 +297,7 @@ func run( if err != nil { return err } + moduleConfigs := moduleConfigSet.ModuleConfigs() var outputDirectory string var singleFileOutputFilename string if flags.Output != "-" {