From 69428362e7985bd1d839d9560c7c369fcdf14292 Mon Sep 17 00:00:00 2001 From: Maxwell Elliott Date: Sun, 26 Apr 2026 22:19:04 -0400 Subject: [PATCH] Fix #326: filter //external:* labels from impacted-targets output on bzlmod Synthetic //external: 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) --- .../cli/GetImpactedTargetsCommand.kt | 31 ++++++- .../CalculateImpactedTargetsInteractor.kt | 8 +- .../test/kotlin/com/bazel_diff/e2e/E2ETest.kt | 90 +++++++++++++++++++ .../CalculateImpactedTargetsInteractorTest.kt | 64 +++++++++++++ 4 files changed, 189 insertions(+), 4 deletions(-) diff --git a/cli/src/main/kotlin/com/bazel_diff/cli/GetImpactedTargetsCommand.kt b/cli/src/main/kotlin/com/bazel_diff/cli/GetImpactedTargetsCommand.kt index e8dbb4f..92e21d8 100644 --- a/cli/src/main/kotlin/com/bazel_diff/cli/GetImpactedTargetsCommand.kt +++ b/cli/src/main/kotlin/com/bazel_diff/cli/GetImpactedTargetsCommand.kt @@ -105,6 +105,22 @@ class GetImpactedTargetsCommand : Callable { scope = CommandLine.ScopeType.LOCAL) var noBazelrc = false + @CommandLine.Option( + names = ["--excludeExternalTargets"], + negatable = true, + description = + [ + "If true, drop labels starting with '//external:' from the impacted-targets output. " + + "These synthetic labels are produced for bzlmod-managed external repos so " + + "generate-hashes can detect dep changes, but they are not buildable in " + + "bzlmod-only mode (Bazel 8.6.0+ with --enable_workspace=false) and will fail " + + "downstream `bazel build`. See https://github.com/Tinder/bazel-diff/issues/326. " + + "When unset, defaults to true if Bzlmod is detected (via `bazel mod graph`), " + + "false otherwise."], + scope = CommandLine.ScopeType.LOCAL, + defaultValue = CommandLine.Parameters.NULL_VALUE) + var excludeExternalTargets: Boolean? = null + @CommandLine.Spec lateinit var spec: CommandLine.Model.CommandSpec override fun call(): Int { @@ -141,6 +157,15 @@ class GetImpactedTargetsCommand : Callable { val fromData = deserialiser.executeTargetHashWithMetadata(startingHashesJSONPath) val toData = deserialiser.executeTargetHashWithMetadata(finalHashesJSONPath) + // If the user did not pass --[no-]excludeExternalTargets, default to true when bzlmod is + // enabled (synthetic //external:* labels are not buildable in bzlmod-only mode — #326), + // otherwise false to preserve WORKSPACE-mode behavior. + val resolvedExcludeExternalTargets = + excludeExternalTargets + ?: org.koin.java.KoinJavaComponent.get( + com.bazel_diff.bazel.BazelModService::class.java) + .isBzlmodEnabled + val outputWriter = BufferedWriter( when (val path = outputPath) { @@ -159,7 +184,8 @@ class GetImpactedTargetsCommand : Callable { outputWriter, targetType, fromData.moduleGraphJson, - toData.moduleGraphJson) + toData.moduleGraphJson, + resolvedExcludeExternalTargets) } else { CalculateImpactedTargetsInteractor() .execute( @@ -168,7 +194,8 @@ class GetImpactedTargetsCommand : Callable { outputWriter, targetType, fromData.moduleGraphJson, - toData.moduleGraphJson) + toData.moduleGraphJson, + resolvedExcludeExternalTargets) } CommandLine.ExitCode.OK } catch (e: IOException) { diff --git a/cli/src/main/kotlin/com/bazel_diff/interactor/CalculateImpactedTargetsInteractor.kt b/cli/src/main/kotlin/com/bazel_diff/interactor/CalculateImpactedTargetsInteractor.kt index 1364bce..09bb21e 100644 --- a/cli/src/main/kotlin/com/bazel_diff/interactor/CalculateImpactedTargetsInteractor.kt +++ b/cli/src/main/kotlin/com/bazel_diff/interactor/CalculateImpactedTargetsInteractor.kt @@ -35,7 +35,8 @@ class CalculateImpactedTargetsInteractor : KoinComponent { outputWriter: Writer, targetTypes: Set?, fromModuleGraphJson: String? = null, - toModuleGraphJson: String? = null + toModuleGraphJson: String? = null, + excludeExternalTargets: Boolean = false, ) { /** This call might be faster if end hashes is a sorted map */ val typeFilter = TargetTypeFilter(targetTypes, to) @@ -59,6 +60,7 @@ class CalculateImpactedTargetsInteractor : KoinComponent { impactedTargets .filter { typeFilter.accepts(it) } + .filter { !excludeExternalTargets || !it.startsWith("//external:") } .sortedWith(impactedTargetOrdering(to, from)) .let { filtered -> outputWriter.use { writer -> filtered.forEach { writer.write("$it\n") } } @@ -87,7 +89,8 @@ class CalculateImpactedTargetsInteractor : KoinComponent { outputWriter: Writer, targetTypes: Set?, fromModuleGraphJson: String? = null, - toModuleGraphJson: String? = null + toModuleGraphJson: String? = null, + excludeExternalTargets: Boolean = false, ) { val typeFilter = TargetTypeFilter(targetTypes, to) @@ -114,6 +117,7 @@ class CalculateImpactedTargetsInteractor : KoinComponent { val ordering = impactedTargetOrdering(to, from) impactedTargets .filterKeys { typeFilter.accepts(it) } + .filterKeys { !excludeExternalTargets || !it.startsWith("//external:") } .toSortedMap(ordering) .let { filtered -> outputWriter.use { writer -> diff --git a/cli/src/test/kotlin/com/bazel_diff/e2e/E2ETest.kt b/cli/src/test/kotlin/com/bazel_diff/e2e/E2ETest.kt index 46fd7e3..ad11f03 100644 --- a/cli/src/test/kotlin/com/bazel_diff/e2e/E2ETest.kt +++ b/cli/src/test/kotlin/com/bazel_diff/e2e/E2ETest.kt @@ -1210,6 +1210,96 @@ class E2ETest { .isEqualTo(false) } + @Test + fun testExcludeExternalTargetsFiltersBzlmodSyntheticLabels() { + // Validates the fix for https://github.com/Tinder/bazel-diff/issues/326. + // + // On Bazel 8.6.0+ with bzlmod, BazelClient queries `bazel mod show_repo` to produce synthetic + // //external: targets so generate-hashes can detect bzlmod dep changes. Those + // labels are unbuildable in bzlmod-only mode (no //external package), so users hit + // "no such package 'external'" when feeding the impacted-targets file to `bazel build`. + // + // Expected behavior: + // - get-impacted-targets defaults --excludeExternalTargets to TRUE when bzlmod is detected + // => no //external:* lines in the output. + // - --no-excludeExternalTargets opts back into the legacy behavior so the synthetic labels + // reappear (proving they're really in the hash maps and the filter is what removes them). + val version = getBazelVersion() + org.junit.Assume.assumeNotNull(version) + val v = version!! + val comparator = compareBy> { it.first }.thenBy { it.second }.thenBy { it.third } + val hasModShowRepo = comparator.compare(v, Triple(8, 6, 0)) >= 0 && v != Triple(9, 0, 0) + org.junit.Assume.assumeTrue( + "Requires Bazel 8.6.0+ or 9.0.1+ (current: ${v.first}.${v.second}.${v.third})", + hasModShowRepo) + + val projectA = extractFixtureProject("/fixture/bzlmod-show-repo-test-1.zip") + val projectB = extractFixtureProject("/fixture/bzlmod-show-repo-test-2.zip") + + val bazelPath = "bazel" + val outputDir = temp.newFolder() + val from = File(outputDir, "starting_hashes.json") + val to = File(outputDir, "final_hashes.json") + val defaultOutput = File(outputDir, "impacted_default.txt") + val optOutOutput = File(outputDir, "impacted_optout.txt") + + val cli = CommandLine(BazelDiff()) + + assertThat( + cli.execute( + "generate-hashes", + "-w", projectA.absolutePath, + "-b", bazelPath, + from.absolutePath)) + .isEqualTo(0) + assertThat( + cli.execute( + "generate-hashes", + "-w", projectB.absolutePath, + "-b", bazelPath, + to.absolutePath)) + .isEqualTo(0) + + // Default invocation: bzlmod is detected, so --excludeExternalTargets auto-defaults to true. + assertThat( + cli.execute( + "get-impacted-targets", + "-w", projectB.absolutePath, + "-b", bazelPath, + "-sh", from.absolutePath, + "-fh", to.absolutePath, + "-o", defaultOutput.absolutePath)) + .isEqualTo(0) + + val defaultLines = defaultOutput.readLines().filter { it.isNotBlank() } + val leakedExternal = defaultLines.filter { it.startsWith("//external:") } + assertThat(leakedExternal.isEmpty()) + .transform( + "default impacted-targets output should not contain //external:* labels, but found: $leakedExternal") { it } + .isEqualTo(true) + + // Opt-out: --no-excludeExternalTargets reproduces the pre-#326 behavior so the synthetic + // labels show up. This proves the labels really exist in the hashes (so the filter is doing + // real work) and gives users an escape hatch. + assertThat( + cli.execute( + "get-impacted-targets", + "-w", projectB.absolutePath, + "-b", bazelPath, + "-sh", from.absolutePath, + "-fh", to.absolutePath, + "--no-excludeExternalTargets", + "-o", optOutOutput.absolutePath)) + .isEqualTo(0) + + val optOutLines = optOutOutput.readLines().filter { it.isNotBlank() } + val externalsWithOptOut = optOutLines.filter { it.startsWith("//external:") } + assertThat(externalsWithOptOut.isNotEmpty()) + .transform( + "with --no-excludeExternalTargets, the impacted-targets output should expose synthetic //external:* labels (none found in: $optOutLines)") { it } + .isEqualTo(true) + } + private fun copyTestWorkspace(path: String): File { val testProject = temp.newFolder() diff --git a/cli/src/test/kotlin/com/bazel_diff/interactor/CalculateImpactedTargetsInteractorTest.kt b/cli/src/test/kotlin/com/bazel_diff/interactor/CalculateImpactedTargetsInteractorTest.kt index b254acb..a8a8faf 100644 --- a/cli/src/test/kotlin/com/bazel_diff/interactor/CalculateImpactedTargetsInteractorTest.kt +++ b/cli/src/test/kotlin/com/bazel_diff/interactor/CalculateImpactedTargetsInteractorTest.kt @@ -533,6 +533,70 @@ class CalculateImpactedTargetsInteractorTest : KoinTest { assertThat(output).containsExactly("//:target1") } + @Test + fun testExecuteFiltersExternalLabelsWhenExcludeFlagSet() { + // Unit-level guard for https://github.com/Tinder/bazel-diff/issues/326. The synthetic + // //external: labels produced for bzlmod repos must drop out of the + // impacted-targets output when --excludeExternalTargets is set, while real workspace + // labels (//foo:bar) and canonical bzlmod labels (@@repo//pkg:tgt) remain. + val from = + mapOf( + "//foo:bar" to TargetHash("Rule", "h1", "h1"), + "//external:boost.assert" to TargetHash("Rule", "h2", "h2"), + "//external:guava" to TargetHash("Rule", "h3", "h3"), + "@@some_repo//pkg:tgt" to TargetHash("Rule", "h4", "h4"), + ) + val to = from.mapValues { (_, v) -> v.copy(hash = v.hash + "-changed") } + + val filteredWriter = StringWriter() + CalculateImpactedTargetsInteractor() + .execute( + from = from, + to = to, + outputWriter = filteredWriter, + targetTypes = null, + excludeExternalTargets = true) + val filteredLines = filteredWriter.toString().trimEnd('\n').split("\n").toSet() + assertThat(filteredLines).containsOnly("//foo:bar", "@@some_repo//pkg:tgt") + + val unfilteredWriter = StringWriter() + CalculateImpactedTargetsInteractor() + .execute( + from = from, + to = to, + outputWriter = unfilteredWriter, + targetTypes = null, + excludeExternalTargets = false) + val unfilteredLines = unfilteredWriter.toString().trimEnd('\n').split("\n").toSet() + assertThat(unfilteredLines) + .containsOnly( + "//foo:bar", "//external:boost.assert", "//external:guava", "@@some_repo//pkg:tgt") + } + + @Test + fun testExecuteWithDistancesFiltersExternalLabelsWhenExcludeFlagSet() { + // Same guarantee for the distance-metrics output path used when --depEdgesFile is provided. + val from = + mapOf( + "//foo:bar" to TargetHash("Rule", "h1", "h1"), + "//external:boost.assert" to TargetHash("Rule", "h2", "h2"), + ) + val to = from.mapValues { (_, v) -> v.copy(hash = v.hash + "-changed", directHash = v.directHash + "-changed") } + + val filteredWriter = StringWriter() + CalculateImpactedTargetsInteractor() + .executeWithDistances( + from = from, + to = to, + depEdges = mapOf(), + outputWriter = filteredWriter, + targetTypes = null, + excludeExternalTargets = true) + val filteredJson = filteredWriter.toString() + assertThat(filteredJson).contains("//foo:bar") + assertThat(filteredJson).doesNotContain("//external:boost.assert") + } + @Test fun testIdenticalModuleGraphsSkipsParsing() { // When module graphs are identical, should skip parsing and use normal hash comparison