From 3bd5c13b8114733a191c9c9d086f2ee2c881ffa1 Mon Sep 17 00:00:00 2001 From: Edward McFarlane Date: Mon, 27 Jan 2025 19:02:13 +0000 Subject: [PATCH] Debug bufmod calc --- .../buf/bufworkspace/workspace_provider.go | 3 + private/bufpkg/bufmodule/added_module.go | 132 ++++++++++-------- private/bufpkg/bufmodule/bufmodule_test.go | 2 +- .../bufmoduleapi/module_data_provider.go | 2 + private/bufpkg/bufmodule/module.go | 117 ++++++++-------- private/bufpkg/bufmodule/module_data.go | 11 ++ .../bufpkg/bufmodule/module_set_builder.go | 28 ++-- 7 files changed, 169 insertions(+), 126 deletions(-) diff --git a/private/buf/bufworkspace/workspace_provider.go b/private/buf/bufworkspace/workspace_provider.go index 5b52d118a2..16507028cf 100644 --- a/private/buf/bufworkspace/workspace_provider.go +++ b/private/buf/bufworkspace/workspace_provider.go @@ -351,6 +351,7 @@ func (w *workspaceProvider) getWorkspaceForBucketAndModuleDirPathsV1Beta1OrV1( moduleSetBuilder.AddRemoteModule( depModuleKey, false, + nil, // TODO ) } } @@ -441,11 +442,13 @@ func (w *workspaceProvider) getWorkspaceForBucketBufYAMLV2( return nil, syserror.Newf("unknown FileVersion: %v", fileVersion) } for _, depModuleKey := range bufLockFile.DepModuleKeys() { + fmt.Println("workspace_provider.go v2 depModuleKey", depModuleKey) // DepModuleKeys from a BufLockFile is expected to have all transitive dependencies, // and we can rely on this property. moduleSetBuilder.AddRemoteModule( depModuleKey, false, + nil, // TODO ) } remotePluginKeys = bufLockFile.RemotePluginKeys() diff --git a/private/bufpkg/bufmodule/added_module.go b/private/bufpkg/bufmodule/added_module.go index 2770778be7..8877bd7779 100644 --- a/private/bufpkg/bufmodule/added_module.go +++ b/private/bufpkg/bufmodule/added_module.go @@ -47,6 +47,7 @@ import ( type addedModule struct { localModule Module remoteModuleKey ModuleKey + dependencies []ModuleKey remoteTargetPaths []string remoteTargetExcludePaths []string isTarget bool @@ -67,10 +68,24 @@ func newRemoteAddedModule( remoteTargetPaths []string, remoteTargetExcludePaths []string, isTarget bool, + dependencies []ModuleKey, ) *addedModule { + digest, err := remoteModuleKey.Digest() + if err != nil { + panic(err) + } + fmt.Println("REMOTE MODULE", remoteModuleKey.FullName().String(), digest) + for _, dep := range dependencies { + digest, err := dep.Digest() + if err != nil { + panic(err) + } + fmt.Println("\tDEP:", dep.FullName().String(), digest) + } return &addedModule{ remoteModuleKey: remoteModuleKey, remoteTargetPaths: remoteTargetPaths, + dependencies: dependencies, remoteTargetExcludePaths: remoteTargetExcludePaths, isTarget: isTarget, } @@ -256,61 +271,66 @@ func (a *addedModule) ToModule( // the Module actually says is in its buf.lock). This function enables us to do that for // digest calculations. Within the Module, we say that if we get a remote Module, use the // declared ModuleKeys instead of whatever Module we have resolved to for a given FullName. - getDeclaredDepModuleKeysB5 := func() ([]ModuleKey, error) { - moduleData, err := getModuleData() - if err != nil { - return nil, err - } - declaredDepModuleKeys, err := moduleData.DeclaredDepModuleKeys() - if err != nil { - return nil, err - } - if len(declaredDepModuleKeys) == 0 { - return nil, nil - } - var digestType DigestType - for i, moduleKey := range declaredDepModuleKeys { - digest, err := moduleKey.Digest() - if err != nil { - return nil, err - } - if i == 0 { - digestType = digest.Type() - } else if digestType != digest.Type() { - return nil, syserror.Newf("multiple digest types found in DeclaredDepModuleKeys: %v, %v", digestType, digest.Type()) - } - } - switch digestType { - case DigestTypeB4: - // The declared ModuleKey dependencies for a commit may be stored in v1 buf.lock file, - // in which case they will use B4 digests. B4 digests aren't allowed to be used as - // input to the B5 digest calculation, so we perform a call to convert all ModuleKeys - // from B4 to B5 by using the commit provider. - commitKeysToFetch := make([]CommitKey, len(declaredDepModuleKeys)) - for i, declaredDepModuleKey := range declaredDepModuleKeys { - commitKey, err := NewCommitKey(declaredDepModuleKey.FullName().Registry(), declaredDepModuleKey.CommitID(), DigestTypeB5) - if err != nil { - return nil, err - } - commitKeysToFetch[i] = commitKey - } - commits, err := commitProvider.GetCommitsForCommitKeys(ctx, commitKeysToFetch) - if err != nil { - return nil, err - } - if len(commits) != len(commitKeysToFetch) { - return nil, syserror.Newf("expected %d commit(s), got %d", commitKeysToFetch, len(commits)) - } - return slicesext.Map(commits, func(commit Commit) ModuleKey { - return commit.ModuleKey() - }), nil - case DigestTypeB5: - // No need to fetch b5 digests - we've already got them stored in the module's declared dependencies. - return declaredDepModuleKeys, nil - default: - return nil, syserror.Newf("unsupported digest type: %v", digestType) - } - } + //getDeclaredDepModuleKeysB5 := func() ([]ModuleKey, error) { + // fmt.Println("Getting declared dependencies") + // if a.dependencies != nil { + // fmt.Println("Overwriting the dependencies", a.dependencies) + // return a.dependencies, nil + // } + // moduleData, err := getModuleData() + // if err != nil { + // return nil, err + // } + // declaredDepModuleKeys, err := moduleData.DeclaredDepModuleKeys() + // if err != nil { + // return nil, err + // } + // if len(declaredDepModuleKeys) == 0 { + // return nil, nil + // } + // var digestType DigestType + // for i, moduleKey := range declaredDepModuleKeys { + // digest, err := moduleKey.Digest() + // if err != nil { + // return nil, err + // } + // if i == 0 { + // digestType = digest.Type() + // } else if digestType != digest.Type() { + // return nil, syserror.Newf("multiple digest types found in DeclaredDepModuleKeys: %v, %v", digestType, digest.Type()) + // } + // } + // switch digestType { + // case DigestTypeB4: + // // The declared ModuleKey dependencies for a commit may be stored in v1 buf.lock file, + // // in which case they will use B4 digests. B4 digests aren't allowed to be used as + // // input to the B5 digest calculation, so we perform a call to convert all ModuleKeys + // // from B4 to B5 by using the commit provider. + // commitKeysToFetch := make([]CommitKey, len(declaredDepModuleKeys)) + // for i, declaredDepModuleKey := range declaredDepModuleKeys { + // commitKey, err := NewCommitKey(declaredDepModuleKey.FullName().Registry(), declaredDepModuleKey.CommitID(), DigestTypeB5) + // if err != nil { + // return nil, err + // } + // commitKeysToFetch[i] = commitKey + // } + // commits, err := commitProvider.GetCommitsForCommitKeys(ctx, commitKeysToFetch) + // if err != nil { + // return nil, err + // } + // if len(commits) != len(commitKeysToFetch) { + // return nil, syserror.Newf("expected %d commit(s), got %d", commitKeysToFetch, len(commits)) + // } + // return slicesext.Map(commits, func(commit Commit) ModuleKey { + // return commit.ModuleKey() + // }), nil + // case DigestTypeB5: + // // No need to fetch b5 digests - we've already got them stored in the module's declared dependencies. + // return declaredDepModuleKeys, nil + // default: + // return nil, syserror.Newf("unsupported digest type: %v", digestType) + // } + //} return newModule( ctx, getBucket, @@ -322,7 +342,7 @@ func (a *addedModule) ToModule( false, getV1BufYAMLObjectData, getV1BufLockObjectData, - getDeclaredDepModuleKeysB5, + //getDeclaredDepModuleKeysB5, a.remoteTargetPaths, a.remoteTargetExcludePaths, "", diff --git a/private/bufpkg/bufmodule/bufmodule_test.go b/private/bufpkg/bufmodule/bufmodule_test.go index 206c032e47..5df67d6054 100644 --- a/private/bufpkg/bufmodule/bufmodule_test.go +++ b/private/bufpkg/bufmodule/bufmodule_test.go @@ -116,7 +116,7 @@ func TestBasic(t *testing.T) { ) require.NoError(t, err) for _, moduleKey := range moduleKeys { - moduleSetBuilder.AddRemoteModule(moduleKey, false) + moduleSetBuilder.AddRemoteModule(moduleKey, false, nil) } // Next, we add the local sources. diff --git a/private/bufpkg/bufmodule/bufmoduleapi/module_data_provider.go b/private/bufpkg/bufmodule/bufmoduleapi/module_data_provider.go index 262b309727..83a05de94a 100644 --- a/private/bufpkg/bufmodule/bufmoduleapi/module_data_provider.go +++ b/private/bufpkg/bufmodule/bufmoduleapi/module_data_provider.go @@ -172,6 +172,8 @@ func (a *moduleDataProvider) getIndexedModuleDatasForRegistryAndIndexedModuleKey return depModuleKeys[i].FullName().String() < depModuleKeys[j].FullName().String() }, ) + fmt.Println("ARE WE GETTING HERE?", moduleKey) + fmt.Println("depModuleKeys", depModuleKeys) universalProtoContent, ok := commitIDToUniversalProtoContent[moduleKey.CommitID()] if !ok { diff --git a/private/bufpkg/bufmodule/module.go b/private/bufpkg/bufmodule/module.go index 4cfbdf0df9..881fd9bd74 100644 --- a/private/bufpkg/bufmodule/module.go +++ b/private/bufpkg/bufmodule/module.go @@ -230,17 +230,17 @@ func ModuleDirectModuleDeps(module Module) ([]ModuleDep, error) { type module struct { ModuleReadBucket - ctx context.Context - getBucket func() (storage.ReadBucket, error) - bucketID string - description string - moduleFullName bufparse.FullName - commitID uuid.UUID - isTarget bool - isLocal bool - getV1BufYAMLObjectData func() (ObjectData, error) - getV1BufLockObjectData func() (ObjectData, error) - getDeclaredDepModuleKeysB5 func() ([]ModuleKey, error) + ctx context.Context + getBucket func() (storage.ReadBucket, error) + bucketID string + description string + moduleFullName bufparse.FullName + commitID uuid.UUID + isTarget bool + isLocal bool + getV1BufYAMLObjectData func() (ObjectData, error) + getV1BufLockObjectData func() (ObjectData, error) + //getDeclaredDepModuleKeysB5 func() ([]ModuleKey, error) moduleSet ModuleSet @@ -261,7 +261,7 @@ func newModule( isLocal bool, getV1BufYAMLObjectData func() (ObjectData, error), getV1BufLockObjectData func() (ObjectData, error), - getDeclaredDepModuleKeysB5 func() ([]ModuleKey, error), + //getDeclaredDepModuleKeysB5 func() ([]ModuleKey, error), targetPaths []string, targetExcludePaths []string, protoFileTargetPath string, @@ -304,17 +304,17 @@ func newModule( } module := &module{ - ctx: ctx, - getBucket: syncOnceValuesGetBucketWithStorageMatcherApplied, - bucketID: bucketID, - description: description, - moduleFullName: moduleFullName, - commitID: commitID, - isTarget: isTarget, - isLocal: isLocal, - getV1BufYAMLObjectData: sync.OnceValues(getV1BufYAMLObjectData), - getV1BufLockObjectData: sync.OnceValues(getV1BufLockObjectData), - getDeclaredDepModuleKeysB5: sync.OnceValues(getDeclaredDepModuleKeysB5), + ctx: ctx, + getBucket: syncOnceValuesGetBucketWithStorageMatcherApplied, + bucketID: bucketID, + description: description, + moduleFullName: moduleFullName, + commitID: commitID, + isTarget: isTarget, + isLocal: isLocal, + getV1BufYAMLObjectData: sync.OnceValues(getV1BufYAMLObjectData), + getV1BufLockObjectData: sync.OnceValues(getV1BufLockObjectData), + //getDeclaredDepModuleKeysB5: sync.OnceValues(getDeclaredDepModuleKeysB5), } moduleReadBucket, err := newModuleReadBucketForModule( ctx, @@ -398,17 +398,17 @@ func (m *module) ModuleSet() ModuleSet { func (m *module) withIsTarget(isTarget bool) (Module, error) { // We don't just call newModule directly as we don't want to double sync.OnceValues stuff. newModule := &module{ - ctx: m.ctx, - getBucket: m.getBucket, - bucketID: m.bucketID, - description: m.description, - moduleFullName: m.moduleFullName, - commitID: m.commitID, - isTarget: isTarget, - isLocal: m.isLocal, - getV1BufYAMLObjectData: m.getV1BufYAMLObjectData, - getV1BufLockObjectData: m.getV1BufLockObjectData, - getDeclaredDepModuleKeysB5: m.getDeclaredDepModuleKeysB5, + ctx: m.ctx, + getBucket: m.getBucket, + bucketID: m.bucketID, + description: m.description, + moduleFullName: m.moduleFullName, + commitID: m.commitID, + isTarget: isTarget, + isLocal: m.isLocal, + getV1BufYAMLObjectData: m.getV1BufYAMLObjectData, + getV1BufLockObjectData: m.getV1BufLockObjectData, + //getDeclaredDepModuleKeysB5: m.getDeclaredDepModuleKeysB5, } moduleReadBucket, ok := m.ModuleReadBucket.(*moduleReadBucket) if !ok { @@ -456,30 +456,31 @@ func newGetDigestFuncForModuleAndDigestType(module *module, digestType DigestTyp if err != nil { return nil, err } - // For remote modules to have consistent B5 digests, they must not change the digests of their - // dependencies based on the local workspace. Use the pruned b5 module keys from - // ModuleData.DeclaredDepModuleKeys to calculate the digest. - if !module.isLocal { - declaredDepModuleKeys, err := module.getDeclaredDepModuleKeysB5() - if err != nil { - return nil, err - } - moduleDepFullNames := make(map[string]struct{}, len(moduleDeps)) - for _, dep := range moduleDeps { - fullName := dep.FullName() - if fullName == nil { - return nil, syserror.Newf("remote module dependencies should have full names") - } - moduleDepFullNames[fullName.String()] = struct{}{} - } - prunedDepModuleKeys := make([]ModuleKey, 0, len(declaredDepModuleKeys)) - for _, dep := range declaredDepModuleKeys { - if _, ok := moduleDepFullNames[dep.FullName().String()]; ok { - prunedDepModuleKeys = append(prunedDepModuleKeys, dep) - } - } - return getB5DigestForBucketAndDepModuleKeys(module.ctx, bucket, prunedDepModuleKeys) - } + //// For remote modules to have consistent B5 digests, they must not change the digests of their + //// dependencies based on the local workspace. Use the pruned b5 module keys from + //// ModuleData.DeclaredDepModuleKeys to calculate the digest. + //if !module.isLocal { + // fmt.Println("getDigest func !module.isLocal") + // declaredDepModuleKeys, err := module.getDeclaredDepModuleKeysB5() + // if err != nil { + // return nil, err + // } + // moduleDepFullNames := make(map[string]struct{}, len(moduleDeps)) + // for _, dep := range moduleDeps { + // fullName := dep.FullName() + // if fullName == nil { + // return nil, syserror.Newf("remote module dependencies should have full names") + // } + // moduleDepFullNames[fullName.String()] = struct{}{} + // } + // prunedDepModuleKeys := make([]ModuleKey, 0, len(declaredDepModuleKeys)) + // for _, dep := range declaredDepModuleKeys { + // if _, ok := moduleDepFullNames[dep.FullName().String()]; ok { + // prunedDepModuleKeys = append(prunedDepModuleKeys, dep) + // } + // } + // return getB5DigestForBucketAndDepModuleKeys(module.ctx, bucket, prunedDepModuleKeys) + //} return getB5DigestForBucketAndModuleDeps(module.ctx, bucket, moduleDeps) default: return nil, syserror.Newf("unknown DigestType: %v", digestType) diff --git a/private/bufpkg/bufmodule/module_data.go b/private/bufpkg/bufmodule/module_data.go index a05045b19f..dfb9282fa7 100644 --- a/private/bufpkg/bufmodule/module_data.go +++ b/private/bufpkg/bufmodule/module_data.go @@ -16,6 +16,7 @@ package bufmodule import ( "context" + "fmt" "sync" "github.com/bufbuild/buf/private/pkg/storage" @@ -112,6 +113,7 @@ func newModuleData( } moduleData.checkDigest = sync.OnceValue( func() error { + fmt.Println("moduleData.checkDigest") // We have to use the get.* functions so that we don't invoke checkDigest. bucket, err := moduleData.getBucket() if err != nil { @@ -144,6 +146,14 @@ func newModuleData( if err != nil { return err } + fmt.Println("DEPS:") + for _, dep := range declaredDepModuleKeys { + digest, err := dep.Digest() + if err != nil { + panic(err) + } + fmt.Printf("\t%s %s\n", dep.FullName(), digest) + } // This isn't the Digest as computed by the Module exactly, as the Module uses // file imports to determine what the dependencies are. However, this is checking whether // or not the digest of the returned information matches the digest we expected, which is @@ -162,6 +172,7 @@ func newModuleData( return err } } + fmt.Printf("DIGEST %t\n\t%s %s\n\t-%s\n\t+%s\n", !DigestEqual(expectedDigest, actualDigest), moduleKey.FullName(), moduleKey.CommitID().String(), expectedDigest, actualDigest) if !DigestEqual(expectedDigest, actualDigest) { return &DigestMismatchError{ FullName: moduleKey.FullName(), diff --git a/private/bufpkg/bufmodule/module_set_builder.go b/private/bufpkg/bufmodule/module_set_builder.go index 4241513469..4496330547 100644 --- a/private/bufpkg/bufmodule/module_set_builder.go +++ b/private/bufpkg/bufmodule/module_set_builder.go @@ -17,6 +17,7 @@ package bufmodule import ( "context" "errors" + "fmt" "log/slog" "sync/atomic" @@ -89,13 +90,13 @@ type ModuleSetBuilder interface { // // Remote modules are rarely targets. However, if we are reading a ModuleSet from a // ModuleProvider for example with a buf build buf.build/foo/bar call, then this - // specific Module will be targeted, while its dependencies will not be. // // Returns the same ModuleSetBuilder. AddRemoteModule( moduleKey ModuleKey, isTarget bool, + dependencies []ModuleKey, options ...RemoteModuleOption, ) ModuleSetBuilder // Build builds the Modules into a ModuleSet. @@ -137,16 +138,19 @@ func NewModuleSetForRemoteModule( options ...RemoteModuleOption, ) (ModuleSet, error) { moduleSetBuilder := NewModuleSetBuilder(ctx, logger, moduleDataProvider, commitProvider) - moduleSetBuilder.AddRemoteModule(moduleKey, true, options...) graph, err := graphProvider.GetGraphForModuleKeys(ctx, []ModuleKey{moduleKey}) if err != nil { return nil, err } if err := graph.WalkNodes( - func(node ModuleKey, _ []ModuleKey, _ []ModuleKey) error { - if node.CommitID() != moduleKey.CommitID() { - // Add the dependency ModuleKey with no path filters. - moduleSetBuilder.AddRemoteModule(node, false) + func(node ModuleKey, inbound []ModuleKey, outbound []ModuleKey) error { + fmt.Println("module_set_builder.go dep", node) + fmt.Println("\tnode", inbound, outbound) + // Add the remote module and its dependencies with with no options if it is not the target. + if isTarget := node.CommitID() == moduleKey.CommitID(); isTarget { + moduleSetBuilder.AddRemoteModule(node, isTarget, outbound, options...) + } else { + moduleSetBuilder.AddRemoteModule(node, isTarget, outbound) } return nil }, @@ -354,11 +358,11 @@ func (b *moduleSetBuilder) AddLocalModule( true, func() (ObjectData, error) { return localModuleOptions.v1BufYAMLObjectData, nil }, func() (ObjectData, error) { return localModuleOptions.v1BufLockObjectData, nil }, - func() ([]ModuleKey, error) { - // See comment in added_module.go when we construct remote Modules for why - // we have this function in the first place. - return nil, syserror.Newf("getDeclaredDepModuleKeysB5 should never be called for a local Module") - }, + //func() ([]ModuleKey, error) { + // // See comment in added_module.go when we construct remote Modules for why + // // we have this function in the first place. + // return nil, syserror.Newf("getDeclaredDepModuleKeysB5 should never be called for a local Module") + //}, localModuleOptions.targetPaths, localModuleOptions.targetExcludePaths, localModuleOptions.protoFileTargetPath, @@ -380,6 +384,7 @@ func (b *moduleSetBuilder) AddLocalModule( func (b *moduleSetBuilder) AddRemoteModule( moduleKey ModuleKey, isTarget bool, + dependencies []ModuleKey, options ...RemoteModuleOption, ) ModuleSetBuilder { if b.buildCalled.Load() { @@ -400,6 +405,7 @@ func (b *moduleSetBuilder) AddRemoteModule( remoteModuleOptions.targetPaths, remoteModuleOptions.targetExcludePaths, isTarget, + dependencies, ), ) return b