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()