Workspace directory (#2495)

Fixes #2480 This fixes the problem where in some occasions dependencies from workspace are not loaded correctly when `--path` is passed. For example, `buf build workspacedir/a --path workspacedir/a/foo.proto` when `workspacedir/b` and `workspacedir/c` also exist. In `module_file_set_builder.go` module b and module c would be hashed based on their `TargetFileInfos`, which are both empty slices due to the `--path` filter. Same hash means only one of them gets added to the list of potential dependencies. In this PR, workspace modules are distinguished by their directories relative to the workspace directory. This is accomplished by adding `WorkspaceDirectory` to interface `bufmodule.Module`. --------- Co-authored-by: bufdev <bufdev-github@buf.build>
bufbuild · Oct 16, 2023 · 9db765f · 9db765f
1 parent 4f7c33b
commit 9db765f
Show file tree

Hide file tree

Showing 21 changed files with 137 additions and 132 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,7 +2,7 @@
 
 ## [Unreleased]
 
-- No changes yet.
+- Fix issue in v1.27.0 where `--path` did not work with workspaces under certain scenarios.
 
 ## [v1.27.0] - 2023-10-04
 

diff --git a/private/buf/bufwire/module_config_reader.go b/private/buf/bufwire/module_config_reader.go
@@ -611,6 +611,7 @@ func (m *moduleConfigReader) getSourceModuleConfig(
 			}
 		}
 	}
+	buildOptions = append(buildOptions, bufmodulebuild.WithWorkspaceDirectory(subDirPath))
 	if len(externalExcludeDirOrFilePaths) > 0 {
 		bucketRelPaths := make([]string, len(externalExcludeDirOrFilePaths))
 		for i, excludeDirOrFilePath := range externalExcludeDirOrFilePaths {

diff --git a/private/buf/bufwork/workspace_builder.go b/private/buf/bufwork/workspace_builder.go
@@ -136,6 +136,7 @@ func (w *workspaceBuilder) BuildWorkspace(
 		if err != nil {
 			return nil, err
 		}
+		buildOptions = append(buildOptions, bufmodulebuild.WithWorkspaceDirectory(directory))
 		module, err := bufmodulebuild.NewModuleBucketBuilder().BuildForBucket(
 			ctx,
 			readBucketForDirectory,

diff --git a/private/buf/cmd/buf/testdata/workspace/success/breaking/a/proto/a/v1/a.proto b/private/buf/cmd/buf/testdata/workspace/success/breaking/a/proto/a/v1/a.proto
@@ -0,0 +1,5 @@
+syntax = "proto3";
+
+package a.v1;
+
+message A {}
diff --git a/private/buf/cmd/buf/testdata/workspace/success/breaking/a/proto/buf.yaml b/private/buf/cmd/buf/testdata/workspace/success/breaking/a/proto/buf.yaml
@@ -0,0 +1 @@
+version: v1
diff --git a/private/buf/cmd/buf/testdata/workspace/success/breaking/buf.work.yaml b/private/buf/cmd/buf/testdata/workspace/success/breaking/buf.work.yaml
@@ -1,4 +1,5 @@
 version: v1
 directories:
+  - a/proto
   - other/proto
   - proto
diff --git a/private/buf/cmd/buf/testdata/workspace/success/dir/a/proto/a/v1/a.proto b/private/buf/cmd/buf/testdata/workspace/success/dir/a/proto/a/v1/a.proto
@@ -0,0 +1,5 @@
+syntax = "proto3";
+
+package a.v1;
+
+message A {}
diff --git a/private/buf/cmd/buf/testdata/workspace/success/dir/a/proto/buf.yaml b/private/buf/cmd/buf/testdata/workspace/success/dir/a/proto/buf.yaml
@@ -0,0 +1 @@
+version: v1
diff --git a/private/buf/cmd/buf/testdata/workspace/success/dir/buf.work.yaml b/private/buf/cmd/buf/testdata/workspace/success/dir/buf.work.yaml
@@ -1,4 +1,5 @@
 version: v1
 directories:
+  - a/proto
   - other/proto
   - proto
diff --git a/private/buf/cmd/buf/testdata/workspace/success/dir_buf_work/a/proto/a/v1/a.proto b/private/buf/cmd/buf/testdata/workspace/success/dir_buf_work/a/proto/a/v1/a.proto
@@ -0,0 +1,5 @@
+syntax = "proto3";
+
+package a.v1;
+
+message A {}
diff --git a/private/buf/cmd/buf/testdata/workspace/success/dir_buf_work/a/proto/buf.yaml b/private/buf/cmd/buf/testdata/workspace/success/dir_buf_work/a/proto/buf.yaml
@@ -0,0 +1 @@
+version: v1
diff --git a/private/buf/cmd/buf/testdata/workspace/success/dir_buf_work/buf.work b/private/buf/cmd/buf/testdata/workspace/success/dir_buf_work/buf.work
@@ -1,4 +1,5 @@
 version: v1
 directories:
+  - a/proto
   - other/proto
   - proto
diff --git a/private/buf/cmd/buf/workspace_test.go b/private/buf/cmd/buf/workspace_test.go
@@ -116,7 +116,8 @@ func TestWorkspaceDir(t *testing.T) {
 			t,
 			nil,
 			0,
-			filepath.FromSlash(`testdata/workspace/success/`+baseDirPath+`/other/proto/request.proto
+			filepath.FromSlash(`testdata/workspace/success/`+baseDirPath+`/a/proto/a/v1/a.proto
+		    testdata/workspace/success/`+baseDirPath+`/other/proto/request.proto
 		    testdata/workspace/success/`+baseDirPath+`/proto/rpc.proto`),
 			"ls-files",
 			filepath.Join("testdata", "workspace", "success", baseDirPath),
@@ -144,7 +145,8 @@ func TestWorkspaceDir(t *testing.T) {
 			t,
 			nil,
 			0,
-			filepath.FromSlash(`testdata/workspace/success/breaking/other/proto/request.proto
+			filepath.FromSlash(`testdata/workspace/success/breaking/a/proto/a/v1/a.proto
+		    testdata/workspace/success/breaking/other/proto/request.proto
 		    testdata/workspace/success/breaking/proto/rpc.proto`),
 			"ls-files",
 			filepath.Join("testdata", "workspace", "success", "breaking"),
@@ -302,48 +304,6 @@ func TestWorkspaceNestedArchive(t *testing.T) {
 	)
 }
 
-func TestWorkspaceGit(t *testing.T) {
-	t.Skip("skip until the move to private/buf is merged")
-	// Directory paths specified as a git reference within a workspace.
-	t.Parallel()
-	testRunStdout(
-		t,
-		nil,
-		0,
-		``,
-		"build",
-		"../../../../.git#ref=HEAD,subdir=private/buf/cmd/buf/testdata/workspace/success/dir/proto",
-	)
-	testRunStdout(
-		t,
-		nil,
-		0,
-		filepath.FromSlash(`private/buf/cmd/buf/testdata/workspace/success/dir/proto/rpc.proto`),
-		"ls-files",
-		"../../../../.git#ref=HEAD,subdir=private/buf/cmd/buf/testdata/workspace/success/dir/proto",
-	)
-	testRunStdout(
-		t,
-		nil,
-		bufcli.ExitCodeFileAnnotation,
-		filepath.FromSlash(`private/buf/cmd/buf/testdata/workspace/success/dir/proto/rpc.proto:3:1:Files with package "example" must be within a directory "example" relative to root but were in directory ".".
-        private/buf/cmd/buf/testdata/workspace/success/dir/proto/rpc.proto:3:1:Package name "example" should be suffixed with a correctly formed version, such as "example.v1".`),
-		"lint",
-		"../../../../.git#ref=HEAD,subdir=private/buf/cmd/buf/testdata/workspace/success/dir/proto",
-	)
-	testRunStdout(
-		t,
-		nil,
-		bufcli.ExitCodeFileAnnotation,
-		filepath.FromSlash(`private/buf/cmd/buf/testdata/workspace/success/dir/proto/rpc.proto:3:1:Files with package "example" must be within a directory "example" relative to root but were in directory ".".
-        private/buf/cmd/buf/testdata/workspace/success/dir/proto/rpc.proto:3:1:Package name "example" should be suffixed with a correctly formed version, such as "example.v1".`),
-		"lint",
-		"../../../../.git#ref=HEAD,subdir=private/buf/cmd/buf/testdata/workspace/success/dir/proto",
-		"--path",
-		filepath.Join("internal", "buf", "cmd", "buf", "testdata", "workspace", "success", "dir", "proto", "rpc.proto"),
-	)
-}
-
 func TestWorkspaceDetached(t *testing.T) {
 	// The workspace doesn't include the 'proto' directory, so
 	// its contents aren't included in the workspace.
@@ -900,7 +860,7 @@ func TestWorkspaceBreakingFail(t *testing.T) {
 		nil,
 		1,
 		``,
-		`Failure: input contained 1 images, whereas against contained 2 images`,
+		`Failure: input contained 1 images, whereas against contained 3 images`,
 		"breaking",
 		filepath.Join("testdata", "workspace", "fail", "breaking"),
 		"--against",

diff --git a/private/buf/cmd/buf/workspace_unix_test.go b/private/buf/cmd/buf/workspace_unix_test.go
@@ -87,3 +87,58 @@ func TestWorkspaceAbsoluteFail(t *testing.T) {
 		filepath.Join("testdata", "workspace", "fail", "absolute"),
 	)
 }
+
+// TODO: Move this back to workspace_test.go. after resolving the issue where git
+// clone failed with "unable to create file filename too long" on Windows CI.
+// Workflow run: https://github.com/bufbuild/buf/actions/runs/6510804063/job/17685247791.
+// Potential fix: https://stackoverflow.com/questions/22575662/filename-too-long-in-git-for-windows.
+func TestWorkspaceGit(t *testing.T) {
+	// Directory paths specified as a git reference within a workspace.
+	t.Parallel()
+	testRunStdout(
+		t,
+		nil,
+		0,
+		``,
+		"build",
+		"../../../../.git#ref=HEAD,subdir=private/buf/cmd/buf/testdata/workspace/success/dir/proto",
+	)
+	testRunStdout(
+		t,
+		nil,
+		0,
+		``,
+		"build",
+		"../../../../.git#ref=HEAD,subdir=private/buf/cmd/buf/testdata/workspace/success/dir/proto",
+		"--path",
+		filepath.Join("private", "buf", "cmd", "buf", "testdata", "workspace", "success", "dir", "proto", "rpc.proto"),
+	)
+	testRunStdout(
+		t,
+		nil,
+		0,
+		filepath.FromSlash(`private/buf/cmd/buf/testdata/workspace/success/dir/proto/rpc.proto`),
+		"ls-files",
+		"../../../../.git#ref=HEAD,subdir=private/buf/cmd/buf/testdata/workspace/success/dir/proto",
+	)
+	testRunStdout(
+		t,
+		nil,
+		bufcli.ExitCodeFileAnnotation,
+		filepath.FromSlash(`private/buf/cmd/buf/testdata/workspace/success/dir/proto/rpc.proto:3:1:Files with package "example" must be within a directory "example" relative to root but were in directory ".".
+        private/buf/cmd/buf/testdata/workspace/success/dir/proto/rpc.proto:3:1:Package name "example" should be suffixed with a correctly formed version, such as "example.v1".`),
+		"lint",
+		"../../../../.git#ref=HEAD,subdir=private/buf/cmd/buf/testdata/workspace/success/dir/proto",
+	)
+	testRunStdout(
+		t,
+		nil,
+		bufcli.ExitCodeFileAnnotation,
+		filepath.FromSlash(`private/buf/cmd/buf/testdata/workspace/success/dir/proto/rpc.proto:3:1:Files with package "example" must be within a directory "example" relative to root but were in directory ".".
+        private/buf/cmd/buf/testdata/workspace/success/dir/proto/rpc.proto:3:1:Package name "example" should be suffixed with a correctly formed version, such as "example.v1".`),
+		"lint",
+		"../../../../.git#ref=HEAD,subdir=private/buf/cmd/buf/testdata/workspace/success/dir/proto",
+		"--path",
+		filepath.Join("private", "buf", "cmd", "buf", "testdata", "workspace", "success", "dir", "proto", "rpc.proto"),
+	)
+}
diff --git a/private/bufpkg/bufgraph/bufgraph_test.go b/private/bufpkg/bufgraph/bufgraph_test.go
@@ -105,6 +105,9 @@ func testBuildWorkspace(ctx context.Context, workspacePath string) (bufmodule.Wo
 			bufmodulebuild.WithModuleIdentity(
 				moduleConfig.ModuleIdentity,
 			),
+			bufmodulebuild.WithWorkspaceDirectory(
+				directory,
+			),
 		)
 		if err != nil {
 			return nil, err

diff --git a/private/bufpkg/bufmodule/bufmodule.go b/private/bufpkg/bufmodule/bufmodule.go
@@ -148,8 +148,6 @@ type Module interface {
 	// of every file in the module, which is useful for caching or recreating a module's
 	// original files.
 	BlobSet() *manifest.BlobSet
-
-	getSourceReadBucket() storage.ReadBucket
 	// ModuleIdentity returns the ModuleIdentity for the Module, if it was
 	// provided at construction time via ModuleWithModuleIdentity or ModuleWithModuleIdentityAndCommit.
 	//
@@ -165,6 +163,17 @@ type Module interface {
 	// even if ModuleIdentity is set, that is commit is optional information
 	// even if we know what module this file came from.
 	Commit() string
+	// WorkspaceDirectory returns the directory within the workspace that this Module was constructed from,
+	// if this Module was constructed from a Workspace. If the Module was not constructed from a Workspace,
+	// This value will be empty.
+	//
+	// This is needed for now because we need to determine if the input for ModuleFileSetBuilder is equal
+	// to any Modules within the given optional Workspace. This is terrible. Once we have refactored
+	// the CLI to have Workspaces as a first-class citizen, where the typical case is a Workspace with
+	// a single Module, we will no longer need to do this type of check, and this can be removed.
+	WorkspaceDirectory() string
+
+	getSourceReadBucket() storage.ReadBucket
 	isModule()
 }
 
@@ -189,6 +198,15 @@ func ModuleWithModuleIdentityAndCommit(moduleIdentity bufmoduleref.ModuleIdentit
 	}
 }
 
+// ModuleWithWorkspaceDirectory returns a new ModuleOption that sets the workspace directory.
+//
+// See the comment on Module.WorkspaceDirectory() for more details.
+func ModuleWithWorkspaceDirectory(workspaceDirectory string) ModuleOption {
+	return func(module *module) {
+		module.workspaceDirectory = workspaceDirectory
+	}
+}
+
 // NewModuleForBucket returns a new Module. It attempts to read dependencies
 // from a lock file in the read bucket.
 func NewModuleForBucket(

diff --git a/private/bufpkg/bufmodule/bufmodulebuild/bufmodulebuild.go b/private/bufpkg/bufmodule/bufmodulebuild/bufmodulebuild.go
@@ -16,7 +16,6 @@ package bufmodulebuild
 
 import (
 	"context"
-	"errors"
 
 	"github.com/bufbuild/buf/private/bufpkg/bufmodule"
 	"github.com/bufbuild/buf/private/bufpkg/bufmodule/bufmoduleconfig"
@@ -26,10 +25,6 @@ import (
 	"go.uber.org/zap"
 )
 
-// ErrDuplicateDependency is the error returned if a two modules have the same set of Protobuf file
-// paths.
-var ErrDuplicateDependency = errors.New("module declared in DependencyModulePins but not in workspace was already added to the dependency Module set")
-
 // ModuleFileSetBuilder builds ModuleFileSets from Modules.
 type ModuleFileSetBuilder interface {
 	Build(
@@ -173,3 +168,12 @@ func WithExcludePathsAllowNotExist(excludePaths []string) BuildOption {
 		buildOptions.pathsAllowNotExist = true
 	}
 }
+
+// WithWorkspaceDirectory returns a new BuildOption that specifies the workspace directory.
+//
+// See the comment on Module.WorkspaceDirectory() for more details.
+func WithWorkspaceDirectory(workspaceDirectory string) BuildOption {
+	return func(buildOptions *buildOptions) {
+		buildOptions.workspaceDirectory = workspaceDirectory
+	}
+}
diff --git a/private/bufpkg/bufmodule/bufmodulebuild/module_bucket_builder.go b/private/bufpkg/bufmodule/bufmodulebuild/module_bucket_builder.go
@@ -141,6 +141,9 @@ func (b *moduleBucketBuilder) buildForBucket(
 		bufmodule.ModuleWithModuleIdentity(
 			buildOptions.moduleIdentity, // This may be nil
 		),
+		bufmodule.ModuleWithWorkspaceDirectory(
+			buildOptions.workspaceDirectory,
+		),
 	)
 	if err != nil {
 		return nil, err