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

Add -strict to exit on build file and directive errors #1214

Merged
merged 7 commits into from
Apr 18, 2022
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions .bazelci/presubmit.yml
Original file line number Diff line number Diff line change
Expand Up @@ -38,3 +38,4 @@ platforms:
# gazelle prints file paths with backward slashes on windows,
# which doesn't match the golden files generated on *nix.
- "-//tests:fix_mode_print0"
- "-//tests:fix_mode_strict"
8 changes: 7 additions & 1 deletion config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,10 @@ type Config struct {
// usage of deprecated rules.
ShouldFix bool

// Strict determines how Gazelle handles build file and directive errors. When
// set, Gazelle will exit with non-zero value after logging such errors.
Strict bool

// IndexLibraries determines whether Gazelle should build an index of
// libraries in the workspace for dependency resolution
IndexLibraries bool
Expand Down Expand Up @@ -186,14 +190,15 @@ type Configurer interface {
// i.e., those that apply to Config itself and not to Config.Exts.
type CommonConfigurer struct {
repoRoot, buildFileNames, readBuildFilesDir, writeBuildFilesDir string
indexLibraries bool
indexLibraries, strict bool
langCsv string
}

func (cc *CommonConfigurer) RegisterFlags(fs *flag.FlagSet, cmd string, c *Config) {
fs.StringVar(&cc.repoRoot, "repo_root", "", "path to a directory which corresponds to go_prefix, otherwise gazelle searches for it.")
fs.StringVar(&cc.buildFileNames, "build_file_name", strings.Join(DefaultValidBuildFileNames, ","), "comma-separated list of valid build file names.\nThe first element of the list is the name of output build files to generate.")
fs.BoolVar(&cc.indexLibraries, "index", true, "when true, gazelle will build an index of libraries in the workspace for dependency resolution")
fs.BoolVar(&cc.strict, "strict", false, "when true, gazelle will exit with none-zero value for build file syntax errors or unknown directives")
fs.StringVar(&cc.readBuildFilesDir, "experimental_read_build_files_dir", "", "path to a directory where build files should be read from (instead of -repo_root)")
fs.StringVar(&cc.writeBuildFilesDir, "experimental_write_build_files_dir", "", "path to a directory where build files should be written to (instead of -repo_root)")
fs.StringVar(&cc.langCsv, "lang", "", "if non-empty, process only these languages (e.g. \"go,proto\")")
Expand Down Expand Up @@ -235,6 +240,7 @@ func (cc *CommonConfigurer) CheckFlags(fs *flag.FlagSet, c *Config) error {
}
}
c.IndexLibraries = cc.indexLibraries
c.Strict = cc.strict
if len(cc.langCsv) > 0 {
c.Langs = strings.Split(cc.langCsv, ",")
}
Expand Down
4 changes: 4 additions & 0 deletions tests/fix_mode_strict/BUILD.in
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
go_library(
name = "syntax_error_no_comma"
visibility = ["//visibility:public"],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if there are multiple errors (maybe even across multiple files)? Do they accumulate and all get printed out? I presume this is mostly going to be used in CI pipelines, should we try to get all of the errors printed out before quitting?

Copy link
Contributor Author

@dr-dime dr-dime Apr 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will exit at the first error. I give that a quick thought as well and decided not to do it.

The bzl parser bailed out at the first encountered error, so we can report at most one error per file. Also, the current interface/API doesn't return errors, so to collect all the errors, some serious refactor needs to take place first. See the discussion in #1029.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the question is, would it be better to "accumulate" to stderr and have a single boolean tracking success and os.Exiting essentially in the main function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to accumulate all errors, but there is no clean way to do so with the current API. I've tried this approach in master...dr-dime:cl/strict-mode-2, which doesn't LGTM as the error handling strategy is not clear at all.

I admit that those log.Fatals are not pretty, but it's following the comment on error handling in lang.Language interface. I can put some big TODOs next to them to make this more explicit as a refactor goal in the future.

)
4 changes: 4 additions & 0 deletions tests/fix_mode_strict/BUILD.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
go_library(
name = "syntax_error_no_comma"
visibility = ["//visibility:public"],
)
4 changes: 4 additions & 0 deletions tests/fix_mode_strict/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
# Gazelle fix -strict mode

When Gazelle is run with the `-strict` flag, it will exit with none-zero value
for build file syntax errors or unknown directives.
Empty file added tests/fix_mode_strict/WORKSPACE
Empty file.
1 change: 1 addition & 0 deletions tests/fix_mode_strict/arguments.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
-strict
1 change: 1 addition & 0 deletions tests/fix_mode_strict/expectedExitCode.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
1
2 changes: 2 additions & 0 deletions tests/fix_mode_strict/expectedStderr.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
gazelle: %WORKSPACEPATH%/BUILD.bazel:3:13: syntax error near visibility
gazelle: Exit as strict mode is on
10 changes: 10 additions & 0 deletions walk/walk.go
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,11 @@ func Walk(c *config.Config, cexts []config.Configurer, dirs []string, mode Mode,
f, err := loadBuildFile(c, rel, dir, files)
if err != nil {
log.Print(err)
if c.Strict {
// TODO(https://github.com/bazelbuild/bazel-gazelle/issues/1029):
// Refactor to accumulate and propagate errors to main.
log.Fatal("Exit as strict mode is on")
}
haveError = true
}

Expand Down Expand Up @@ -275,6 +280,11 @@ func configure(cexts []config.Configurer, knownDirectives map[string]bool, c *co
for _, d := range f.Directives {
if !knownDirectives[d.Key] {
log.Printf("%s: unknown directive: gazelle:%s", f.Path, d.Key)
if c.Strict {
// TODO(https://github.com/bazelbuild/bazel-gazelle/issues/1029):
// Refactor to accumulate and propagate errors to main.
log.Fatal("Exit as strict mode is on")
}
}
}
}
Expand Down