From 1729ad3e3cf2958c942efa85d6e5b6740131514d Mon Sep 17 00:00:00 2001 From: Doria Keung Date: Tue, 3 Dec 2024 15:39:28 -0500 Subject: [PATCH] Use ordered set for imports to iterate deterministically when filtering an image (#3507) --- CHANGELOG.md | 2 + .../bufimage/bufimageutil/bufimageutil.go | 79 ++++++++++++++++--- 2 files changed, 71 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fcd8101d81..8579493b7b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ - Add `buf registry plugin {create,delete,info,update}` commands to manage BSR plugins. - Breaking analysis support for `buf beta lsp`. +- Fix bug when using the `--type` flag filter for `buf build` where import ordering is not + determinisitic. ## [v1.47.2] - 2024-11-14 diff --git a/private/bufpkg/bufimage/bufimageutil/bufimageutil.go b/private/bufpkg/bufimage/bufimageutil/bufimageutil.go index 414dbb7ef2..c37338791e 100644 --- a/private/bufpkg/bufimage/bufimageutil/bufimageutil.go +++ b/private/bufpkg/bufimage/bufimageutil/bufimageutil.go @@ -266,16 +266,20 @@ func ImageFilteredByTypesWithOptions(image bufimage.Image, types []string, opts // the file's WeakDependency field. indexFromTo := make(map[int32]int32) indexTo := 0 + // Only handle imports and dependencies if there are any. for indexFrom, importPath := range imageFileDescriptor.GetDependency() { path := append(basePath, int32(indexFrom)) - if _, ok := importsRequired[importPath]; ok { + // We check if the import path exists among required imports. If yes, we + // move and then delete from required imports as we go. + if importsRequired != nil && importsRequired.index(importPath) != -1 { sourcePathRemapper.markMoved(path, int32(indexTo)) indexFromTo[int32(indexFrom)] = int32(indexTo) imageFileDescriptor.Dependency[indexTo] = importPath indexTo++ - // markDeleted them as we go, so we know which ones weren't in the list - delete(importsRequired, importPath) + // delete them as we go, so we know which ones weren't in the list + importsRequired.delete(importPath) } else { + // Path did not exist in required imports, we mark as deleted. sourcePathRemapper.markDeleted(path) } } @@ -284,9 +288,12 @@ func ImageFilteredByTypesWithOptions(image bufimage.Image, types []string, opts // Add any other imports (which may not have been in the list because // they were picked up via a public import). The filtered files will not // use public imports. - for importPath := range importsRequired { - imageFileDescriptor.Dependency = append(imageFileDescriptor.Dependency, importPath) + // The imports are added in the order they are encountered when importing + // to maintain a deterministic ordering. + if importsRequired != nil { + imageFileDescriptor.Dependency = append(imageFileDescriptor.Dependency, importsRequired.keys()...) } + imageFileDescriptor.PublicDependency = nil sourcePathRemapper.markDeleted([]int32{filePublicDependencyTag}) @@ -458,9 +465,9 @@ type transitiveClosure struct { // entire package being included). The above fields are used to filter the // contents of files. But files named in this set will not be filtered. completeFiles map[string]struct{} - // The set of imports for each file. This allows for re-writing imports + // The ordered set of imports for each file. This allows for re-writing imports // for files whose contents have been pruned. - imports map[string]map[string]struct{} + imports map[string]*orderedImports } type closureInclusionMode int @@ -485,7 +492,7 @@ func newTransitiveClosure() *transitiveClosure { elements: map[namedDescriptor]closureInclusionMode{}, files: map[string]struct{}{}, completeFiles: map[string]struct{}{}, - imports: map[string]map[string]struct{}{}, + imports: map[string]*orderedImports{}, } } @@ -495,10 +502,10 @@ func (t *transitiveClosure) addImport(fromPath, toPath string) { } imps := t.imports[fromPath] if imps == nil { - imps = map[string]struct{}{} + imps = newOrderedImports() t.imports[fromPath] = imps } - imps[toPath] = struct{}{} + imps.add(toPath) } func (t *transitiveClosure) addFile(file string, imageIndex *imageIndex, opts *imageFilterOptions) error { @@ -1000,3 +1007,55 @@ func stripSourceRetentionOptionsFromFile(imageFile bufimage.ImageFile) (bufimage imageFile.UnusedDependencyIndexes(), ) } + +// orderedImports is a structure to maintain an ordered set of imports. This is needed +// because we want to be able to iterate through imports in a deterministic way when filtering +// the image. +type orderedImports struct { + pathToIndex map[string]int + paths []string +} + +// newOrderedImports creates a new orderedImports structure. +func newOrderedImports() *orderedImports { + return &orderedImports{ + pathToIndex: map[string]int{}, + } +} + +// index returns the index for a given path. If the path does not exist in index map, -1 +// is returned and should be considered deleted. +func (o *orderedImports) index(path string) int { + if index, ok := o.pathToIndex[path]; ok { + return index + } + return -1 +} + +// add appends a path to the paths list and the index in the map. If a key already exists, +// then this is a no-op. +func (o *orderedImports) add(path string) { + if _, ok := o.pathToIndex[path]; !ok { + o.pathToIndex[path] = len(o.paths) + o.paths = append(o.paths, path) + } +} + +// delete removes a key from the index map of ordered imports. If a non-existent path is +// set for deletion, then this is a no-op. +// Note that the path is not removed from the paths list. If you want to iterate through +// the paths, use keys() to get all non-deleted keys. +func (o *orderedImports) delete(path string) { + delete(o.pathToIndex, path) +} + +// keys provides all non-deleted keys from the ordered imports. +func (o *orderedImports) keys() []string { + keys := make([]string, 0, len(o.pathToIndex)) + for _, path := range o.paths { + if _, ok := o.pathToIndex[path]; ok { + keys = append(keys, path) + } + } + return keys +}