Fix #326: filter //external:* labels from impacted-targets output on bzlmod#334
Merged
tinder-maxwellelliott merged 1 commit intomasterfrom Apr 27, 2026
Conversation
…bzlmod Synthetic //external:<apparent_name> targets created by `bazel mod show_repo` are needed during generate-hashes (so dep changes are detected) but are not buildable in bzlmod-only mode (Bazel 8.6.0+ with WORKSPACE disabled), causing downstream `bazel build` of the impacted-targets file to fail with "no such package 'external'". Adds --excludeExternalTargets to get-impacted-targets (negatable, auto-defaults to true when bzlmod is detected), filters those labels at the output stage, and adds unit + e2e coverage. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
tinder-maxwellelliott
deleted the
fix-326-external-targets-in-impacted-output
branch
Contributor
|
Thanks! |
rdark
added a commit
to rdark/bazel-diff
that referenced
this pull request
Apr 28, 2026
The per-repo rdeps query-failure fallback filtered out every `@@` label on the assumption that the hash set was mostly workspace-local `//...` targets. That assumption does not hold on bzlmod-only workspaces where `generate-hashes` emits almost entirely `@@canonical` labels plus a small number of `//external:<apparent>` bzlmod-synthetic bridges, with zero workspace-local targets. On such workspaces, a single failed `rdeps(//..., @@<repo>//...)` query (for example, one caused by an unrelated loading error elsewhere in the dep graph) left the filter with only the `//external:*` bridges, which the downstream `excludeExternalTargets=true` default (Tinder#334) then strips entirely — producing silently-empty impacted output on what should be a "conservatively rebuild everything" signal. Mirror the outer catch at line 401 and mark every hashed target as impacted on per-repo query failure. Downstream filters (`targetTypes`, `excludeExternalTargets`) remain the caller's responsibility, which is the whole point of surfacing the full set upstream. Add a regression test in CalculateImpactedTargetsInteractorModuleQueryTest that constructs a bzlmod-shaped hash set (two `@@abseil-cpp+` targets + one `//external:*` bridge, no workspace targets), stubs `queryService.query` to throw, and asserts all three labels appear in the output.
rdark
added a commit
to rdark/bazel-diff
that referenced
this pull request
Apr 28, 2026
The unioned rdeps query-failure fallback filtered out every `@@` label on the assumption that the hash set was mostly workspace-local `//...` targets. That assumption does not hold on bzlmod-only workspaces where `generate-hashes` emits almost entirely `@@canonical` labels plus a small number of `//external:<apparent>` bzlmod-synthetic bridges, with zero workspace-local targets. On such workspaces, a query failure (for example, one caused by an unrelated loading error elsewhere in the dep graph) left the filter with only the `//external:*` bridges, which the downstream `excludeExternalTargets=true` default (Tinder#334) then strips entirely — producing silently-empty impacted output on what should be a "conservatively rebuild everything" signal. Mark every hashed target as impacted on query failure. Downstream filters (`targetTypes`, `excludeExternalTargets`) remain the caller's responsibility, which is the whole point of surfacing the full set upstream. Add a regression test that constructs a bzlmod-shaped hash set (two `@@abseil-cpp+` targets + one `//external:*` bridge, no workspace targets), stubs `queryService.query` to throw, and asserts all three labels appear in the output.
rdark
added a commit
to rdark/bazel-diff
that referenced
this pull request
Apr 28, 2026
The per-repo rdeps query-failure fallback filtered out every `@@` label on the assumption that the hash set was mostly workspace-local `//...` targets. That assumption does not hold on bzlmod-only workspaces where `generate-hashes` emits almost entirely `@@canonical` labels plus a small number of `//external:<apparent>` bzlmod-synthetic bridges, with zero workspace-local targets. On such workspaces, a single failed `rdeps(//..., @@<repo>//...)` query (for example, one caused by an unrelated loading error elsewhere in the dep graph) left the filter with only the `//external:*` bridges, which the downstream `excludeExternalTargets=true` default (Tinder#334) then strips entirely — producing silently-empty impacted output on what should be a "conservatively rebuild everything" signal. Replace the unconditional `!startsWith("@@")` filter with a shape-aware fallback: - Compute a "buildable workspace" set that excludes BOTH `@@canonical` transitives AND `//external:<apparent>` bridges. - If that set is non-empty (mixed WORKSPACE + bzlmod, or WORKSPACE-only shapes), emit it — preserves pre-existing granularity, avoids leaking tens of thousands of `@@` transitives into the impacted set on a single flaky `bazel query`. - If it is empty (bzlmod-only shape), fall through to `allTargets.keys` so the downstream filter has something to keep — rebuild-everything signal rather than a directly-buildable label list. Adds a mixed-shape regression test asserting that `@@` transitives and `//external:*` bridges do NOT leak into the fallback when buildable workspace labels are present, complementing the existing bzlmod-only test. An unconditional `allTargets.keys` fallback fails the new test; the pre-commit `!startsWith("@@")` filter fails the existing bzlmod-only one. Only the shape-aware fallback passes both.
rdark
added a commit
to rdark/bazel-diff
that referenced
this pull request
Apr 28, 2026
The per-repo rdeps query-failure fallback filtered out every `@@` label on the assumption that the hash set was mostly workspace-local `//...` targets. That assumption does not hold on bzlmod-only workspaces where `generate-hashes` emits almost entirely `@@canonical` labels plus a small number of `//external:<apparent>` bzlmod-synthetic bridges, with zero workspace-local targets. On such workspaces, a single failed `rdeps(//..., @@<repo>//...)` query (for example, one caused by an unrelated loading error elsewhere in the dep graph) left the filter with only the `//external:*` bridges, which the downstream `excludeExternalTargets=true` default (Tinder#334) then strips entirely — producing silently-empty impacted output on what should be a "conservatively rebuild everything" signal. Replace the unconditional `!startsWith("@@")` filter with a shape-aware fallback: - Compute a "buildable workspace" set that excludes BOTH `@@canonical` transitives AND `//external:<apparent>` bridges. - If that set is non-empty (mixed WORKSPACE + bzlmod, or WORKSPACE-only shapes), emit it — preserves pre-existing granularity, avoids leaking tens of thousands of `@@` transitives into the impacted set on a single flaky `bazel query`. - If it is empty (bzlmod-only shape), fall through to `allTargets.keys` so the downstream filter has something to keep — rebuild-everything signal rather than a directly-buildable label list. Adds a mixed-shape regression test asserting that `@@` transitives and `//external:*` bridges do NOT leak into the fallback when buildable workspace labels are present, complementing the existing bzlmod-only test. An unconditional `allTargets.keys` fallback fails the new test; the pre-commit `!startsWith("@@")` filter fails the existing bzlmod-only one. Only the shape-aware fallback passes both.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #326.
On Bazel 8.6.0+ in bzlmod-only mode (
--enable_workspace=false),bazel-diff get-impacted-targetswas emitting synthetic//external:<apparent_name>labels created byBazelClient.queryAllTargets()→BazelQueryService.queryBzlmodRepos(). Feeding the output to downstreambazel buildfailed withno such package 'external': //external package is not available since the WORKSPACE file is disabled. The synthetic labels are required duringgenerate-hashesto detect bzlmod dep version changes (#322), but they should never reach the user-facing impacted-targets output. The existing--excludeExternalTargetsflag ongenerate-hashesonly suppressed the legacy WORKSPACE-style//external:all-targetsquery — it did not affect the bzlmod synthetic targets, which is why both reporters in the issue thread had to fall back togrep -v //external.This PR adds the same flag to
get-impacted-targets, filters//external:*labels at the output stage, and auto-defaults the flag totruewheneverBazelModService.isBzlmodEnabledreturns true (matching the auto-default behavior already ingenerate-hashes). Users can opt back into the legacy behavior with--no-excludeExternalTargets.Changes
GetImpactedTargetsCommand.kt: new--excludeExternalTargets(negatable) Picocli option with bzlmod auto-default resolved at runtime via Koin.CalculateImpactedTargetsInteractor.kt: newexcludeExternalTargetsparameter on bothexecute()andexecuteWithDistances(); drops labels starting with//external:from output.CalculateImpactedTargetsInteractorTestfor both code paths, plus an e2e test gated on Bazel 8.6.0+ that uses the existingbzlmod-show-repo-test-{1,2}fixtures and asserts both default-on filtering and the--no-excludeExternalTargetsopt-out.Test plan
bazel test //cli:CalculateImpactedTargetsInteractorTestpasses locally//cli:E2ETestcases pass locally (cquery tests fail in my local sandbox due to a pre-existing JDK/coursier issue, unrelated)🤖 Generated with Claude Code