From f8780ab36230cbb3fbd8c4d52403b038e2fdc9d1 Mon Sep 17 00:00:00 2001 From: Keith Smiley Date: Tue, 11 Aug 2026 10:39:59 -0700 Subject: [PATCH 1/2] Add native -exclude to buildifier This gives cross platform consistency instead of relying on shell / bat scripts that might differ depending on the tools you call. --- buildifier/README.md | 9 ++++ buildifier/buildifier.go | 2 +- buildifier/config/config.go | 3 ++ buildifier/config/config_test.go | 7 ++++ buildifier/integration_test.sh | 6 ++- buildifier/utils/utils.go | 58 +++++++++++++++++++++++--- buildifier/utils/utils_test.go | 71 ++++++++++++++++++++++++++++++++ 7 files changed, 148 insertions(+), 8 deletions(-) diff --git a/buildifier/README.md b/buildifier/README.md index b3a3a7c93..94de3f4ce 100644 --- a/buildifier/README.md +++ b/buildifier/README.md @@ -32,6 +32,15 @@ in a directory recursively: buildifier -r path/to/dir ``` +Paths can be excluded from recursive discovery with repeatable `-exclude` +patterns. Patterns use Go's `path.Match` syntax, are matched against the full +traversed path, allow `*` and `?` to match path separators, and use +forward-slash separators on every platform: + +```bash +buildifier -r -exclude='path/to/dir/vendor/*' path/to/dir +``` + Buildifier supports the following file types: `BUILD`, `WORKSPACE`, `.bzl`, and default, the latter is reserved for Starlark files buildifier doesn't know about (e.g. configuration files for third-party projects that use Starlark). The diff --git a/buildifier/buildifier.go b/buildifier/buildifier.go index 276c04d0a..572643941 100644 --- a/buildifier/buildifier.go +++ b/buildifier/buildifier.go @@ -192,7 +192,7 @@ func (b *buildifier) run(args []string) int { files := args if b.config.Recursive { var err error - files, err = utils.ExpandDirectories(&args) + files, err = utils.ExpandDirectories(&args, b.config.Exclude...) if err != nil { fmt.Fprintf(os.Stderr, "buildifier: %v\n", err) return 3 diff --git a/buildifier/config/config.go b/buildifier/config/config.go index 4efa1914d..11308a23b 100644 --- a/buildifier/config/config.go +++ b/buildifier/config/config.go @@ -111,6 +111,8 @@ type Config struct { WarningsList []string `json:"warningsList,omitempty"` // Recursive instructs buildifier to find starlark files recursively Recursive bool `json:"recursive,omitempty"` + // Exclude contains path patterns to skip during recursive file discovery + Exclude ArrayFlags `json:"exclude,omitempty"` // Verbose instructs buildifier to output verbose diagnostics Verbose bool `json:"verbose,omitempty"` // DiffCommand is the command to run when the formatting mode is diff @@ -177,6 +179,7 @@ func (c *Config) FlagSet(name string, errorHandling flag.ErrorHandling) *flag.Fl flags.BoolVar(&c.Verbose, "v", c.Verbose, "print verbose information to standard error") flags.BoolVar(&c.DiffMode, "d", c.DiffMode, "alias for -mode=diff") flags.BoolVar(&c.Recursive, "r", c.Recursive, "find starlark files recursively") + flags.Var(&c.Exclude, "exclude", "path pattern to exclude when finding starlark files recursively") flags.BoolVar(&c.MultiDiff, "multi_diff", c.MultiDiff, "the command specified by the -diff_command flag can diff multiple files in the style of tkdiff (default false)") flags.StringVar(&c.Mode, "mode", c.Mode, "formatting mode: check, diff, or fix (default fix)") flags.StringVar(&c.Format, "format", c.Format, "diagnostics format: text, json or sarif (default text)") diff --git a/buildifier/config/config_test.go b/buildifier/config/config_test.go index c73643338..67585f7d4 100644 --- a/buildifier/config/config_test.go +++ b/buildifier/config/config_test.go @@ -160,6 +160,7 @@ func ExampleFlagSet() { // d: alias for -mode=diff ("false") // diff_command: command to run when the formatting mode is diff (default uses the BUILDIFIER_DIFF, BUILDIFIER_MULTIDIFF, and DISPLAY environment variables to create the diff command) ("") // disable_symlink_safety: per default Buildifier will not write to symlinks pointing outside of the Bazel workspace. Setting this to true will disable this behavior ("false") + // exclude: path pattern to exclude when finding starlark files recursively ("") // format: diagnostics format: text, json or sarif (default text) ("") // help: print usage information ("false") // lint: lint mode: off, warn, or fix (default off) ("") @@ -186,6 +187,8 @@ func ExampleFlagSet_parse() { "-d", "--diff_command=diff", "--disable_symlink_safety=true", + "--exclude=vendor/*", + "--exclude=third_party/*", "--format=json", "--help", "--lint=fix", @@ -215,6 +218,10 @@ func ExampleFlagSet_parse() { // "lint": "fix", // "warnings": "+print,-no-effect", // "recursive": true, + // "exclude": [ + // "vendor/*", + // "third_party/*" + // ], // "verbose": true, // "diffCommand": "diff", // "multiDiff": true, diff --git a/buildifier/integration_test.sh b/buildifier/integration_test.sh index e7ae2e764..2d789f60a 100755 --- a/buildifier/integration_test.sh +++ b/buildifier/integration_test.sh @@ -47,6 +47,8 @@ echo -e '{ "type": "build" }' > test_dir/.buildifier.test.json # demonstrate con mkdir test_dir/workspace # name of a starlark file, but a directory mkdir test_dir/.git # contents should be ignored echo -e "a+b" > test_dir/.git/git.bzl +mkdir -p test_dir/excluded/nested +echo -e "$INPUT" > test_dir/excluded/nested/excluded.bzl cat > test_dir/MODULE.bazel <<'EOF' module(name='my-module',version='1.0',compatibility_level=1) include("cpp.MODULE.bazel") @@ -134,9 +136,10 @@ EOF cp test_dir/foo.bar golden/foo.bar cp test_dir/subdir/build golden/build cp test_dir/.git/git.bzl golden/git.bzl +cp test_dir/excluded/nested/excluded.bzl golden/excluded.bzl "$buildifier" < test_dir/BUILD > stdout -"$buildifier" -r test_dir +"$buildifier" -r --exclude='test_dir/excluded/*' test_dir "$buildifier" test.bzl "$buildifier" --path=foo.bzl test2.bzl "$buildifier" --config=test_dir/.buildifier.test.json < test_dir/test.bzl > test_dir/test.bzl.BUILD.out @@ -390,6 +393,7 @@ diff -u test.bzl golden/test.bzl.golden diff -u test2.bzl golden/test.bzl.golden diff -u stdout golden/test.bzl.golden diff -u test_dir/.git/git.bzl golden/git.bzl +diff -u test_dir/excluded/nested/excluded.bzl golden/excluded.bzl diff -u test_dir/MODULE.bazel golden/MODULE.bazel.golden diff -u test_dir/REPO.bazel golden/REPO.bazel.golden diff -u test_dir/VENDOR.bazel golden/VENDOR.bazel.golden diff --git a/buildifier/utils/utils.go b/buildifier/utils/utils.go index a132d753e..e28e22228 100644 --- a/buildifier/utils/utils.go +++ b/buildifier/utils/utils.go @@ -19,7 +19,9 @@ limitations under the License. package utils import ( + "fmt" "os" + "path" "path/filepath" "strings" @@ -46,9 +48,46 @@ func skip(info os.FileInfo) bool { return info.IsDir() && info.Name() == ".git" } +func normalizePathForMatch(value string) string { + value = filepath.ToSlash(value) + for strings.HasPrefix(value, "./") { + value = strings.TrimPrefix(value, "./") + } + return value +} + +// matchPath uses path.Match syntax, but allows '*' and '?' to match path separators. +func matchPath(pattern, filename string) (bool, error) { + const separatorPlaceholder = "\x00" + pattern = strings.ReplaceAll(normalizePathForMatch(pattern), "/", separatorPlaceholder) + filename = strings.ReplaceAll(normalizePathForMatch(filename), "/", separatorPlaceholder) + return path.Match(pattern, filename) +} + +func isExcluded(filename string, patterns []string) (bool, error) { + for _, pattern := range patterns { + matched, err := matchPath(pattern, filename) + if err != nil { + return false, err + } + if matched { + return true, nil + } + } + return false, nil +} + // ExpandDirectories takes a list of file/directory names and returns a list with file names -// by traversing each directory recursively and searching for relevant Starlark files. -func ExpandDirectories(args *[]string) ([]string, error) { +// by traversing each directory recursively and searching for relevant Starlark files. Paths +// matching any of the optional exclude patterns are skipped. Exclude patterns use path.Match +// syntax and are matched with slash separators on every platform. +func ExpandDirectories(args *[]string, excludePatterns ...string) ([]string, error) { + for _, pattern := range excludePatterns { + if _, err := matchPath(pattern, ""); err != nil { + return nil, fmt.Errorf("invalid exclude pattern %q: %w", pattern, err) + } + } + files := []string{} for _, arg := range *args { info, err := os.Stat(arg) @@ -59,23 +98,30 @@ func ExpandDirectories(args *[]string) ([]string, error) { files = append(files, arg) continue } - err = filepath.Walk(arg, func(path string, info os.FileInfo, err error) error { + err = filepath.Walk(arg, func(filename string, info os.FileInfo, err error) error { if err != nil { return err } - if skip(info) { + excluded, err := isExcluded(filename, excludePatterns) + if err != nil { + return err + } + if info.IsDir() && (skip(info) || excluded) { return filepath.SkipDir } + if excluded { + return nil + } if !info.IsDir() && isStarlarkFile(info.Name()) { // Don't traverse into directory symlinks such as bazel-foo.bzl // for a project called foo.bzl. if info.Mode()&os.ModeSymlink != 0 { - stat, err := os.Stat(path) + stat, err := os.Stat(filename) if err != nil || stat.IsDir() { return nil } } - files = append(files, path) + files = append(files, filename) } return nil }) diff --git a/buildifier/utils/utils_test.go b/buildifier/utils/utils_test.go index 579c12939..034e1bee5 100644 --- a/buildifier/utils/utils_test.go +++ b/buildifier/utils/utils_test.go @@ -17,6 +17,10 @@ limitations under the License. package utils import ( + "os" + "path/filepath" + "reflect" + "strings" "testing" ) @@ -165,3 +169,70 @@ func TestIsStarlarkFile(t *testing.T) { } } } + +func TestExpandDirectoriesExcludesPaths(t *testing.T) { + root := t.TempDir() + for _, filename := range []string{ + "BUILD", + "included/defs.bzl", + "excluded/direct.bzl", + "excluded/nested/BUILD.bazel", + "other/skip.sky", + "other/keep.star", + } { + path := filepath.Join(root, filepath.FromSlash(filename)) + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, nil, 0o644); err != nil { + t.Fatal(err) + } + } + + args := []string{root} + files, err := ExpandDirectories( + &args, + filepath.Join(root, "excluded", "*"), + filepath.Join(root, "*", "skip.sky"), + ) + if err != nil { + t.Fatal(err) + } + + want := []string{ + filepath.Join(root, "BUILD"), + filepath.Join(root, "included", "defs.bzl"), + filepath.Join(root, "other", "keep.star"), + } + if !reflect.DeepEqual(files, want) { + t.Errorf("ExpandDirectories() = %q, want %q", files, want) + } +} + +func TestMatchPathAllowsWildcardsAcrossSeparators(t *testing.T) { + for _, tc := range []struct { + pattern string + filename string + want bool + }{ + {pattern: "./vendor/*", filename: "vendor/direct.bzl", want: true}, + {pattern: "./vendor/*", filename: "vendor/nested/defs.bzl", want: true}, + {pattern: "./vendor/*.bzl", filename: "vendor/nested/defs.bzl", want: true}, + {pattern: "./vendor/*.bzl", filename: "third_party/defs.bzl", want: false}, + } { + got, err := matchPath(tc.pattern, tc.filename) + if err != nil { + t.Errorf("matchPath(%q, %q) returned error: %v", tc.pattern, tc.filename, err) + } else if got != tc.want { + t.Errorf("matchPath(%q, %q) = %t, want %t", tc.pattern, tc.filename, got, tc.want) + } + } +} + +func TestExpandDirectoriesRejectsInvalidExcludePattern(t *testing.T) { + args := []string{t.TempDir()} + _, err := ExpandDirectories(&args, "[") + if err == nil || !strings.Contains(err.Error(), "syntax error in pattern") { + t.Fatalf("ExpandDirectories() error = %v, want invalid pattern error", err) + } +} From 01209f8236c4acd364f0d268a0deae76d32b9806 Mon Sep 17 00:00:00 2001 From: Titouan BION Date: Wed, 28 Oct 2026 19:07:36 +0100 Subject: [PATCH 2/2] Change pattern syntax + respect .bazelignore/ignore_directories() Fixes several issues of the commit introducing `-exclude`: * Patterns use the syntax of Bazel's glob(): '*' and '?' match within a single path segment, and '**' as a whole segment matches any number of segments. * Patterns are relative to the workspace root, not the working directory, so that they mean the same thing wherever buildifier runs and in .buildifier.json. * The paths of .bazelignore and the patterns of ignore_directories() in REPO.bazel are excluded by default; -respect_bazelignore=false turns that off. * Excluded paths are checked before walk errors, so unreadable excluded directories do not fail the run. --- buildifier/README.md | 13 +- buildifier/buildifier.go | 8 +- buildifier/config/config.go | 13 +- buildifier/config/config_test.go | 10 +- buildifier/integration_test.sh | 3 +- buildifier/utils/BUILD.bazel | 3 + buildifier/utils/exclude.go | 268 +++++++++++++++++++++++++++++++ buildifier/utils/exclude_test.go | 188 ++++++++++++++++++++++ buildifier/utils/utils.go | 57 ++----- buildifier/utils/utils_test.go | 71 -------- 10 files changed, 503 insertions(+), 131 deletions(-) create mode 100644 buildifier/utils/exclude.go create mode 100644 buildifier/utils/exclude_test.go diff --git a/buildifier/README.md b/buildifier/README.md index 94de3f4ce..c77b6bfe7 100644 --- a/buildifier/README.md +++ b/buildifier/README.md @@ -33,14 +33,19 @@ buildifier -r path/to/dir ``` Paths can be excluded from recursive discovery with repeatable `-exclude` -patterns. Patterns use Go's `path.Match` syntax, are matched against the full -traversed path, allow `*` and `?` to match path separators, and use -forward-slash separators on every platform: +patterns. Patterns use the syntax of Bazel's `glob()`: `*` and `?` match within a +single path segment and `**` matches any number of segments. They are relative to +the workspace root, wherever buildifier runs, and use forward slashes on every +platform. A pattern matching a directory excludes everything below it: ```bash -buildifier -r -exclude='path/to/dir/vendor/*' path/to/dir +buildifier -r -exclude='third_party/**/vendor' -exclude='**/node_modules' . ``` +By default, the paths listed in the workspace's `.bazelignore` and the patterns +passed to `ignore_directories()` in its `REPO.bazel` are excluded too. Use +`-respect_bazelignore=false` to discover them anyway. + Buildifier supports the following file types: `BUILD`, `WORKSPACE`, `.bzl`, and default, the latter is reserved for Starlark files buildifier doesn't know about (e.g. configuration files for third-party projects that use Starlark). The diff --git a/buildifier/buildifier.go b/buildifier/buildifier.go index 572643941..116824ff4 100644 --- a/buildifier/buildifier.go +++ b/buildifier/buildifier.go @@ -191,8 +191,12 @@ func (b *buildifier) run(args []string) int { } else { files := args if b.config.Recursive { - var err error - files, err = utils.ExpandDirectories(&args, b.config.Exclude...) + excluder, err := utils.NewExcluder(b.config.Exclude, b.config.RespectBazelignore) + if err != nil { + fmt.Fprintf(os.Stderr, "buildifier: %v\n", err) + return 3 + } + files, err = utils.ExpandDirectories(&args, excluder) if err != nil { fmt.Fprintf(os.Stderr, "buildifier: %v\n", err) return 3 diff --git a/buildifier/config/config.go b/buildifier/config/config.go index 11308a23b..99fa9516a 100644 --- a/buildifier/config/config.go +++ b/buildifier/config/config.go @@ -37,7 +37,8 @@ const buildifierJSONFilename = ".buildifier.json" // New constructs a Config with default values. func New() *Config { return &Config{ - InputType: "auto", + InputType: "auto", + RespectBazelignore: true, } } @@ -111,8 +112,13 @@ type Config struct { WarningsList []string `json:"warningsList,omitempty"` // Recursive instructs buildifier to find starlark files recursively Recursive bool `json:"recursive,omitempty"` - // Exclude contains path patterns to skip during recursive file discovery + // Exclude contains glob() patterns, relative to the workspace root, to skip + // during recursive file discovery Exclude ArrayFlags `json:"exclude,omitempty"` + // RespectBazelignore also skips the paths ignored by the workspace's + // .bazelignore and the ignore_directories() of its REPO.bazel during + // recursive file discovery + RespectBazelignore bool `json:"respectBazelignore"` // Verbose instructs buildifier to output verbose diagnostics Verbose bool `json:"verbose,omitempty"` // DiffCommand is the command to run when the formatting mode is diff @@ -179,7 +185,8 @@ func (c *Config) FlagSet(name string, errorHandling flag.ErrorHandling) *flag.Fl flags.BoolVar(&c.Verbose, "v", c.Verbose, "print verbose information to standard error") flags.BoolVar(&c.DiffMode, "d", c.DiffMode, "alias for -mode=diff") flags.BoolVar(&c.Recursive, "r", c.Recursive, "find starlark files recursively") - flags.Var(&c.Exclude, "exclude", "path pattern to exclude when finding starlark files recursively") + flags.Var(&c.Exclude, "exclude", "glob() pattern, relative to the workspace root, to exclude when finding starlark files recursively") + flags.BoolVar(&c.RespectBazelignore, "respect_bazelignore", c.RespectBazelignore, "also exclude the paths ignored by .bazelignore and the ignore_directories() of REPO.bazel when finding starlark files recursively") flags.BoolVar(&c.MultiDiff, "multi_diff", c.MultiDiff, "the command specified by the -diff_command flag can diff multiple files in the style of tkdiff (default false)") flags.StringVar(&c.Mode, "mode", c.Mode, "formatting mode: check, diff, or fix (default fix)") flags.StringVar(&c.Format, "format", c.Format, "diagnostics format: text, json or sarif (default text)") diff --git a/buildifier/config/config_test.go b/buildifier/config/config_test.go index 67585f7d4..94ca5eaa0 100644 --- a/buildifier/config/config_test.go +++ b/buildifier/config/config_test.go @@ -30,7 +30,8 @@ func ExampleNew() { fmt.Print(c.String()) // Output: // { - // "type": "auto" + // "type": "auto", + // "respectBazelignore": true // } } @@ -142,7 +143,8 @@ func ExampleExample() { // "unreachable", // "unsorted-dict-items", // "unused-variable" - // ] + // ], + // "respectBazelignore": true // } } @@ -160,7 +162,7 @@ func ExampleFlagSet() { // d: alias for -mode=diff ("false") // diff_command: command to run when the formatting mode is diff (default uses the BUILDIFIER_DIFF, BUILDIFIER_MULTIDIFF, and DISPLAY environment variables to create the diff command) ("") // disable_symlink_safety: per default Buildifier will not write to symlinks pointing outside of the Bazel workspace. Setting this to true will disable this behavior ("false") - // exclude: path pattern to exclude when finding starlark files recursively ("") + // exclude: glob() pattern, relative to the workspace root, to exclude when finding starlark files recursively ("") // format: diagnostics format: text, json or sarif (default text) ("") // help: print usage information ("false") // lint: lint mode: off, warn, or fix (default off) ("") @@ -168,6 +170,7 @@ func ExampleFlagSet() { // multi_diff: the command specified by the -diff_command flag can diff multiple files in the style of tkdiff (default false) ("false") // path: assume BUILD file has this path relative to the workspace directory ("") // r: find starlark files recursively ("false") + // respect_bazelignore: also exclude the paths ignored by .bazelignore and the ignore_directories() of REPO.bazel when finding starlark files recursively ("true") // tables: path to JSON file with custom table definitions which will replace the built-in tables ("") // type: Input file type: build (for BUILD files), bzl (for .bzl files), workspace (for WORKSPACE files), module (for MODULE.bazel files), repo (for REPO.bazel files), vendor (for VENDOR.bazel files), default (for generic Starlark files) or auto (default, based on the filename) ("auto") // v: print verbose information to standard error ("false") @@ -222,6 +225,7 @@ func ExampleFlagSet_parse() { // "vendor/*", // "third_party/*" // ], + // "respectBazelignore": true, // "verbose": true, // "diffCommand": "diff", // "multiDiff": true, diff --git a/buildifier/integration_test.sh b/buildifier/integration_test.sh index 2d789f60a..dd64f2cd8 100755 --- a/buildifier/integration_test.sh +++ b/buildifier/integration_test.sh @@ -379,7 +379,8 @@ cat > golden/.buildifier.example.json < 0 && matchSegments(pattern, segments[1:])) + } + if len(segments) == 0 { + return false + } + // Patterns are validated when added, so there is no error to handle. + if matched, _ := path.Match(pattern[0], segments[0]); !matched { + return false + } + return matchSegments(pattern[1:], segments[1:]) +} + +// escapeGlob escapes the glob metacharacters of a literal path. +func escapeGlob(literal string) string { + var b strings.Builder + for _, r := range literal { + switch r { + case '*', '?', '[', '\\': + b.WriteRune('\\') + } + b.WriteRune(r) + } + return b.String() +} + +// readBazelignore returns the paths listed in a .bazelignore file, if any. +func readBazelignore(filename string) ([]string, error) { + file, err := os.Open(filename) + if errors.Is(err, os.ErrNotExist) { + return nil, nil + } + if err != nil { + return nil, err + } + defer file.Close() + + var dirs []string + scanner := bufio.NewScanner(file) + for scanner.Scan() { + line := strings.TrimSpace(scanner.Text()) + if line == "" || strings.HasPrefix(line, "#") { + continue + } + for strings.HasPrefix(line, "./") { + line = strings.TrimPrefix(line, "./") + } + if line = strings.TrimSuffix(line, "/"); line != "" { + dirs = append(dirs, line) + } + } + return dirs, scanner.Err() +} + +// readIgnoreDirectories returns the patterns passed to ignore_directories() in +// a REPO.bazel file, if any. +func readIgnoreDirectories(filename string) ([]string, error) { + data, err := os.ReadFile(filename) + if errors.Is(err, os.ErrNotExist) { + return nil, nil + } + if err != nil { + return nil, err + } + f, err := build.ParseRepo(filename, data) + if err != nil { + return nil, err + } + + var patterns []string + for _, stmt := range f.Stmt { + call, ok := stmt.(*build.CallExpr) + if !ok { + continue + } + if ident, ok := call.X.(*build.Ident); !ok || ident.Name != "ignore_directories" { + continue + } + for _, arg := range call.List { + if assign, ok := arg.(*build.AssignExpr); ok { + arg = assign.RHS + } + list, ok := arg.(*build.ListExpr) + if !ok { + continue + } + for _, item := range list.List { + if str, ok := item.(*build.StringExpr); ok { + patterns = append(patterns, str.Value) + } + } + } + } + return patterns, nil +} diff --git a/buildifier/utils/exclude_test.go b/buildifier/utils/exclude_test.go new file mode 100644 index 000000000..d32590501 --- /dev/null +++ b/buildifier/utils/exclude_test.go @@ -0,0 +1,188 @@ +/* +Copyright 2026 Google LLC + +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 + + https://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 utils + +import ( + "os" + "path/filepath" + "reflect" + "runtime" + "strings" + "testing" +) + +func writeFiles(t *testing.T, root string, files map[string]string) { + t.Helper() + for filename, content := range files { + path := filepath.Join(root, filepath.FromSlash(filename)) + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + } +} + +func TestExcluderMatchesGlobSyntax(t *testing.T) { + root := t.TempDir() + for _, tc := range []struct { + pattern string + path string + want bool + }{ + {"vendor", "vendor", true}, + {"vendor", "a/vendor", false}, + {"vendor/", "vendor", true}, + {"**/node_modules", "node_modules", true}, + {"**/node_modules", "a/b/node_modules", true}, + {"**/node_modules", "a/node_modules_x", false}, + {"vendor/*", "vendor/x", true}, + {"vendor/*", "vendor/x/y", false}, + {"*.bzl", "defs.bzl", true}, + {"*.bzl", "pkg/defs.bzl", false}, + {"a/**/b.bzl", "a/b.bzl", true}, + {"a/**/b.bzl", "a/x/y/b.bzl", true}, + {"a/**", "a", true}, + {"a/**", "a/x/y", true}, + {"**", "anything/at/all", true}, + {"?.bzl", "a.bzl", true}, + {"?.bzl", "ab.bzl", false}, + } { + e, err := newExcluder(root, []string{tc.pattern}, false) + if err != nil { + t.Fatalf("newExcluder(%q) returned error: %v", tc.pattern, err) + } + if got := e.excludedAbs(filepath.Join(root, filepath.FromSlash(tc.path))); got != tc.want { + t.Errorf("pattern %q, path %q: excluded = %t, want %t", tc.pattern, tc.path, got, tc.want) + } + } +} + +func TestExcluderNeverExcludesTheRootOrOutsidePaths(t *testing.T) { + root := t.TempDir() + e, err := newExcluder(root, []string{"**"}, false) + if err != nil { + t.Fatal(err) + } + if e.excludedAbs(root) { + t.Error("the root itself is excluded") + } + if e.excludedAbs(filepath.Dir(root)) { + t.Error("a path outside the root is excluded") + } +} + +func TestExcluderRejectsInvalidPatterns(t *testing.T) { + for _, pattern := range []string{"", "/abs", "a//b", "./a", "a/../b", "a**", "**b/c", "["} { + if _, err := newExcluder(t.TempDir(), []string{pattern}, false); err == nil { + t.Errorf("newExcluder(%q) succeeded, want an error", pattern) + } else if !strings.Contains(err.Error(), "invalid exclude pattern") { + t.Errorf("newExcluder(%q) error = %v, want an invalid exclude pattern error", pattern, err) + } + } +} + +func TestExcluderRespectsBazelignoreAndIgnoreDirectories(t *testing.T) { + root := t.TempDir() + writeFiles(t, root, map[string]string{ + ".bazelignore": "# comment\n\nignored\n./dotted/\nlit*eral\n", + "REPO.bazel": "ignore_directories([\"**/node_modules\"])\n", + }) + + for _, respect := range []bool{true, false} { + e, err := newExcluder(root, nil, respect) + if err != nil { + t.Fatal(err) + } + for _, tc := range []struct { + path string + want bool + }{ + {"ignored", respect}, + {"dotted", respect}, + {"lit*eral", respect}, + // .bazelignore entries are literal paths, not globs. + {"litteral", false}, + {"pkg/node_modules", respect}, + {"pkg/ignored", false}, + } { + if got := e.excludedAbs(filepath.Join(root, filepath.FromSlash(tc.path))); got != tc.want { + t.Errorf("respect=%t, path %q: excluded = %t, want %t", respect, tc.path, got, tc.want) + } + } + } +} + +func TestExpandDirectoriesExcludes(t *testing.T) { + root := t.TempDir() + writeFiles(t, root, map[string]string{ + "MODULE.bazel": "", + "BUILD": "", + "included/defs.bzl": "", + "excluded/direct.bzl": "", + "excluded/nested/BUILD.bazel": "", + "other/skip.sky": "", + "other/keep.star": "", + "unreadable/BUILD": "", + }) + if runtime.GOOS != "windows" { + unreadable := filepath.Join(root, "unreadable") + if err := os.Chmod(unreadable, 0); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { os.Chmod(unreadable, 0o755) }) + } + t.Chdir(root) + + excluder, err := NewExcluder([]string{"excluded", "**/skip.sky", "unreadable"}, true) + if err != nil { + t.Fatal(err) + } + args := []string{"."} + files, err := ExpandDirectories(&args, excluder) + if err != nil { + t.Fatal(err) + } + want := []string{"BUILD", "MODULE.bazel", filepath.Join("included", "defs.bzl"), filepath.Join("other", "keep.star")} + if !reflect.DeepEqual(files, want) { + t.Errorf("ExpandDirectories() = %q, want %q", files, want) + } +} + +func TestExpandDirectoriesExcludesRelativeToTheWorkspaceRoot(t *testing.T) { + root := t.TempDir() + writeFiles(t, root, map[string]string{ + "MODULE.bazel": "", + "pkg/BUILD": "", + "pkg/excluded/BUILD.bazel": "", + }) + t.Chdir(filepath.Join(root, "pkg")) + + excluder, err := NewExcluder([]string{"pkg/excluded"}, true) + if err != nil { + t.Fatal(err) + } + args := []string{"."} + files, err := ExpandDirectories(&args, excluder) + if err != nil { + t.Fatal(err) + } + if want := []string{"BUILD"}; !reflect.DeepEqual(files, want) { + t.Errorf("ExpandDirectories() = %q, want %q", files, want) + } +} diff --git a/buildifier/utils/utils.go b/buildifier/utils/utils.go index e28e22228..3c15c03a7 100644 --- a/buildifier/utils/utils.go +++ b/buildifier/utils/utils.go @@ -19,9 +19,7 @@ limitations under the License. package utils import ( - "fmt" "os" - "path" "path/filepath" "strings" @@ -48,46 +46,10 @@ func skip(info os.FileInfo) bool { return info.IsDir() && info.Name() == ".git" } -func normalizePathForMatch(value string) string { - value = filepath.ToSlash(value) - for strings.HasPrefix(value, "./") { - value = strings.TrimPrefix(value, "./") - } - return value -} - -// matchPath uses path.Match syntax, but allows '*' and '?' to match path separators. -func matchPath(pattern, filename string) (bool, error) { - const separatorPlaceholder = "\x00" - pattern = strings.ReplaceAll(normalizePathForMatch(pattern), "/", separatorPlaceholder) - filename = strings.ReplaceAll(normalizePathForMatch(filename), "/", separatorPlaceholder) - return path.Match(pattern, filename) -} - -func isExcluded(filename string, patterns []string) (bool, error) { - for _, pattern := range patterns { - matched, err := matchPath(pattern, filename) - if err != nil { - return false, err - } - if matched { - return true, nil - } - } - return false, nil -} - // ExpandDirectories takes a list of file/directory names and returns a list with file names // by traversing each directory recursively and searching for relevant Starlark files. Paths -// matching any of the optional exclude patterns are skipped. Exclude patterns use path.Match -// syntax and are matched with slash separators on every platform. -func ExpandDirectories(args *[]string, excludePatterns ...string) ([]string, error) { - for _, pattern := range excludePatterns { - if _, err := matchPath(pattern, ""); err != nil { - return nil, fmt.Errorf("invalid exclude pattern %q: %w", pattern, err) - } - } - +// that the optional excluder excludes are skipped, without reading excluded directories. +func ExpandDirectories(args *[]string, excluder *Excluder) ([]string, error) { files := []string{} for _, arg := range *args { info, err := os.Stat(arg) @@ -99,19 +61,20 @@ func ExpandDirectories(args *[]string, excludePatterns ...string) ([]string, err continue } err = filepath.Walk(arg, func(filename string, info os.FileInfo, err error) error { - if err != nil { - return err + // Check exclusions first, so that excluded paths never fail the run, + // e.g. when they are unreadable. + if excluder.Excluded(filename) { + if err == nil && info.IsDir() { + return filepath.SkipDir + } + return nil } - excluded, err := isExcluded(filename, excludePatterns) if err != nil { return err } - if info.IsDir() && (skip(info) || excluded) { + if skip(info) { return filepath.SkipDir } - if excluded { - return nil - } if !info.IsDir() && isStarlarkFile(info.Name()) { // Don't traverse into directory symlinks such as bazel-foo.bzl // for a project called foo.bzl. diff --git a/buildifier/utils/utils_test.go b/buildifier/utils/utils_test.go index 034e1bee5..579c12939 100644 --- a/buildifier/utils/utils_test.go +++ b/buildifier/utils/utils_test.go @@ -17,10 +17,6 @@ limitations under the License. package utils import ( - "os" - "path/filepath" - "reflect" - "strings" "testing" ) @@ -169,70 +165,3 @@ func TestIsStarlarkFile(t *testing.T) { } } } - -func TestExpandDirectoriesExcludesPaths(t *testing.T) { - root := t.TempDir() - for _, filename := range []string{ - "BUILD", - "included/defs.bzl", - "excluded/direct.bzl", - "excluded/nested/BUILD.bazel", - "other/skip.sky", - "other/keep.star", - } { - path := filepath.Join(root, filepath.FromSlash(filename)) - if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { - t.Fatal(err) - } - if err := os.WriteFile(path, nil, 0o644); err != nil { - t.Fatal(err) - } - } - - args := []string{root} - files, err := ExpandDirectories( - &args, - filepath.Join(root, "excluded", "*"), - filepath.Join(root, "*", "skip.sky"), - ) - if err != nil { - t.Fatal(err) - } - - want := []string{ - filepath.Join(root, "BUILD"), - filepath.Join(root, "included", "defs.bzl"), - filepath.Join(root, "other", "keep.star"), - } - if !reflect.DeepEqual(files, want) { - t.Errorf("ExpandDirectories() = %q, want %q", files, want) - } -} - -func TestMatchPathAllowsWildcardsAcrossSeparators(t *testing.T) { - for _, tc := range []struct { - pattern string - filename string - want bool - }{ - {pattern: "./vendor/*", filename: "vendor/direct.bzl", want: true}, - {pattern: "./vendor/*", filename: "vendor/nested/defs.bzl", want: true}, - {pattern: "./vendor/*.bzl", filename: "vendor/nested/defs.bzl", want: true}, - {pattern: "./vendor/*.bzl", filename: "third_party/defs.bzl", want: false}, - } { - got, err := matchPath(tc.pattern, tc.filename) - if err != nil { - t.Errorf("matchPath(%q, %q) returned error: %v", tc.pattern, tc.filename, err) - } else if got != tc.want { - t.Errorf("matchPath(%q, %q) = %t, want %t", tc.pattern, tc.filename, got, tc.want) - } - } -} - -func TestExpandDirectoriesRejectsInvalidExcludePattern(t *testing.T) { - args := []string{t.TempDir()} - _, err := ExpandDirectories(&args, "[") - if err == nil || !strings.Contains(err.Error(), "syntax error in pattern") { - t.Fatalf("ExpandDirectories() error = %v, want invalid pattern error", err) - } -}