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.

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

Use ordered set for imports to iterate deterministically when filtering an image #3507

Merged
merged 7 commits into from
Dec 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
79 changes: 69 additions & 10 deletions private/bufpkg/bufimage/bufimageutil/bufimageutil.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
Expand All @@ -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()...)
}

emcfarlane marked this conversation as resolved.
Show resolved Hide resolved
imageFileDescriptor.PublicDependency = nil
sourcePathRemapper.markDeleted([]int32{filePublicDependencyTag})

Expand Down Expand Up @@ -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
Expand All @@ -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{},
}
}

Expand All @@ -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 {
Expand Down Expand Up @@ -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
emcfarlane marked this conversation as resolved.
Show resolved Hide resolved
}
Loading