From 6d477be5b1a0fb4b7171b1d6b7e28d8a3c660dd7 Mon Sep 17 00:00:00 2001 From: Paul Johnston Date: Tue, 15 Sep 2026 11:03:50 -0600 Subject: [PATCH] fix: track proto_repository configuration inputs Watch YAML configs and import CSVs so changes regenerate external repositories. Have Gazelle report loaded Starlark sources for automatic tracking, and watch Go sources used to build repository Gazelle. Add regression coverage for cache invalidation, downstream propagation, Starlark symlink changes, and reuse when inputs remain unchanged. --- docs/DEVELOPMENT.md | 27 +++ pkg/language/protobuf/config.go | 15 ++ pkg/language/protobuf/config_filemode_test.go | 95 +++++++++ pkg/language/protobuf/lang.go | 2 + pkg/protoc/package_config.go | 49 ++++- rules/private/proto_repository_tools.bzl | 9 +- rules/proto/proto_repository.bzl | 20 ++ tools/test_proto_repository_inputs.py | 197 ++++++++++++++++++ 8 files changed, 409 insertions(+), 5 deletions(-) create mode 100644 tools/test_proto_repository_inputs.py diff --git a/docs/DEVELOPMENT.md b/docs/DEVELOPMENT.md index 10390fdd6..eb972d2d7 100644 --- a/docs/DEVELOPMENT.md +++ b/docs/DEVELOPMENT.md @@ -79,3 +79,30 @@ For example `(cd /private/var/tmp/_bazel_i868039/7b08591af2b3d71f45f2e4029050db37/bazel_testing/bazel_go_test/main && code .)`. You can then use `bazel build` directly in this space to explore what's going on. + +## Repository input tracking + +`proto_repository` watches its `cfgs` YAML files and `imports` CSV files. +Gazelle also writes a temporary JSON manifest through +`-proto_config_inputs_out` listing the Starlark plugin and rule files loaded +from YAML or `-proto_plugin` / `-proto_rule`. The repository rule watches those +files and removes the manifest. Sources inside the fetched repository are +already covered by the repository definition and are not watched separately. +The repository Gazelle tool also watches its Go sources so source changes +rebuild the executable. + +The following regression test uses real Bazel and Gazelle in a temporary +workspace. It checks configuration, CSV, and Starlark edits, propagation to a +consumer repository, and reuse when no inputs change. Archives and dependencies +are supplied locally; the test does not require network access. + +```sh +go build -mod=vendor -o /tmp/rules-proto-gazelle ./cmd/gazelle +python3 tools/test_proto_repository_inputs.py \ + --bazel "$(command -v bazel)" \ + --gazelle /tmp/rules-proto-gazelle \ + --gazelle-repo "$(bazel info output_base)/external/gazelle+" +``` + +`--gazelle-repo` must point to the fetched bazel-gazelle sources; adjust the +canonical repository name if your Bazel version uses a different spelling. diff --git a/pkg/language/protobuf/config.go b/pkg/language/protobuf/config.go index 18693a8b7..cb97dec04 100644 --- a/pkg/language/protobuf/config.go +++ b/pkg/language/protobuf/config.go @@ -1,9 +1,11 @@ package protobuf import ( + "encoding/json" "flag" "fmt" "log" + "os" "strings" "github.com/bazelbuild/bazel-gazelle/config" @@ -20,6 +22,9 @@ func (pl *protobufLang) RegisterFlags(fs *flag.FlagSet, cmd string, c *config.Co fs.StringVar(&pl.configFiles, "proto_configs", "", "optional config.yaml file(s) that provide preconfiguration") + fs.StringVar(&pl.configInputsOutFile, + "proto_config_inputs_out", "", + "write loaded Starlark source paths as JSON for repository input tracking") fs.StringVar(&pl.importsInFiles, "proto_imports_in", "", "index files to parse and load symbols from") @@ -74,6 +79,16 @@ func (pl *protobufLang) CheckFlags(fs *flag.FlagSet, c *config.Config) error { } } + if pl.configInputsOutFile != "" { + data, err := json.Marshal(protoc.StarlarkFiles(c)) + if err != nil { + return err + } + if err := os.WriteFile(pl.configInputsOutFile, data, 0o644); err != nil { + return fmt.Errorf("writing -proto_config_inputs_out: %w", err) + } + } + return nil } diff --git a/pkg/language/protobuf/config_filemode_test.go b/pkg/language/protobuf/config_filemode_test.go index cc7751c94..6fc586c7d 100644 --- a/pkg/language/protobuf/config_filemode_test.go +++ b/pkg/language/protobuf/config_filemode_test.go @@ -1,6 +1,12 @@ package protobuf import ( + "encoding/json" + "flag" + "fmt" + "os" + "path/filepath" + "reflect" "testing" "github.com/bazelbuild/bazel-gazelle/config" @@ -35,3 +41,92 @@ func TestGetOrCreatePackageConfig_RebindsConfig(t *testing.T) { t.Errorf("expected FileMode via cfg.Config, got %v", gproto.GetProtoConfig(cfg.Config).Mode) } } + +// The manifest must use the paths Gazelle actually loads, including the source +// root fallback used when Gazelle runs inside a fetched repository. +func TestConfigInputsManifest(t *testing.T) { + for _, sourceRootFallback := range []bool{false, true} { + t.Run(fmt.Sprint(sourceRootFallback), func(t *testing.T) { + dir := t.TempDir() + workDir := dir + if sourceRootFallback { + workDir = filepath.Join(dir, "external") + if err := os.Mkdir(workDir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(workDir, "DO_NOT_BUILD_HERE"), []byte(dir), 0o644); err != nil { + t.Fatal(err) + } + } + // Unique registry keys allow repeated tests in the same process. + name := filepath.Base(filepath.Dir(dir)) + fmt.Sprint(sourceRootFallback) + ".star" + filename := filepath.Join(dir, name) + code := ` +protoc.Plugin(name = "yaml_plugin", configure = lambda ctx: None) +protoc.Plugin(name = "flag_plugin", configure = lambda ctx: None) +protoc.Rule( + name = "yaml_rule", + load_info = lambda: None, + kind_info = lambda: None, + provide_rule = lambda rctx, pctx: None, +) +` + if err := os.WriteFile(filename, []byte(code), 0o644); err != nil { + t.Fatal(err) + } + yaml := fmt.Sprintf("starlarkPlugins:\n - %s%%yaml_plugin\nstarlarkRules:\n - %s%%yaml_rule\n", name, name) + cfgFile := filepath.Join(dir, "config.yaml") + if err := os.WriteFile(cfgFile, []byte(yaml), 0o644); err != nil { + t.Fatal(err) + } + out := filepath.Join(dir, "inputs.json") + c := config.New() + c.WorkDir = workDir + pl := NewProtobufLang("protobuf") + fs := flag.NewFlagSet("test", flag.ContinueOnError) + pl.RegisterFlags(fs, "update", c) + if err := fs.Parse([]string{"-proto_configs", cfgFile, "-proto_plugin", name + "%flag_plugin", "-proto_config_inputs_out", out}); err != nil { + t.Fatal(err) + } + if err := pl.CheckFlags(fs, c); err != nil { + t.Fatal(err) + } + data, err := os.ReadFile(out) + if err != nil { + t.Fatal(err) + } + var got []string + if err := json.Unmarshal(data, &got); err != nil { + t.Fatal(err) + } + if want := []string{filename}; !reflect.DeepEqual(got, want) { + t.Fatalf("manifest = %v, want %v", got, want) + } + }) + } +} + +func TestConfigInputsManifestEmpty(t *testing.T) { + c := config.New() + pl := NewProtobufLang("protobuf") + out := filepath.Join(t.TempDir(), "inputs.json") + fs := flag.NewFlagSet("test", flag.ContinueOnError) + pl.RegisterFlags(fs, "update", c) + if err := fs.Parse([]string{"-proto_config_inputs_out", out}); err != nil { + t.Fatal(err) + } + if err := pl.CheckFlags(fs, c); err != nil { + t.Fatal(err) + } + data, err := os.ReadFile(out) + if err != nil { + t.Fatal(err) + } + if string(data) != "[]" { + t.Fatalf("manifest = %s, want []", data) + } + pl.configInputsOutFile = filepath.Join(out, "cannot-write.json") + if err := pl.CheckFlags(fs, c); err == nil { + t.Fatal("expected an error writing the manifest") + } +} diff --git a/pkg/language/protobuf/lang.go b/pkg/language/protobuf/lang.go index 343c3d1b9..3bb472c7b 100644 --- a/pkg/language/protobuf/lang.go +++ b/pkg/language/protobuf/lang.go @@ -24,6 +24,8 @@ type protobufLang struct { packages map[string]*protoc.Package // configFiles contains yconfig yaml files to parse. May be comma-separated. configFiles string + // configInputsOutFile is a JSON manifest of loaded Starlark source paths. + configInputsOutFile string // repoName is the name (if this an external repository) repoName string // importsOutFile is the name of the file to create. If "", skip writing diff --git a/pkg/protoc/package_config.go b/pkg/protoc/package_config.go index e9ddf9ca4..531490c95 100644 --- a/pkg/protoc/package_config.go +++ b/pkg/protoc/package_config.go @@ -3,6 +3,7 @@ package protoc import ( "fmt" "log" + "path/filepath" "sort" "strings" @@ -265,9 +266,16 @@ func RegisterStarlarkPlugin(c *config.Config, starlarkPlugin string) error { if len(parts) != 2 { return fmt.Errorf("invalid starlark plugin name %q", starlarkPlugin) } - fileName := parts[0] + fileName, err := resolveStarlarkFilename(c.WorkDir, parts[0]) + if err != nil { + return err + } + fileName, err = filepath.Abs(fileName) + if err != nil { + return err + } ruleName := parts[1] - impl, err := LoadStarlarkPluginFromFile(c.WorkDir, fileName, ruleName, func(msg string) { + impl, err := LoadStarlarkPluginFromFile("", fileName, ruleName, func(msg string) { log.Printf("%s: %v", starlarkPlugin, msg) }, func(err error) { log.Fatalf("starlark plugin configuration error (plugin %q will not be registered): %v", starlarkPlugin, err) @@ -275,6 +283,7 @@ func RegisterStarlarkPlugin(c *config.Config, starlarkPlugin string) error { if err != nil { return err } + recordStarlarkFile(c, fileName) Plugins().RegisterPlugin(starlarkPlugin, impl) return nil } @@ -284,16 +293,48 @@ func RegisterStarlarkRule(c *config.Config, starlarkRule string) error { if len(parts) != 2 { return fmt.Errorf("invalid starlark rule name %q", starlarkRule) } - fileName := parts[0] + fileName, err := resolveStarlarkFilename(c.WorkDir, parts[0]) + if err != nil { + return err + } + fileName, err = filepath.Abs(fileName) + if err != nil { + return err + } ruleName := parts[1] - impl, err := LoadStarlarkLanguageRuleFromFile(c.WorkDir, fileName, ruleName, func(msg string) { + impl, err := LoadStarlarkLanguageRuleFromFile("", fileName, ruleName, func(msg string) { }, func(err error) { log.Panicf("starlark rule configuration error (rule %q will not be registered): %v", starlarkRule, err) }) if err != nil { return err } + recordStarlarkFile(c, fileName) Rules().MustRegisterRule(starlarkRule, impl) return nil } + +const starlarkFilesKey = "rules_proto_starlark_files" + +func recordStarlarkFile(c *config.Config, filename string) { + files, ok := c.Exts[starlarkFilesKey].(map[string]bool) + if !ok { + files = make(map[string]bool) + c.Exts[starlarkFilesKey] = files + } + files[filename] = true +} + +// StarlarkFiles returns the sorted, absolute paths of plugin and rule sources +// loaded by this configuration. Repository rules use them to track inputs read +// by the Gazelle subprocess. +func StarlarkFiles(c *config.Config) []string { + files, _ := c.Exts[starlarkFilesKey].(map[string]bool) + out := make([]string, 0, len(files)) + for filename := range files { + out = append(out, filename) + } + sort.Strings(out) + return out +} diff --git a/rules/private/proto_repository_tools.bzl b/rules/private/proto_repository_tools.bzl index 957e01c90..6cec2a265 100644 --- a/rules/private/proto_repository_tools.bzl +++ b/rules/private/proto_repository_tools.bzl @@ -13,7 +13,7 @@ # limitations under the License. """""" -load("@bazel_gazelle//internal:common.bzl", "env_execute", "executable_extension") +load("@bazel_gazelle//internal:common.bzl", "env_execute", "executable_extension", "watch") load("@bazel_gazelle//internal:go_repository_cache.bzl", "read_cache_env") load("@build_stack_rules_proto//rules/private:proto_repository_tools_srcs.bzl", "PROTO_REPOSITORY_TOOLS_SRCS") @@ -33,6 +33,13 @@ def _proto_repository_tools_impl(ctx): go_tool = env["GOROOT"] + "/bin/go" + extension rules_proto_path = ctx.path(Label("@build_stack_rules_proto//:MODULE.bazel")) + + # The compiler reads sources through the symlink below. Resolve paths from + # the source root: the generated list can still name deleted packages. + for src in ctx.attr._proto_repository_tools_srcs + [ctx.attr._list_repository_tools_srcs]: + relative_path = src.package + "/" + src.name if src.package else src.name + watch(ctx, rules_proto_path.dirname.get_child(relative_path)) + ctx.symlink( rules_proto_path.dirname, "src/github.com/stackb/rules_proto/v4", diff --git a/rules/proto/proto_repository.bzl b/rules/proto/proto_repository.bzl index 8f87f99e2..352d4c795 100644 --- a/rules/proto/proto_repository.bzl +++ b/rules/proto/proto_repository.bzl @@ -318,6 +318,13 @@ def _proto_repository_impl(ctx): if is_module_extension_repo: cmd.append("-bzlmod") + # These files are read by Gazelle, not by repository_ctx. + for input in ctx.attr.cfgs + ctx.attr.imports: + watch(ctx, ctx.path(input)) + + config_inputs = ctx.path(".gazelle_config_inputs.json") + cmd.extend(["-proto_config_inputs_out", config_inputs]) + # BEGIN protobuf extension flags if ctx.attr.languages: cmd.extend(["-lang", ",".join(ctx.attr.languages)]) @@ -344,6 +351,19 @@ def _proto_repository_impl(ctx): ctx.attr.importpath, result.stderr, )) + + # YAML and command-line configuration can load Starlark files outside + # this repository. Gazelle reports the actual paths it resolved. + repo_root = str(ctx.path("")) + "/" + real_repo_root = str(ctx.path("").realpath) + "/" + for filename in json.decode(ctx.read(config_inputs, watch = "no")): + input_path = ctx.path(filename) + if str(input_path).startswith(repo_root): + input_path = input_path.realpath + if not str(input_path.realpath).startswith(real_repo_root): + watch(ctx, input_path) + ctx.delete(config_inputs) + if ctx.attr.debug_mode and result.stderr: # buildifier: disable=print print("%s gazelle.stdout: %s" % (ctx.name, result.stdout)) diff --git a/tools/test_proto_repository_inputs.py b/tools/test_proto_repository_inputs.py new file mode 100644 index 000000000..d85cbf58e --- /dev/null +++ b/tools/test_proto_repository_inputs.py @@ -0,0 +1,197 @@ +#!/usr/bin/env python3 +"""Exercise proto_repository invalidation with real Bazel and Gazelle, offline. + +Pass --gazelle (the built //cmd/gazelle binary), --gazelle-repo (the fetched +bazel-gazelle source directory), and optionally --bazel. All generated files +and Bazel state live in a temporary directory. +""" + +import argparse +import hashlib +import io +import pathlib +import shutil +import subprocess +import sys +import tarfile +import tempfile +import textwrap + + +def write(path: pathlib.Path, content: str, executable: bool = False) -> None: + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(content) + if executable: + path.chmod(0o755) + + +def main() -> None: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--bazel", default="bazel") + parser.add_argument("--gazelle", type=pathlib.Path, required=True) + parser.add_argument("--gazelle-repo", type=pathlib.Path, required=True) + args = parser.parse_args() + root = pathlib.Path(__file__).resolve().parents[1] + gazelle = args.gazelle.resolve() + with tempfile.TemporaryDirectory(prefix="proto-repository-inputs-") as tmp: + base = pathlib.Path(tmp) + workspace = base / "workspace" + workspace.mkdir() + write(workspace / "WORKSPACE", "") + write(workspace / "BUILD.bazel", 'exports_files(["config.yaml", "seed.csv"])\n') + shutil.copy(root / "rules/proto/proto_repository.bzl", workspace / "repo.bzl") + module = ['module(name = "test")'] + for name in [ + "bazel_gazelle", + "bazel_gazelle_go_repository_cache", + "bazel_gazelle_go_repository_tools", + "proto_repository_tools", + # Bazel's built-in module declares these even for a fetch-only test. + "rules_license", + "buildozer", + "platforms", + "zlib", + "rules_proto", + "bazel_features", + "protobuf", + "rules_java", + "rules_cc", + "rules_python", + "rules_shell", + "apple_support", + ]: + directory = base / name + write(directory / "MODULE.bazel", f'module(name = "{name}")\n') + write(directory / "BUILD.bazel", 'exports_files(glob(["**"]))\n') + module += [ + f'bazel_dep(name = "{name}")', + f'local_path_override(module_name = "{name}", path = "{directory}")', + ] + for name in ["common.bzl", "go_repository_cache.bzl"]: + dest = base / "bazel_gazelle/internal" / name + dest.parent.mkdir(parents=True, exist_ok=True) + shutil.copy(args.gazelle_repo / "internal" / name, dest) + write( + base / "bazel_gazelle/internal/BUILD.bazel", + 'exports_files(glob(["*.bzl"]))\n', + ) + write(base / "bazel_gazelle_go_repository_cache/go.env", "") + # The archive is local and already extracted; fetch_repo has no work. + write( + base / "bazel_gazelle_go_repository_tools/bin/fetch_repo", + "#!/bin/sh\nexit 0\n", + True, + ) + # A changing comment makes upstream regeneration observable downstream, + # even when a config edit leaves Gazelle's semantic index unchanged. + wrapper = f"#!{sys.executable}\n" + textwrap.dedent(f"""\ + import pathlib + import subprocess + import sys + import uuid + subprocess.run([{str(gazelle)!r}, *sys.argv[1:]], check=True) + with pathlib.Path('imports.csv').open('a') as out: + out.write('# generation ' + str(uuid.uuid4()) + '\\n') + """) + write(base / "proto_repository_tools/bin/gazelle", wrapper, True) + archive = base / "source.tar.gz" + with tarfile.open(archive, "w:gz") as tar: + for name, content in { + "BUILD.bazel": 'exports_files(["imports.csv"])\n', + "REPO.bazel": "", + "WORKSPACE": "", + "example.proto": 'syntax = "proto3"; package example; message Example {}\n', + # Also exercise a Starlark file inside the fetched repository: + # it must not be passed to ctx.watch, which forbids own paths. + "internal.star": 'protoc.Plugin(name = "internal", configure = lambda ctx: None)\n', + }.items(): + data = content.encode() + info = tarfile.TarInfo(name) + info.size = len(data) + tar.addfile(info, io.BytesIO(data)) + digest = hashlib.sha256(archive.read_bytes()).hexdigest() + module.append( + 'proto_repository = use_repo_rule("//:repo.bzl", "protobuf_go_repository")' + ) + common = f'urls = ["{archive.as_uri()}"], sha256 = "{digest}", build_config = "//:WORKSPACE", build_file_generation = "on"' + module += [ + f'proto_repository(name = "producer", apparent_name = "producer", cfgs = ["//:config.yaml"], imports = ["//:seed.csv"], {common})', + f'proto_repository(name = "consumer", apparent_name = "consumer", imports = ["@producer//:imports.csv"], {common})', + ] + write(workspace / "MODULE.bazel", "\n".join(module) + "\n") + config = workspace / "config.yaml" + write( + config, + "starlarkPlugins:\n - plugin.star%test\n - internal.star%internal\n", + ) + plugin = workspace / "plugin.star" + write(plugin, 'protoc.Plugin(name = "test", configure = lambda ctx: None)\n') + seed = workspace / "seed.csv" + write(seed, "# seed\n") + output = base / "output" + # Match the source-root marker used by Gazelle in external repositories. + write(output / "DO_NOT_BUILD_HERE", str(workspace)) + command = [ + args.bazel, + "--batch", + f"--output_user_root={base / 'bazel'}", + f"--output_base={output}", + "--ignore_all_rc_files", + "fetch", + "--repo=@consumer", + "--incompatible_autoload_externally=", + "--lockfile_mode=off", + "--noshow_progress", + ] + + def fetch() -> tuple[str, str]: + result = subprocess.run( + command, cwd=workspace, capture_output=True, text=True, timeout=60 + ) + if result.returncode: + raise RuntimeError(result.stdout + result.stderr) + indexes = [] + for name in ["producer", "consumer"]: + files = list((output / "external").glob(f"*+{name}/imports.csv")) + if len(files) != 1: + raise AssertionError(f"expected one {name} index, found {files}") + indexes.append(files[0].read_text()) + return indexes[0], indexes[1] + + previous = fetch() + assert fetch() == previous, "unchanged repositories should be reused" + for path in [config, seed, plugin]: + with path.open("a") as out: + out.write("\n# changed\n") + current = fetch() + assert all(a != b for a, b in zip(previous, current)), ( + f"{path.name} did not invalidate both repositories" + ) + assert fetch() == current, f"{path.name} caused unnecessary regeneration" + previous = current + print( + f"PASS: {path.name} invalidates producer and consumer; unchanged rerun is stable", + flush=True, + ) + + for name in ["first.star", "second.star"]: + target = workspace / name + write(target, plugin.read_text() + f"\n# {name}\n") + plugin.unlink() + plugin.symlink_to(target) + current = fetch() + assert all(a != b for a, b in zip(previous, current)), ( + "Starlark symlink change did not invalidate both repositories" + ) + assert fetch() == current, ( + "Starlark symlink caused unnecessary regeneration" + ) + previous = current + print( + "PASS: Starlark symlink replacement and retargeting invalidate both repositories", + flush=True, + ) + + +if __name__ == "__main__": + main()