fix: handle Bazel load statements in extension-check - #7083
Conversation
|
😊 Welcome @immanuwell! This is either your first contribution to the Istio proxy repo, or it's been You can learn more about the Istio working groups, Code of Conduct, and contribution guidelines Thanks for contributing! Courtesy of your friendly welcome wagon. |
istio-testing
added
the
size/M
|
Hi @immanuwell. Thanks for your PR. I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
istio-testing
added
the
needs-rebase
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
| } | ||
| globals, err := starlark.ExecFile(thread, filename, nil, nil) | ||
| globals, err := starlark.ExecFile(thread, filename, stripLoadStatements(string(src)), nil) | ||
| if err != nil { |
There was a problem hiding this comment.
This PR uses starlark.ExecFile but master already migrated to starlark.ExecFileOptions in commit 1c3f7912. The PR is in a CONFLICTING state and needs a rebase.
The correct resolution should use the new API:
globals, err := starlark.ExecFileOptions(syntax.LegacyFileOptions(), thread, filename, stripLoadStatements(string(src)), nil)| @@ -0,0 +1,41 @@ | |||
| package main | |||
There was a problem hiding this comment.
This file is missing the required Apache 2.0 copyright header. make lint (lint-copyright-banner) will fail — it scans all *.go files for Apache License, Version 2 and Copyright. See main.go for the standard 13-line header.
| skippingLoad = false | ||
| parenDepth = 0 | ||
| } | ||
| continue |
There was a problem hiding this comment.
💡 Line-number shift in error messages
stripLoadStatements removes load-statement lines entirely, which shifts starlark error positions by the number of stripped lines. For example, a 4-line load block followed by an error on line 10 would be reported at line 6.
Consider replacing removed lines with empty strings instead of skipping them:
// Instead of: continue
filtered = append(filtered, "")| func stripLoadStatements(src string) string { | ||
| lines := strings.Split(src, "\n") | ||
| filtered := make([]string, 0, len(lines)) | ||
| skippingLoad := false |
There was a problem hiding this comment.
💡 Redundant state variable
skippingLoad is always equivalent to parenDepth > 0 at iteration boundaries. You could collapse both into just parenDepth, simplifying the logic and avoiding potential desync:
if parenDepth == 0 && strings.HasPrefix(trimmed, "load(") {
// enter skipping mode via parenDepth alone
}|
|
||
| // stripLoadStatements removes Bazel load() statements so build config files can | ||
| // be evaluated without resolving external repositories. | ||
| func stripLoadStatements(src string) string { |
There was a problem hiding this comment.
💭 Consider AST-level filtering
Text-level line removal is fragile against edge cases (parentheses in string literals, load()-like text in triple-quoted strings). The starlark library provides syntax.FileOptions.Parse() and syntax.LoadStmt — you could parse the file, filter out LoadStmt nodes from File.Stmts, and compile with starlark.FileProgram. This would handle all formatting edge cases correctly by construction and avoid the line-number shift issue above.
Not a blocker since the current approach works for the known .bzl files, but worth considering for robustness.
4 participants
What this PR does / why we need it:
tools/extension-checkblows up on normal Bazel config files because the embedded Starlark runner does not handleload(...).This strips top level
load(...)statements before evaluation, returns regular errors instead of panics, and removes the README workaround. Pretty small fix, but it makes the tool usable out of the box.Which issue this PR fixes:
N/A, this repo has issues disabled.
Special notes for your reviewer:
Repro before this patch:
go run ./tools/extension-check --envoy-extensions-build-config ./bazel/extension_config/extensions_build_config.bzl --proxy-extensions-build-config ./bazel/extension_config/extensions_build_config.bzl --ignore-extensions ./tools/extension-check/wellknown-extensionsBefore:
panic: load not implemented by this applicationAfter:
it loads the checked-in file and reaches the normal missing-extension output. so yeah, this is a real bug on a real file, not some weird edge case.
Checks:
go test ./tools/extension-checkgo test ./... -run '^$'