From 294598a962f635695ee4ce820035cee01ee6e67e Mon Sep 17 00:00:00 2001 From: Maxwell Elliott Date: Mon, 2 Nov 2026 11:49:41 -0500 Subject: [PATCH 1/3] Reproducer for #603: label-only extension bump reaches modules depending on the owner Adds the extension_owner_dependent_module fixture: module `ext` owns an extension generating a hub and spokes, and module `dependent` only has a bazel_dep on `ext`. With the extension listed in --labelOnlyModuleExtensions, bumping one spoke still impacts //:uses_dependent, a target that uses a rule of `dependent` and names nothing the extension generates, because //external:dependent depends on the full //external:ext node. The test also pins that a change to `ext` itself must keep reaching `dependent`. Co-Authored-By: Claude Opus 5.5 --- tests/e2e/external.rs | 51 +++++++++++++++++++ .../.bazelignore | 2 + .../BUILD.bazel | 18 +++++++ .../MODULE.bazel | 15 ++++++ .../dependent/BUILD.bazel | 1 + .../dependent/MODULE.bazel | 3 ++ .../dependent/defs.bzl | 10 ++++ .../dependent/tool.sh | 1 + .../ext/BUILD.bazel | 1 + .../ext/MODULE.bazel | 6 +++ .../ext/crates.bzl | 30 +++++++++++ .../ext/defs.bzl | 10 ++++ .../ext/tool.sh | 1 + .../input.txt | 1 + .../versions.json | 1 + tools/e2e/rust_e2e_cases.bzl | 4 ++ 16 files changed, 155 insertions(+) create mode 100644 tests/resources/workspaces/extension_owner_dependent_module/.bazelignore create mode 100644 tests/resources/workspaces/extension_owner_dependent_module/BUILD.bazel create mode 100644 tests/resources/workspaces/extension_owner_dependent_module/MODULE.bazel create mode 100644 tests/resources/workspaces/extension_owner_dependent_module/dependent/BUILD.bazel create mode 100644 tests/resources/workspaces/extension_owner_dependent_module/dependent/MODULE.bazel create mode 100644 tests/resources/workspaces/extension_owner_dependent_module/dependent/defs.bzl create mode 100644 tests/resources/workspaces/extension_owner_dependent_module/dependent/tool.sh create mode 100644 tests/resources/workspaces/extension_owner_dependent_module/ext/BUILD.bazel create mode 100644 tests/resources/workspaces/extension_owner_dependent_module/ext/MODULE.bazel create mode 100644 tests/resources/workspaces/extension_owner_dependent_module/ext/crates.bzl create mode 100644 tests/resources/workspaces/extension_owner_dependent_module/ext/defs.bzl create mode 100644 tests/resources/workspaces/extension_owner_dependent_module/ext/tool.sh create mode 100644 tests/resources/workspaces/extension_owner_dependent_module/input.txt create mode 100644 tests/resources/workspaces/extension_owner_dependent_module/versions.json diff --git a/tests/e2e/external.rs b/tests/e2e/external.rs index 7bbc1231..d62d5264 100644 --- a/tests/e2e/external.rs +++ b/tests/e2e/external.rs @@ -370,6 +370,57 @@ fn fine_grained_spoke_does_not_reach_its_siblings_through_the_owning_module() { ); } +// Issue #603. Module `dependent` has a `bazel_dep` on `ext`, which owns the `@crates` extension, +// the way rules_js reaches gazelle (owner of `go_deps`) through protobuf and rules_android. In +// query mode `//external:ext` depends on every repository its extensions generate, and +// `//external:dependent` depends on `//external:ext`, so bumping spoke `b` impacts every target +// that uses a rule of `dependent` -- here `//:uses_dependent`, which names nothing in `@crates`. +// `--labelOnlyModuleExtensions` gives the spokes themselves a view of `ext` without its extension +// repositories, but a dependent module still reaches the full node. With the extension listed, +// the bump should impact only `//:uses_b`; a change to `ext` itself must still reach +// `//:uses_dependent`. +#[test] +fn label_only_extension_bump_does_not_reach_modules_depending_on_the_owner() { + if !supports_mod_show_repo() { + return; + } + let fine_grained = [ + "--fineGrainedHashExternalRepos", + "@crates,@@ext++crates+crates__a,@@ext++crates+crates__b", + "--labelOnlyModuleExtensions", + "@ext//:crates.bzl%crates", + ]; + let main = |impacted: BTreeSet| -> BTreeSet { + impacted + .into_iter() + .map(|label| label.trim_start_matches("@@").to_owned()) + .filter(|label| label.starts_with("//") && !label.starts_with("//external:")) + .collect() + }; + let first = copy_workspace("extension_owner_dependent_module"); + let bumped = copy_workspace("extension_owner_dependent_module"); + edit(&bumped.path().join("versions.json"), |text| { + text.replace("\"b\": \"1.0\"", "\"b\": \"2.0\"") + }); + // Without the opt-in the conservative owner edge stays, and the bump reaches the dependent + // module's rule. This is the behavior the issue reports. + let impacted = main(diff_local(first.path(), bumped.path(), &fine_grained[..2])); + assert!(impacted.contains("//:uses_dependent"), "{impacted:?}"); + + let impacted = main(diff_local(first.path(), bumped.path(), &fine_grained)); + assert_eq!(impacted, BTreeSet::from(["//:uses_b".to_owned()])); + + // `dependent` really depends on `ext`: a change to `ext`'s own tree must still reach it. + let changed_owner = copy_workspace("extension_owner_dependent_module"); + fs::write(changed_owner.path().join("ext/tool.sh"), "exit 1\n").unwrap(); + let impacted = main(diff_local( + first.path(), + changed_owner.path(), + &fine_grained, + )); + assert!(impacted.contains("//:uses_dependent"), "{impacted:?}"); +} + const ISOLATED_ROOT: &[&str] = &["--isolatedDependencyRoot", "@hub"]; fn assert_product(workspace: &std::path::Path, target: &str, file: &str, expected: &str) { diff --git a/tests/resources/workspaces/extension_owner_dependent_module/.bazelignore b/tests/resources/workspaces/extension_owner_dependent_module/.bazelignore new file mode 100644 index 00000000..f530c4a8 --- /dev/null +++ b/tests/resources/workspaces/extension_owner_dependent_module/.bazelignore @@ -0,0 +1,2 @@ +ext +dependent diff --git a/tests/resources/workspaces/extension_owner_dependent_module/BUILD.bazel b/tests/resources/workspaces/extension_owner_dependent_module/BUILD.bazel new file mode 100644 index 00000000..eb14633e --- /dev/null +++ b/tests/resources/workspaces/extension_owner_dependent_module/BUILD.bazel @@ -0,0 +1,18 @@ +load("@dependent//:defs.bzl", "wrap") + +filegroup( + name = "uses_a", + srcs = ["@crates//:a"], +) + +filegroup( + name = "uses_b", + srcs = ["@crates//:b"], +) + +# Uses a rule of `dependent` and nothing from `@crates`, like a `copy_file` from +# bazel_lib or a `jest_test` from rules_jest. +wrap( + name = "uses_dependent", + srcs = ["input.txt"], +) diff --git a/tests/resources/workspaces/extension_owner_dependent_module/MODULE.bazel b/tests/resources/workspaces/extension_owner_dependent_module/MODULE.bazel new file mode 100644 index 00000000..2f53a439 --- /dev/null +++ b/tests/resources/workspaces/extension_owner_dependent_module/MODULE.bazel @@ -0,0 +1,15 @@ +module(name = "extension_owner_dependent_module_test") + +bazel_dep(name = "ext") +local_path_override(module_name = "ext", path = "ext") + +bazel_dep(name = "dependent") +local_path_override(module_name = "dependent", path = "dependent") + +# Module `ext` owns an extension that generates a hub `@crates` and one spoke per +# entry of versions.json, like gazelle's `go_deps` or rules_js's `npm`. Module +# `dependent` merely has a `bazel_dep` on `ext`, like rules_js -> protobuf -> +# rules_android -> gazelle. See issue #603 and external.rs. +crates = use_extension("@ext//:crates.bzl", "crates") +crates.lock(file = "//:versions.json") +use_repo(crates, "crates") diff --git a/tests/resources/workspaces/extension_owner_dependent_module/dependent/BUILD.bazel b/tests/resources/workspaces/extension_owner_dependent_module/dependent/BUILD.bazel new file mode 100644 index 00000000..13bdef8e --- /dev/null +++ b/tests/resources/workspaces/extension_owner_dependent_module/dependent/BUILD.bazel @@ -0,0 +1 @@ +exports_files(["tool.sh"]) diff --git a/tests/resources/workspaces/extension_owner_dependent_module/dependent/MODULE.bazel b/tests/resources/workspaces/extension_owner_dependent_module/dependent/MODULE.bazel new file mode 100644 index 00000000..d236159f --- /dev/null +++ b/tests/resources/workspaces/extension_owner_dependent_module/dependent/MODULE.bazel @@ -0,0 +1,3 @@ +module(name = "dependent") + +bazel_dep(name = "ext") diff --git a/tests/resources/workspaces/extension_owner_dependent_module/dependent/defs.bzl b/tests/resources/workspaces/extension_owner_dependent_module/dependent/defs.bzl new file mode 100644 index 00000000..ed48e425 --- /dev/null +++ b/tests/resources/workspaces/extension_owner_dependent_module/dependent/defs.bzl @@ -0,0 +1,10 @@ +def _wrap_impl(ctx): + return [DefaultInfo(files = depset(ctx.files.srcs))] + +wrap = rule( + implementation = _wrap_impl, + attrs = { + "srcs": attr.label_list(allow_files = True), + "_tool": attr.label(default = "@dependent//:tool.sh", allow_single_file = True), + }, +) diff --git a/tests/resources/workspaces/extension_owner_dependent_module/dependent/tool.sh b/tests/resources/workspaces/extension_owner_dependent_module/dependent/tool.sh new file mode 100644 index 00000000..ca916d09 --- /dev/null +++ b/tests/resources/workspaces/extension_owner_dependent_module/dependent/tool.sh @@ -0,0 +1 @@ +exit 0 diff --git a/tests/resources/workspaces/extension_owner_dependent_module/ext/BUILD.bazel b/tests/resources/workspaces/extension_owner_dependent_module/ext/BUILD.bazel new file mode 100644 index 00000000..13bdef8e --- /dev/null +++ b/tests/resources/workspaces/extension_owner_dependent_module/ext/BUILD.bazel @@ -0,0 +1 @@ +exports_files(["tool.sh"]) diff --git a/tests/resources/workspaces/extension_owner_dependent_module/ext/MODULE.bazel b/tests/resources/workspaces/extension_owner_dependent_module/ext/MODULE.bazel new file mode 100644 index 00000000..deccca41 --- /dev/null +++ b/tests/resources/workspaces/extension_owner_dependent_module/ext/MODULE.bazel @@ -0,0 +1,6 @@ +module(name = "ext") + +# Like a rules module that uses its own extension for its tools: `ext` imports a +# repository the extension generates, which defines no toolchain. +crates = use_extension("//:crates.bzl", "crates") +use_repo(crates, "ext_tools") diff --git a/tests/resources/workspaces/extension_owner_dependent_module/ext/crates.bzl b/tests/resources/workspaces/extension_owner_dependent_module/ext/crates.bzl new file mode 100644 index 00000000..fbe7d7f7 --- /dev/null +++ b/tests/resources/workspaces/extension_owner_dependent_module/ext/crates.bzl @@ -0,0 +1,30 @@ +def _spoke_impl(rctx): + rctx.file("data.txt", rctx.attr.version) + rctx.file("BUILD.bazel", """load("@ext//:defs.bzl", "crate") +crate(name = "lib", srcs = ["data.txt"], visibility = ["//visibility:public"]) +""") + +spoke = repository_rule(implementation = _spoke_impl, attrs = {"version": attr.string()}) + +def _hub_impl(rctx): + rctx.file("BUILD.bazel", "\n".join([ + 'alias(name = "{0}", actual = "@crates__{0}//:lib", visibility = ["//visibility:public"])'.format(name) + for name in rctx.attr.names + ])) + +hub = repository_rule(implementation = _hub_impl, attrs = {"names": attr.string_list()}) + +def _crates_impl(mctx): + versions = {} + for mod in mctx.modules: + for lock in mod.tags.lock: + versions.update(json.decode(mctx.read(lock.file))) + for name, version in versions.items(): + spoke(name = "crates__" + name, version = version) + hub(name = "crates", names = sorted(versions)) + hub(name = "ext_tools", names = []) + +crates = module_extension( + implementation = _crates_impl, + tag_classes = {"lock": tag_class(attrs = {"file": attr.label()})}, +) diff --git a/tests/resources/workspaces/extension_owner_dependent_module/ext/defs.bzl b/tests/resources/workspaces/extension_owner_dependent_module/ext/defs.bzl new file mode 100644 index 00000000..86fd4345 --- /dev/null +++ b/tests/resources/workspaces/extension_owner_dependent_module/ext/defs.bzl @@ -0,0 +1,10 @@ +def _crate_impl(ctx): + return [DefaultInfo(files = depset(ctx.files.srcs))] + +crate = rule( + implementation = _crate_impl, + attrs = { + "srcs": attr.label_list(allow_files = True), + "_tool": attr.label(default = "@ext//:tool.sh", allow_single_file = True), + }, +) diff --git a/tests/resources/workspaces/extension_owner_dependent_module/ext/tool.sh b/tests/resources/workspaces/extension_owner_dependent_module/ext/tool.sh new file mode 100644 index 00000000..ca916d09 --- /dev/null +++ b/tests/resources/workspaces/extension_owner_dependent_module/ext/tool.sh @@ -0,0 +1 @@ +exit 0 diff --git a/tests/resources/workspaces/extension_owner_dependent_module/input.txt b/tests/resources/workspaces/extension_owner_dependent_module/input.txt new file mode 100644 index 00000000..ce013625 --- /dev/null +++ b/tests/resources/workspaces/extension_owner_dependent_module/input.txt @@ -0,0 +1 @@ +hello diff --git a/tests/resources/workspaces/extension_owner_dependent_module/versions.json b/tests/resources/workspaces/extension_owner_dependent_module/versions.json new file mode 100644 index 00000000..1dd29ea0 --- /dev/null +++ b/tests/resources/workspaces/extension_owner_dependent_module/versions.json @@ -0,0 +1 @@ +{"a": "1.0", "b": "1.0"} diff --git a/tools/e2e/rust_e2e_cases.bzl b/tools/e2e/rust_e2e_cases.bzl index 53cda3f8..405240c3 100644 --- a/tools/e2e/rust_e2e_cases.bzl +++ b/tools/e2e/rust_e2e_cases.bzl @@ -131,6 +131,10 @@ RUST_E2E_SUITES = [ "name": "external::isolated_dependency_root_tracks_repository_read_bytes", "timeout": "moderate", }, + { + "name": "external::label_only_extension_bump_does_not_reach_modules_depending_on_the_owner", + "timeout": "moderate", + }, { "name": "external::remote_proto_bump_impacts_consumer", "timeout": "moderate", From 1f29a1668037661e091284a0b5ca2e6ddd0f3c07 Mon Sep 17 00:00:00 2001 From: Maxwell Elliott Date: Mon, 2 Nov 2026 21:43:07 -0500 Subject: [PATCH 2/3] Extend #603 reproducer: owner's own rule users, versioned spokes, no fine-graining Per the reporter's feedback on #603, the fixture now also covers the npm half of the issue: - //:uses_ext uses `ext`'s own rule directly (like a js_binary using rules_js), with no dependent module on the path. - Spokes are named after their version (like npm__user-agents__2.1.229), so a bump replaces a spoke instead of changing it in place. - `dependent` reaches `ext` through `middle`, two module hops. - The main check runs without --fineGrainedHashExternalRepos, the reporter's configuration; a second run fine-grains only the @crates hub (a versioned spoke can't be named in a flag shared by both revisions, since Bazel rejects an undefined repository in the query). Today, with the extension listed in --labelOnlyModuleExtensions, the bump impacts //:uses_ext and //:uses_dependent in both configurations. Co-Authored-By: Claude Opus 5.5 --- tests/e2e/external.rs | 62 +++++++++++-------- .../.bazelignore | 1 + .../BUILD.bazel | 12 +++- .../MODULE.bazel | 10 ++- .../dependent/MODULE.bazel | 2 +- .../ext/crates.bzl | 16 +++-- .../middle/BUILD.bazel | 0 .../middle/MODULE.bazel | 3 + tools/e2e/rust_e2e_cases.bzl | 2 +- 9 files changed, 70 insertions(+), 38 deletions(-) create mode 100644 tests/resources/workspaces/extension_owner_dependent_module/middle/BUILD.bazel create mode 100644 tests/resources/workspaces/extension_owner_dependent_module/middle/MODULE.bazel diff --git a/tests/e2e/external.rs b/tests/e2e/external.rs index d62d5264..86ae78e4 100644 --- a/tests/e2e/external.rs +++ b/tests/e2e/external.rs @@ -370,25 +370,27 @@ fn fine_grained_spoke_does_not_reach_its_siblings_through_the_owning_module() { ); } -// Issue #603. Module `dependent` has a `bazel_dep` on `ext`, which owns the `@crates` extension, -// the way rules_js reaches gazelle (owner of `go_deps`) through protobuf and rules_android. In -// query mode `//external:ext` depends on every repository its extensions generate, and -// `//external:dependent` depends on `//external:ext`, so bumping spoke `b` impacts every target -// that uses a rule of `dependent` -- here `//:uses_dependent`, which names nothing in `@crates`. -// `--labelOnlyModuleExtensions` gives the spokes themselves a view of `ext` without its extension -// repositories, but a dependent module still reaches the full node. With the extension listed, -// the bump should impact only `//:uses_b`; a change to `ext` itself must still reach -// `//:uses_dependent`. +// Issue #603. Module `ext` owns the `@crates` extension, whose spokes are named after their +// version the way rules_js's `npm` names `npm__user-agents__2.1.229`, so a bump replaces spoke `b` +// rather than changing it. In query mode `//external:ext` depends on every repository its +// extensions generate, so the bump reaches every target that uses `ext`'s own rule (`//:uses_ext`, +// like a js_binary) and every target of a module with a `bazel_dep` path to `ext` +// (`//:uses_dependent`, whose `dependent` reaches `ext` through `middle`, like rules_jest reaching +// gazelle). Neither names anything in `@crates`. `--labelOnlyModuleExtensions` gives only the +// extension's own repositories a view of `ext` without its generated repositories. With the +// extension listed, the bump should impact only the hub's consumers, with or without +// fine-graining; a change to `ext` itself must still reach both. #[test] -fn label_only_extension_bump_does_not_reach_modules_depending_on_the_owner() { +fn label_only_extension_bump_does_not_reach_users_of_the_owner_or_its_dependents() { if !supports_mod_show_repo() { return; } + let label_only = ["--labelOnlyModuleExtensions", "@ext//:crates.bzl%crates"]; let fine_grained = [ + label_only[0], + label_only[1], "--fineGrainedHashExternalRepos", - "@crates,@@ext++crates+crates__a,@@ext++crates+crates__b", - "--labelOnlyModuleExtensions", - "@ext//:crates.bzl%crates", + "@crates", ]; let main = |impacted: BTreeSet| -> BTreeSet { impacted @@ -397,28 +399,38 @@ fn label_only_extension_bump_does_not_reach_modules_depending_on_the_owner() { .filter(|label| label.starts_with("//") && !label.starts_with("//external:")) .collect() }; + let set = |labels: &[&str]| -> BTreeSet { + labels.iter().map(|label| label.to_string()).collect() + }; let first = copy_workspace("extension_owner_dependent_module"); let bumped = copy_workspace("extension_owner_dependent_module"); edit(&bumped.path().join("versions.json"), |text| { text.replace("\"b\": \"1.0\"", "\"b\": \"2.0\"") }); - // Without the opt-in the conservative owner edge stays, and the bump reaches the dependent - // module's rule. This is the behavior the issue reports. - let impacted = main(diff_local(first.path(), bumped.path(), &fine_grained[..2])); - assert!(impacted.contains("//:uses_dependent"), "{impacted:?}"); + // Without the opt-in the conservative owner edge stays and the bump reaches both, which is + // what the issue reports. + let impacted = main(diff_local(first.path(), bumped.path(), &[])); + assert!( + impacted.contains("//:uses_ext") && impacted.contains("//:uses_dependent"), + "{impacted:?}" + ); + // Without fine-graining every consumer of the `@crates` hub sees the bump. + let impacted = main(diff_local(first.path(), bumped.path(), &label_only)); + assert_eq!(impacted, set(&["//:uses_a", "//:uses_b"]), "label-only"); let impacted = main(diff_local(first.path(), bumped.path(), &fine_grained)); - assert_eq!(impacted, BTreeSet::from(["//:uses_b".to_owned()])); + assert_eq!(impacted, set(&["//:uses_b"]), "label-only, fine-grained"); - // `dependent` really depends on `ext`: a change to `ext`'s own tree must still reach it. + // `dependent` and `//:uses_ext` really depend on `ext`: a change to its own tree reaches them. let changed_owner = copy_workspace("extension_owner_dependent_module"); fs::write(changed_owner.path().join("ext/tool.sh"), "exit 1\n").unwrap(); - let impacted = main(diff_local( - first.path(), - changed_owner.path(), - &fine_grained, - )); - assert!(impacted.contains("//:uses_dependent"), "{impacted:?}"); + for flags in [&label_only[..], &fine_grained[..]] { + let impacted = main(diff_local(first.path(), changed_owner.path(), flags)); + assert!( + impacted.contains("//:uses_ext") && impacted.contains("//:uses_dependent"), + "{flags:?}: {impacted:?}" + ); + } } const ISOLATED_ROOT: &[&str] = &["--isolatedDependencyRoot", "@hub"]; diff --git a/tests/resources/workspaces/extension_owner_dependent_module/.bazelignore b/tests/resources/workspaces/extension_owner_dependent_module/.bazelignore index f530c4a8..285b146f 100644 --- a/tests/resources/workspaces/extension_owner_dependent_module/.bazelignore +++ b/tests/resources/workspaces/extension_owner_dependent_module/.bazelignore @@ -1,2 +1,3 @@ ext +middle dependent diff --git a/tests/resources/workspaces/extension_owner_dependent_module/BUILD.bazel b/tests/resources/workspaces/extension_owner_dependent_module/BUILD.bazel index eb14633e..49e77971 100644 --- a/tests/resources/workspaces/extension_owner_dependent_module/BUILD.bazel +++ b/tests/resources/workspaces/extension_owner_dependent_module/BUILD.bazel @@ -1,4 +1,5 @@ load("@dependent//:defs.bzl", "wrap") +load("@ext//:defs.bzl", "crate") filegroup( name = "uses_a", @@ -10,8 +11,15 @@ filegroup( srcs = ["@crates//:b"], ) -# Uses a rule of `dependent` and nothing from `@crates`, like a `copy_file` from -# bazel_lib or a `jest_test` from rules_jest. +# Uses `ext`'s own rule and nothing from `@crates`, like a js_binary that does not +# depend on the bumped npm package. +crate( + name = "uses_ext", + srcs = ["input.txt"], +) + +# Uses a rule of `dependent` and nothing from `@crates`, like a `jest_test` from +# rules_jest. wrap( name = "uses_dependent", srcs = ["input.txt"], diff --git a/tests/resources/workspaces/extension_owner_dependent_module/MODULE.bazel b/tests/resources/workspaces/extension_owner_dependent_module/MODULE.bazel index 2f53a439..1d357cef 100644 --- a/tests/resources/workspaces/extension_owner_dependent_module/MODULE.bazel +++ b/tests/resources/workspaces/extension_owner_dependent_module/MODULE.bazel @@ -6,10 +6,14 @@ local_path_override(module_name = "ext", path = "ext") bazel_dep(name = "dependent") local_path_override(module_name = "dependent", path = "dependent") +local_path_override(module_name = "middle", path = "middle") + # Module `ext` owns an extension that generates a hub `@crates` and one spoke per -# entry of versions.json, like gazelle's `go_deps` or rules_js's `npm`. Module -# `dependent` merely has a `bazel_dep` on `ext`, like rules_js -> protobuf -> -# rules_android -> gazelle. See issue #603 and external.rs. +# entry of versions.json, named after its version, like rules_js's `npm` or +# gazelle's `go_deps`. Root targets use `ext`'s rule directly, like a js_binary +# uses rules_js. Module `dependent` reaches `ext` only through `middle`'s +# `bazel_dep`, like rules_jest -> rules_js -> protobuf -> rules_android -> gazelle. +# See issue #603 and external.rs. crates = use_extension("@ext//:crates.bzl", "crates") crates.lock(file = "//:versions.json") use_repo(crates, "crates") diff --git a/tests/resources/workspaces/extension_owner_dependent_module/dependent/MODULE.bazel b/tests/resources/workspaces/extension_owner_dependent_module/dependent/MODULE.bazel index d236159f..67b92f3e 100644 --- a/tests/resources/workspaces/extension_owner_dependent_module/dependent/MODULE.bazel +++ b/tests/resources/workspaces/extension_owner_dependent_module/dependent/MODULE.bazel @@ -1,3 +1,3 @@ module(name = "dependent") -bazel_dep(name = "ext") +bazel_dep(name = "middle") diff --git a/tests/resources/workspaces/extension_owner_dependent_module/ext/crates.bzl b/tests/resources/workspaces/extension_owner_dependent_module/ext/crates.bzl index fbe7d7f7..e2a44e0d 100644 --- a/tests/resources/workspaces/extension_owner_dependent_module/ext/crates.bzl +++ b/tests/resources/workspaces/extension_owner_dependent_module/ext/crates.bzl @@ -8,21 +8,25 @@ spoke = repository_rule(implementation = _spoke_impl, attrs = {"version": attr.s def _hub_impl(rctx): rctx.file("BUILD.bazel", "\n".join([ - 'alias(name = "{0}", actual = "@crates__{0}//:lib", visibility = ["//visibility:public"])'.format(name) - for name in rctx.attr.names + 'alias(name = "{}", actual = "@{}//:lib", visibility = ["//visibility:public"])'.format(name, spoke) + for name, spoke in rctx.attr.spokes.items() ])) -hub = repository_rule(implementation = _hub_impl, attrs = {"names": attr.string_list()}) +hub = repository_rule(implementation = _hub_impl, attrs = {"spokes": attr.string_dict()}) def _crates_impl(mctx): versions = {} for mod in mctx.modules: for lock in mod.tags.lock: versions.update(json.decode(mctx.read(lock.file))) + spokes = {} for name, version in versions.items(): - spoke(name = "crates__" + name, version = version) - hub(name = "crates", names = sorted(versions)) - hub(name = "ext_tools", names = []) + # Like npm's `npm__user-agents__2.1.229`: a bump replaces the spoke rather + # than changing it in place. + spokes[name] = "crates__{}__{}".format(name, version.replace(".", "_")) + spoke(name = spokes[name], version = version) + hub(name = "crates", spokes = spokes) + hub(name = "ext_tools", spokes = {}) crates = module_extension( implementation = _crates_impl, diff --git a/tests/resources/workspaces/extension_owner_dependent_module/middle/BUILD.bazel b/tests/resources/workspaces/extension_owner_dependent_module/middle/BUILD.bazel new file mode 100644 index 00000000..e69de29b diff --git a/tests/resources/workspaces/extension_owner_dependent_module/middle/MODULE.bazel b/tests/resources/workspaces/extension_owner_dependent_module/middle/MODULE.bazel new file mode 100644 index 00000000..1d495a74 --- /dev/null +++ b/tests/resources/workspaces/extension_owner_dependent_module/middle/MODULE.bazel @@ -0,0 +1,3 @@ +module(name = "middle") + +bazel_dep(name = "ext") diff --git a/tools/e2e/rust_e2e_cases.bzl b/tools/e2e/rust_e2e_cases.bzl index 405240c3..14ac6d94 100644 --- a/tools/e2e/rust_e2e_cases.bzl +++ b/tools/e2e/rust_e2e_cases.bzl @@ -132,7 +132,7 @@ RUST_E2E_SUITES = [ "timeout": "moderate", }, { - "name": "external::label_only_extension_bump_does_not_reach_modules_depending_on_the_owner", + "name": "external::label_only_extension_bump_does_not_reach_users_of_the_owner_or_its_dependents", "timeout": "moderate", }, { From bd2033b04d907512467abf961ca200d6fa48b621 Mon Sep 17 00:00:00 2001 From: Maxwell Elliott Date: Mon, 2 Nov 2026 22:04:00 -0500 Subject: [PATCH 3/3] Fix #603: a label-only extension's owner no longer stands in for its repositories In query mode the module that owns an extension depends on every repository the extension generates. #559 gave targets inside a listed extension's repositories a view of the owner without them, but every other reader still reached the full owner node: a target using the owner's own rule (a js_binary on rules_js) and every module with a bazel_dep path to the owner (rules_jest -> ... -> gazelle). One go.mod or pnpm-lock.yaml bump then impacted most of the workspace. For an extension listed in --labelOnlyModuleExtensions the owner now gets no stand-in edges at all, the same way --isolatedDependencyRoot members already don't. The flag's assertion becomes "used only through labels, never by resolving a toolchain from one of them", which go_deps/npm/crate repositories satisfy. With the owner node itself reduced, the per-extension %view nodes and their per-rule lookup in hash.rs are redundant and are removed. Hub-to-spoke edges stay. Users of the flag see a one-time hash shift on the owner's node and its dependents. Co-Authored-By: Claude Opus 5.5 --- README.md | 21 ++++--- src/bazel.rs | 122 +++++++++++++++++++-------------------- src/hash.rs | 45 +-------------- src/main.rs | 8 +-- tests/e2e/external.rs | 8 +-- tools/readme_template.md | 17 +++--- 6 files changed, 89 insertions(+), 132 deletions(-) diff --git a/README.md b/README.md index 38fd2147..469d11be 100644 --- a/README.md +++ b/README.md @@ -656,7 +656,7 @@ Options: --fineGrainedHashExternalReposFile --labelOnlyModuleExtensions - Module extensions (`@rules_rs//rs:extensions.bzl%crate`) whose repositories use each other only through labels, never by resolving a toolchain from one another. A fine-grained target in one of their repositories then does not change whenever any other repository of the extension does + Module extensions (`@rules_rs//rs:extensions.bzl%crate`) whose repositories are used only through labels, never by resolving a toolchain from one of them. The module that owns such an extension then does not stand in for its repositories, so a lockfile bump impacts only the targets that depend on the bumped repository --isolatedDependencyRoot Assert that consumers outside this root-isolated usage reach it through ordinary target inputs or BUILD loads. The named root import must cover every generated repository. Keep the whole usage on each such consumer --useCquery[=] @@ -814,7 +814,7 @@ Options: --fineGrainedHashExternalReposFile --labelOnlyModuleExtensions - Module extensions (`@rules_rs//rs:extensions.bzl%crate`) whose repositories use each other only through labels, never by resolving a toolchain from one another. A fine-grained target in one of their repositories then does not change whenever any other repository of the extension does + Module extensions (`@rules_rs//rs:extensions.bzl%crate`) whose repositories are used only through labels, never by resolving a toolchain from one of them. The module that owns such an extension then does not stand in for its repositories, so a lockfile bump impacts only the targets that depend on the bumped repository --isolatedDependencyRoot Assert that consumers outside this root-isolated usage reach it through ordinary target inputs or BUILD loads. The named root import must cover every generated repository. Keep the whole usage on each such consumer --useCquery[=] @@ -914,13 +914,16 @@ A few inputs are normalized or extended on the way in: in the workspace, a `file://%workspace%` registry URL) is hashed relative to the workspace, so identical checkouts at different paths hash the same. In query mode the module that owns a module extension stands in for every repository the - extension generates, because a toolchain resolved from one of them is not a query input. A - fine-grained target inside one of those repositories that uses a rule from the owning module - therefore changes whenever any repository of the extension does. Listing the extension in - `--labelOnlyModuleExtensions` (for example `@rules_rs//rs:extensions.bzl%crate`) asserts that - its repositories use each other only through labels, so such a target reaches the owning module - without them. Do not list an extension whose targets resolve a toolchain the same extension - generates: that change would be missed. An extension whose name another `.bzl` file of the same + extension generates, because a toolchain resolved from one of them is not a query input. Every + target that uses a rule of the owning module, every target of a module with a `bazel_dep` path + to it, and every fine-grained target inside one of those repositories therefore changes whenever + any repository of the extension does: a `go.mod` bump through gazelle's `go_deps` or a + `pnpm-lock.yaml` bump through rules_js's `npm` impacts most of the workspace. Listing the + extension in `--labelOnlyModuleExtensions` (for example + `@gazelle//:extensions.bzl%go_deps` or `@rules_rs//rs:extensions.bzl%crate`) asserts that its + repositories are used only through labels, which the query reports, so the owning module no + longer stands in for them. Do not list an extension that generates a toolchain, such as + `rules_go`'s `go_sdk`: a change to it would be missed. An extension whose name another `.bzl` file of the same module also exports is reported and ignored, since Bazel tells their repositories apart only by a numeric suffix. `--isolatedDependencyRoot=@crates` asserts that every outside consumer of this diff --git a/src/bazel.rs b/src/bazel.rs index c0ad5872..c5b1abc4 100644 --- a/src/bazel.rs +++ b/src/bazel.rs @@ -49,9 +49,8 @@ pub struct BazelOptions { pub fine_grained_external_repos: BTreeSet, /// Module extensions, as `bazel mod show_extension` names them /// (`@rules_rs//rs:extensions.bzl%crate`), whose repositories use each other only through - /// labels: no target in one of them resolves a toolchain from another. A target inside such an - /// extension's repository reaches the module that owns the extension without the extension's - /// other repositories (see `extension_view_node`). + /// labels: no target resolves a toolchain from one of them. The module that owns such an + /// extension then does not stand in for its repositories (see `lower_repositories`). pub label_only_module_extensions: BTreeSet, /// A root-isolated extension import whose external consumers are discoverable through /// ordinary target inputs and BUILD loads. @@ -723,7 +722,7 @@ impl BazelOptions { }; let mut scope = IsolatedRepoScope::default(); let label_only = LabelOnlyRepositories { - sibling_extensions: self + extensions: self .label_only_extension_prefixes(&canonical_to_apparent, &extension_repos.generated), owner_exemptions: if let Some(root) = &self.isolated_dependency_root { if let Some(extension) = failed.extensions.first() { @@ -1924,7 +1923,8 @@ impl IsolatedRepoScope { #[derive(Default)] struct LabelOnlyRepositories { - sibling_extensions: BTreeSet, + /// Usage prefixes of `label_only_module_extensions`; their owners get no stand-in edges. + extensions: BTreeSet, owner_exemptions: BTreeSet, } @@ -2434,15 +2434,15 @@ fn lower_repositories( // and each imported repository of an extension depends on the siblings nobody imports (the // `@bd_crates` hub reaches its `bd_crates__*` spokes). Spokes themselves stay leaves, so no // cycle forms. - // For an extension the user lists in `label_only_module_extensions`, a target inside one of - // its repositories reaches the owning module through a view without the extension's own - // repositories (`extension_view_node`): the user asserts that it uses its siblings only - // through labels, which the query reports. Otherwise every fine-grained spoke would change - // whenever any other spoke does. The view is opt-in because a toolchain resolved from a - // sibling is invisible to a query, however it is registered. + // An extension the user lists in `label_only_module_extensions` gets no owner edges: the user + // asserts that its repositories are reached only through labels, which the query reports, + // never through a resolved toolchain. Otherwise every target of the owning module, of every + // module with a `bazel_dep` path to it, and every fine-grained spoke would change whenever + // any repository of the extension does (issue #603). It is opt-in because a toolchain + // resolved from one of them is invisible to a query, however it is registered. // An explicitly label-only imported repository needs no owner fallback. Its own node, // the hub-to-spoke inputs and the owner's edges to other generators remain. - let mut module_extension_repos = BTreeMap::>>::new(); + let mut module_extension_repos = BTreeMap::>::new(); let mut extension_spokes = BTreeMap::>::new(); if let Some(imported) = extension_edges { for repository in &repositories { @@ -2453,15 +2453,15 @@ fn lower_repositories( if let Some((module_repo, _definition_hash)) = extension_repo_owner(extension, &module_definitions).filter( |(module_repo, _definition_hash)| { - !module_repo.is_empty() && !label_only.owner_exemptions.contains(canonical) + !module_repo.is_empty() + && !label_only.owner_exemptions.contains(canonical) + && !label_only.extensions.contains(extension) }, ) { module_extension_repos .entry(module_repo.clone()) .or_default() - .entry(extension.to_owned()) - .or_default() .extend(node_names(canonical)); } if !imported.contains(canonical) { @@ -2490,8 +2490,7 @@ fn lower_repositories( .filter_map(|dependency| module_to_canonical.get(dependency)) .flat_map(|canonical| node_names(canonical)) .collect::>(); - let extensions = module_extension_repos.get(canonical); - for repos in extensions.into_iter().flat_map(BTreeMap::values) { + if let Some(repos) = module_extension_repos.get(canonical) { dependencies.extend(repos.iter().cloned()); } let spokes = extension @@ -2509,17 +2508,12 @@ fn lower_repositories( if names.is_empty() { continue; } - let views = extensions - .into_iter() - .flatten() - .filter(|(extension, _)| label_only.sibling_extensions.contains(*extension)) - .collect::>(); let files = module_files .remove(&index) .unwrap_or_else(|| prepare_repository(&mut repository, &workspace)); let rule = repository_rule(repository, files, owner_definition_hash); - // All apparent names and views have the same definition. Move it into the final output. - let mut rules = std::iter::repeat_n(rule, names.len() * (1 + views.len())); + // All apparent names have the same definition. Move it into the final output. + let mut rules = std::iter::repeat_n(rule, names.len()); for apparent in names { // Imported hubs of one extension share its ordered hidden siblings. Pass // the same allocation to the hash traversal when no owner-specific edge @@ -2543,26 +2537,11 @@ fn lower_repositories( let mut target = repository_target(rules.next().unwrap(), &apparent, inputs); transform(&mut target, shared); targets.push(target); - for (extension, repos) in &views { - let mut view = repository_target( - rules.next().unwrap(), - &extension_view_node(&apparent, extension), - &dependencies.difference(repos).cloned().collect(), - ); - transform(&mut view, None); - targets.push(view); - } } } targets } -/// The node a target in a repository of `extension` reaches its owning module `node` through: -/// the module without the extension's own repositories (see `lower_repositories`). -pub(crate) fn extension_view_node(node: &str, extension: &str) -> String { - format!("{node}%{extension}") -} - fn parse_bazel_version(text: &str) -> Option { let version = text.lines().find_map(|line| { line.strip_prefix("Build label: ") @@ -4557,7 +4536,7 @@ mod tests { } #[test] - fn apparent_names_and_views_share_one_read_of_the_repository_inputs() { + fn apparent_names_share_one_read_of_the_repository_inputs() { let workspace = tempfile::tempdir().unwrap(); let patch = workspace.path().join("dep.patch"); fs::write(&patch, "first").unwrap(); @@ -4585,10 +4564,7 @@ mod tests { &parse_repo_mapping(r#"{"before":"before+","a":"dep+","b":"dep+"}"#), &BTreeMap::new(), Some(&BTreeSet::new()), - &LabelOnlyRepositories { - sibling_extensions: BTreeSet::from(["dep++ext".into()]), - ..Default::default() - }, + &LabelOnlyRepositories::default(), workspace.path(), &mut |target, _| { // The module definitions have been hashed, but dep's targets have not been emitted. @@ -4619,9 +4595,7 @@ mod tests { [ ("//external:before", None), ("//external:a", first), - ("//external:a%dep++ext", first), ("//external:b", first), - ("//external:b%dep++ext", first), ("//external:dep++ext+spoke", Some(second.as_str())), ] ); @@ -5367,17 +5341,11 @@ exit 1 &mapping, &edges, Some(&imported), - &LabelOnlyRepositories { - sibling_extensions: BTreeSet::from([ - "rules_go++go_sdk".to_owned(), - "rules_rs++crate".to_owned(), - ]), - ..Default::default() - }, + &LabelOnlyRepositories::default(), Path::new("."), &mut materialize_repository_inputs, ); - assert_eq!(query.len(), 10); + assert_eq!(query.len(), 8); // The owning module reaches everything its extensions generate, after its module deps. assert_eq!( inputs_of(&query, "//external:rules_go"), @@ -5395,16 +5363,6 @@ exit 1 "//external:rules_rs++crate+bd_crates__anyhow-1.0.104", ] ); - // Its view from inside one extension leaves out that extension's repositories, and keeps - // those of its other usages, an isolated usage of the same extension included. - assert_eq!( - inputs_of(&query, "//external:rules_go%rules_go++go_sdk"), - ["//external:rules_rs"] - ); - assert_eq!( - inputs_of(&query, "//external:rules_rs%rules_rs++crate"), - ["//external:rules_rs++_crate+++isolated_crates+crate_repo"] - ); // An imported repository reaches the siblings of its extension nobody imports. assert_eq!( inputs_of(&query, "//external:bd_crates"), @@ -5423,6 +5381,42 @@ exit 1 .is_empty()); assert!(inputs_of(&query, "//external:+local+root_ext_repo").is_empty()); + // A label-only extension's repositories leave its owner's node, while those of its other + // usages stay, an isolated usage of the same extension included (issue #603). The + // hub-to-spoke edges remain. + let label_only = lower_repositories( + repositories(), + &mapping, + &edges, + Some(&imported), + &LabelOnlyRepositories { + extensions: BTreeSet::from([ + "rules_go++go_sdk".to_owned(), + "rules_rs++crate".to_owned(), + ]), + ..Default::default() + }, + Path::new("."), + &mut materialize_repository_inputs, + ); + assert_eq!(label_only.len(), 8); + assert_eq!( + inputs_of(&label_only, "//external:rules_go"), + ["//external:rules_rs"] + ); + assert_eq!( + inputs_of(&label_only, "//external:rules_rs"), + ["//external:rules_rs++_crate+++isolated_crates+crate_repo"] + ); + assert_eq!( + inputs_of(&label_only, "//external:bd_crates"), + ["//external:rules_rs++crate+bd_crates__anyhow-1.0.104"] + ); + assert_eq!( + inputs_of(&label_only, "//external:rules_go++go_sdk+go_toolchains"), + ["//external:rules_go++go_sdk+go_default_sdk"] + ); + let cquery = lower_repositories( repositories(), &mapping, diff --git a/src/hash.rs b/src/hash.rs index 9084eddf..5ccb0a24 100644 --- a/src/hash.rs +++ b/src/hash.rs @@ -1072,12 +1072,6 @@ impl HashContext<'_> { builder.direct(self.always_affected_seed); } - // Only a listed extension has views, so a run without the flag never resolves the - // rule's repository, which can cost a `bazel mod` call. - let extension = (!self.options.bazel.use_cquery - && !self.options.bazel.label_only_module_extensions.is_empty()) - .then(|| self.rule_extension(&rule.rule.name)) - .flatten(); let mut inputs: Vec> = if rule.input_positions.is_some() { rule.rule .rule_input @@ -1122,29 +1116,10 @@ impl HashContext<'_> { } } for (position, encoded_input) in inputs.into_iter().enumerate() { - let mut input_slot = rule + let input_slot = rule .input_positions .as_ref() .and_then(|ids| ids.get(position).copied().flatten()); - // A target in an extension's repository reaches its owning module through the view - // without that extension's repositories, when there is one (query mode only). - let encoded_input = match extension.as_deref().and_then(|extension| { - let node = encoded_input.strip_prefix("//external:")?; - let view = format!( - "//external:{}", - crate::bazel::extension_view_node(node, extension) - ); - self.rules - .get(&view) - .is_some_and(|slot| slot.rule.is_some()) - .then_some(view) - }) { - Some(view) => { - input_slot = None; - Cow::Owned(view) - } - None => encoded_input, - }; builder.direct(encoded_input.as_bytes()); let input_label = target_label(&encoded_input); match self.resolve_input(&encoded_input, input_label, input_slot, path) { @@ -1266,24 +1241,6 @@ impl HashContext<'_> { .and_then(|slot| slot.resolving(Cow::Owned(fallback.clone()))) } - /// The usage prefix of the module extension that generated the repository `label` is in - /// (`rules_rs++crate` for `@@rules_rs++crate+crates__anyhow//:anyhow`), if any. - fn rule_extension(&self, label: &str) -> Option { - let label = target_label(label); - let repo = label.strip_prefix('@')?.split_once("//")?.0; - let canonical = match repo.strip_prefix('@') { - Some("") => return None, - Some(canonical) => canonical, - None => self - .source_hasher - .external_resolver - .repo_mapping() - .get(repo)? - .as_str(), - }; - crate::bazel::extension_usage_prefix(canonical).map(str::to_owned) - } - fn rule_bzl_seed(&mut self, rule: &Rule) -> Result> { if rule.instantiation_stack.is_empty() { return Ok(None); diff --git a/src/main.rs b/src/main.rs index c182d97d..6b3cb7a4 100644 --- a/src/main.rs +++ b/src/main.rs @@ -119,10 +119,10 @@ struct HashingArgs { #[arg( long = "labelOnlyModuleExtensions", value_delimiter = ',', - help = "Module extensions (`@rules_rs//rs:extensions.bzl%crate`) whose repositories use each \ - other only through labels, never by resolving a toolchain from one another. A \ - fine-grained target in one of their repositories then does not change whenever any \ - other repository of the extension does" + help = "Module extensions (`@rules_rs//rs:extensions.bzl%crate`) whose repositories are \ + used only through labels, never by resolving a toolchain from one of them. The \ + module that owns such an extension then does not stand in for its repositories, so \ + a lockfile bump impacts only the targets that depend on the bumped repository" )] label_only_module_extensions: Vec, diff --git a/tests/e2e/external.rs b/tests/e2e/external.rs index 86ae78e4..c70d638d 100644 --- a/tests/e2e/external.rs +++ b/tests/e2e/external.rs @@ -376,10 +376,10 @@ fn fine_grained_spoke_does_not_reach_its_siblings_through_the_owning_module() { // extensions generate, so the bump reaches every target that uses `ext`'s own rule (`//:uses_ext`, // like a js_binary) and every target of a module with a `bazel_dep` path to `ext` // (`//:uses_dependent`, whose `dependent` reaches `ext` through `middle`, like rules_jest reaching -// gazelle). Neither names anything in `@crates`. `--labelOnlyModuleExtensions` gives only the -// extension's own repositories a view of `ext` without its generated repositories. With the -// extension listed, the bump should impact only the hub's consumers, with or without -// fine-graining; a change to `ext` itself must still reach both. +// gazelle). Neither names anything in `@crates`. With the extension listed in +// `--labelOnlyModuleExtensions`, `ext` no longer stands in for its repositories, so the bump +// impacts only the hub's consumers, with or without fine-graining; a change to `ext` itself must +// still reach both. #[test] fn label_only_extension_bump_does_not_reach_users_of_the_owner_or_its_dependents() { if !supports_mod_show_repo() { diff --git a/tools/readme_template.md b/tools/readme_template.md index 4173a6d6..7af963f4 100644 --- a/tools/readme_template.md +++ b/tools/readme_template.md @@ -624,13 +624,16 @@ A few inputs are normalized or extended on the way in: in the workspace, a `file://%workspace%` registry URL) is hashed relative to the workspace, so identical checkouts at different paths hash the same. In query mode the module that owns a module extension stands in for every repository the - extension generates, because a toolchain resolved from one of them is not a query input. A - fine-grained target inside one of those repositories that uses a rule from the owning module - therefore changes whenever any repository of the extension does. Listing the extension in - `--labelOnlyModuleExtensions` (for example `@rules_rs//rs:extensions.bzl%crate`) asserts that - its repositories use each other only through labels, so such a target reaches the owning module - without them. Do not list an extension whose targets resolve a toolchain the same extension - generates: that change would be missed. An extension whose name another `.bzl` file of the same + extension generates, because a toolchain resolved from one of them is not a query input. Every + target that uses a rule of the owning module, every target of a module with a `bazel_dep` path + to it, and every fine-grained target inside one of those repositories therefore changes whenever + any repository of the extension does: a `go.mod` bump through gazelle's `go_deps` or a + `pnpm-lock.yaml` bump through rules_js's `npm` impacts most of the workspace. Listing the + extension in `--labelOnlyModuleExtensions` (for example + `@gazelle//:extensions.bzl%go_deps` or `@rules_rs//rs:extensions.bzl%crate`) asserts that its + repositories are used only through labels, which the query reports, so the owning module no + longer stands in for them. Do not list an extension that generates a toolchain, such as + `rules_go`'s `go_sdk`: a change to it would be missed. An extension whose name another `.bzl` file of the same module also exports is reported and ignored, since Bazel tells their repositories apart only by a numeric suffix. `--isolatedDependencyRoot=@crates` asserts that every outside consumer of this