From 7df2f0922f3d04ab5e93590e1067ae9ccfa1d008 Mon Sep 17 00:00:00 2001 From: bufdev <4228796+bufdev@users.noreply.github.com> Date: Thu, 29 Jun 2023 11:33:59 -0400 Subject: [PATCH] Add bufimage.ImageModuleDependencies (#2235) --- private/bufpkg/bufimage/bufimage.go | 95 +++++++++++++++++++ .../bufimage/bufimagebuild/bufimagebuild.go | 6 ++ .../bufpkg/bufimage/bufimagebuild/builder.go | 3 + private/bufpkg/bufimage/image.go | 12 +++ .../bufimage/image_module_dependency.go | 61 ++++++++++++ private/bufpkg/bufimage/util.go | 67 +++++++++++++ 6 files changed, 244 insertions(+) create mode 100644 private/bufpkg/bufimage/image_module_dependency.go diff --git a/private/bufpkg/bufimage/bufimage.go b/private/bufpkg/bufimage/bufimage.go index 24edb1d8df..f97085b080 100644 --- a/private/bufpkg/bufimage/bufimage.go +++ b/private/bufpkg/bufimage/bufimage.go @@ -87,6 +87,9 @@ type Image interface { // // This contains all files, including imports if available. // The returned files are in correct DAG order. + // + // All files that have the same ModuleIdentity will also have the same commit, or no commit. + // This is enforced at construction time. Files() []ImageFile // GetFile gets the file for the root relative file path. // @@ -467,6 +470,98 @@ func ProtoImageToFileDescriptors(protoImage *imagev1.Image) []protodescriptor.Fi return protoImageFilesToFileDescriptors(protoImage.File) } +// ImageDependency is a dependency of an image. +// +// This could conceivably be part of ImageFile or bufmoduleref.FileInfo. +// For ImageFile, this would be a field that is ignored when translated to proto, +// and is calculated on creation from proto. IsImport would become ImportType. +// You could go a step further and make this optionally part of the proto definition. +// +// You could even go down to bufmoduleref.FileInfo if you used the AST, but this +// could be error prone. +// +// However, for simplicity now (and to not rewrite the whole codebase), we make +// this a separate type that is calculated off of an Image after the fact. +// +// If this became part of ImageFile or bufmoduleref.FileInfo, you would get +// all the ImageDependencies from the ImageFiles, and then sort | uniq them +// to get the ImageDependencies for an Image. This would remove the requirement +// of this associated type to have a ModuleIdentity and commit, so in +// the IsDirect example below, d.proto would not be "ignored" - it would +// be an ImageFile like any other, with ImportType DIRECT. +// +// Note that if we ever do this, there is validation in newImage that enforces +// that all ImageFiles with the same ModuleIdentity have the same commit. This +// validation will likely have to be moved around. +type ImageModuleDependency interface { + // String() returns remote/owner/repository[:commit]. + fmt.Stringer + + // Required. Will never be nil. + ModuleIdentity() bufmoduleref.ModuleIdentity + // Optional. May be empty. + Commit() string + + // IsDirect returns true if the dependency is a direct dependency. + // + // A dependency is direct if it is only an import of non-imports in the image. + // + // Example: + // + // a.proto, module buf.build/foo/a, is non-import, imports b.proto + // b.proto, module buf.build/foo/b, is import, imports c.proto + // c.proto, module buf.build/foo/c, is import + // + // In this case, the list would contain only buf.build/foo/b, as buf.build/foo/a + // for a.proto is a non-import, and buf.build/foo/c for c.proto is only imported + // by an import + IsDirect() bool + + isImageModuleDependency() +} + +// ImageModuleDependency returns all ImageModuleDependencies for the Image. +// +// Does not return any ImageModuleDependencies for non-imports, that is the +// ModuleIdentities and commits represented by non-imports are not represented +// in this list. +func ImageModuleDependencies(image Image) []ImageModuleDependency { + importsOfNonImports := make(map[string]struct{}) + for _, imageFile := range image.Files() { + if !imageFile.IsImport() { + for _, dependency := range imageFile.FileDescriptor().GetDependency() { + importsOfNonImports[dependency] = struct{}{} + } + } + } + // We know that all ImageFiles with the same ModuleIdentity + // have the same commit or no commit, so using String() will properly identify + // unique dependencies. + stringToImageModuleDependency := make(map[string]ImageModuleDependency) + for _, imageFile := range image.Files() { + if imageFile.IsImport() { + if moduleIdentity := imageFile.ModuleIdentity(); moduleIdentity != nil { + _, isDirect := importsOfNonImports[imageFile.Path()] + imageModuleDependency := newImageModuleDependency( + moduleIdentity, + imageFile.Commit(), + isDirect, + ) + stringToImageModuleDependency[imageModuleDependency.String()] = imageModuleDependency + } + } + } + imageModuleDependencies := make([]ImageModuleDependency, 0, len(stringToImageModuleDependency)) + for _, imageModuleDependency := range stringToImageModuleDependency { + imageModuleDependencies = append( + imageModuleDependencies, + imageModuleDependency, + ) + } + sortImageModuleDependencies(imageModuleDependencies) + return imageModuleDependencies +} + type newImageForProtoOptions struct { noReparse bool computeUnusedImports bool diff --git a/private/bufpkg/bufimage/bufimagebuild/bufimagebuild.go b/private/bufpkg/bufimage/bufimagebuild/bufimagebuild.go index aab3728c7c..1d4ceb882a 100644 --- a/private/bufpkg/bufimage/bufimagebuild/bufimagebuild.go +++ b/private/bufpkg/bufimage/bufimagebuild/bufimagebuild.go @@ -67,6 +67,12 @@ func WithExpectedDirectDependencies(expectedDirectDependencies []bufmoduleref.Mo } // WithWorkspace sets the workspace to be read from instead of ModuleReader, and to not warn imports for. +// +// TODO: this can probably be dealt with by finding out if an ImageFile has a commit +// or not, although that is hacky, that's an implementation detail in practice, but perhaps +// we could justify it - transitive dependencies without commits don't make sense? +// +// TODO: shouldn't buf.yamls in workspaces have deps properly declared in them anyways? Why not warn? func WithWorkspace(workspace bufmodule.Workspace) BuildOption { return func(buildOptions *buildOptions) { buildOptions.workspace = workspace diff --git a/private/bufpkg/bufimage/bufimagebuild/builder.go b/private/bufpkg/bufimage/bufimagebuild/builder.go index 728d3fa094..684bf3a3bb 100644 --- a/private/bufpkg/bufimage/bufimagebuild/builder.go +++ b/private/bufpkg/bufimage/bufimagebuild/builder.go @@ -163,6 +163,9 @@ func (b *builder) build( // warnInvalidImports checks that all the target image files have valid imports statements that // point to files in the local module, in a direct dependency, or in a workspace local unnamed // module. It outputs WARN messages otherwise, one per invalid import statement. +// +// TODO: Understand this code before doing anything +// TODO: switch to use bufimage.ImageModuleDependencies func (b *builder) warnInvalidImports( ctx context.Context, builtImage bufimage.Image, diff --git a/private/bufpkg/bufimage/image.go b/private/bufpkg/bufimage/image.go index 6c17741e24..5ed7f0a11d 100644 --- a/private/bufpkg/bufimage/image.go +++ b/private/bufpkg/bufimage/image.go @@ -31,12 +31,24 @@ func newImage(files []ImageFile, reorder bool) (*image, error) { return nil, errors.New("image contains no files") } pathToImageFile := make(map[string]ImageFile, len(files)) + identityStringToCommit := make(map[string]string) for _, file := range files { path := file.Path() if _, ok := pathToImageFile[path]; ok { return nil, fmt.Errorf("duplicate file: %s", path) } pathToImageFile[path] = file + if moduleIdentity := file.ModuleIdentity(); moduleIdentity != nil { + identityString := moduleIdentity.IdentityString() + existingCommit, ok := identityStringToCommit[identityString] + if ok { + if existingCommit != file.Commit() { + return nil, fmt.Errorf("image had two different commits for the same module: %q and %q", existingCommit, file.Commit()) + } + } else { + identityStringToCommit[identityString] = file.Commit() + } + } } if reorder { files = orderImageFiles(files, pathToImageFile) diff --git a/private/bufpkg/bufimage/image_module_dependency.go b/private/bufpkg/bufimage/image_module_dependency.go new file mode 100644 index 0000000000..9451e15c46 --- /dev/null +++ b/private/bufpkg/bufimage/image_module_dependency.go @@ -0,0 +1,61 @@ +// Copyright 2020-2023 Buf Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package bufimage + +import ( + "github.com/bufbuild/buf/private/bufpkg/bufmodule/bufmoduleref" +) + +var _ ImageModuleDependency = &imageModuleDependency{} + +type imageModuleDependency struct { + moduleIdentity bufmoduleref.ModuleIdentity + commit string + isDirect bool +} + +func newImageModuleDependency( + moduleIdentity bufmoduleref.ModuleIdentity, + commit string, + isDirect bool, +) *imageModuleDependency { + return &imageModuleDependency{ + moduleIdentity: moduleIdentity, + commit: commit, + isDirect: isDirect, + } +} + +func (i *imageModuleDependency) ModuleIdentity() bufmoduleref.ModuleIdentity { + return i.moduleIdentity +} + +func (i *imageModuleDependency) Commit() string { + return i.commit +} + +func (i *imageModuleDependency) IsDirect() bool { + return i.isDirect +} + +func (i *imageModuleDependency) String() string { + moduleIdentityString := i.moduleIdentity.IdentityString() + if i.commit != "" { + return moduleIdentityString + ":" + i.commit + } + return moduleIdentityString +} + +func (*imageModuleDependency) isImageModuleDependency() {} diff --git a/private/bufpkg/bufimage/util.go b/private/bufpkg/bufimage/util.go index d46ca11a4b..bea4309f98 100644 --- a/private/bufpkg/bufimage/util.go +++ b/private/bufpkg/bufimage/util.go @@ -17,6 +17,7 @@ package bufimage import ( "errors" "fmt" + "sort" "github.com/bufbuild/buf/private/bufpkg/bufmodule/bufmoduleref" "github.com/bufbuild/buf/private/gen/data/datawkt" @@ -499,3 +500,69 @@ func isFileToGenerate( } return true } + +func sortImageModuleDependencies(imageModuleDependencies []ImageModuleDependency) { + sort.Slice(imageModuleDependencies, func(i, j int) bool { + return imageModuleDependencyLess(imageModuleDependencies[i], imageModuleDependencies[j]) + }) +} + +func imageModuleDependencyLess(a ImageModuleDependency, b ImageModuleDependency) bool { + return imageModuleDependencyCompareTo(a, b) < 0 +} + +// return -1 if less +// return 1 if greater +// return 0 if equal +func imageModuleDependencyCompareTo(a ImageModuleDependency, b ImageModuleDependency) int { + if a == nil && b == nil { + return 0 + } + if a == nil && b != nil { + return -1 + } + if a != nil && b == nil { + return 1 + } + aModuleIdentity := a.ModuleIdentity() + bModuleIdentity := b.ModuleIdentity() + if aModuleIdentity != nil || bModuleIdentity != nil { + if aModuleIdentity == nil && bModuleIdentity != nil { + return -1 + } + if aModuleIdentity != nil && bModuleIdentity == nil { + return 1 + } + if aModuleIdentity.Remote() < bModuleIdentity.Remote() { + return -1 + } + if aModuleIdentity.Remote() > bModuleIdentity.Remote() { + return 1 + } + if aModuleIdentity.Owner() < bModuleIdentity.Owner() { + return -1 + } + if aModuleIdentity.Owner() > bModuleIdentity.Owner() { + return 1 + } + if aModuleIdentity.Repository() < bModuleIdentity.Repository() { + return -1 + } + if aModuleIdentity.Repository() > bModuleIdentity.Repository() { + return 1 + } + } + if a.Commit() < b.Commit() { + return -1 + } + if a.Commit() > b.Commit() { + return 1 + } + if a.IsDirect() && !b.IsDirect() { + return -1 + } + if !a.IsDirect() && b.IsDirect() { + return 1 + } + return 0 +}