Skip to content

Commit f845845

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 f40009d commit f845845

2 files changed

Lines changed: 71 additions & 2 deletions

File tree

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

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -373,8 +373,18 @@ class CalculateImpactedTargetsInteractor : KoinComponent {
373373
logger.i { "Found ${rdepLabels.size} workspace targets depending on changed modules" }
374374
impactedTargets.addAll(rdepLabels)
375375
} catch (e: Exception) {
376-
logger.e(e) { "Unioned rdeps query failed - conservatively marking all workspace targets impacted" }
377-
impactedTargets.addAll(allTargets.keys.filter { !it.startsWith("@@") })
376+
logger.e(e) { "Unioned rdeps query failed - conservatively marking all targets impacted" }
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: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -328,6 +328,65 @@ class CalculateImpactedTargetsInteractorModuleQueryTest : KoinTest {
328328
assertThat(queryCaptor.firstValue).contains("@@abseil-cpp+//...")
329329
} }
330330

331+
@Test
332+
fun queryFailureOnBzlmodOnlyShapeEmitsAllHashedLabels() { runBlocking {
333+
// No workspace-local `//...` labels, so the fallback must surface the
334+
// full hash set. Otherwise the downstream `excludeExternalTargets`
335+
// strip reduces it to empty.
336+
val hashes = mapOf(
337+
"@@abseil-cpp+//absl:strings" to TargetHash("Rule", "a", "a"),
338+
"@@abseil-cpp+//absl:base" to TargetHash("Rule", "b", "b"),
339+
"//external:com_google_absl" to TargetHash("Rule", "c", "c"))
340+
341+
whenever(queryService.discoverRepoMapping())
342+
.thenReturn(mapOf("abseil-cpp" to "abseil-cpp+"))
343+
whenever(queryService.query(any(), any()))
344+
.thenThrow(RuntimeException("simulated bazel query failure"))
345+
346+
val writer = StringWriter()
347+
CalculateImpactedTargetsInteractor().execute(
348+
from = hashes,
349+
to = hashes,
350+
outputWriter = writer,
351+
targetTypes = null,
352+
fromModuleGraphJson = graph("abseil-cpp", "20240116.2"),
353+
toModuleGraphJson = graph("abseil-cpp", "20240722.0"))
354+
355+
assertThat(outputLines(writer)).containsExactlyInAnyOrder(
356+
"@@abseil-cpp+//absl:strings",
357+
"@@abseil-cpp+//absl:base",
358+
"//external:com_google_absl")
359+
} }
360+
361+
@Test
362+
fun queryFailureOnMixedWorkspacePreservesGranularity() { runBlocking {
363+
// Fallback must return only the buildable `//...` subset when one
364+
// exists, not every hashed label.
365+
val hashes = mapOf(
366+
"//app:app" to TargetHash("Rule", "a", "a"),
367+
"//lib:util" to TargetHash("Rule", "b", "b"),
368+
"@@abseil-cpp+//absl:strings" to TargetHash("Rule", "c", "c"),
369+
"@@other+//x:y" to TargetHash("Rule", "d", "d"),
370+
"//external:abseil-cpp" to TargetHash("Rule", "e", "e"))
371+
372+
whenever(queryService.discoverRepoMapping())
373+
.thenReturn(mapOf("abseil-cpp" to "abseil-cpp+"))
374+
whenever(queryService.query(any(), any()))
375+
.thenThrow(RuntimeException("simulated bazel query failure"))
376+
377+
val writer = StringWriter()
378+
CalculateImpactedTargetsInteractor().execute(
379+
from = hashes,
380+
to = hashes,
381+
outputWriter = writer,
382+
targetTypes = null,
383+
fromModuleGraphJson = graph("abseil-cpp", "20240116.2"),
384+
toModuleGraphJson = graph("abseil-cpp", "20240722.0"))
385+
386+
assertThat(outputLines(writer)).containsExactlyInAnyOrder(
387+
"//app:app", "//lib:util")
388+
} }
389+
331390
@Test
332391
fun executeWithDistancesRunsModuleQueryPath() { runBlocking {
333392
// Covers the parallel module-query call site in `executeWithDistances`.

0 commit comments

Comments
 (0)