diff --git a/CHANGELOG.md b/CHANGELOG.md index fb9a7e0d96..9cbdfb9b37 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,8 @@ - Update `buf push` to push the license file or doc file (e.g. `README.md`, `LICENSE`) in the same directory as `buf.yaml` if a module does not have a license file or doc file in the module's directory. +- Fix constraints of `--path` flag for lint and breaking rules to avoid resolving all files + within a module. This change can result in a performance improvement for large workspaces. ## [v1.37.0] - 2024-08-16 diff --git a/private/buf/bufctl/controller.go b/private/buf/bufctl/controller.go index 503ca76aa2..363f94bdba 100644 --- a/private/buf/bufctl/controller.go +++ b/private/buf/bufctl/controller.go @@ -1010,6 +1010,8 @@ func (c *controller) buildImage( return filterImage(image, functionOptions, true) } +// buildTargetImageWithConfigs builds an image for each module in the workspace. +// This is used to associate LintConfig and BreakingConfig on a per-module basis. func (c *controller) buildTargetImageWithConfigs( ctx context.Context, workspace bufworkspace.Workspace, @@ -1029,14 +1031,12 @@ func (c *controller) buildTargetImageWithConfigs( if err != nil { return nil, err } - module := moduleSet.GetModuleForOpaqueID(opaqueID) - if module == nil { - return nil, syserror.Newf("new ModuleSet from WithTargetOpaqueIDs did not have opaqueID %q", opaqueID) - } - moduleReadBucket, err := bufmodule.ModuleToSelfContainedModuleReadBucketWithOnlyProtoFiles(module) - if err != nil { - return nil, err - } + // The moduleReadBucket may include more modules than the target module + // and its dependencies. This is because the moduleSet is constructed from + // the workspace. Targeting the module does not remove non-related modules. + // Build image will use the target info to build the image for the specific + // module. Non-targeted modules will not be included in the image. + moduleReadBucket := bufmodule.ModuleSetToModuleReadBucketWithOnlyProtoFiles(moduleSet) targetFileInfos, err := bufmodule.GetTargetFileInfos(ctx, moduleReadBucket) if err != nil { return nil, err diff --git a/private/buf/cmd/buf/buf_test.go b/private/buf/cmd/buf/buf_test.go index d0b0e94854..3c1ef89f2b 100644 --- a/private/buf/cmd/buf/buf_test.go +++ b/private/buf/cmd/buf/buf_test.go @@ -294,10 +294,15 @@ func TestFail7(t *testing.T) { t, nil, bufctl.ExitCodeFileAnnotation, - "", // stdout is empty + filepath.FromSlash(`testdata/fail/buf/buf.proto:3:1:Files with package "other" must be within a directory "other" relative to root but were in directory "fail/buf". +testdata/fail/buf/buf.proto:6:9:Field name "oneTwo" should be lower_snake_case, such as "one_two".`), // This is new behavior we introduced. When setting a config override, we no longer do // a search for the controlling workspace. See bufctl/option.go for additional details. - filepath.FromSlash(`Failure: testdata/export/other/proto/unimported.proto: import "another.proto": file does not exist`), + // Only the paths specified by "--path" in the command are considered. This avoids build + // failures from other proto files under testdata. Command "build" succeeds with this path + // restriction, "lint" should be able to build the image and only fail on lint issue for + // the specified file. + "", "lint", "--path", filepath.Join("testdata", "fail", "buf", "buf.proto"), diff --git a/private/buf/cmd/buf/workspace_test.go b/private/buf/cmd/buf/workspace_test.go index d801fbd85e..c188be3685 100644 --- a/private/buf/cmd/buf/workspace_test.go +++ b/private/buf/cmd/buf/workspace_test.go @@ -145,8 +145,8 @@ func TestWorkspaceDir(t *testing.T) { t, nil, bufctl.ExitCodeFileAnnotation, + filepath.FromSlash(`testdata/workspace/success/`+baseDirPath+`/proto/rpc.proto:5:8:import "request.proto": file does not exist`), "", - filepath.FromSlash(`Failure: testdata/workspace/success/`+baseDirPath+`/proto/rpc.proto: import "request.proto": file does not exist`), "lint", filepath.Join("testdata", "workspace", "success", baseDirPath), "--config", @@ -156,8 +156,8 @@ func TestWorkspaceDir(t *testing.T) { t, nil, bufctl.ExitCodeFileAnnotation, + filepath.FromSlash(`testdata/workspace/success/`+baseDirPath+`/proto/rpc.proto:5:8:import "request.proto": file does not exist`), "", - filepath.FromSlash(`Failure: testdata/workspace/success/`+baseDirPath+`/proto/rpc.proto: import "request.proto": file does not exist`), "lint", filepath.Join("testdata", "workspace", "success", baseDirPath), "--config", @@ -392,8 +392,8 @@ func TestWorkspaceDetached(t *testing.T) { t, nil, bufctl.ExitCodeFileAnnotation, + filepath.FromSlash(`testdata/workspace/success/`+dirPath+`/proto/rpc.proto:5:8:import "request.proto": file does not exist`), ``, - filepath.FromSlash(`Failure: testdata/workspace/success/`+dirPath+`/proto/rpc.proto: import "request.proto": file does not exist`), "lint", filepath.Join("testdata", "workspace", "success", dirPath, "proto"), ) diff --git a/private/bufpkg/bufcheck/buflint/buflint_test.go b/private/bufpkg/bufcheck/buflint/buflint_test.go index fdc24de1cb..b24483c83d 100644 --- a/private/bufpkg/bufcheck/buflint/buflint_test.go +++ b/private/bufpkg/bufcheck/buflint/buflint_test.go @@ -1243,10 +1243,7 @@ func testLintWithOptions( // build the image for the specified module string (opaqueID) moduleSet, err := workspace.WithTargetOpaqueIDs(opaqueID) require.NoError(t, err) - module := moduleSet.GetModuleForOpaqueID(opaqueID) - require.NotNil(t, module) - moduleReadBucket, err := bufmodule.ModuleToSelfContainedModuleReadBucketWithOnlyProtoFiles(module) - require.NoError(t, err) + moduleReadBucket := bufmodule.ModuleSetToModuleReadBucketWithOnlyProtoFiles(moduleSet) image, err := bufimage.BuildImage( ctx, tracing.NopTracer, diff --git a/private/bufpkg/bufimage/bufimage.go b/private/bufpkg/bufimage/bufimage.go index ce9298a275..dc55ca53f4 100644 --- a/private/bufpkg/bufimage/bufimage.go +++ b/private/bufpkg/bufimage/bufimage.go @@ -262,9 +262,11 @@ func NewImage(imageFiles []ImageFile) (Image, error) { // The given ModuleReadBucket must be self-contained. // // A ModuleReadBucket is self-contained if it was constructed from -// ModuleSetToModuleReadBucketWithOnlyProtoFiles or -// ModuleToSelfContainedModuleReadBucketWithOnlyProtoFiles. These are likely -// the only two ways you should have a ModuleReadBucket that you pass to BuildImage. +// ModuleSetToModuleReadBucketWithOnlyProtoFiles. This is likely the only way you +// should have a ModuleReadBucket that you pass to BuildImage. +// +// The image will contain only files targeted by the input moduleReadBucket, +// those returned by [bufmodule.GetTargetFileInfos]. func BuildImage( ctx context.Context, tracer tracing.Tracer, diff --git a/private/bufpkg/bufimage/build_image_test.go b/private/bufpkg/bufimage/build_image_test.go index b52ce6e236..5084df31ea 100644 --- a/private/bufpkg/bufimage/build_image_test.go +++ b/private/bufpkg/bufimage/build_image_test.go @@ -318,6 +318,57 @@ func TestCompareSemicolons(t *testing.T) { testCompare(t, runner, "semicolons") } +func TestModuleTargetFiles(t *testing.T) { + t.Parallel() + moduleSet, err := bufmoduletesting.NewModuleSet( + bufmoduletesting.ModuleData{ + Name: "buf.build/foo/a", + PathToData: map[string][]byte{ + "a.proto": []byte( + `syntax = "proto3"; package a; import "b.proto";`, + ), + }, + }, + bufmoduletesting.ModuleData{ + Name: "buf.build/foo/b", + PathToData: map[string][]byte{ + "b.proto": []byte( + `syntax = "proto3"; package b; import "c.proto";`, + ), + }, + }, + bufmoduletesting.ModuleData{ + Name: "buf.build/foo/c", + PathToData: map[string][]byte{ + "c.proto": []byte( + `syntax = "proto3"; package c;`, + ), + }, + }, + ) + require.NoError(t, err) + testTagetImageFiles := func(t *testing.T, want []string, opaqueID ...string) { + targetModuleSet := moduleSet + if len(opaqueID) > 0 { + var err error + targetModuleSet, err = moduleSet.WithTargetOpaqueIDs(opaqueID...) + require.NoError(t, err) + } + image, err := bufimage.BuildImage( + context.Background(), + tracing.NopTracer, + bufmodule.ModuleSetToModuleReadBucketWithOnlyProtoFiles(targetModuleSet), + ) + require.NoError(t, err) + assert.Equal(t, want, testGetImageFilePaths(image)) + } + testTagetImageFiles(t, []string{"a.proto", "b.proto", "c.proto"}) + testTagetImageFiles(t, []string{"a.proto", "b.proto", "c.proto"}, "buf.build/foo/a") + testTagetImageFiles(t, []string{"b.proto", "c.proto"}, "buf.build/foo/b") + testTagetImageFiles(t, []string{"c.proto"}, "buf.build/foo/c") + testTagetImageFiles(t, []string{"b.proto", "c.proto"}, "buf.build/foo/b", "buf.build/foo/c") +} + func testCompare(t *testing.T, runner command.Runner, relDirPath string) { dirPath := filepath.Join("testdata", relDirPath) image, fileAnnotations := testBuild(t, false, dirPath, false) diff --git a/private/bufpkg/bufmodule/module.go b/private/bufpkg/bufmodule/module.go index a30ca5efcc..fae6484881 100644 --- a/private/bufpkg/bufmodule/module.go +++ b/private/bufpkg/bufmodule/module.go @@ -196,32 +196,6 @@ func ModuleToModuleKey(module Module, digestType DigestType) (ModuleKey, error) ) } -// ModuleToSelfContainedModuleReadBucketWithOnlyProtoFiles converts the Module to a -// ModuleReadBucket that contains all the .proto files of the Module and its dependencies. -// -// Targeting information will remain the same. Note that this means that the result ModuleReadBucket -// may have no target files! This can occur when path filtering was applied, but the path filters did -// not match any files in the Module, and none of the Module's files were targeted. -// It can also happen if the Module nor any of its dependencies were targeted. -// -// *** THIS IS PROBABLY NOT THE FUNCTION YOU ARE LOOKING FOR. *** You probably want -// ModuleSetToModuleReadBucketWithOnlyProtoFiles to convert a ModuleSet/Workspace to a -// ModuleReadBucket. This function is used for cases where we want to create an Image -// specifically for one Module, such as when we need to associate LintConfig and BreakingConfig -// on a per-Module basis for buf lint and buf breaking. See bufctl.Controller, which is likely -// the only place this should be used outside of testing. -func ModuleToSelfContainedModuleReadBucketWithOnlyProtoFiles(module Module) (ModuleReadBucket, error) { - modules := []Module{module} - moduleDeps, err := module.ModuleDeps() - if err != nil { - return nil, err - } - for _, moduleDep := range moduleDeps { - modules = append(modules, moduleDep) - } - return newMultiProtoFileModuleReadBucket(modules, true), nil -} - // ModuleDirectModuleDeps is a convenience function that returns only the direct dependencies of the Module. func ModuleDirectModuleDeps(module Module) ([]ModuleDep, error) { moduleDeps, err := module.ModuleDeps() diff --git a/private/bufpkg/bufmodule/module_read_bucket.go b/private/bufpkg/bufmodule/module_read_bucket.go index 4c346b4e32..0d1076c036 100644 --- a/private/bufpkg/bufmodule/module_read_bucket.go +++ b/private/bufpkg/bufmodule/module_read_bucket.go @@ -73,8 +73,7 @@ type ModuleReadBucket interface { // being self-contained. // // A ModuleReadBucket is self-contained if it was constructed from - // ModuleSetToModuleReadBucketWithOnlyProtoFiles or - // ModuleToSelfContainedModuleReadBucketWithOnlyProtoFiles. + // ModuleSetToModuleReadBucketWithOnlyProtoFiles. // // A ModuleReadBucket as inherited from a Module is not self-contained. //