refactor: Add GAZELLE_VERBOSE env and log parser failures #2420

dougthor42 · 2024-11-17T05:19:10Z

While investigating #2396 and why #2413 doesn't appear to be working for us, I realized that one of the things I was making heavy use of was additional parser logging that I had added. This adds some of that logging. I also throw in some documentation because I found it helpful.

Users can (attempt to) get additional parse failure information by setting the GAZELLE_VERBOSE environment variable to 1. Eg:

$ GAZELLE_VERBOSE=1 bazel run //:gazelle

Here are some example logs:

$ GAZELLE_VERBOSE=1 bazel run //:gazelle
INFO: Invocation ID: a4e026d8-17df-426c-b1cc-d3980690dd53
...
INFO: Running command line: bazel-bin/gazelle
INFO: Streaming build results to: https://btx.cloud.google.com/invocations/a4e026d8-17df-426c-b1cc-d3980690dd53
gazelle: WARNING: failed to parse "hello/get_deps.py". The resulting BUILD target may be incorrect.
gazelle: Parse error at {Row:1 Column:0}:
def search_one_more_level[T]():
gazelle: The above was parsed as: (ERROR (identifier) (call function: (list (identifier)) arguments: (argument_list)))
gazelle: ERROR: failed to generate target "//hello:get_deps" of kind "py_library": a target of kind "pyle_py_binary" with the same name already exists. Use the '# gazelle:python_library_naming_convention' directive to change the naming convention.
$
$ bazel run //:gazelle
INFO: Invocation ID: 726c9fd6-f566-4c30-95ef-c4781ad155de
...
INFO: Running command line: bazel-bin/gazelle
INFO: Streaming build results to: https://btx.cloud.google.com/invocations/726c9fd6-f566-4c30-95ef-c4781ad155de
gazelle: WARNING: failed to parse "hello/get_deps.py". The resulting BUILD target may be incorrect.
gazelle: ERROR: failed to generate target "//hello:get_deps" of kind "py_library": a target of kind "pyle_py_binary" with the same name already exists. Use the '# gazelle:python_library_naming_convention' directive to change the naming convention.

dougthor42 · 2024-11-17T05:22:16Z

gazelle/python/file_parser.go

+		verbose, envExists := os.LookupEnv("GAZELLE_VERBOSE")
+		if envExists && verbose == "1" {


If people are happy with this idea, I will make a more generalized function that can be reused. Maybe even add verbosity levels.

I didn't see any option in the core gazelle that allows for different verbosity levels, so if there is one please LMK.

dougthor42 · 2024-11-17T05:22:56Z

gazelle/python/file_parser.go

+					log.Printf("Parse error at %+v:\n%+v", child.StartPoint(), child.Content(code))
+					log.Printf("The above was parsed as: %v", child.String())


Add comments demo'ing the logged message.

Lol this was a TODO for me but now it's merged. I'll add this to the followup PR.

rickeylev

I changed the title to refactor, since it isn't providing a feature, more just better diagnostics for debugging failures.

gazelle/python/file_parser.go

rickeylev · 2024-11-18T00:22:56Z

CHANGELOG.md

@@ -61,7 +61,8 @@ Unreleased changes template.

 {#v0-0-0-added}
 ### Added
-* Nothing yet.
+* (gazelle): Parser failures will now be logged to the terminal. Additional
+  details can be logged by setting `GAZELLE_VERBOSE=1`.


You can add this to docs/environment-variables.md, too.

TIL! ok I'll add that soon.

And change it to be prefixed with RULES_PYTHON_ to match the others.

aignas

In general LGTM, but have questions if we should return err=nil if root.HasError(). If the Python file cannot be parsed, why should we continue?

aignas · 2024-11-18T00:35:20Z

gazelle/python/file_parser.go

+	if root.HasError() {
+		log.Printf("WARNING: failed to parse %q. The resulting BUILD target may be incorrect.", path)
+
+		verbose, envExists := os.LookupEnv("GAZELLE_VERBOSE")
+		if envExists && verbose == "1" {
+			for i := 0; i < int(root.ChildCount()); i++ {
+				child := root.Child(i)
+				if child.IsError() {
+					log.Printf("Parse error at %+v:\n%+v", child.StartPoint(), child.Content(code))
+					log.Printf("The above was parsed as: %v", child.String())
+				}
+			}
+		}
+	}


nit, it is much more idiomatic to do early returns:

Suggested change

if root.HasError() {

log.Printf("WARNING: failed to parse %q. The resulting BUILD target may be incorrect.", path)

verbose, envExists := os.LookupEnv("GAZELLE_VERBOSE")

if envExists && verbose == "1" {

for i := 0; i < int(root.ChildCount()); i++ {

child := root.Child(i)

if child.IsError() {

log.Printf("Parse error at %+v:\n%+v", child.StartPoint(), child.Content(code))

log.Printf("The above was parsed as: %v", child.String())

}

}

}

}

if !root.HasError() {

return root, nil

}

log.Printf("WARNING: failed to parse %q. The resulting BUILD target may be incorrect.", path)

// TODO: if root.HasError, should we still return err=nil?

verbose, envExists := os.LookupEnv("GAZELLE_VERBOSE")

if !envExists || verbose != "1" {

return root, nil

}

for i := 0; i < int(root.ChildCount()); i++ {

child := root.Child(i)

if child.IsError() {

log.Printf("Parse error at %+v:\n%+v", child.StartPoint(), child.Content(code))

log.Printf("The above was parsed as: %v", child.String())

}

}

return root, nil

Thanks! I'll update to that in a followup PR.

dougthor42

if we should return err=nil if root.HasError(). If the Python file cannot be parsed, why should we continue?

IMO we should continue on parse errors. It's possible that the parse error is unrelated to anything that gazelle cares about, and in that case the resulting target definition will be correct.

dougthor42 · 2024-11-18T16:58:44Z

CHANGELOG.md

@@ -61,7 +61,8 @@ Unreleased changes template.

 {#v0-0-0-added}
 ### Added
-* Nothing yet.
+* (gazelle): Parser failures will now be logged to the terminal. Additional
+  details can be logged by setting `GAZELLE_VERBOSE=1`.


TIL! ok I'll add that soon.

And change it to be prefixed with RULES_PYTHON_ to match the others.

dougthor42 · 2024-11-18T16:59:12Z

gazelle/python/file_parser.go

+					log.Printf("Parse error at %+v:\n%+v", child.StartPoint(), child.Content(code))
+					log.Printf("The above was parsed as: %v", child.String())


Lol this was a TODO for me but now it's merged. I'll add this to the followup PR.

dougthor42 · 2024-11-18T17:03:25Z

gazelle/python/file_parser.go

+	if root.HasError() {
+		log.Printf("WARNING: failed to parse %q. The resulting BUILD target may be incorrect.", path)
+
+		verbose, envExists := os.LookupEnv("GAZELLE_VERBOSE")
+		if envExists && verbose == "1" {
+			for i := 0; i < int(root.ChildCount()); i++ {
+				child := root.Child(i)
+				if child.IsError() {
+					log.Printf("Parse error at %+v:\n%+v", child.StartPoint(), child.Content(code))
+					log.Printf("The above was parsed as: %v", child.String())
+				}
+			}
+		}
+	}


Thanks! I'll update to that in a followup PR.

… comments (#2428) Address PR comments from #2420: + Add and update comments + idiomatic go: return early + rename and document env var

dougthor42 added 3 commits November 17, 2024 05:05

feat(gazelle): Add GAZELLE_VERBOSE env and log parser failure.

953ad39

update changelog

534142c

spelling

4d9f1c6

dougthor42 commented Nov 17, 2024

View reviewed changes

dougthor42 marked this pull request as ready for review November 17, 2024 05:24

dougthor42 requested review from f0rmiga, rickeylev and aignas as code owners November 17, 2024 05:24

rickeylev changed the title ~~feat(gazelle): Add GAZELLE_VERBOSE env and log parser failures~~ refactor: Add GAZELLE_VERBOSE env and log parser failures Nov 18, 2024

rickeylev approved these changes Nov 18, 2024

View reviewed changes

rickeylev and others added 2 commits November 17, 2024 16:24

fix typo

5982c29

Merge branch 'main' into u/dthor/parser-logging

d1838e0

rickeylev enabled auto-merge November 18, 2024 00:25

aignas approved these changes Nov 18, 2024

View reviewed changes

rickeylev added this pull request to the merge queue Nov 18, 2024

Merged via the queue into bazelbuild:main with commit b28db69 Nov 18, 2024
4 checks passed

dougthor42 commented Nov 18, 2024

View reviewed changes

dougthor42 mentioned this pull request Nov 19, 2024

chore(gazelle): Rename GAZELLE_VERBOSE env var; use idiomatic go; add comments #2428

Merged

dougthor42 deleted the u/dthor/parser-logging branch November 19, 2024 18:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Add GAZELLE_VERBOSE env and log parser failures #2420

refactor: Add GAZELLE_VERBOSE env and log parser failures #2420

dougthor42 commented Nov 17, 2024

dougthor42 Nov 17, 2024

dougthor42 Nov 17, 2024

dougthor42 Nov 18, 2024

rickeylev left a comment

rickeylev Nov 18, 2024

dougthor42 Nov 18, 2024

aignas left a comment

aignas Nov 18, 2024

dougthor42 Nov 18, 2024

dougthor42 left a comment

dougthor42 Nov 18, 2024

dougthor42 Nov 18, 2024

dougthor42 Nov 18, 2024

		verbose, envExists := os.LookupEnv("GAZELLE_VERBOSE")
		if envExists && verbose == "1" {

		log.Printf("Parse error at %+v:\n%+v", child.StartPoint(), child.Content(code))
		log.Printf("The above was parsed as: %v", child.String())

refactor: Add GAZELLE_VERBOSE env and log parser failures #2420

refactor: Add GAZELLE_VERBOSE env and log parser failures #2420

Conversation

dougthor42 commented Nov 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickeylev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aignas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougthor42 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment