diff --git a/buildifier/README.md b/buildifier/README.md index f183c334b..97f90442b 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 60124fc2b..1ea37e36b 100644 --- a/buildifier/buildifier.go +++ b/buildifier/buildifier.go @@ -190,7 +190,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 e9783b949..146e40717 100644 --- a/buildifier/config/config.go +++ b/buildifier/config/config.go @@ -110,6 +110,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 @@ -172,6 +174,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 or json (default text)") diff --git a/buildifier/config/config_test.go b/buildifier/config/config_test.go index c9291769e..a8dd5809a 100644 --- a/buildifier/config/config_test.go +++ b/buildifier/config/config_test.go @@ -160,6 +160,7 @@ func ExampleFlagSet() { // config: path to .buildifier.json config file ("") // 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) ("") + // exclude: path pattern to exclude when finding starlark files recursively ("") // format: diagnostics format: text or json (default text) ("") // help: print usage information ("false") // lint: lint mode: off, warn, or fix (default off) ("") @@ -185,6 +186,8 @@ func ExampleFlagSet_parse() { "--config=/path/to/.buildifier.json", "-d", "--diff_command=diff", + "--exclude=vendor/*", + "--exclude=third_party/*", "--format=json", "--help", "--lint=fix", @@ -214,6 +217,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 b9a6389c5..9d6b38243 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") @@ -126,9 +128,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 @@ -372,6 +375,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/.buildifier.example.json golden/.buildifier.example.json diff --git a/buildifier/internal/factory.bzl b/buildifier/internal/factory.bzl index 88b813238..b3ca3390e 100644 --- a/buildifier/internal/factory.bzl +++ b/buildifier/internal/factory.bzl @@ -48,7 +48,7 @@ def buildifier_attr_factory(test_rule = False): ), "exclude_patterns": attr.string_list( allow_empty = True, - doc = "A list of glob patterns passed to the find command. E.g. './vendor/*' to exclude the Go vendor directory. In test rules, this attribute requires the use of the no_sandbox attribute.", + doc = "A list of Go-style path patterns excluded from recursive file discovery. E.g. './vendor/*' to exclude the Go vendor directory. In test rules, this attribute requires the use of the no_sandbox attribute.", ), "mode": attr.string( default = "fix" if not test_rule else "diff", @@ -155,12 +155,10 @@ def buildifier_impl_factory(ctx, test_rule = False): if ctx.attr.add_tables: args.append("-add_tables=%s" % ctx.file.add_tables.path) - exclude_patterns_str = "" if ctx.attr.exclude_patterns: if test_rule and not ctx.attr.no_sandbox: fail("Cannot use 'exclude_patterns' in a test rule without 'no_sandbox'") - exclude_patterns = ["\\! -path %s" % shell.quote(pattern) for pattern in ctx.attr.exclude_patterns] - exclude_patterns_str = " ".join(exclude_patterns) + args.extend(["--exclude=%s" % pattern for pattern in ctx.attr.exclude_patterns]) workspace = "" if test_rule and ctx.attr.no_sandbox: @@ -172,7 +170,6 @@ def buildifier_impl_factory(ctx, test_rule = False): substitutions = { "@@ARGS@@": shell.array_literal(args), "@@BUILDIFIER_SHORT_PATH@@": shell.quote(ctx.executable.buildifier.short_path), - "@@EXCLUDE_PATTERNS@@": exclude_patterns_str, "@@WORKSPACE@@": workspace, } diff --git a/buildifier/runner.bash.template b/buildifier/runner.bash.template index e509163ec..6d86f45b5 100644 --- a/buildifier/runner.bash.template +++ b/buildifier/runner.bash.template @@ -9,11 +9,8 @@ buildifier_short_path=$(readlink "$BUILDIFIER_SHORT_PATH") # Use TEST_WORKSPACE to determine if the script is being ran under a test if [[ ! -z "${TEST_WORKSPACE+x}" && -z "${BUILD_WORKSPACE_DIRECTORY+x}" ]]; then - FIND_FILE_TYPE="l" # If WORKSPACE was provided, then the script is being run under a test in no_sandbox mode if [[ ! -z "${WORKSPACE:+x}" ]]; then - FIND_FILE_TYPE="f" - # resolve the WORKSPACE symlink # use `realpath` if available and `readlink` otherwise (typically macOS) if command -v realpath &> /dev/null; then @@ -41,20 +38,5 @@ else fi fi -# Run buildifier on all starlark files -find . \ - -type "${FIND_FILE_TYPE:-f}" \ - @@EXCLUDE_PATTERNS@@ \ - \( -name '*.bzl' \ - -o -name '*.sky' \ - -o -name '*.star' \ - -o -name '*.bazel' \ - -o -name BUILD \ - -o -name '*.BUILD' \ - -o -name BUILD.oss \ - -o -name 'BUILD.*.oss' \ - -o -name WORKSPACE \ - -o -name WORKSPACE.oss \ - -o -name WORKSPACE.bzlmod \ - -o -name 'WORKSPACE.*.oss' \ - \) -print | xargs "$buildifier_short_path" "${ARGS[@]}" +# Run buildifier on all starlark files. +"$buildifier_short_path" "${ARGS[@]}" -r . diff --git a/buildifier/runner.bat.template b/buildifier/runner.bat.template index f5d1e5820..effcd76f1 100644 --- a/buildifier/runner.bat.template +++ b/buildifier/runner.bat.template @@ -9,29 +9,12 @@ rem Unquote the arguments set stripped_args=%stripped_args:'=% rem Get the absolute path to the buildifier executable -for /f "tokens=2" %%i in ('findstr /r "\" MANIFEST') do (set buildifier_abs_path=%%i) +for /f "tokens=1,* delims= " %%i in ('findstr /r "\" MANIFEST') do set "buildifier_abs_path=%%j" -powershell ^ -$Files = Get-ChildItem -LiteralPath '%BUILD_WORKSPACE_DIRECTORY:/=\%' -Recurse -File -ErrorAction SilentlyContinue ^|^ - Where-Object {^ - $_.Name -eq 'BUILD' `^ - -or $_.Name -eq 'WORKSPACE' `^ - -or $_.Name -eq 'WORKSPACE.oss' `^ - -or $_.Name -eq 'WORKSPACE.bzlmod' `^ - -or $_.Name -eq 'BUILD.oss' `^ - -or $_.Name -clike '*.bazel' `^ - -or $_.Name -clike '*.bzl' `^ - -or $_.Name -clike '*.sky' `^ - -or $_.Name -clike '*.star' `^ - -or $_.Name -clike '*.BUILD' `^ - -or $_.Name -clike 'BUILD.*.oss' `^ - -or $_.Name -clike 'WORKSPACE.*.oss'^ - };^ - ^<# Process files in batches of 100- to avoid exceeding CreateProcess' maximum length of 32,767 characters #^> ^ -$i = 0;^ -while ($i -lt $Files.Count)^ -{^ - $Batch = $Files[$i..($i + 99)];^ - ^& '%buildifier_abs_path%' %stripped_args% $Batch.FullName;^ - $i += $Batch.Count;^ -}; +if defined BUILD_WORKSPACE_DIRECTORY ( + cd /d "%BUILD_WORKSPACE_DIRECTORY%" + if errorlevel 1 exit /b 1 +) + +"%buildifier_abs_path%" %stripped_args% -r . +exit /b %ERRORLEVEL% diff --git a/buildifier/utils/utils.go b/buildifier/utils/utils.go index 988f0d0f0..7d6855ba7 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) + } +}