DO NOT MERGE: Fix issue where transitive dependencies in a workspace were not added to ModuleFileSet

private/buf/bufcli/bufcli.go

@@ -394,6 +394,7 @@ func NewWireImageConfigReader(
 	moduleResolver := bufapimodule.NewModuleResolver(
 		logger,
 		bufapimodule.NewRepositoryCommitServiceClientFactory(clientConfig),
+		bufapimodule.NewResolveServiceClientFactory(clientConfig),
 	)
 	moduleReader, err := NewModuleReaderAndCreateCacheDirs(container, clientConfig)
 	if err != nil {
@@ -404,7 +405,7 @@ func NewWireImageConfigReader(
 		storageosProvider,
 		NewFetchReader(logger, storageosProvider, runner, moduleResolver, moduleReader),
 		bufmodulebuild.NewModuleBucketBuilder(),
-		bufimagebuild.NewBuilder(logger, moduleReader),
+		bufimagebuild.NewBuilder(logger, moduleReader, moduleResolver),
 	), nil
 }
 
@@ -419,6 +420,7 @@ func NewWireModuleConfigReader(
 	moduleResolver := bufapimodule.NewModuleResolver(
 		logger,
 		bufapimodule.NewRepositoryCommitServiceClientFactory(clientConfig),
+		bufapimodule.NewResolveServiceClientFactory(clientConfig),
 	)
 	moduleReader, err := NewModuleReaderAndCreateCacheDirs(container, clientConfig)
 	if err != nil {
@@ -445,6 +447,7 @@ func NewWireModuleConfigReaderForModuleReader(
 	moduleResolver := bufapimodule.NewModuleResolver(
 		logger,
 		bufapimodule.NewRepositoryCommitServiceClientFactory(clientConfig),
+		bufapimodule.NewResolveServiceClientFactory(clientConfig),
 	)
 	return bufwire.NewModuleConfigReader(
 		logger,
@@ -465,6 +468,7 @@ func NewWireFileLister(
 	moduleResolver := bufapimodule.NewModuleResolver(
 		logger,
 		bufapimodule.NewRepositoryCommitServiceClientFactory(clientConfig),
+		bufapimodule.NewResolveServiceClientFactory(clientConfig),
 	)
 	moduleReader, err := NewModuleReaderAndCreateCacheDirs(container, clientConfig)
 	if err != nil {
@@ -475,7 +479,7 @@ func NewWireFileLister(
 		storageosProvider,
 		NewFetchReader(logger, storageosProvider, runner, moduleResolver, moduleReader),
 		bufmodulebuild.NewModuleBucketBuilder(),
-		bufimagebuild.NewBuilder(logger, moduleReader),
+		bufimagebuild.NewBuilder(logger, moduleReader, moduleResolver),
 	), nil
 }
 
@@ -830,7 +834,11 @@ func WellKnownTypeImage(ctx context.Context, logger *zap.Logger, wellKnownType s
 	if err != nil {
 		return nil, err
 	}
-	image, _, err := bufimagebuild.NewBuilder(logger, bufmodule.NewNopModuleReader()).Build(ctx, module)
+	image, _, err := bufimagebuild.NewBuilder(
+		logger,
+		bufmodule.NewNopModuleReader(),
+		bufmodule.NewNopModuleResolver(),
+	).Build(ctx, module)
 	if err != nil {
 		return nil, err
 	}

private/buf/cmd/buf/buf_test.go

@@ -2166,7 +2166,16 @@ func TestFormatExitCode(t *testing.T) {
 // equivalent to the original result.
 func TestFormatEquivalence(t *testing.T) {
 	t.Parallel()
-	tempDir := t.TempDir()
+	// The test data is in a workspace -- every module in this repository
+	// is in a workspace -- so we can't output the formatted module to a
+	// temp dir _outside_ this repository as it would suddenly exit the
+	// workspace. Instead, we make a temp dir in this repository and clean
+	// it up after.
+	tempDir, err := os.MkdirTemp(".", "TestFormatEquivalence")
+	require.NoError(t, err)
+	t.Cleanup(func() {
+		require.NoError(t, os.RemoveAll(tempDir))
+	})
 	testRunStdout(
 		t,
 		nil,

private/buf/cmd/buf/command/alpha/protoc/protoc.go

@@ -23,6 +23,7 @@ import (
 	"github.com/bufbuild/buf/private/buf/bufcli"
 	"github.com/bufbuild/buf/private/buf/buffetch"
 	"github.com/bufbuild/buf/private/bufpkg/bufanalysis"
+	"github.com/bufbuild/buf/private/bufpkg/bufapimodule"
 	"github.com/bufbuild/buf/private/bufpkg/bufimage"
 	"github.com/bufbuild/buf/private/bufpkg/bufimage/bufimagebuild"
 	"github.com/bufbuild/buf/private/bufpkg/bufimage/bufimageutil"
@@ -132,7 +133,16 @@ func run(
 	if len(env.PluginNameToPluginInfo) == 0 && !env.IncludeSourceInfo {
 		buildOptions = append(buildOptions, bufimagebuild.WithExcludeSourceCodeInfo())
 	}
-	image, fileAnnotations, err := bufimagebuild.NewBuilder(container.Logger(), moduleReader).Build(
+	moduleResolver := bufapimodule.NewModuleResolver(
+		container.Logger(),
+		bufapimodule.NewRepositoryCommitServiceClientFactory(clientConfig),
+		bufapimodule.NewResolveServiceClientFactory(clientConfig),
+	)
+	image, fileAnnotations, err := bufimagebuild.NewBuilder(
+		container.Logger(),
+		moduleReader,
+		moduleResolver,
+	).Build(
 		ctx,
 		module,
 		buildOptions...,

private/buf/cmd/buf/command/beta/graph/graph.go

@@ -118,6 +118,7 @@ func run(
 	moduleResolver := bufapimodule.NewModuleResolver(
 		container.Logger(),
 		bufapimodule.NewRepositoryCommitServiceClientFactory(clientConfig),
+		bufapimodule.NewResolveServiceClientFactory(clientConfig),
 	)
 	moduleReader, err := bufcli.NewModuleReaderAndCreateCacheDirs(container, clientConfig)
 	if err != nil {

private/buf/cmd/buf/command/export/export.go

@@ -22,6 +22,7 @@ import (
 	"github.com/bufbuild/buf/private/buf/bufcli"
 	"github.com/bufbuild/buf/private/buf/buffetch"
 	"github.com/bufbuild/buf/private/bufpkg/bufanalysis"
+	"github.com/bufbuild/buf/private/bufpkg/bufapimodule"
 	"github.com/bufbuild/buf/private/bufpkg/bufimage"
 	"github.com/bufbuild/buf/private/bufpkg/bufimage/bufimagebuild"
 	"github.com/bufbuild/buf/private/bufpkg/bufmodule"
@@ -152,6 +153,14 @@ func run(
 	if err != nil {
 		return err
 	}
+	moduleResolver := bufapimodule.NewModuleResolver(
+		container.Logger(),
+		bufapimodule.NewRepositoryCommitServiceClientFactory(clientConfig),
+		bufapimodule.NewResolveServiceClientFactory(clientConfig),
+	)
+	if err != nil {
+		return err
+	}
 	moduleConfigReader, err := bufcli.NewWireModuleConfigReaderForModuleReader(
 		container,
 		storageosProvider,
@@ -178,6 +187,7 @@ func run(
 	moduleFileSetBuilder := bufmodulebuild.NewModuleFileSetBuilder(
 		container.Logger(),
 		moduleReader,
+		moduleResolver,
 	)
 	// TODO: this is going to be a mess when we want to remove ModuleFileSet
 	moduleFileSets := make([]bufmodule.ModuleFileSet, len(moduleConfigs))
@@ -244,7 +254,11 @@ func run(
 			images = append(images, imageConfig.Image())
 		}
 	} else {
-		imageBuilder := bufimagebuild.NewBuilder(container.Logger(), moduleReader)
+		imageBuilder := bufimagebuild.NewBuilder(
+			container.Logger(),
+			moduleReader,
+			moduleResolver,
+		)
 		for _, moduleFileSet := range moduleFileSets {
 			targetFileInfos, err := moduleFileSet.TargetFileInfos(ctx)
 			if err != nil {

private/buf/cmd/buf/command/mod/modupdate/modupdate.go

@@ -20,13 +20,12 @@ import (
 
 	"connectrpc.com/connect"
 	"github.com/bufbuild/buf/private/buf/bufcli"
+	"github.com/bufbuild/buf/private/bufpkg/bufapimodule"
 	"github.com/bufbuild/buf/private/bufpkg/bufconfig"
-	"github.com/bufbuild/buf/private/bufpkg/bufconnect"
 	"github.com/bufbuild/buf/private/bufpkg/buflock"
 	"github.com/bufbuild/buf/private/bufpkg/bufmodule"
 	"github.com/bufbuild/buf/private/bufpkg/bufmodule/bufmoduleref"
 	"github.com/bufbuild/buf/private/gen/proto/connect/buf/alpha/registry/v1alpha1/registryv1alpha1connect"
-	modulev1alpha1 "github.com/bufbuild/buf/private/gen/proto/go/buf/alpha/module/v1alpha1"
 	registryv1alpha1 "github.com/bufbuild/buf/private/gen/proto/go/buf/alpha/registry/v1alpha1"
 	"github.com/bufbuild/buf/private/pkg/app/appcmd"
 	"github.com/bufbuild/buf/private/pkg/app/appflag"
@@ -215,29 +214,10 @@ func getDependencies(
 	if len(moduleConfig.Build.DependencyModuleReferences) == 0 {
 		return nil, nil
 	}
-	var remote string
-	if moduleConfig.ModuleIdentity != nil && moduleConfig.ModuleIdentity.Remote() != "" {
-		remote = moduleConfig.ModuleIdentity.Remote()
-	} else {
-		// At this point we know there's at least one dependency. If it's an unnamed module, select
-		// the right remote from the list of dependencies.
-		selectedRef := bufcli.SelectReferenceForRemote(moduleConfig.Build.DependencyModuleReferences)
-		if selectedRef == nil {
-			return nil, fmt.Errorf(`File %q has invalid "deps" references`, existingConfigFilePath)
-		}
-		remote = selectedRef.Remote()
-		container.Logger().Debug(fmt.Sprintf(
-			`File %q does not specify the "name" field. Based on the dependency %q, it appears that you are using a BSR instance at %q. Did you mean to specify "name: %s/..." within %q?`,
-			existingConfigFilePath,
-			selectedRef.IdentityString(),
-			remote,
-			remote,
-			existingConfigFilePath,
-		))
-	}
-	service := connectclient.Make(clientConfig, remote, registryv1alpha1connect.NewResolveServiceClient)
-	var protoDependencyModuleReferences []*modulev1alpha1.ModuleReference
-	var currentProtoModulePins []*modulev1alpha1.ModulePin
+	var (
+		moduleReferencesToUpdate []bufmoduleref.ModuleReference
+		existingModulePins       []bufmoduleref.ModulePin
+	)
 	if len(flags.Only) > 0 {
 		referencesByIdentity := map[string]bufmoduleref.ModuleReference{}
 		for _, reference := range moduleConfig.Build.DependencyModuleReferences {
@@ -248,32 +228,26 @@ func getDependencies(
 			if !ok {
 				return nil, fmt.Errorf("%q is not a valid --only input: no such dependency in current module deps", only)
 			}
-			protoDependencyModuleReferences = append(protoDependencyModuleReferences, bufmoduleref.NewProtoModuleReferenceForModuleReference(moduleReference))
+			moduleReferencesToUpdate = append(moduleReferencesToUpdate, moduleReference)
 		}
 		currentModulePins, err := bufmoduleref.DependencyModulePinsForBucket(ctx, readWriteBucket)
 		if err != nil {
 			return nil, fmt.Errorf("couldn't read current dependencies: %w", err)
 		}
-		currentProtoModulePins = bufmoduleref.NewProtoModulePinsForModulePins(currentModulePins...)
+		existingModulePins = currentModulePins
 	} else {
-		protoDependencyModuleReferences = bufmoduleref.NewProtoModuleReferencesForModuleReferences(
-			moduleConfig.Build.DependencyModuleReferences...,
-		)
+		moduleReferencesToUpdate = moduleConfig.Build.DependencyModuleReferences
 	}
-	resp, err := service.GetModulePins(
+	moduleResolver := bufapimodule.NewModuleResolver(
+		container.Logger(),
+		bufapimodule.NewRepositoryCommitServiceClientFactory(clientConfig),
+		bufapimodule.NewResolveServiceClientFactory(clientConfig),
+	)
+	dependencyModulePins, err := moduleResolver.GetModulePins(
 		ctx,
-		connect.NewRequest(&registryv1alpha1.GetModulePinsRequest{
-			ModuleReferences:  protoDependencyModuleReferences,
-			CurrentModulePins: currentProtoModulePins,
-		}),
+		moduleReferencesToUpdate,
+		existingModulePins,
 	)
-	if err != nil {
-		if remote != bufconnect.DefaultRemote {
-			return nil, bufcli.NewInvalidRemoteError(err, remote, moduleConfig.ModuleIdentity.IdentityString())
-		}
-		return nil, err
-	}
-	dependencyModulePins, err := bufmoduleref.NewModulePinsForProtos(resp.Msg.ModulePins...)
 	if err != nil {
 		return nil, bufcli.NewInternalError(err)
 	}

private/buf/cmd/buf/testdata/workspace/fail/pinconflict/a/a.proto

@@ -0,0 +1,9 @@
+syntax = "proto3";
+
+package a;
+
+import "b.proto";
+
+message A {
+    b.B b = 1;
+}

private/buf/cmd/buf/testdata/workspace/fail/pinconflict/a/buf.lock

@@ -0,0 +1,16 @@
+# Generated by buf. DO NOT EDIT.
+deps:
+  - remote: bufbuild.test
+    owner: workspace
+    repository: second
+    branch: main
+    commit: aced890b7e944f9ca174481ad810ca5f
+    digest: b1-guLHfQhGOhlEA-QaPyQLoT-ZVFGJkRkluWgkzPT0ma0=
+    create_time: 2021-05-04T14:52:47.094456Z
+  - remote: bufbuild.test
+    owner: workspace
+    repository: conflict
+    branch: main
+    commit: aced890b7e944f9ca174481ad810ca5f
+    digest: b1-guLHfQhGOhlEA-QaPyQLoT-ZVFGJkRkluWgkzPT0ma0=
+    create_time: 2021-05-04T14:52:47.094456Z

private/buf/cmd/buf/testdata/workspace/fail/pinconflict/a/buf.yaml

@@ -0,0 +1,5 @@
+version: v1
+name: bufbuild.test/workspace/first
+deps:
+  - bufbuild.test/workspace/second
+  - bufbuild.test/workspace/conflict

private/buf/cmd/buf/testdata/workspace/fail/pinconflict/b/b.proto

@@ -0,0 +1,5 @@
+syntax = "proto3";
+
+package b;
+
+message B {}

private/buf/cmd/buf/testdata/workspace/fail/pinconflict/b/buf.lock

@@ -0,0 +1,9 @@
+# Generated by buf. DO NOT EDIT.
+deps:
+  - remote: bufbuild.test
+    owner: workspace
+    repository: conflict
+    branch: main
+    commit: aced890b7e944f9ca174481ad810ca5g
+    digest: b1-guLHfQhGOhlEA-QaPyQLoT-ZVFGJkRkluWgkzPT0ma0=
+    create_time: 2021-05-04T14:52:47.094456Z

private/buf/cmd/buf/testdata/workspace/fail/pinconflict/b/buf.yaml

@@ -0,0 +1,4 @@
+version: v1
+name: bufbuild.test/workspace/second
+deps:
+  - bufbuild.test/workspace/conflict

private/buf/cmd/buf/testdata/workspace/fail/pinconflict/buf.work.yaml

@@ -0,0 +1,4 @@
+version: v1
+directories:
+  - a
+  - b

private/buf/cmd/buf/workspace_test.go

@@ -497,6 +497,21 @@ func TestWorkspaceWithLock(t *testing.T) {
 	)
 }
 
+func TestWorkspaceWithDependencyConflicts(t *testing.T) {
+	// The workspace contains two modules, one which depends on the other,
+	// and both of them depend on some other module but at different commits.
+	testRunStdoutStderrNoWarn(
+		t,
+		nil,
+		1,
+		``,
+		// This indicates that we tried to resolve the module pins, which fails because those modules are fake.
+		`Failure: the server hosted at that remote is unavailable. Are you sure "bufbuild.test" is a valid remote address?`,
+		"build",
+		filepath.Join("testdata", "workspace", "fail", "pinconflict", "a"),
+	)
+}
+
 func TestWorkspaceWithTransitiveDependencies(t *testing.T) {
 	// The workspace points to a module that includes transitive
 	// dependencies (i.e. a depends on b, and b depends on c).

private/bufpkg/bufapimodule/bufapimodule.go

@@ -24,6 +24,7 @@ import (
 
 type DownloadServiceClientFactory func(address string) registryv1alpha1connect.DownloadServiceClient
 type RepositoryCommitServiceClientFactory func(address string) registryv1alpha1connect.RepositoryCommitServiceClient
+type ResolveServiceClientFactory func(address string) registryv1alpha1connect.ResolveServiceClient
 
 func NewDownloadServiceClientFactory(clientConfig *connectclient.Config) DownloadServiceClientFactory {
 	return func(address string) registryv1alpha1connect.DownloadServiceClient {
@@ -37,6 +38,12 @@ func NewRepositoryCommitServiceClientFactory(clientConfig *connectclient.Config)
 	}
 }
 
+func NewResolveServiceClientFactory(clientConfig *connectclient.Config) ResolveServiceClientFactory {
+	return func(address string) registryv1alpha1connect.ResolveServiceClient {
+		return connectclient.Make(clientConfig, address, registryv1alpha1connect.NewResolveServiceClient)
+	}
+}
+
 // NewModuleReader returns a new ModuleReader backed by the download service.
 func NewModuleReader(
 	downloadClientFactory DownloadServiceClientFactory,
@@ -54,7 +61,12 @@ type ModuleReaderOption func(reader *moduleReader)
 // NewModuleResolver returns a new ModuleResolver backed by the resolve service.
 func NewModuleResolver(
 	logger *zap.Logger,
-	repositoryCommitClientFactory RepositoryCommitServiceClientFactory,
+	repositoryCommitServiceClientFactory RepositoryCommitServiceClientFactory,
+	resolveServiceClientFactory ResolveServiceClientFactory,
 ) bufmodule.ModuleResolver {
-	return newModuleResolver(logger, repositoryCommitClientFactory)
+	return newModuleResolver(
+		logger,
+		repositoryCommitServiceClientFactory,
+		resolveServiceClientFactory,
+	)
 }