Skip to content

Commit 1f737d4

Browse files
committed
Fix query-failure fallback for bzlmod-only workspaces
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.
1 parent a61697d commit 1f737d4

2 files changed

Lines changed: 71 additions & 1 deletion

File tree

cli/src/main/kotlin/com/bazel_diff/interactor/CalculateImpactedTargetsInteractor.kt

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -374,7 +374,17 @@ class CalculateImpactedTargetsInteractor : KoinComponent {
374374
} catch (e: Exception) {
375375
logger.w { "Failed to query rdeps for @@$repoName: ${e.message}" }
376376
logger.w { "Conservatively marking all targets as impacted for this module" }
377-
impactedTargets.addAll(allTargets.keys.filter { !it.startsWith("@@") })
377+
// Emit the buildable workspace subset when it exists; otherwise
378+
// (bzlmod-only shape) fall through to every hashed label so the
379+
// downstream `excludeExternalTargets` strip does not reduce the
380+
// fallback to empty.
381+
val buildableWorkspaceTargets = allTargets.keys.filter {
382+
!it.startsWith("@@") && !it.startsWith("//external:")
383+
}
384+
impactedTargets.addAll(
385+
if (buildableWorkspaceTargets.isEmpty()) allTargets.keys
386+
else buildableWorkspaceTargets
387+
)
378388
}
379389
}
380390

cli/src/test/kotlin/com/bazel_diff/interactor/CalculateImpactedTargetsInteractorModuleQueryTest.kt

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -312,6 +312,66 @@ class CalculateImpactedTargetsInteractorModuleQueryTest : KoinTest {
312312
verify(queryService).query(eq("rdeps(//..., @@abseil-cpp+//...)"), eq(false))
313313
} }
314314

315+
@Test
316+
fun queryFailureOnBzlmodOnlyShapeEmitsAllHashedLabels() { runBlocking {
317+
// No workspace-local `//...` labels, so the fallback must surface the
318+
// full hash set. Otherwise the downstream `excludeExternalTargets`
319+
// strip reduces it to empty.
320+
val hashes = mapOf(
321+
"@@abseil-cpp+//absl:strings" to TargetHash("Rule", "a", "a"),
322+
"@@abseil-cpp+//absl:base" to TargetHash("Rule", "b", "b"),
323+
"//external:com_google_absl" to TargetHash("Rule", "c", "c"))
324+
325+
whenever(queryService.discoverRepoMapping())
326+
.thenReturn(mapOf("abseil-cpp" to "abseil-cpp+"))
327+
whenever(queryService.query(any(), any()))
328+
.thenThrow(RuntimeException("simulated bazel query failure"))
329+
330+
val writer = StringWriter()
331+
CalculateImpactedTargetsInteractor().execute(
332+
from = hashes,
333+
to = hashes,
334+
outputWriter = writer,
335+
targetTypes = null,
336+
fromModuleGraphJson = graph("abseil-cpp", "20240116.2"),
337+
toModuleGraphJson = graph("abseil-cpp", "20240722.0"))
338+
339+
verify(queryService).query(eq("rdeps(//..., @@abseil-cpp+//...)"), eq(false))
340+
assertThat(outputLines(writer)).containsExactlyInAnyOrder(
341+
"@@abseil-cpp+//absl:strings",
342+
"@@abseil-cpp+//absl:base",
343+
"//external:com_google_absl")
344+
} }
345+
346+
@Test
347+
fun queryFailureOnMixedWorkspacePreservesGranularity() { runBlocking {
348+
// Fallback must return only the buildable `//...` subset when one
349+
// exists, not every hashed label.
350+
val hashes = mapOf(
351+
"//app:app" to TargetHash("Rule", "a", "a"),
352+
"//lib:util" to TargetHash("Rule", "b", "b"),
353+
"@@abseil-cpp+//absl:strings" to TargetHash("Rule", "c", "c"),
354+
"@@other+//x:y" to TargetHash("Rule", "d", "d"),
355+
"//external:abseil-cpp" to TargetHash("Rule", "e", "e"))
356+
357+
whenever(queryService.discoverRepoMapping())
358+
.thenReturn(mapOf("abseil-cpp" to "abseil-cpp+"))
359+
whenever(queryService.query(any(), any()))
360+
.thenThrow(RuntimeException("simulated bazel query failure"))
361+
362+
val writer = StringWriter()
363+
CalculateImpactedTargetsInteractor().execute(
364+
from = hashes,
365+
to = hashes,
366+
outputWriter = writer,
367+
targetTypes = null,
368+
fromModuleGraphJson = graph("abseil-cpp", "20240116.2"),
369+
toModuleGraphJson = graph("abseil-cpp", "20240722.0"))
370+
371+
assertThat(outputLines(writer)).containsExactlyInAnyOrder(
372+
"//app:app", "//lib:util")
373+
} }
374+
315375
@Test
316376
fun executeWithDistancesRunsModuleQueryPath() { runBlocking {
317377
// Covers the parallel module-query call site in `executeWithDistances`.

0 commit comments

Comments
 (0)