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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 11 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
e6c35dc
Fix issue where transitive dependencies in a workspace were not added…
bufdev Oct 31, 2023
69fc948
commit
bufdev Oct 31, 2023
26a9246
Merge branch 'main' into workspace-transitive-dependencies
saquibmian Nov 2, 2023
dbdd56c
add modulepinresolver, no tests yet, still WIP
saquibmian Nov 2, 2023
d5f2aa9
pr comments
saquibmian Nov 3, 2023
6724e5c
add tests
saquibmian Nov 3, 2023
2acb1a1
Merge branch 'main' into workspace-transitive-dependencies
bufdev Nov 3, 2023
a74f191
Merge branch 'main' into workspace-transitive-dependencies
saquibmian Nov 6, 2023
00fd86e
Fix broken test
saquibmian Nov 7, 2023
c733b65
Merge branch 'main' into workspace-transitive-dependencies
saquibmian Nov 20, 2023
09cd93c
Rename var
saquibmian Nov 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions private/buf/bufcli/bufcli.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -404,7 +404,7 @@ func NewWireImageConfigReader(
storageosProvider,
NewFetchReader(logger, storageosProvider, runner, moduleResolver, moduleReader),
bufmodulebuild.NewModuleBucketBuilder(),
bufimagebuild.NewBuilder(logger, moduleReader),
bufimagebuild.NewBuilder(logger, moduleReader, bufmoduleref.NewModulePinResolver(clientConfig)),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you match the style in the codebase and either have all args on one line, or all args separated by newlines?

), nil
}

Expand Down Expand Up @@ -475,7 +475,7 @@ func NewWireFileLister(
storageosProvider,
NewFetchReader(logger, storageosProvider, runner, moduleResolver, moduleReader),
bufmodulebuild.NewModuleBucketBuilder(),
bufimagebuild.NewBuilder(logger, moduleReader),
bufimagebuild.NewBuilder(logger, moduleReader, bufmoduleref.NewModulePinResolver(clientConfig)),
), nil
}

Expand Down Expand Up @@ -830,7 +830,7 @@ 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(), bufmoduleref.NewNopModulePinResolver()).Build(ctx, module)
if err != nil {
return nil, err
}
Expand Down
3 changes: 2 additions & 1 deletion private/buf/cmd/buf/command/alpha/protoc/protoc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
"github.com/bufbuild/buf/private/bufpkg/bufimage/bufimagebuild"
"github.com/bufbuild/buf/private/bufpkg/bufimage/bufimageutil"
"github.com/bufbuild/buf/private/bufpkg/bufmodule/bufmodulebuild"
"github.com/bufbuild/buf/private/bufpkg/bufmodule/bufmoduleref"
"github.com/bufbuild/buf/private/bufpkg/bufpluginexec"
"github.com/bufbuild/buf/private/bufpkg/bufwasm"
"github.com/bufbuild/buf/private/pkg/app"
Expand Down Expand Up @@ -132,7 +133,7 @@ func run(
if len(env.PluginNameToPluginInfo) == 0 && !env.IncludeSourceInfo {
buildOptions = append(buildOptions, bufimagebuild.WithExcludeSourceCodeInfo())
}
image, fileAnnotations, err := bufimagebuild.NewBuilder(container.Logger(), moduleReader).Build(
image, fileAnnotations, err := bufimagebuild.NewBuilder(container.Logger(), moduleReader, bufmoduleref.NewModulePinResolver(clientConfig)).Build(
ctx,
module,
buildOptions...,
Expand Down
2 changes: 2 additions & 0 deletions private/buf/cmd/buf/command/beta/graph/graph.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"github.com/bufbuild/buf/private/bufpkg/bufgraph"
"github.com/bufbuild/buf/private/bufpkg/bufmodule"
"github.com/bufbuild/buf/private/bufpkg/bufmodule/bufmodulebuild"
"github.com/bufbuild/buf/private/bufpkg/bufmodule/bufmoduleref"
"github.com/bufbuild/buf/private/pkg/app/appcmd"
"github.com/bufbuild/buf/private/pkg/app/appflag"
"github.com/bufbuild/buf/private/pkg/command"
Expand Down Expand Up @@ -136,6 +137,7 @@ func run(
container.Logger(),
moduleResolver,
moduleReader,
bufmoduleref.NewModulePinResolver(clientConfig),
)
moduleConfigSet, err := moduleConfigReader.GetModuleConfigSet(
ctx,
Expand Down
3 changes: 2 additions & 1 deletion private/buf/cmd/buf/command/export/export.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,7 @@ func run(
moduleFileSetBuilder := bufmodulebuild.NewModuleFileSetBuilder(
container.Logger(),
moduleReader,
bufmoduleref.NewModulePinResolver(clientConfig),
)
// TODO: this is going to be a mess when we want to remove ModuleFileSet
moduleFileSets := make([]bufmodule.ModuleFileSet, len(moduleConfigs))
Expand Down Expand Up @@ -244,7 +245,7 @@ func run(
images = append(images, imageConfig.Image())
}
} else {
imageBuilder := bufimagebuild.NewBuilder(container.Logger(), moduleReader)
imageBuilder := bufimagebuild.NewBuilder(container.Logger(), moduleReader, bufmoduleref.NewModulePinResolver(clientConfig))
for _, moduleFileSet := range moduleFileSets {
targetFileInfos, err := moduleFileSet.TargetFileInfos(ctx)
if err != nil {
Expand Down
51 changes: 9 additions & 42 deletions private/buf/cmd/buf/command/mod/modupdate/modupdate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,10 @@ import (
"connectrpc.com/connect"
"github.com/bufbuild/buf/private/buf/bufcli"
"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"
Expand Down Expand Up @@ -215,29 +213,8 @@ 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
var resolveOptions []bufmoduleref.ResolveModulePinsOption
if len(flags.Only) > 0 {
referencesByIdentity := map[string]bufmoduleref.ModuleReference{}
for _, reference := range moduleConfig.Build.DependencyModuleReferences {
Expand All @@ -248,32 +225,22 @@ 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...)
resolveOptions = append(resolveOptions, bufmoduleref.ResolveModulePinsWithExistingModulePins(currentModulePins))
} else {
protoDependencyModuleReferences = bufmoduleref.NewProtoModuleReferencesForModuleReferences(
moduleConfig.Build.DependencyModuleReferences...,
)
moduleReferencesToUpdate = moduleConfig.Build.DependencyModuleReferences
}
resp, err := service.GetModulePins(
modulePinResolver := bufmoduleref.NewModulePinResolver(clientConfig)
dependencyModulePins, err := modulePinResolver.ResolveModulePins(
ctx,
connect.NewRequest(&registryv1alpha1.GetModulePinsRequest{
ModuleReferences: protoDependencyModuleReferences,
CurrentModulePins: currentProtoModulePins,
}),
moduleReferencesToUpdate,
resolveOptions...,
)
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)
}
Expand Down
3 changes: 3 additions & 0 deletions private/bufpkg/bufcheck/bufbreaking/bufbreaking_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"github.com/bufbuild/buf/private/bufpkg/bufimage/bufimagebuild"
"github.com/bufbuild/buf/private/bufpkg/bufmodule"
"github.com/bufbuild/buf/private/bufpkg/bufmodule/bufmodulebuild"
"github.com/bufbuild/buf/private/bufpkg/bufmodule/bufmoduleref"
"github.com/bufbuild/buf/private/pkg/storage"
"github.com/bufbuild/buf/private/pkg/storage/storageos"
"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -787,6 +788,7 @@ func testBreaking(
previousImage, previousFileAnnotations, err := bufimagebuild.NewBuilder(
zap.NewNop(),
bufmodule.NewNopModuleReader(),
bufmoduleref.NewNopModulePinResolver(),
).Build(
ctx,
previousModule,
Expand All @@ -805,6 +807,7 @@ func testBreaking(
image, fileAnnotations, err := bufimagebuild.NewBuilder(
zap.NewNop(),
bufmodule.NewNopModuleReader(),
bufmoduleref.NewNopModulePinResolver(),
).Build(
ctx,
module,
Expand Down
2 changes: 2 additions & 0 deletions private/bufpkg/bufcheck/buflint/buflint_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"github.com/bufbuild/buf/private/bufpkg/bufimage/bufimagebuild"
"github.com/bufbuild/buf/private/bufpkg/bufmodule"
"github.com/bufbuild/buf/private/bufpkg/bufmodule/bufmodulebuild"
"github.com/bufbuild/buf/private/bufpkg/bufmodule/bufmoduleref"
"github.com/bufbuild/buf/private/pkg/storage"
"github.com/bufbuild/buf/private/pkg/storage/storageos"
"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -1164,6 +1165,7 @@ func testLintWithModifiers(
image, fileAnnotations, err := bufimagebuild.NewBuilder(
zap.NewNop(),
bufmodule.NewNopModuleReader(),
bufmoduleref.NewNopModulePinResolver(),
).Build(
ctx,
module,
Expand Down
3 changes: 3 additions & 0 deletions private/bufpkg/bufgraph/bufgraph.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (

"github.com/bufbuild/buf/private/bufpkg/bufanalysis"
"github.com/bufbuild/buf/private/bufpkg/bufmodule"
"github.com/bufbuild/buf/private/bufpkg/bufmodule/bufmoduleref"
"github.com/bufbuild/buf/private/pkg/dag"
"go.uber.org/zap"
)
Expand Down Expand Up @@ -68,11 +69,13 @@ func NewBuilder(
logger *zap.Logger,
moduleResolver bufmodule.ModuleResolver,
moduleReader bufmodule.ModuleReader,
modulePinResolver bufmoduleref.ModulePinResolver,
) Builder {
return newBuilder(
logger,
moduleResolver,
moduleReader,
modulePinResolver,
)
}

Expand Down
2 changes: 2 additions & 0 deletions private/bufpkg/bufgraph/bufgraph_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"github.com/bufbuild/buf/private/bufpkg/bufconfig"
"github.com/bufbuild/buf/private/bufpkg/bufmodule"
"github.com/bufbuild/buf/private/bufpkg/bufmodule/bufmodulebuild"
"github.com/bufbuild/buf/private/bufpkg/bufmodule/bufmoduleref"
"github.com/bufbuild/buf/private/pkg/storage"
"github.com/bufbuild/buf/private/pkg/storage/storageos"
"github.com/stretchr/testify/require"
Expand All @@ -39,6 +40,7 @@ func TestBasic(t *testing.T) {
zap.NewNop(),
bufmodule.NewNopModuleResolver(),
bufmodule.NewNopModuleReader(),
bufmoduleref.NewNopModulePinResolver(),
)
graph, fileAnnotations, err := builder.Build(
ctx,
Expand Down
2 changes: 2 additions & 0 deletions private/bufpkg/bufgraph/builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ func newBuilder(
logger *zap.Logger,
moduleResolver bufmodule.ModuleResolver,
moduleReader bufmodule.ModuleReader,
modulePinResolver bufmoduleref.ModulePinResolver,
) *builder {
return &builder{
logger: logger,
Expand All @@ -46,6 +47,7 @@ func newBuilder(
imageBuilder: bufimagebuild.NewBuilder(
logger,
moduleReader,
modulePinResolver,
),
}
}
Expand Down
4 changes: 2 additions & 2 deletions private/bufpkg/bufimage/bufimagebuild/bufimagebuild.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,8 @@ type Builder interface {
}

// NewBuilder returns a new Builder.
func NewBuilder(logger *zap.Logger, moduleReader bufmodule.ModuleReader) Builder {
return newBuilder(logger, moduleReader)
func NewBuilder(logger *zap.Logger, moduleReader bufmodule.ModuleReader, modulePinResolver bufmoduleref.ModulePinResolver) Builder {
return newBuilder(logger, moduleReader, modulePinResolver)
}

// BuildOption is an option for Build.
Expand Down
3 changes: 2 additions & 1 deletion private/bufpkg/bufimage/bufimagebuild/bufimagebuildtesting/bufimagebuildtesting.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
"github.com/bufbuild/buf/private/bufpkg/bufmodule"
"github.com/bufbuild/buf/private/bufpkg/bufmodule/bufmodulebuild"
"github.com/bufbuild/buf/private/bufpkg/bufmodule/bufmoduleconfig"
"github.com/bufbuild/buf/private/bufpkg/bufmodule/bufmoduleref"
"github.com/bufbuild/buf/private/bufpkg/buftesting"
"github.com/bufbuild/buf/private/pkg/command"
"github.com/bufbuild/buf/private/pkg/prototesting"
Expand Down Expand Up @@ -96,7 +97,7 @@ func fuzzBuild(ctx context.Context, dirPath string) (bufimage.Image, []bufanalys
if err != nil {
return nil, nil, err
}
builder := bufimagebuild.NewBuilder(zap.NewNop(), bufmodule.NewNopModuleReader())
builder := bufimagebuild.NewBuilder(zap.NewNop(), bufmodule.NewNopModuleReader(), bufmoduleref.NewNopModulePinResolver())
opt := bufimagebuild.WithExcludeSourceCodeInfo()
return builder.Build(ctx, module, opt)
}
Expand Down
3 changes: 2 additions & 1 deletion private/bufpkg/bufimage/bufimagebuild/builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,12 +50,13 @@ type builder struct {
tracer trace.Tracer
}

func newBuilder(logger *zap.Logger, moduleReader bufmodule.ModuleReader) *builder {
func newBuilder(logger *zap.Logger, moduleReader bufmodule.ModuleReader, modulePinResolver bufmoduleref.ModulePinResolver) *builder {
return &builder{
logger: logger.Named(loggerName),
moduleFileSetBuilder: bufmodulebuild.NewModuleFileSetBuilder(
logger,
moduleReader,
modulePinResolver,
),
tracer: otel.GetTracerProvider().Tracer(tracerName),
}
Expand Down
5 changes: 3 additions & 2 deletions private/bufpkg/bufimage/bufimagebuild/builder_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (
"github.com/bufbuild/buf/private/bufpkg/bufmodule"
"github.com/bufbuild/buf/private/bufpkg/bufmodule/bufmodulebuild"
"github.com/bufbuild/buf/private/bufpkg/bufmodule/bufmoduleconfig"
"github.com/bufbuild/buf/private/bufpkg/bufmodule/bufmoduleref"
"github.com/bufbuild/buf/private/bufpkg/buftesting"
"github.com/bufbuild/buf/private/pkg/command"
"github.com/bufbuild/buf/private/pkg/normalpath"
Expand Down Expand Up @@ -300,7 +301,7 @@ func TestOptionPanic(t *testing.T) {
t.Parallel()
require.NotPanics(t, func() {
module := testGetModule(t, filepath.Join("testdata", "optionpanic"))
_, _, err := NewBuilder(zap.NewNop(), bufmodule.NewNopModuleReader()).Build(
_, _, err := NewBuilder(zap.NewNop(), bufmodule.NewNopModuleReader(), bufmoduleref.NewNopModulePinResolver()).Build(
context.Background(),
module,
)
Expand Down Expand Up @@ -338,7 +339,7 @@ func testBuild(t *testing.T, includeSourceInfo bool, dirPath string) (bufimage.I
if !includeSourceInfo {
options = append(options, WithExcludeSourceCodeInfo())
}
image, fileAnnotations, err := NewBuilder(zap.NewNop(), bufmodule.NewNopModuleReader()).Build(
image, fileAnnotations, err := NewBuilder(zap.NewNop(), bufmodule.NewNopModuleReader(), bufmoduleref.NewNopModulePinResolver()).Build(
context.Background(),
module,
options...,
Expand Down
1 change: 1 addition & 0 deletions private/bufpkg/bufimage/bufimagemodify/bufimagemodify_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ func testGetImage(t *testing.T, dirPath string, includeSourceInfo bool) bufimage
image, annotations, err := bufimagebuild.NewBuilder(
zap.NewNop(),
bufmodule.NewNopModuleReader(),
bufmoduleref.NewNopModulePinResolver(),
).Build(
context.Background(),
module,
Expand Down
4 changes: 3 additions & 1 deletion private/bufpkg/bufimage/bufimageutil/bufimageutil_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,7 @@ func TestTransitivePublic(t *testing.T) {
image, analysis, err := bufimagebuild.NewBuilder(
zaptest.NewLogger(t),
bufmodule.NewNopModuleReader(),
bufmoduleref.NewNopModulePinResolver(),
).Build(
ctx,
module,
Expand Down Expand Up @@ -218,6 +219,7 @@ func TestTypesFromMainModule(t *testing.T) {
moduleIdentityDep.IdentityString(): moduleDep,
},
),
bufmoduleref.NewNopModulePinResolver(),
).Build(
ctx,
module,
Expand Down Expand Up @@ -251,7 +253,7 @@ func getImage(ctx context.Context, logger *zap.Logger, testdataDir string, optio
if err != nil {
return nil, nil, err
}
builder := bufimagebuild.NewBuilder(logger, bufmodule.NewNopModuleReader())
builder := bufimagebuild.NewBuilder(logger, bufmodule.NewNopModuleReader(), bufmoduleref.NewNopModulePinResolver())
image, analysis, err := builder.Build(
ctx,
module,
Expand Down
3 changes: 2 additions & 1 deletion private/bufpkg/bufmodule/bufmodulebuild/bufmodulebuild.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,9 @@ type ModuleFileSetBuilder interface {
func NewModuleFileSetBuilder(
logger *zap.Logger,
moduleReader bufmodule.ModuleReader,
modulePinResolver bufmoduleref.ModulePinResolver,
) ModuleFileSetBuilder {
return newModuleFileSetBuilder(logger, moduleReader)
return newModuleFileSetBuilder(logger, moduleReader, modulePinResolver)
}

// BuildModuleFileSetOption is an option for Build.
Expand Down