From 30c0f197b7f2ebf0ed2a3bd46c1ff6d019809109 Mon Sep 17 00:00:00 2001 From: Alex Torok Date: Sat, 28 Sep 2024 14:59:49 +0000 Subject: [PATCH 01/14] refactor: Update hashing logic to use TargetHash and TargetDigest The new fields in TargetHash and TargetDigest are to track the direct srcs/attrs hash separately from the overall hash. Currently, the directHash is not used at all. --- cli/BUILD | 11 ++++ .../cli/GetImpactedTargetsCommand.kt | 4 +- .../com/bazel_diff/hash/BuildGraphHasher.kt | 8 ++- .../kotlin/com/bazel_diff/hash/RuleHasher.kt | 20 +++++-- .../kotlin/com/bazel_diff/hash/TargetHash.kt | 28 +++++++-- .../com/bazel_diff/hash/TargetHasher.kt | 13 +++-- .../CalculateImpactedTargetsInteractor.kt | 3 +- .../interactor/DeserialiseHashesInteractor.kt | 30 +++++++--- .../com/bazel_diff/io/ContentHashProvider.kt | 2 +- .../bazel_diff/hash/BuildGraphHasherTest.kt | 24 ++++---- .../com/bazel_diff/hash/TargetHashTest.kt | 57 +++++++++++++++++++ .../CalculateImpactedTargetsInteractorTest.kt | 7 ++- .../DeserialiseHashesInteractorTest.kt | 37 ++++++------ 13 files changed, 187 insertions(+), 57 deletions(-) create mode 100644 cli/src/test/kotlin/com/bazel_diff/hash/TargetHashTest.kt diff --git a/cli/BUILD b/cli/BUILD index 6a0b1be..bd97caf 100644 --- a/cli/BUILD +++ b/cli/BUILD @@ -63,6 +63,17 @@ kt_jvm_test( runtime_deps = [":cli-test-lib"], ) +kt_jvm_test( + name = "TargetHashTest", + jvm_flags = select({ + ":macos": ["-Djava.security.manager=allow"], + "//conditions:default": [], + }), + test_class = "com.bazel_diff.hash.TargetHashTest", + runtime_deps = [":cli-test-lib"], +) + + kt_jvm_test( name = "SourceFileHasherTest", data = [ 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 cd8c4e7..7b1e90f 100644 --- a/cli/src/main/kotlin/com/bazel_diff/cli/GetImpactedTargetsCommand.kt +++ b/cli/src/main/kotlin/com/bazel_diff/cli/GetImpactedTargetsCommand.kt @@ -66,8 +66,8 @@ class GetImpactedTargetsCommand : Callable { validate() val deserialiser = DeserialiseHashesInteractor() - val from = deserialiser.execute(startingHashesJSONPath, targetType) - val to = deserialiser.execute(finalHashesJSONPath, targetType) + val from = deserialiser.executeTargetHash(startingHashesJSONPath, targetType) + val to = deserialiser.executeTargetHash(finalHashesJSONPath, targetType) val impactedTargets = CalculateImpactedTargetsInteractor().execute(from, to) diff --git a/cli/src/main/kotlin/com/bazel_diff/hash/BuildGraphHasher.kt b/cli/src/main/kotlin/com/bazel_diff/hash/BuildGraphHasher.kt index 950879f..1d736d6 100644 --- a/cli/src/main/kotlin/com/bazel_diff/hash/BuildGraphHasher.kt +++ b/cli/src/main/kotlin/com/bazel_diff/hash/BuildGraphHasher.kt @@ -99,7 +99,7 @@ class BuildGraphHasher(private val bazelClient: BazelClient) : KoinComponent { ignoredAttrs: Set, modifiedFilepaths: Set ): Map { - val ruleHashes: ConcurrentMap = ConcurrentHashMap() + val ruleHashes: ConcurrentMap = ConcurrentHashMap() val targetToRule: MutableMap = HashMap() traverseGraph(allTargets, targetToRule) @@ -114,7 +114,11 @@ class BuildGraphHasher(private val bazelClient: BazelClient) : KoinComponent { ignoredAttrs, modifiedFilepaths ) - Pair(target.name, TargetHash(target.javaClass.name.substringAfterLast('$'), targetDigest.toHexString())) + Pair(target.name, TargetHash( + target.javaClass.name.substringAfterLast('$'), + targetDigest.overallDigest.toHexString(), + targetDigest.directDigest.toHexString(), + )) } .filter { targetEntry: Pair? -> targetEntry != null } .collect( diff --git a/cli/src/main/kotlin/com/bazel_diff/hash/RuleHasher.kt b/cli/src/main/kotlin/com/bazel_diff/hash/RuleHasher.kt index 2af8de0..c1b4b1d 100644 --- a/cli/src/main/kotlin/com/bazel_diff/hash/RuleHasher.kt +++ b/cli/src/main/kotlin/com/bazel_diff/hash/RuleHasher.kt @@ -9,6 +9,16 @@ import org.koin.core.component.inject import java.util.concurrent.ConcurrentMap import java.nio.file.Path +data class TargetDigest( + val overallDigest: ByteArray, + val directDigest: ByteArray, +) { + + fun clone(): TargetDigest { + return TargetDigest(overallDigest.clone(), directDigest.clone()) + } +} + class RuleHasher(private val useCquery: Boolean, private val fineGrainedHashExternalRepos: Set) : KoinComponent { private val logger: Logger by inject() private val sourceFileHasher: SourceFileHasher by inject() @@ -28,13 +38,13 @@ class RuleHasher(private val useCquery: Boolean, private val fineGrainedHashExte fun digest( rule: BazelRule, allRulesMap: Map, - ruleHashes: ConcurrentMap, + ruleHashes: ConcurrentMap, sourceDigests: ConcurrentMap, seedHash: ByteArray?, depPath: LinkedHashSet?, ignoredAttrs: Set, modifiedFilepaths: Set - ): ByteArray { + ): TargetDigest { val depPathClone = if (depPath != null) LinkedHashSet(depPath) else LinkedHashSet() if (depPathClone.contains(rule.name)) { throw raiseCircularDependency(depPathClone, rule.name) @@ -42,7 +52,7 @@ class RuleHasher(private val useCquery: Boolean, private val fineGrainedHashExte depPathClone.add(rule.name) ruleHashes[rule.name]?.let { return it } - val finalHashValue = sha256 { + val overallDigest = sha256 { safePutBytes(rule.digest(ignoredAttrs)) safePutBytes(seedHash) @@ -66,7 +76,7 @@ class RuleHasher(private val useCquery: Boolean, private val fineGrainedHashExte ignoredAttrs, modifiedFilepaths ) - safePutBytes(ruleInputHash) + safePutBytes(ruleInputHash.overallDigest) } else -> { @@ -85,6 +95,8 @@ class RuleHasher(private val useCquery: Boolean, private val fineGrainedHashExte } } + val finalHashValue = TargetDigest(overallDigest, ByteArray(0)) + return finalHashValue.also { ruleHashes[rule.name] = it } } } diff --git a/cli/src/main/kotlin/com/bazel_diff/hash/TargetHash.kt b/cli/src/main/kotlin/com/bazel_diff/hash/TargetHash.kt index 78e9d54..503c5d5 100644 --- a/cli/src/main/kotlin/com/bazel_diff/hash/TargetHash.kt +++ b/cli/src/main/kotlin/com/bazel_diff/hash/TargetHash.kt @@ -2,17 +2,37 @@ package com.bazel_diff.hash data class TargetHash( val type: String, // Rule/GeneratedFile/SourceFile/... - val hash: String + val hash: String, + val directHash: String ) { val hashWithType by lazy { - "${type}#${hash}" + "${type}#${hash}#${directHash}" + } + + val totalHash by lazy { + "${hash}#${directHash}" } fun toJson(includeTargetType: Boolean): String { return if (includeTargetType) { hashWithType } else { - hash + totalHash + } + } + + fun hasType(): Boolean { + return type.isNotEmpty() + } + + companion object { + fun fromJson(json: String): TargetHash { + val parts = json.split("#") + return when (parts.size) { + 2 -> TargetHash("", parts[0], parts[1]) + 3 -> TargetHash(parts[0], parts[1], parts[2]) + else -> throw IllegalArgumentException("Invalid JSON format") + } } } -} \ No newline at end of file +} diff --git a/cli/src/main/kotlin/com/bazel_diff/hash/TargetHasher.kt b/cli/src/main/kotlin/com/bazel_diff/hash/TargetHasher.kt index 4cc5a45..9944d0a 100644 --- a/cli/src/main/kotlin/com/bazel_diff/hash/TargetHasher.kt +++ b/cli/src/main/kotlin/com/bazel_diff/hash/TargetHasher.kt @@ -14,11 +14,11 @@ class TargetHasher : KoinComponent { target: BazelTarget, allRulesMap: Map, sourceDigests: ConcurrentMap, - ruleHashes: ConcurrentMap, + ruleHashes: ConcurrentMap, seedHash: ByteArray?, ignoredAttrs: Set, modifiedFilepaths: Set - ): ByteArray { + ): TargetDigest { return when (target) { is BazelTarget.GeneratedFile -> { val generatingRuleDigest = ruleHashes[target.generatingRuleName] @@ -51,9 +51,12 @@ class TargetHasher : KoinComponent { modifiedFilepaths ) } - is BazelTarget.SourceFile -> sha256 { - safePutBytes(sourceDigests[target.sourceFileName]) - safePutBytes(seedHash) + is BazelTarget.SourceFile -> { + val digest = sha256 { + safePutBytes(sourceDigests[target.sourceFileName]) + safePutBytes(seedHash) + } + TargetDigest(digest, digest) } } } 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 be44d4d..4f3f442 100644 --- a/cli/src/main/kotlin/com/bazel_diff/interactor/CalculateImpactedTargetsInteractor.kt +++ b/cli/src/main/kotlin/com/bazel_diff/interactor/CalculateImpactedTargetsInteractor.kt @@ -1,11 +1,12 @@ package com.bazel_diff.interactor +import com.bazel_diff.hash.TargetHash import com.google.common.collect.Maps import org.koin.core.component.KoinComponent import java.io.File class CalculateImpactedTargetsInteractor : KoinComponent { - fun execute(from: Map, to: Map): Set { + fun execute(from: Map, to: Map): Set { /** * This call might be faster if end hashes is a sorted map */ diff --git a/cli/src/main/kotlin/com/bazel_diff/interactor/DeserialiseHashesInteractor.kt b/cli/src/main/kotlin/com/bazel_diff/interactor/DeserialiseHashesInteractor.kt index 02e9b63..1068cdb 100644 --- a/cli/src/main/kotlin/com/bazel_diff/interactor/DeserialiseHashesInteractor.kt +++ b/cli/src/main/kotlin/com/bazel_diff/interactor/DeserialiseHashesInteractor.kt @@ -6,6 +6,7 @@ import org.koin.core.component.KoinComponent import org.koin.core.component.inject import java.io.File import java.io.FileReader +import com.bazel_diff.hash.TargetHash class DeserialiseHashesInteractor : KoinComponent { private val gson: Gson by inject() @@ -14,20 +15,35 @@ class DeserialiseHashesInteractor : KoinComponent { * @param file path to file that has been pre-validated * @param targetTypes the target types to filter. If null, all targets will be returned */ - fun execute(file: File, targetTypes: Set? = null): Map { + fun executeTargetHash(file: File, targetTypes: Set? = null): Map { val shape = object : TypeToken>() {}.type val result: Map = gson.fromJson(FileReader(file), shape) if (targetTypes == null) { - return result.mapValues { it.value.substringAfter("#") } + return result.mapValues { TargetHash.fromJson(it.value) } } else { - val prefixes = targetTypes.map { "${it}#" }.toSet() - return result.filter { entry -> - if (entry.value.contains("#")) { - prefixes.any { entry.value.startsWith(it) } + return result.mapValues { + TargetHash.fromJson(it.value) + }.filter { (_, targetHash) -> + if (targetHash.hasType()) { + targetTypes.contains(targetHash.type) } else { throw IllegalStateException("No type info found in ${file}, please re-generate the JSON with --includeTypeTarget!") } - }.mapValues { it.value.substringAfter("#") } + } } } + + /** + * Deserializes hashes from the given file. + * + * Used for deserializing the content hashes of files, which are represented as + * a map of file paths to their content hashes. + * + * @param file The path to the file that has been pre-validated. + * @return A map containing the deserialized hashes. + */ + fun executeSimple(file: File): Map { + val shape = object : TypeToken>() {}.type + return gson.fromJson(FileReader(file), shape) + } } diff --git a/cli/src/main/kotlin/com/bazel_diff/io/ContentHashProvider.kt b/cli/src/main/kotlin/com/bazel_diff/io/ContentHashProvider.kt index 9041bff..7647cfe 100644 --- a/cli/src/main/kotlin/com/bazel_diff/io/ContentHashProvider.kt +++ b/cli/src/main/kotlin/com/bazel_diff/io/ContentHashProvider.kt @@ -9,6 +9,6 @@ class ContentHashProvider(file: File?) { private fun readJson(file: File): Map { val deserialiser = DeserialiseHashesInteractor() - return deserialiser.execute(file) + return deserialiser.executeSimple(file) } } diff --git a/cli/src/test/kotlin/com/bazel_diff/hash/BuildGraphHasherTest.kt b/cli/src/test/kotlin/com/bazel_diff/hash/BuildGraphHasherTest.kt index 7b4cf22..314f57b 100644 --- a/cli/src/test/kotlin/com/bazel_diff/hash/BuildGraphHasherTest.kt +++ b/cli/src/test/kotlin/com/bazel_diff/hash/BuildGraphHasherTest.kt @@ -71,8 +71,8 @@ class BuildGraphHasherTest : KoinTest { val hash = hasher.hashAllBazelTargetsAndSourcefiles() assertThat(hash).containsOnly( - "rule1" to TargetHash("Rule", "2c963f7c06bc1cead7e3b4759e1472383d4469fc3238dc42f8848190887b4775"), - "rule2" to TargetHash("Rule", "bdc1abd0a07103cea34199a9c0d1020619136ff90fb88dcc3a8f873c811c1fe9"), + "rule1" to TargetHash("Rule", "2c963f7c06bc1cead7e3b4759e1472383d4469fc3238dc42f8848190887b4775", ""), + "rule2" to TargetHash("Rule", "bdc1abd0a07103cea34199a9c0d1020619136ff90fb88dcc3a8f873c811c1fe9", ""), ) } @@ -83,8 +83,8 @@ class BuildGraphHasherTest : KoinTest { whenever(bazelClientMock.queryAllTargets()).thenReturn(defaultTargets) val hash = hasher.hashAllBazelTargetsAndSourcefiles(seedFilepaths) assertThat(hash).containsOnly( - "rule1" to TargetHash("Rule", "0404d80eadcc2dbfe9f0d7935086e1115344a06bd76d4e16af0dfd7b4913ee60"), - "rule2" to TargetHash("Rule", "6fe63fa16340d18176e6d6021972c65413441b72135247179362763508ebddfe"), + "rule1" to TargetHash("Rule", "0404d80eadcc2dbfe9f0d7935086e1115344a06bd76d4e16af0dfd7b4913ee60", ""), + "rule2" to TargetHash("Rule", "6fe63fa16340d18176e6d6021972c65413441b72135247179362763508ebddfe", ""), ) } @@ -99,10 +99,10 @@ class BuildGraphHasherTest : KoinTest { whenever(bazelClientMock.queryAllTargets()).thenReturn(defaultTargets) val hash = hasher.hashAllBazelTargetsAndSourcefiles() assertThat(hash).containsOnly( - "rule1" to TargetHash("Rule", "2c963f7c06bc1cead7e3b4759e1472383d4469fc3238dc42f8848190887b4775"), - "rule2" to TargetHash("Rule", "bdc1abd0a07103cea34199a9c0d1020619136ff90fb88dcc3a8f873c811c1fe9"), - "rule3" to TargetHash("Rule", "87dd050f1ca0f684f37970092ff6a02677d995718b5a05461706c0f41ffd4915"), - "rule4" to TargetHash("Rule", "a7bc5d23cd98c4942dc879c649eb9646e38eddd773f9c7996fa0d96048cf63dc"), + "rule1" to TargetHash("Rule", "2c963f7c06bc1cead7e3b4759e1472383d4469fc3238dc42f8848190887b4775", ""), + "rule2" to TargetHash("Rule", "bdc1abd0a07103cea34199a9c0d1020619136ff90fb88dcc3a8f873c811c1fe9", ""), + "rule3" to TargetHash("Rule", "87dd050f1ca0f684f37970092ff6a02677d995718b5a05461706c0f41ffd4915", ""), + "rule4" to TargetHash("Rule", "a7bc5d23cd98c4942dc879c649eb9646e38eddd773f9c7996fa0d96048cf63dc", ""), ) } @@ -117,10 +117,10 @@ class BuildGraphHasherTest : KoinTest { whenever(bazelClientMock.queryAllTargets()).thenReturn(defaultTargets) val hash = hasher.hashAllBazelTargetsAndSourcefiles() assertThat(hash).containsOnly( - "rule1" to TargetHash("Rule", "2c963f7c06bc1cead7e3b4759e1472383d4469fc3238dc42f8848190887b4775"), - "rule2" to TargetHash("Rule", "bdc1abd0a07103cea34199a9c0d1020619136ff90fb88dcc3a8f873c811c1fe9"), - "rule3" to TargetHash("Rule", "ca2f970a5a5a18730d7633cc32b48b1d94679f4ccaea56c4924e1f9913bd9cb5"), - "rule4" to TargetHash("Rule", "bf15e616e870aaacb02493ea0b8e90c6c750c266fa26375e22b30b78954ee523"), + "rule1" to TargetHash("Rule", "2c963f7c06bc1cead7e3b4759e1472383d4469fc3238dc42f8848190887b4775", ""), + "rule2" to TargetHash("Rule", "bdc1abd0a07103cea34199a9c0d1020619136ff90fb88dcc3a8f873c811c1fe9", ""), + "rule3" to TargetHash("Rule", "ca2f970a5a5a18730d7633cc32b48b1d94679f4ccaea56c4924e1f9913bd9cb5", ""), + "rule4" to TargetHash("Rule", "bf15e616e870aaacb02493ea0b8e90c6c750c266fa26375e22b30b78954ee523", ""), ) } diff --git a/cli/src/test/kotlin/com/bazel_diff/hash/TargetHashTest.kt b/cli/src/test/kotlin/com/bazel_diff/hash/TargetHashTest.kt new file mode 100644 index 0000000..fd4a149 --- /dev/null +++ b/cli/src/test/kotlin/com/bazel_diff/hash/TargetHashTest.kt @@ -0,0 +1,57 @@ +package com.bazel_diff.hash + +import assertk.assertThat +import assertk.assertions.* +import org.junit.Test + + +class TargetHashTest { + + @Test + fun testRoundTripJson() { + val th = TargetHash( + "Rule", + "hash", + "directHash", + ) + assertThat(TargetHash.fromJson(th.toJson(includeTargetType = true))).isEqualTo(th) + } + + @Test + fun testRoundTripJsonWithoutType() { + val th = TargetHash( + "Rule", + "hash", + "directHash", + ) + assertThat(th.hasType()).isTrue() + + val newTh = TargetHash.fromJson(th.toJson(includeTargetType = false)) + + assertThat(newTh.type).isEqualTo("") + assertThat(newTh.hash).isEqualTo(th.hash) + assertThat(newTh.directHash).isEqualTo(th.directHash) + + assertThat(newTh.hasType()).isFalse() + } + + @Test + fun testInvalidFromJson() { + + + assertThat { + TargetHash.fromJson("invalid") + }.isFailure() + + assertThat { + TargetHash.fromJson("") + }.isFailure() + + assertThat { + TargetHash.fromJson("too#many#delimeters#here#") + }.isFailure() + + + } +} + 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 84cbc53..eeb3c2b 100644 --- a/cli/src/test/kotlin/com/bazel_diff/interactor/CalculateImpactedTargetsInteractorTest.kt +++ b/cli/src/test/kotlin/com/bazel_diff/interactor/CalculateImpactedTargetsInteractorTest.kt @@ -8,6 +8,7 @@ import org.junit.Test import org.koin.test.KoinTest import org.koin.test.KoinTestRule import org.mockito.junit.MockitoJUnit +import com.bazel_diff.hash.TargetHash class CalculateImpactedTargetsInteractorTest : KoinTest { @get:Rule @@ -24,11 +25,15 @@ class CalculateImpactedTargetsInteractorTest : KoinTest { val start: MutableMap = HashMap() start["1"] = "a" start["2"] = "b" + val startHashes = start.mapValues { TargetHash("", it.value, it.value) } + val end: MutableMap = HashMap() end["1"] = "c" end["2"] = "b" end["3"] = "d" - val impacted = interactor.execute(start, end) + val endHashes = end.mapValues { TargetHash("", it.value, it.value) } + + val impacted = interactor.execute(startHashes, endHashes) assertThat(impacted).containsExactlyInAnyOrder( "1", "3" ) diff --git a/cli/src/test/kotlin/com/bazel_diff/interactor/DeserialiseHashesInteractorTest.kt b/cli/src/test/kotlin/com/bazel_diff/interactor/DeserialiseHashesInteractorTest.kt index da781d3..3812d56 100644 --- a/cli/src/test/kotlin/com/bazel_diff/interactor/DeserialiseHashesInteractorTest.kt +++ b/cli/src/test/kotlin/com/bazel_diff/interactor/DeserialiseHashesInteractorTest.kt @@ -6,6 +6,7 @@ import assertk.assertions.isEqualTo import assertk.assertions.isFailure import assertk.assertions.messageContains import com.bazel_diff.testModule +import com.bazel_diff.hash.TargetHash import org.junit.Rule import org.junit.Test import org.junit.rules.TemporaryFolder @@ -30,22 +31,22 @@ class DeserialiseHashesInteractorTest : KoinTest { @Test fun testDeserialisation() { val file = temp.newFile().apply { - writeText("""{"target-name":"hash"}""") + writeText("""{"target-name":"hash#direct"}""") } - val actual = interactor.execute(file) + val actual = interactor.executeTargetHash(file) assertThat(actual).isEqualTo(mapOf( - "target-name" to "hash" + "target-name" to TargetHash("", "hash", "direct") )) } @Test fun testDeserialisatingFileWithoutType() { val file = temp.newFile().apply { - writeText("""{"target-name":"hash"}""") + writeText("""{"target-name":"hash#direct"}""") } - assertThat { interactor.execute(file, setOf("Whatever"))} + assertThat { interactor.executeTargetHash(file, setOf("Whatever"))} .isFailure().apply { messageContains("please re-generate the JSON with --includeTypeTarget!") hasClass(IllegalStateException::class) @@ -56,25 +57,25 @@ class DeserialiseHashesInteractorTest : KoinTest { fun testDeserialisationWithType() { val file = temp.newFile().apply { writeText("""{ - | "target-1":"GeneratedFile#hash1", - | "target-2":"Rule#hash2", - | "target-3":"SourceFile#hash3" + | "target-1":"GeneratedFile#hash1#direct1", + | "target-2":"Rule#hash2#direct2", + | "target-3":"SourceFile#hash3#direct3" |}""".trimMargin()) } - assertThat(interactor.execute(file, null)).isEqualTo(mapOf( - "target-1" to "hash1", - "target-2" to "hash2", - "target-3" to "hash3" + assertThat(interactor.executeTargetHash(file, null)).isEqualTo(mapOf( + "target-1" to TargetHash("GeneratedFile", "hash1", "direct1"), + "target-2" to TargetHash("Rule", "hash2", "direct2"), + "target-3" to TargetHash("SourceFile", "hash3", "direct3") )) - assertThat(interactor.execute(file, setOf("GeneratedFile"))).isEqualTo(mapOf( - "target-1" to "hash1" + assertThat(interactor.executeTargetHash(file, setOf("GeneratedFile"))).isEqualTo(mapOf( + "target-1" to TargetHash("GeneratedFile", "hash1", "direct1") )) - assertThat(interactor.execute(file, setOf("Rule"))).isEqualTo(mapOf( - "target-2" to "hash2" + assertThat(interactor.executeTargetHash(file, setOf("Rule"))).isEqualTo(mapOf( + "target-2" to TargetHash("Rule", "hash2", "direct2") )) - assertThat(interactor.execute(file, setOf("SourceFile"))).isEqualTo(mapOf( - "target-3" to "hash3" + assertThat(interactor.executeTargetHash(file, setOf("SourceFile"))).isEqualTo(mapOf( + "target-3" to TargetHash("SourceFile", "hash3", "direct3") )) } } From 39bb6fcfbedd9c1f258d49564f9329c5d5a4604f Mon Sep 17 00:00:00 2001 From: Alex Torok Date: Sat, 28 Sep 2024 15:20:11 +0000 Subject: [PATCH 02/14] Update RuleHasher to track separate digests --- .../main/kotlin/com/bazel_diff/di/Modules.kt | 2 +- .../kotlin/com/bazel_diff/hash/RuleHasher.kt | 26 +--- .../com/bazel_diff/hash/SourceFileHasher.kt | 13 +- .../com/bazel_diff/hash/TargetDigest.kt | 41 ++++++ cli/src/test/kotlin/com/bazel_diff/Modules.kt | 2 +- .../bazel_diff/hash/BuildGraphHasherTest.kt | 124 ++++++++++++++++-- .../bazel_diff/hash/FakeSourceFileHasher.kt | 18 +++ .../kotlin/com/bazel_diff/hash/HashDiffer.kt | 42 ++++++ .../bazel_diff/hash/SourceFileHasherTest.kt | 24 ++-- 9 files changed, 242 insertions(+), 50 deletions(-) create mode 100644 cli/src/main/kotlin/com/bazel_diff/hash/TargetDigest.kt create mode 100644 cli/src/test/kotlin/com/bazel_diff/hash/FakeSourceFileHasher.kt create mode 100644 cli/src/test/kotlin/com/bazel_diff/hash/HashDiffer.kt diff --git a/cli/src/main/kotlin/com/bazel_diff/di/Modules.kt b/cli/src/main/kotlin/com/bazel_diff/di/Modules.kt index 7b6f337..29ab68d 100644 --- a/cli/src/main/kotlin/com/bazel_diff/di/Modules.kt +++ b/cli/src/main/kotlin/com/bazel_diff/di/Modules.kt @@ -62,7 +62,7 @@ fun hasherModule( single { BuildGraphHasher(get()) } single { TargetHasher() } single { RuleHasher(useCquery, fineGrainedHashExternalRepos) } - single { SourceFileHasher(fineGrainedHashExternalRepos) } + single { SourceFileHasherImpl(fineGrainedHashExternalRepos) } single { ExternalRepoResolver(workingDirectory, bazelPath, outputPath) } single(named("working-directory")) { workingDirectory } single(named("output-base")) { outputPath } diff --git a/cli/src/main/kotlin/com/bazel_diff/hash/RuleHasher.kt b/cli/src/main/kotlin/com/bazel_diff/hash/RuleHasher.kt index c1b4b1d..e41126b 100644 --- a/cli/src/main/kotlin/com/bazel_diff/hash/RuleHasher.kt +++ b/cli/src/main/kotlin/com/bazel_diff/hash/RuleHasher.kt @@ -9,16 +9,6 @@ import org.koin.core.component.inject import java.util.concurrent.ConcurrentMap import java.nio.file.Path -data class TargetDigest( - val overallDigest: ByteArray, - val directDigest: ByteArray, -) { - - fun clone(): TargetDigest { - return TargetDigest(overallDigest.clone(), directDigest.clone()) - } -} - class RuleHasher(private val useCquery: Boolean, private val fineGrainedHashExternalRepos: Set) : KoinComponent { private val logger: Logger by inject() private val sourceFileHasher: SourceFileHasher by inject() @@ -52,17 +42,17 @@ class RuleHasher(private val useCquery: Boolean, private val fineGrainedHashExte depPathClone.add(rule.name) ruleHashes[rule.name]?.let { return it } - val overallDigest = sha256 { - safePutBytes(rule.digest(ignoredAttrs)) - safePutBytes(seedHash) + val finalHashValue = targetSha256 { + putDirectBytes(rule.digest(ignoredAttrs)) + putDirectBytes(seedHash) for (ruleInput in rule.ruleInputList(useCquery, fineGrainedHashExternalRepos)) { - safePutBytes(ruleInput.toByteArray()) + putDirectBytes(ruleInput.toByteArray()) val inputRule = allRulesMap[ruleInput] when { inputRule == null && sourceDigests.containsKey(ruleInput) -> { - safePutBytes(sourceDigests[ruleInput]) + putDirectBytes(sourceDigests[ruleInput]) } inputRule?.name != null && inputRule.name != rule.name -> { @@ -76,7 +66,7 @@ class RuleHasher(private val useCquery: Boolean, private val fineGrainedHashExte ignoredAttrs, modifiedFilepaths ) - safePutBytes(ruleInputHash.overallDigest) + putBytes(ruleInputHash.overallDigest) } else -> { @@ -85,7 +75,7 @@ class RuleHasher(private val useCquery: Boolean, private val fineGrainedHashExte heuristicDigest != null -> { logger.i { "Source file $ruleInput picked up as an input for rule ${rule.name}" } sourceDigests[ruleInput] = heuristicDigest - safePutBytes(heuristicDigest) + putDirectBytes(heuristicDigest) } else -> logger.w { "Unable to calculate digest for input $ruleInput for rule ${rule.name}" } @@ -95,8 +85,6 @@ class RuleHasher(private val useCquery: Boolean, private val fineGrainedHashExte } } - val finalHashValue = TargetDigest(overallDigest, ByteArray(0)) - return finalHashValue.also { ruleHashes[rule.name] = it } } } diff --git a/cli/src/main/kotlin/com/bazel_diff/hash/SourceFileHasher.kt b/cli/src/main/kotlin/com/bazel_diff/hash/SourceFileHasher.kt index b53af85..7a159ce 100644 --- a/cli/src/main/kotlin/com/bazel_diff/hash/SourceFileHasher.kt +++ b/cli/src/main/kotlin/com/bazel_diff/hash/SourceFileHasher.kt @@ -9,7 +9,12 @@ import org.koin.core.qualifier.named import java.nio.file.Path import java.nio.file.Paths -class SourceFileHasher : KoinComponent { +interface SourceFileHasher { + fun digest(sourceFileTarget: BazelSourceFileTarget, modifiedFilepaths: Set = emptySet()): ByteArray + fun softDigest(sourceFileTarget: BazelSourceFileTarget, modifiedFilepaths: Set = emptySet()): ByteArray? +} + +class SourceFileHasherImpl : KoinComponent, SourceFileHasher { private val workingDirectory: Path private val logger: Logger private val relativeFilenameToContentHash: Map? @@ -38,9 +43,9 @@ class SourceFileHasher : KoinComponent { this.externalRepoResolver = externalRepoResolver } - fun digest( + override fun digest( sourceFileTarget: BazelSourceFileTarget, - modifiedFilepaths: Set = emptySet() + modifiedFilepaths: Set ): ByteArray { return sha256 { val name = sourceFileTarget.name @@ -89,7 +94,7 @@ class SourceFileHasher : KoinComponent { } } - fun softDigest(sourceFileTarget: BazelSourceFileTarget, modifiedFilepaths: Set = emptySet()): ByteArray? { + override fun softDigest(sourceFileTarget: BazelSourceFileTarget, modifiedFilepaths: Set): ByteArray? { val name = sourceFileTarget.name if (!name.startsWith("//")) return null diff --git a/cli/src/main/kotlin/com/bazel_diff/hash/TargetDigest.kt b/cli/src/main/kotlin/com/bazel_diff/hash/TargetDigest.kt new file mode 100644 index 0000000..1f9d89a --- /dev/null +++ b/cli/src/main/kotlin/com/bazel_diff/hash/TargetDigest.kt @@ -0,0 +1,41 @@ +package com.bazel_diff.hash +// import com.google.common.hash.Hasher +import com.google.common.hash.Hashing + +data class TargetDigest( + val overallDigest: ByteArray, + val directDigest: ByteArray, +) { + fun clone(): TargetDigest { + return TargetDigest(overallDigest.clone(), directDigest.clone()) + } +} + +fun targetSha256(block: TargetDigestBuilder.() -> Unit): TargetDigest { + val hasher = TargetDigestBuilder() + hasher.apply(block) + return hasher.finish() +} + +class TargetDigestBuilder { + private val overallHasher = Hashing.sha256().newHasher() + private val directHasher = Hashing.sha256().newHasher() + + fun putDirectBytes(block: ByteArray?) { + block?.let { directHasher.putBytes(it) } + } + + fun putBytes(block: ByteArray?) { + block?.let { overallHasher.putBytes(it) } + } + + fun finish(): TargetDigest { + val directHash = directHasher.hash().asBytes().clone() + overallHasher.putBytes(directHash) + + return TargetDigest( + overallHasher.hash().asBytes().clone(), + directHash, + ) + } +} diff --git a/cli/src/test/kotlin/com/bazel_diff/Modules.kt b/cli/src/test/kotlin/com/bazel_diff/Modules.kt index 096a456..bea36d6 100644 --- a/cli/src/test/kotlin/com/bazel_diff/Modules.kt +++ b/cli/src/test/kotlin/com/bazel_diff/Modules.kt @@ -19,7 +19,7 @@ fun testModule(): Module = module { single { TargetHasher() } single { RuleHasher(false, emptySet()) } single { ExternalRepoResolver(workingDirectory, Paths.get("bazel"), outputBase) } - single { SourceFileHasher() } + single { SourceFileHasherImpl() } single { GsonBuilder().disableHtmlEscaping().setPrettyPrinting().create() } single(named("working-directory")) { workingDirectory } single(named("output-base")) { outputBase } diff --git a/cli/src/test/kotlin/com/bazel_diff/hash/BuildGraphHasherTest.kt b/cli/src/test/kotlin/com/bazel_diff/hash/BuildGraphHasherTest.kt index 314f57b..fcfec83 100644 --- a/cli/src/test/kotlin/com/bazel_diff/hash/BuildGraphHasherTest.kt +++ b/cli/src/test/kotlin/com/bazel_diff/hash/BuildGraphHasherTest.kt @@ -17,6 +17,7 @@ import org.koin.test.KoinTest import org.koin.test.KoinTestRule import org.koin.test.mock.MockProviderRule import org.koin.test.mock.declareMock +import org.koin.dsl.module import org.mockito.Mockito import org.mockito.junit.MockitoJUnit import org.mockito.kotlin.mock @@ -29,7 +30,10 @@ class BuildGraphHasherTest : KoinTest { @get:Rule val koinTestRule = KoinTestRule.create { - modules(testModule()) + val mod = module { + single { fakeSourceFileHasher } + } + modules(testModule(), mod) } @get:Rule @@ -46,6 +50,8 @@ class BuildGraphHasherTest : KoinTest { var defaultTargets: MutableList = mutableListOf() + var fakeSourceFileHasher: FakeSourceFileHasher = FakeSourceFileHasher() + @Before fun setUp() { defaultTargets = ArrayList().apply { @@ -71,8 +77,8 @@ class BuildGraphHasherTest : KoinTest { val hash = hasher.hashAllBazelTargetsAndSourcefiles() assertThat(hash).containsOnly( - "rule1" to TargetHash("Rule", "2c963f7c06bc1cead7e3b4759e1472383d4469fc3238dc42f8848190887b4775", ""), - "rule2" to TargetHash("Rule", "bdc1abd0a07103cea34199a9c0d1020619136ff90fb88dcc3a8f873c811c1fe9", ""), + "rule1" to TargetHash("Rule", "7b3149cbd2219ca05bc80a557a701ddee18bd3bbe9afa8e851df64b999155c5e", "2c963f7c06bc1cead7e3b4759e1472383d4469fc3238dc42f8848190887b4775"), + "rule2" to TargetHash("Rule", "24f12d22ab247c5af32f954ca46dd4f6323ab2eef28455411b912aaf44a7c322", "bdc1abd0a07103cea34199a9c0d1020619136ff90fb88dcc3a8f873c811c1fe9"), ) } @@ -83,11 +89,93 @@ class BuildGraphHasherTest : KoinTest { whenever(bazelClientMock.queryAllTargets()).thenReturn(defaultTargets) val hash = hasher.hashAllBazelTargetsAndSourcefiles(seedFilepaths) assertThat(hash).containsOnly( - "rule1" to TargetHash("Rule", "0404d80eadcc2dbfe9f0d7935086e1115344a06bd76d4e16af0dfd7b4913ee60", ""), - "rule2" to TargetHash("Rule", "6fe63fa16340d18176e6d6021972c65413441b72135247179362763508ebddfe", ""), + "rule1" to TargetHash("Rule", "ddf7345122667dda1bbbdc813a6d029795b243e729bcfdcd520cde12cac877f1", "0404d80eadcc2dbfe9f0d7935086e1115344a06bd76d4e16af0dfd7b4913ee60"), + "rule2" to TargetHash("Rule", "41cbcdcbc90aafbeb74b903e7cadcf0c16f36f28dbd7f4cb72699449ff1ab11d", "6fe63fa16340d18176e6d6021972c65413441b72135247179362763508ebddfe"), ) } + @Test + fun hashAllBazelTargets_directHashUnchangedByDeps() = runBlocking { + val rule1 = createRuleTarget(name = "rule1", inputs = ArrayList(), digest = "rule1Digest") + val rule2 = createRuleTarget(name = "rule2", inputs = arrayListOf("rule1"), digest = "rule2Digest") + val rule3 = createRuleTarget(name = "rule3", inputs = arrayListOf("rule2"), digest = "rule3Digest") + val targets = arrayListOf(rule1, rule2, rule3) + + whenever(bazelClientMock.queryAllTargets()).thenReturn(targets) + val baseHashes = hasher.hashAllBazelTargetsAndSourcefiles() + + val newRule1 = createRuleTarget(name = "rule1", inputs = ArrayList(), digest = "rule1Digest--changed") + val newTargets = arrayListOf(newRule1, rule2, rule3) + + whenever(bazelClientMock.queryAllTargets()).thenReturn(newTargets) + val newHashes = hasher.hashAllBazelTargetsAndSourcefiles() + + val hashes = HashDiffer(baseHashes, newHashes) + + hashes.assertThat("rule1").hash().changed() + hashes.assertThat("rule2").hash().changed() + hashes.assertThat("rule3").hash().changed() + + hashes.assertThat("rule1").directHash().changed() + hashes.assertThat("rule2").directHash().didNotChange() + hashes.assertThat("rule3").directHash().didNotChange() + } + + @Test + fun hashAllBazelTargets_directHashChangedBySrcFiles() = runBlocking { + val src1 = createSrcTarget(name = "src1", digest = "src1") + val src2 = createSrcTarget(name = "src2", digest = "src2") + val src3 = createSrcTarget(name = "src3", digest = "src3") + val rule1 = createRuleTarget(name = "rule1", inputs = arrayListOf("src1", "src2"), digest = "rule1Digest") + val rule2 = createRuleTarget(name = "rule2", inputs = arrayListOf("src3", "rule1"), digest = "rule2Digest") + val targets = arrayListOf(src1, src2, src3, rule1, rule2) + + whenever(bazelClientMock.queryAllTargets()).thenReturn(targets) + val baseHashes = hasher.hashAllBazelTargetsAndSourcefiles() + + val newSrc1 = createSrcTarget(name = "src1", digest = "src1--modified") + val newTargets = arrayListOf(newSrc1, src2, src3, rule1, rule2) + + whenever(bazelClientMock.queryAllTargets()).thenReturn(newTargets) + val newHashes = hasher.hashAllBazelTargetsAndSourcefiles() + + val hashes = HashDiffer(baseHashes, newHashes) + + hashes.assertThat("rule1").hash().changed() + hashes.assertThat("rule1").directHash().changed() + + hashes.assertThat("rule2").hash().changed() + hashes.assertThat("rule2").directHash().didNotChange() + } + + @Test + fun hashAllBazelTargets_generatedSrcDoesNotContributeToDirect() = runBlocking { + val rule1 = createRuleTarget(name = "rule1", inputs = emptyList(), digest = "rule1Digest") + val src1 = createGeneratedTarget("gen1", "rule1") + val rule2 = createRuleTarget(name = "rule2", inputs = arrayListOf("gen1"), digest = "rule2Digest") + val targets = arrayListOf(rule1, src1, rule2) + + whenever(bazelClientMock.queryAllTargets()).thenReturn(targets) + val baseHashes = hasher.hashAllBazelTargetsAndSourcefiles() + + val newRule1 = createRuleTarget(name = "rule1", inputs = emptyList(), digest = "rule1Digest--changed") + val newTargets = arrayListOf(newRule1, src1, rule2) + + whenever(bazelClientMock.queryAllTargets()).thenReturn(newTargets) + val newHashes = hasher.hashAllBazelTargetsAndSourcefiles() + + val hashes = HashDiffer(baseHashes, newHashes) + + hashes.assertThat("rule1").hash().changed() + hashes.assertThat("rule1").directHash().changed() + + hashes.assertThat("gen1").hash().changed() + hashes.assertThat("gen1").directHash().changed() + + hashes.assertThat("rule2").hash().changed() + hashes.assertThat("rule2").directHash().didNotChange() + } + @Test fun hashAllBazelTargets_ruleTargets_ruleInputs() = runBlocking { val inputs = listOf("rule1") @@ -99,10 +187,10 @@ class BuildGraphHasherTest : KoinTest { whenever(bazelClientMock.queryAllTargets()).thenReturn(defaultTargets) val hash = hasher.hashAllBazelTargetsAndSourcefiles() assertThat(hash).containsOnly( - "rule1" to TargetHash("Rule", "2c963f7c06bc1cead7e3b4759e1472383d4469fc3238dc42f8848190887b4775", ""), - "rule2" to TargetHash("Rule", "bdc1abd0a07103cea34199a9c0d1020619136ff90fb88dcc3a8f873c811c1fe9", ""), - "rule3" to TargetHash("Rule", "87dd050f1ca0f684f37970092ff6a02677d995718b5a05461706c0f41ffd4915", ""), - "rule4" to TargetHash("Rule", "a7bc5d23cd98c4942dc879c649eb9646e38eddd773f9c7996fa0d96048cf63dc", ""), + "rule1" to TargetHash("Rule", "7b3149cbd2219ca05bc80a557a701ddee18bd3bbe9afa8e851df64b999155c5e", "2c963f7c06bc1cead7e3b4759e1472383d4469fc3238dc42f8848190887b4775"), + "rule2" to TargetHash("Rule", "24f12d22ab247c5af32f954ca46dd4f6323ab2eef28455411b912aaf44a7c322", "bdc1abd0a07103cea34199a9c0d1020619136ff90fb88dcc3a8f873c811c1fe9"), + "rule3" to TargetHash("Rule", "c7018bbfed16f4f6f0ef1f258024a50c56ba916b3b9ed4f00972a233d5d11b42", "4aeafed087a9c78a4efa11b6f7831c38d775ddb244a9fabbf21d78c1666a2ea9"), + "rule4" to TargetHash("Rule", "020720dfbb969ef9629e51a624a616f015fe07c7b779a5b4f82a8b36c9d3cbe9", "82b46404c8a1ec402a60de72d42a14f6a080e938e5ebaf26203c5ef480558767"), ) } @@ -117,10 +205,10 @@ class BuildGraphHasherTest : KoinTest { whenever(bazelClientMock.queryAllTargets()).thenReturn(defaultTargets) val hash = hasher.hashAllBazelTargetsAndSourcefiles() assertThat(hash).containsOnly( - "rule1" to TargetHash("Rule", "2c963f7c06bc1cead7e3b4759e1472383d4469fc3238dc42f8848190887b4775", ""), - "rule2" to TargetHash("Rule", "bdc1abd0a07103cea34199a9c0d1020619136ff90fb88dcc3a8f873c811c1fe9", ""), - "rule3" to TargetHash("Rule", "ca2f970a5a5a18730d7633cc32b48b1d94679f4ccaea56c4924e1f9913bd9cb5", ""), - "rule4" to TargetHash("Rule", "bf15e616e870aaacb02493ea0b8e90c6c750c266fa26375e22b30b78954ee523", ""), + "rule1" to TargetHash("Rule", "7b3149cbd2219ca05bc80a557a701ddee18bd3bbe9afa8e851df64b999155c5e", "2c963f7c06bc1cead7e3b4759e1472383d4469fc3238dc42f8848190887b4775"), + "rule2" to TargetHash("Rule", "24f12d22ab247c5af32f954ca46dd4f6323ab2eef28455411b912aaf44a7c322", "bdc1abd0a07103cea34199a9c0d1020619136ff90fb88dcc3a8f873c811c1fe9"), + "rule3" to TargetHash("Rule", "be17f1e1884037b970e6b7c86bb6533b253a12d967029adc711e50d4662237e8", "91ea3015d4424bb8c1ecf381c30166c386c161d31b70967f3a021c1dc43c7774"), + "rule4" to TargetHash("Rule", "f3e5675e30fe25ff9b61a0c7f64c423964f886799407a9438e692fd803ecd47c", "bce09e1689cc7a8172653981582fea70954f8acd58985c92026582e4b75ec8d2"), ) } @@ -181,4 +269,14 @@ class BuildGraphHasherTest : KoinTest { whenever(target.generatingRuleName).thenReturn(generatingRuleName) return target } + + private fun createSrcTarget(name: String, digest: String): BazelTarget { + fakeSourceFileHasher.add(name, digest.toByteArray()) + + val target = mock() + whenever(target.name).thenReturn(name) + whenever(target.sourceFileName).thenReturn(name) + whenever(target.subincludeList).thenReturn(listOf()) + return target + } } diff --git a/cli/src/test/kotlin/com/bazel_diff/hash/FakeSourceFileHasher.kt b/cli/src/test/kotlin/com/bazel_diff/hash/FakeSourceFileHasher.kt new file mode 100644 index 0000000..b54d371 --- /dev/null +++ b/cli/src/test/kotlin/com/bazel_diff/hash/FakeSourceFileHasher.kt @@ -0,0 +1,18 @@ +package com.bazel_diff.hash + +import java.nio.file.Path +import com.bazel_diff.bazel.BazelSourceFileTarget + +class FakeSourceFileHasher : SourceFileHasher { + var fakeDigests: MutableMap = mutableMapOf() + override fun digest(sourceFileTarget: BazelSourceFileTarget, modifiedFilepaths: Set ): ByteArray { + return fakeDigests[sourceFileTarget.name] ?: throw IllegalArgumentException("Digest not found for ${sourceFileTarget.name}") + } + override fun softDigest(sourceFileTarget: BazelSourceFileTarget, modifiedFilepaths: Set ): ByteArray? { + return "fake-soft-digest".toByteArray() + } + + fun add(name: String, digest: ByteArray) { + fakeDigests[name] = digest + } +} diff --git a/cli/src/test/kotlin/com/bazel_diff/hash/HashDiffer.kt b/cli/src/test/kotlin/com/bazel_diff/hash/HashDiffer.kt new file mode 100644 index 0000000..95c6086 --- /dev/null +++ b/cli/src/test/kotlin/com/bazel_diff/hash/HashDiffer.kt @@ -0,0 +1,42 @@ +package com.bazel_diff.hash + +import assertk.assertThat +import assertk.assertions.* + +/** + * Utility class for performing hash comparisons between two maps of target hashes. + * + * Allows the creation of test assertions that don't depend on the underlying hash values, + * but instead assert properties based on expected changes to hash values when comparing + * the results of hashing two build graphs. + * + * @property from The map of target hashes to compare from. + * @property to The map of target hashes to compare to. + */ +class HashDiffer(private val from: Map, private val to: Map) { + + fun assertThat(ruleName: String): SingleTarget { + return SingleTarget(from[ruleName], to[ruleName]) + } + + inner class SingleTarget(private val fromHash: TargetHash?, private val toHash: TargetHash?) { + + fun hash(): HashAssertion { + return HashAssertion(fromHash?.hash, toHash?.hash) + } + + fun directHash(): HashAssertion { + return HashAssertion(fromHash?.directHash, toHash?.directHash) + } + } + + inner class HashAssertion(private val fromHash: String?, private val toHash: String?) { + fun changed() { + assertThat(fromHash).isNotEqualTo(toHash) + } + + fun didNotChange() { + assertThat(fromHash).isEqualTo(toHash) + } + } +} diff --git a/cli/src/test/kotlin/com/bazel_diff/hash/SourceFileHasherTest.kt b/cli/src/test/kotlin/com/bazel_diff/hash/SourceFileHasherTest.kt index a746052..32b7627 100644 --- a/cli/src/test/kotlin/com/bazel_diff/hash/SourceFileHasherTest.kt +++ b/cli/src/test/kotlin/com/bazel_diff/hash/SourceFileHasherTest.kt @@ -36,7 +36,7 @@ internal class SourceFileHasherTest : KoinTest { @Test fun testHashConcreteFile() = runBlocking { - val hasher = SourceFileHasher(repoAbsolutePath, null, externalRepoResolver) + val hasher = SourceFileHasherImpl(repoAbsolutePath, null, externalRepoResolver) val bazelSourceFileTarget = BazelSourceFileTarget(fixtureFileTarget, seed) val actual = hasher.digest(bazelSourceFileTarget).toHexString() val expected = sha256 { @@ -49,7 +49,7 @@ internal class SourceFileHasherTest : KoinTest { @Test fun testHashConcreteFileWithModifiedFilepathsEnabled() = runBlocking { - val hasher = SourceFileHasher(repoAbsolutePath, null, externalRepoResolver) + val hasher = SourceFileHasherImpl(repoAbsolutePath, null, externalRepoResolver) val bazelSourceFileTarget = BazelSourceFileTarget(fixtureFileTarget, seed) val actual = hasher.digest( bazelSourceFileTarget, @@ -65,7 +65,7 @@ internal class SourceFileHasherTest : KoinTest { @Test fun testHashConcreteFileWithModifiedFilepathsEnabledNoMatch() = runBlocking { - val hasher = SourceFileHasher(repoAbsolutePath, null, externalRepoResolver) + val hasher = SourceFileHasherImpl(repoAbsolutePath, null, externalRepoResolver) val bazelSourceFileTarget = BazelSourceFileTarget(fixtureFileTarget, seed) val actual = hasher.digest( bazelSourceFileTarget, @@ -80,7 +80,7 @@ internal class SourceFileHasherTest : KoinTest { @Test fun testHashConcreteFileInExternalRepo() = runBlocking { - val hasher = SourceFileHasher(repoAbsolutePath, null, externalRepoResolver, setOf("external_repo")) + val hasher = SourceFileHasherImpl(repoAbsolutePath, null, externalRepoResolver, setOf("external_repo")) val externalRepoFilePath = outputBasePath.resolve("external/external_repo/path/to/my_file.txt") Files.createDirectories(externalRepoFilePath.parent) val externalRepoFileTarget = "@external_repo//path/to:my_file.txt" @@ -98,7 +98,7 @@ internal class SourceFileHasherTest : KoinTest { @Test fun testSoftHashConcreteFile() = runBlocking { - val hasher = SourceFileHasher(repoAbsolutePath, null, externalRepoResolver) + val hasher = SourceFileHasherImpl(repoAbsolutePath, null, externalRepoResolver) val bazelSourceFileTarget = BazelSourceFileTarget(fixtureFileTarget, seed) val actual = hasher.softDigest(bazelSourceFileTarget)?.toHexString() val expected = sha256 { @@ -111,7 +111,7 @@ internal class SourceFileHasherTest : KoinTest { @Test fun testSoftHashNonExistedFile() = runBlocking { - val hasher = SourceFileHasher(repoAbsolutePath, null, externalRepoResolver) + val hasher = SourceFileHasherImpl(repoAbsolutePath, null, externalRepoResolver) val bazelSourceFileTarget = BazelSourceFileTarget("//i/do/not/exist", seed) val actual = hasher.softDigest(bazelSourceFileTarget) assertThat(actual).isNull() @@ -120,7 +120,7 @@ internal class SourceFileHasherTest : KoinTest { @Test fun testSoftHashExternalTarget() = runBlocking { val target = "@bazel-diff//some:file" - val hasher = SourceFileHasher(repoAbsolutePath, null, externalRepoResolver) + val hasher = SourceFileHasherImpl(repoAbsolutePath, null, externalRepoResolver) val bazelSourceFileTarget = BazelSourceFileTarget(target, seed) val actual = hasher.softDigest(bazelSourceFileTarget) assertThat(actual).isNull() @@ -129,7 +129,7 @@ internal class SourceFileHasherTest : KoinTest { @Test fun testHashNonExistedFile() = runBlocking { val target = "//i/do/not/exist" - val hasher = SourceFileHasher(repoAbsolutePath, null, externalRepoResolver) + val hasher = SourceFileHasherImpl(repoAbsolutePath, null, externalRepoResolver) val bazelSourceFileTarget = BazelSourceFileTarget(target, seed) val actual = hasher.digest(bazelSourceFileTarget).toHexString() val expected = sha256 { @@ -142,7 +142,7 @@ internal class SourceFileHasherTest : KoinTest { @Test fun testHashExternalTarget() = runBlocking { val target = "@bazel-diff//some:file" - val hasher = SourceFileHasher(repoAbsolutePath, null, externalRepoResolver) + val hasher = SourceFileHasherImpl(repoAbsolutePath, null, externalRepoResolver) val bazelSourceFileTarget = BazelSourceFileTarget(target, seed) val actual = hasher.digest(bazelSourceFileTarget).toHexString() val expected = sha256 {}.toHexString() @@ -152,7 +152,7 @@ internal class SourceFileHasherTest : KoinTest { @Test fun testHashWithProvidedContentHash() = runBlocking { val filenameToContentHash = hashMapOf("cli/src/test/kotlin/com/bazel_diff/hash/fixture/foo.ts" to "foo-content-hash") - val hasher = SourceFileHasher(repoAbsolutePath, filenameToContentHash, externalRepoResolver) + val hasher = SourceFileHasherImpl(repoAbsolutePath, filenameToContentHash, externalRepoResolver) val bazelSourceFileTarget = BazelSourceFileTarget(fixtureFileTarget, seed) val actual = hasher.digest(bazelSourceFileTarget).toHexString() val expected = sha256 { @@ -166,7 +166,7 @@ internal class SourceFileHasherTest : KoinTest { @Test fun testHashWithProvidedContentHashButNotInKey() = runBlocking { val filenameToContentHash = hashMapOf("cli/src/test/kotlin/com/bazel_diff/hash/fixture/bar.ts" to "foo-content-hash") - val hasher = SourceFileHasher(repoAbsolutePath, filenameToContentHash, externalRepoResolver) + val hasher = SourceFileHasherImpl(repoAbsolutePath, filenameToContentHash, externalRepoResolver) val bazelSourceFileTarget = BazelSourceFileTarget(fixtureFileTarget, seed) val actual = hasher.digest(bazelSourceFileTarget).toHexString() val expected = sha256 { @@ -181,7 +181,7 @@ internal class SourceFileHasherTest : KoinTest { fun testHashWithProvidedContentHashWithLeadingColon() = runBlocking { val targetName = "//:cli/src/test/kotlin/com/bazel_diff/hash/fixture/bar.ts" val filenameToContentHash = hashMapOf("cli/src/test/kotlin/com/bazel_diff/hash/fixture/bar.ts" to "foo-content-hash") - val hasher = SourceFileHasher(repoAbsolutePath, filenameToContentHash, externalRepoResolver) + val hasher = SourceFileHasherImpl(repoAbsolutePath, filenameToContentHash, externalRepoResolver) val bazelSourceFileTarget = BazelSourceFileTarget(targetName, seed) val actual = hasher.digest(bazelSourceFileTarget).toHexString() val expected = sha256 { From edfe0c8a3dcc7ccab53e1d3c612206976c24589d Mon Sep 17 00:00:00 2001 From: Alex Torok Date: Sat, 28 Sep 2024 20:12:57 +0000 Subject: [PATCH 03/14] Add ability to compute target distance metrics --- .../CalculateImpactedTargetsInteractor.kt | 89 ++++++++ .../CalculateImpactedTargetsInteractorTest.kt | 211 +++++++++++++++++- 2 files changed, 299 insertions(+), 1 deletion(-) 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 4f3f442..1206769 100644 --- a/cli/src/main/kotlin/com/bazel_diff/interactor/CalculateImpactedTargetsInteractor.kt +++ b/cli/src/main/kotlin/com/bazel_diff/interactor/CalculateImpactedTargetsInteractor.kt @@ -4,8 +4,27 @@ import com.bazel_diff.hash.TargetHash import com.google.common.collect.Maps import org.koin.core.component.KoinComponent import java.io.File +import java.util.concurrent.ConcurrentMap +import java.util.concurrent.ConcurrentHashMap +import java.util.stream.Stream +import java.util.stream.Collectors +import com.google.common.annotations.VisibleForTesting + +data class TargetDistanceMetrics( + val targetDistance: Int, + val packageDistance: Int +) {} class CalculateImpactedTargetsInteractor : KoinComponent { + + @VisibleForTesting + class InvalidDependencyEdgesException(message: String) : Exception(message) + + enum class ImpactType { + DIRECT, + INDIRECT + } + fun execute(from: Map, to: Map): Set { /** * This call might be faster if end hashes is a sorted map @@ -19,4 +38,74 @@ class CalculateImpactedTargetsInteractor : KoinComponent { } return impactedTargets } + + fun executeWithDistances(from: Map, to: Map, depEdges: Map>): Map { + val difference = Maps.difference(to, from) + + val newLabels = difference.entriesOnlyOnLeft().keys + val existingImpactedLabels = difference.entriesDiffering().keys + + val impactedLabels = HashMap().apply { + newLabels.forEach { this[it] = ImpactType.DIRECT } + existingImpactedLabels.forEach { + this[it] = if (from[it]!!.directHash != to[it]!!.directHash) ImpactType.DIRECT else ImpactType.INDIRECT + } + } + + val computedResult: ConcurrentMap = ConcurrentHashMap() + + impactedLabels.keys.parallelStream() + .forEach { + calculateDistance(it, depEdges, computedResult, impactedLabels) + } + + return computedResult + } + + fun calculateDistance( + label: String, + depEdges: Map>, + impactedTargets: ConcurrentMap, + impactedLabels: Map + ): TargetDistanceMetrics { + impactedTargets[label]?.let { return it } + + if (label !in impactedLabels) { + throw IllegalArgumentException("$label was not impacted, but was requested to calculate distance.") + } + + // If the label is directly impacted, it has a distance of 0 + if (impactedLabels[label] == ImpactType.DIRECT) { + return TargetDistanceMetrics(0, 0).also { impactedTargets[label] = it } + } + + val directDeps = depEdges[label] ?: throw InvalidDependencyEdgesException("$label was indirectly impacted, but has no dependencies.") + + // Now compute the distance for label, which was indirectly impacted + val (targetDistance, packageDistance) = directDeps.parallelStream() + .filter {it in impactedLabels} + .collect(Collectors.toList()) + .let { impactedDepLabels -> + if (impactedDepLabels.isEmpty()) { + throw InvalidDependencyEdgesException("$label was indirectly impacted, but has no impacted dependencies.") + } + impactedDepLabels.parallelStream() + }.map { dep -> + val distanceMetrics = calculateDistance(dep, depEdges, impactedTargets, impactedLabels) + val crossesPackageBoundary = label.split(":")[0] != dep.split(":")[0] + Pair( + distanceMetrics.targetDistance + 1, + distanceMetrics.packageDistance + if (crossesPackageBoundary) 1 else 0 + ) + } + .collect(Collectors.toList()) + .let { distances -> + val minTargetDistance = distances.minOf { it.first } + val minPackageDistance = distances.minOf { it.second } + Pair(minTargetDistance, minPackageDistance) + } + + + return TargetDistanceMetrics(targetDistance, packageDistance).also { impactedTargets[label] = it } + } } 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 eeb3c2b..2a952fb 100644 --- a/cli/src/test/kotlin/com/bazel_diff/interactor/CalculateImpactedTargetsInteractorTest.kt +++ b/cli/src/test/kotlin/com/bazel_diff/interactor/CalculateImpactedTargetsInteractorTest.kt @@ -1,7 +1,7 @@ package com.bazel_diff.interactor import assertk.assertThat -import assertk.assertions.containsExactlyInAnyOrder +import assertk.assertions.* import com.bazel_diff.testModule import org.junit.Rule import org.junit.Test @@ -9,6 +9,7 @@ import org.koin.test.KoinTest import org.koin.test.KoinTestRule import org.mockito.junit.MockitoJUnit import com.bazel_diff.hash.TargetHash +import com.bazel_diff.interactor.CalculateImpactedTargetsInteractor.InvalidDependencyEdgesException class CalculateImpactedTargetsInteractorTest : KoinTest { @get:Rule @@ -38,4 +39,212 @@ class CalculateImpactedTargetsInteractorTest : KoinTest { "1", "3" ) } + + @Test + fun testOmitsUnchangedTargets() { + val (depEdges, startHashes) = createTargetHashes( + "//:1 <- //:2 <- //:3", + "//:unchanged <- //:3", + ) + + val endHashes = startHashes.toMutableMap() + + makeDirectlyChanged(endHashes, "//:1", "//:2") + makeIndirectlyChanged(endHashes, "//:3") + + val interactor = CalculateImpactedTargetsInteractor() + val impacted = interactor.executeWithDistances(startHashes, endHashes, depEdges) + + assertThat(impacted).containsOnly( + "//:1" to TargetDistanceMetrics(0, 0), + "//:2" to TargetDistanceMetrics(0, 0), + "//:3" to TargetDistanceMetrics(1, 0), + ) + } + + @Test + fun testNewTargetsDirectlyModified() { + val startHashes: Map = mapOf() + + val endHashes = mapOf( + "//:1" to TargetHash("", "a", "a"), + "//:2" to TargetHash("", "b", "b"), + ) + + val interactor = CalculateImpactedTargetsInteractor() + val impacted = interactor.executeWithDistances(startHashes, endHashes, mapOf()) + + assertThat(impacted).containsOnly( + "//:1" to TargetDistanceMetrics(0, 0), + "//:2" to TargetDistanceMetrics(0, 0), + ) + } + + @Test + fun testTargetDistances() { + var (depEdges, startHashes) = createTargetHashes( + "//:1 <- //:2 <- //:3" + ) + val endHashes = startHashes.toMutableMap() + + makeDirectlyChanged(endHashes, "//:1") + makeIndirectlyChanged(endHashes, "//:2", "//:3") + + val interactor = CalculateImpactedTargetsInteractor() + val impacted = interactor.executeWithDistances(startHashes, endHashes, depEdges) + + assertThat(impacted).containsOnly( + "//:1" to TargetDistanceMetrics(0,0), + "//:2" to TargetDistanceMetrics(1,0), + "//:3" to TargetDistanceMetrics(2,0), + ) + } + + @Test + fun testPackageDistance() { + var (depEdges, startHashes) = createTargetHashes( + "//A:1 <- //A:2 <- //B:3 <- //B:4 <- //C:5" + ) + val endHashes = startHashes.toMutableMap() + + makeDirectlyChanged(endHashes, "//A:1") + makeIndirectlyChanged(endHashes, "//A:2", "//B:3", "//B:4", "//C:5") + + val interactor = CalculateImpactedTargetsInteractor() + val impacted = interactor.executeWithDistances(startHashes, endHashes, depEdges) + + assertThat(impacted).containsOnly( + "//A:1" to TargetDistanceMetrics(0,0), + "//A:2" to TargetDistanceMetrics(1,0), + "//B:3" to TargetDistanceMetrics(2,1), + "//B:4" to TargetDistanceMetrics(3,1), + "//C:5" to TargetDistanceMetrics(4,2), + ) + } + + @Test + fun testFindsShortestDistances() { + // Test that we find the shortest target and package distance, even if they each pass + // through different dependency chains. + // + // //final:final's targetDistance should be 4 due to passing through //B, //C, and //D, + // but its packageDistance should be 2 due to passing through //A. + val (depEdges, startHashes) = createTargetHashes( + "//changed:target <- //A:target_1 <- //A:target_2 <- //A:target_3 <- //A:target_4 <- //final:final", + "//changed:target <- //B:target <- //C:target <- //D:target <- //final:final", + ) + val endHashes = startHashes.toMutableMap() + + makeDirectlyChanged(endHashes, "//changed:target") + makeIndirectlyChanged(endHashes, "//A:target_1", "//A:target_2", "//A:target_3", "//A:target_4", "//B:target", "//C:target", "//D:target", "//final:final") + + val interactor = CalculateImpactedTargetsInteractor() + val impacted = interactor.executeWithDistances(startHashes, endHashes, depEdges) + + assertThat(impacted["//final:final"]).isEqualTo(TargetDistanceMetrics(4,2)) + } + + @Test + fun testDependsOnNewTarget() { + var (depEdges, startHashes) = createTargetHashes( + "//:1 <- //:2 <- //:3" + ) + val endHashes = startHashes.toMutableMap() + + // Remove //:1 so that it is a new target in the end hashes, but //:2 is + // only indirectly modified. + // Technically, this probably can't happen, as //:2 would have to have had + // one of its deps attrs modified, which would have caused it to be directly + // modified, but we should handle it anyway. + startHashes.remove("//:1") + + makeIndirectlyChanged(endHashes, "//:2", "//:3") + + val interactor = CalculateImpactedTargetsInteractor() + val impacted = interactor.executeWithDistances(startHashes, endHashes, depEdges) + + assertThat(impacted).containsOnly( + "//:1" to TargetDistanceMetrics(0,0), + "//:2" to TargetDistanceMetrics(1,0), + "//:3" to TargetDistanceMetrics(2,0), + ) + } + + @Test + fun testInvalidEdgesRaises() { + var (depEdges, startHashes) = createTargetHashes( + "//:1 <- //:2" + ) + val endHashes = startHashes.toMutableMap() + + makeIndirectlyChanged(endHashes, "//:2") + + val interactor = CalculateImpactedTargetsInteractor() + assertThat { + interactor.executeWithDistances(startHashes, endHashes, depEdges) + }.isFailure().message().isEqualTo("//:2 was indirectly impacted, but has no impacted dependencies.") + + assertThat { + // empty dep edges + interactor.executeWithDistances(startHashes, endHashes, mapOf()) + }.isFailure().message().isEqualTo("//:2 was indirectly impacted, but has no dependencies.") + + } + + + + /** + * Creates a mapping of target hashes and dependency edges from the provided graph specifications. + * + * @param graphSpecs Vararg parameter representing the graph specifications. Each specification is a string + * where targets are separated by " <- " indicating a dependency relationship. + * @return A pair consisting of: + * - A map where the keys are target labels and the values are lists of labels that depend on the key. + * - A map where the keys are target labels and the values are `TargetHash` objects representing the target hashes. + */ + fun createTargetHashes(vararg graphSpecs: String): Pair>, MutableMap> { + val targetHashMap = mutableMapOf() + val depEdges = mutableMapOf>() + + for (spec in graphSpecs) { + val labels = spec.split(" <- ") + labels.zipWithNext().forEach { (prevLabel, label) -> + + if (!targetHashMap.containsKey(prevLabel)) { + targetHashMap[prevLabel] = TargetHash("", prevLabel, prevLabel) + } + + if (!targetHashMap.containsKey(label)) { + targetHashMap[label] = TargetHash("", label, label) + } + + depEdges.computeIfAbsent(label) { mutableListOf() }.add(prevLabel) + } + } + + return depEdges to targetHashMap + } + + fun makeDirectlyChanged(targetHashes: MutableMap, vararg labels: String) { + for (label in labels) { + if (!targetHashes.containsKey(label)) { + throw IllegalArgumentException("Label $label not found in target hashes") + } + val orig = targetHashes[label]!! + targetHashes[label] = orig.copy( + directHash = orig.directHash + "-changed", + hash = orig.hash + "-changed" + ) + } + } + + fun makeIndirectlyChanged(targetHashes: MutableMap, vararg labels: String) { + for (label in labels) { + if (!targetHashes.containsKey(label)) { + throw IllegalArgumentException("Label $label not found in target hashes") + } + val orig = targetHashes[label]!! + targetHashes[label] = orig.copy(hash = orig.hash + "-changed") + } + } } From aba2f8787361cbda6674fdd94907d2938636917b Mon Sep 17 00:00:00 2001 From: Alex Torok Date: Sun, 29 Sep 2024 13:27:48 +0000 Subject: [PATCH 04/14] Add ability to dump dependency edges to json file. --- .../bazel_diff/cli/GenerateHashesCommand.kt | 11 ++++- .../main/kotlin/com/bazel_diff/di/Modules.kt | 3 +- .../com/bazel_diff/hash/BuildGraphHasher.kt | 1 + .../kotlin/com/bazel_diff/hash/RuleHasher.kt | 6 +-- .../com/bazel_diff/hash/TargetDigest.kt | 25 ++++++++-- .../kotlin/com/bazel_diff/hash/TargetHash.kt | 3 +- .../com/bazel_diff/hash/TargetHasher.kt | 9 +++- .../interactor/GenerateHashesInteractor.kt | 7 ++- cli/src/test/kotlin/com/bazel_diff/Modules.kt | 2 +- .../bazel_diff/hash/BuildGraphHasherTest.kt | 48 ++++++++++++++----- 10 files changed, 88 insertions(+), 27 deletions(-) diff --git a/cli/src/main/kotlin/com/bazel_diff/cli/GenerateHashesCommand.kt b/cli/src/main/kotlin/com/bazel_diff/cli/GenerateHashesCommand.kt index cb4ca7a..09ad385 100644 --- a/cli/src/main/kotlin/com/bazel_diff/cli/GenerateHashesCommand.kt +++ b/cli/src/main/kotlin/com/bazel_diff/cli/GenerateHashesCommand.kt @@ -136,6 +136,14 @@ class GenerateHashesCommand : Callable { ) var ignoredRuleHashingAttributes: Set = emptySet() + @CommandLine.Option( + names = ["-d", "--depsFile"], + description = ["Path to the file where dependency edges are written to. If not specified, the dependency edges will not be written to a file."], + scope = CommandLine.ScopeType.INHERIT, + defaultValue = CommandLine.Parameters.NULL_VALUE + ) + var depsFile: File? = null + @CommandLine.Option( names = ["-m", "--modified-filepaths"], description = ["Experimental: A text file containing a newline separated list of filepaths (relative to the workspace) these filepaths should represent the modified files between the specified revisions and will be used to scope what files are hashed during hash generation."] @@ -159,6 +167,7 @@ class GenerateHashesCommand : Callable { cqueryCommandOptions, useCquery, keepGoing, + depsFile != null, fineGrainedHashExternalRepos, ), loggingModule(parent.verbose), @@ -166,7 +175,7 @@ class GenerateHashesCommand : Callable { ) } - return when (GenerateHashesInteractor().execute(seedFilepaths, outputPath, ignoredRuleHashingAttributes, targetType, includeTargetType, modifiedFilepaths)) { + return when (GenerateHashesInteractor().execute(seedFilepaths, outputPath, depsFile, ignoredRuleHashingAttributes, targetType, includeTargetType, modifiedFilepaths)) { true -> CommandLine.ExitCode.OK false -> CommandLine.ExitCode.SOFTWARE }.also { stopKoin() } diff --git a/cli/src/main/kotlin/com/bazel_diff/di/Modules.kt b/cli/src/main/kotlin/com/bazel_diff/di/Modules.kt index 29ab68d..3094fc3 100644 --- a/cli/src/main/kotlin/com/bazel_diff/di/Modules.kt +++ b/cli/src/main/kotlin/com/bazel_diff/di/Modules.kt @@ -28,6 +28,7 @@ fun hasherModule( cqueryOptions: List, useCquery: Boolean, keepGoing: Boolean, + trackDeps: Boolean, fineGrainedHashExternalRepos: Set, ): Module = module { val cmd: MutableList = ArrayList().apply { @@ -61,7 +62,7 @@ fun hasherModule( single { BazelClient(useCquery, fineGrainedHashExternalRepos) } single { BuildGraphHasher(get()) } single { TargetHasher() } - single { RuleHasher(useCquery, fineGrainedHashExternalRepos) } + single { RuleHasher(useCquery, trackDeps, fineGrainedHashExternalRepos) } single { SourceFileHasherImpl(fineGrainedHashExternalRepos) } single { ExternalRepoResolver(workingDirectory, bazelPath, outputPath) } single(named("working-directory")) { workingDirectory } diff --git a/cli/src/main/kotlin/com/bazel_diff/hash/BuildGraphHasher.kt b/cli/src/main/kotlin/com/bazel_diff/hash/BuildGraphHasher.kt index 1d736d6..bc3cc77 100644 --- a/cli/src/main/kotlin/com/bazel_diff/hash/BuildGraphHasher.kt +++ b/cli/src/main/kotlin/com/bazel_diff/hash/BuildGraphHasher.kt @@ -118,6 +118,7 @@ class BuildGraphHasher(private val bazelClient: BazelClient) : KoinComponent { target.javaClass.name.substringAfterLast('$'), targetDigest.overallDigest.toHexString(), targetDigest.directDigest.toHexString(), + targetDigest.deps, )) } .filter { targetEntry: Pair? -> targetEntry != null } diff --git a/cli/src/main/kotlin/com/bazel_diff/hash/RuleHasher.kt b/cli/src/main/kotlin/com/bazel_diff/hash/RuleHasher.kt index e41126b..3a00c93 100644 --- a/cli/src/main/kotlin/com/bazel_diff/hash/RuleHasher.kt +++ b/cli/src/main/kotlin/com/bazel_diff/hash/RuleHasher.kt @@ -9,7 +9,7 @@ import org.koin.core.component.inject import java.util.concurrent.ConcurrentMap import java.nio.file.Path -class RuleHasher(private val useCquery: Boolean, private val fineGrainedHashExternalRepos: Set) : KoinComponent { +class RuleHasher(private val useCquery: Boolean, private val trackDepLabels: Boolean, private val fineGrainedHashExternalRepos: Set) : KoinComponent { private val logger: Logger by inject() private val sourceFileHasher: SourceFileHasher by inject() @@ -42,7 +42,7 @@ class RuleHasher(private val useCquery: Boolean, private val fineGrainedHashExte depPathClone.add(rule.name) ruleHashes[rule.name]?.let { return it } - val finalHashValue = targetSha256 { + val finalHashValue = targetSha256(trackDepLabels) { putDirectBytes(rule.digest(ignoredAttrs)) putDirectBytes(seedHash) @@ -66,7 +66,7 @@ class RuleHasher(private val useCquery: Boolean, private val fineGrainedHashExte ignoredAttrs, modifiedFilepaths ) - putBytes(ruleInputHash.overallDigest) + putTransitiveBytes(ruleInput, ruleInputHash.overallDigest) } else -> { diff --git a/cli/src/main/kotlin/com/bazel_diff/hash/TargetDigest.kt b/cli/src/main/kotlin/com/bazel_diff/hash/TargetDigest.kt index 1f9d89a..24c86ee 100644 --- a/cli/src/main/kotlin/com/bazel_diff/hash/TargetDigest.kt +++ b/cli/src/main/kotlin/com/bazel_diff/hash/TargetDigest.kt @@ -5,21 +5,28 @@ import com.google.common.hash.Hashing data class TargetDigest( val overallDigest: ByteArray, val directDigest: ByteArray, + val deps: List? = null, ) { - fun clone(): TargetDigest { - return TargetDigest(overallDigest.clone(), directDigest.clone()) + fun clone(newDeps: List? = null): TargetDigest { + var newDeps = newDeps + if (newDeps == null) { + newDeps = deps + } + return TargetDigest(overallDigest.clone(), directDigest.clone(), newDeps) } } -fun targetSha256(block: TargetDigestBuilder.() -> Unit): TargetDigest { - val hasher = TargetDigestBuilder() +fun targetSha256(trackDepLabels: Boolean, block: TargetDigestBuilder.() -> Unit): TargetDigest { + val hasher = TargetDigestBuilder(trackDepLabels) hasher.apply(block) return hasher.finish() } -class TargetDigestBuilder { +class TargetDigestBuilder(trackDepLabels: Boolean) { + private val overallHasher = Hashing.sha256().newHasher() private val directHasher = Hashing.sha256().newHasher() + private val deps: MutableList? = if (trackDepLabels) mutableListOf() else null fun putDirectBytes(block: ByteArray?) { block?.let { directHasher.putBytes(it) } @@ -29,6 +36,13 @@ class TargetDigestBuilder { block?.let { overallHasher.putBytes(it) } } + fun putTransitiveBytes(dep: String, block: ByteArray?) { + block?.let { overallHasher.putBytes(it) } + if (deps != null) { + deps.add(dep) + } + } + fun finish(): TargetDigest { val directHash = directHasher.hash().asBytes().clone() overallHasher.putBytes(directHash) @@ -36,6 +50,7 @@ class TargetDigestBuilder { return TargetDigest( overallHasher.hash().asBytes().clone(), directHash, + deps, ) } } diff --git a/cli/src/main/kotlin/com/bazel_diff/hash/TargetHash.kt b/cli/src/main/kotlin/com/bazel_diff/hash/TargetHash.kt index 503c5d5..1da4b29 100644 --- a/cli/src/main/kotlin/com/bazel_diff/hash/TargetHash.kt +++ b/cli/src/main/kotlin/com/bazel_diff/hash/TargetHash.kt @@ -3,7 +3,8 @@ package com.bazel_diff.hash data class TargetHash( val type: String, // Rule/GeneratedFile/SourceFile/... val hash: String, - val directHash: String + val directHash: String, + val deps: List? = null ) { val hashWithType by lazy { "${type}#${hash}#${directHash}" diff --git a/cli/src/main/kotlin/com/bazel_diff/hash/TargetHasher.kt b/cli/src/main/kotlin/com/bazel_diff/hash/TargetHasher.kt index 9944d0a..af51359 100644 --- a/cli/src/main/kotlin/com/bazel_diff/hash/TargetHasher.kt +++ b/cli/src/main/kotlin/com/bazel_diff/hash/TargetHasher.kt @@ -22,12 +22,13 @@ class TargetHasher : KoinComponent { return when (target) { is BazelTarget.GeneratedFile -> { val generatingRuleDigest = ruleHashes[target.generatingRuleName] + var digest: TargetDigest if (generatingRuleDigest != null) { - generatingRuleDigest.clone() + digest = generatingRuleDigest } else { val generatingRule = allRulesMap[target.generatingRuleName] ?: throw RuntimeException("Unexpected generating rule ${target.generatingRuleName}") - ruleHasher.digest( + digest = ruleHasher.digest( generatingRule, allRulesMap, ruleHashes, @@ -38,6 +39,10 @@ class TargetHasher : KoinComponent { modifiedFilepaths ) } + + // Add the generating rule name as a dep of the generated file. + digest = digest.clone(newDeps = listOf(target.generatingRuleName)) + digest } is BazelTarget.Rule -> { ruleHasher.digest( diff --git a/cli/src/main/kotlin/com/bazel_diff/interactor/GenerateHashesInteractor.kt b/cli/src/main/kotlin/com/bazel_diff/interactor/GenerateHashesInteractor.kt index ba6111e..e98ba49 100644 --- a/cli/src/main/kotlin/com/bazel_diff/interactor/GenerateHashesInteractor.kt +++ b/cli/src/main/kotlin/com/bazel_diff/interactor/GenerateHashesInteractor.kt @@ -18,7 +18,7 @@ class GenerateHashesInteractor : KoinComponent { private val logger: Logger by inject() private val gson: Gson by inject() - fun execute(seedFilepaths: File?, outputPath: File?, ignoredRuleHashingAttributes: Set, targetTypes:Set?, includeTargetType: Boolean = false, modifiedFilepaths: File?): Boolean { + fun execute(seedFilepaths: File?, outputPath: File?, depsFile: File?, ignoredRuleHashingAttributes: Set, targetTypes:Set?, includeTargetType: Boolean = false, modifiedFilepaths: File?): Boolean { return try { val epoch = Calendar.getInstance().getTimeInMillis() val seedFilepathsSet: Set = when { @@ -58,6 +58,11 @@ class GenerateHashesInteractor : KoinComponent { }.use { fileWriter -> fileWriter.write(gson.toJson(hashes.mapValues { it.value.toJson(includeTargetType) })) } + if (depsFile != null) { + FileWriter(depsFile).use { fileWriter -> + fileWriter.write(gson.toJson(hashes.mapValues { it.value.deps })) + } + } val duration = Calendar.getInstance().getTimeInMillis() - epoch; logger.i { "generate-hashes finished in $duration" } true diff --git a/cli/src/test/kotlin/com/bazel_diff/Modules.kt b/cli/src/test/kotlin/com/bazel_diff/Modules.kt index bea36d6..018c04a 100644 --- a/cli/src/test/kotlin/com/bazel_diff/Modules.kt +++ b/cli/src/test/kotlin/com/bazel_diff/Modules.kt @@ -17,7 +17,7 @@ fun testModule(): Module = module { single { BazelClient(false, emptySet()) } single { BuildGraphHasher(get()) } single { TargetHasher() } - single { RuleHasher(false, emptySet()) } + single { RuleHasher(false, true, emptySet()) } single { ExternalRepoResolver(workingDirectory, Paths.get("bazel"), outputBase) } single { SourceFileHasherImpl() } single { GsonBuilder().disableHtmlEscaping().setPrettyPrinting().create() } diff --git a/cli/src/test/kotlin/com/bazel_diff/hash/BuildGraphHasherTest.kt b/cli/src/test/kotlin/com/bazel_diff/hash/BuildGraphHasherTest.kt index fcfec83..6cb2bab 100644 --- a/cli/src/test/kotlin/com/bazel_diff/hash/BuildGraphHasherTest.kt +++ b/cli/src/test/kotlin/com/bazel_diff/hash/BuildGraphHasherTest.kt @@ -77,8 +77,8 @@ class BuildGraphHasherTest : KoinTest { val hash = hasher.hashAllBazelTargetsAndSourcefiles() assertThat(hash).containsOnly( - "rule1" to TargetHash("Rule", "7b3149cbd2219ca05bc80a557a701ddee18bd3bbe9afa8e851df64b999155c5e", "2c963f7c06bc1cead7e3b4759e1472383d4469fc3238dc42f8848190887b4775"), - "rule2" to TargetHash("Rule", "24f12d22ab247c5af32f954ca46dd4f6323ab2eef28455411b912aaf44a7c322", "bdc1abd0a07103cea34199a9c0d1020619136ff90fb88dcc3a8f873c811c1fe9"), + "rule1" to TargetHash("Rule", "7b3149cbd2219ca05bc80a557a701ddee18bd3bbe9afa8e851df64b999155c5e", "2c963f7c06bc1cead7e3b4759e1472383d4469fc3238dc42f8848190887b4775", emptyList()), + "rule2" to TargetHash("Rule", "24f12d22ab247c5af32f954ca46dd4f6323ab2eef28455411b912aaf44a7c322", "bdc1abd0a07103cea34199a9c0d1020619136ff90fb88dcc3a8f873c811c1fe9", emptyList()), ) } @@ -89,8 +89,8 @@ class BuildGraphHasherTest : KoinTest { whenever(bazelClientMock.queryAllTargets()).thenReturn(defaultTargets) val hash = hasher.hashAllBazelTargetsAndSourcefiles(seedFilepaths) assertThat(hash).containsOnly( - "rule1" to TargetHash("Rule", "ddf7345122667dda1bbbdc813a6d029795b243e729bcfdcd520cde12cac877f1", "0404d80eadcc2dbfe9f0d7935086e1115344a06bd76d4e16af0dfd7b4913ee60"), - "rule2" to TargetHash("Rule", "41cbcdcbc90aafbeb74b903e7cadcf0c16f36f28dbd7f4cb72699449ff1ab11d", "6fe63fa16340d18176e6d6021972c65413441b72135247179362763508ebddfe"), + "rule1" to TargetHash("Rule", "ddf7345122667dda1bbbdc813a6d029795b243e729bcfdcd520cde12cac877f1", "0404d80eadcc2dbfe9f0d7935086e1115344a06bd76d4e16af0dfd7b4913ee60", emptyList()), + "rule2" to TargetHash("Rule", "41cbcdcbc90aafbeb74b903e7cadcf0c16f36f28dbd7f4cb72699449ff1ab11d", "6fe63fa16340d18176e6d6021972c65413441b72135247179362763508ebddfe", emptyList()), ) } @@ -187,10 +187,10 @@ class BuildGraphHasherTest : KoinTest { whenever(bazelClientMock.queryAllTargets()).thenReturn(defaultTargets) val hash = hasher.hashAllBazelTargetsAndSourcefiles() assertThat(hash).containsOnly( - "rule1" to TargetHash("Rule", "7b3149cbd2219ca05bc80a557a701ddee18bd3bbe9afa8e851df64b999155c5e", "2c963f7c06bc1cead7e3b4759e1472383d4469fc3238dc42f8848190887b4775"), - "rule2" to TargetHash("Rule", "24f12d22ab247c5af32f954ca46dd4f6323ab2eef28455411b912aaf44a7c322", "bdc1abd0a07103cea34199a9c0d1020619136ff90fb88dcc3a8f873c811c1fe9"), - "rule3" to TargetHash("Rule", "c7018bbfed16f4f6f0ef1f258024a50c56ba916b3b9ed4f00972a233d5d11b42", "4aeafed087a9c78a4efa11b6f7831c38d775ddb244a9fabbf21d78c1666a2ea9"), - "rule4" to TargetHash("Rule", "020720dfbb969ef9629e51a624a616f015fe07c7b779a5b4f82a8b36c9d3cbe9", "82b46404c8a1ec402a60de72d42a14f6a080e938e5ebaf26203c5ef480558767"), + "rule1" to TargetHash("Rule", "7b3149cbd2219ca05bc80a557a701ddee18bd3bbe9afa8e851df64b999155c5e", "2c963f7c06bc1cead7e3b4759e1472383d4469fc3238dc42f8848190887b4775", emptyList()), + "rule2" to TargetHash("Rule", "24f12d22ab247c5af32f954ca46dd4f6323ab2eef28455411b912aaf44a7c322", "bdc1abd0a07103cea34199a9c0d1020619136ff90fb88dcc3a8f873c811c1fe9", emptyList()), + "rule3" to TargetHash("Rule", "c7018bbfed16f4f6f0ef1f258024a50c56ba916b3b9ed4f00972a233d5d11b42", "4aeafed087a9c78a4efa11b6f7831c38d775ddb244a9fabbf21d78c1666a2ea9", listOf("rule1")), + "rule4" to TargetHash("Rule", "020720dfbb969ef9629e51a624a616f015fe07c7b779a5b4f82a8b36c9d3cbe9", "82b46404c8a1ec402a60de72d42a14f6a080e938e5ebaf26203c5ef480558767", listOf("rule1")), ) } @@ -205,10 +205,10 @@ class BuildGraphHasherTest : KoinTest { whenever(bazelClientMock.queryAllTargets()).thenReturn(defaultTargets) val hash = hasher.hashAllBazelTargetsAndSourcefiles() assertThat(hash).containsOnly( - "rule1" to TargetHash("Rule", "7b3149cbd2219ca05bc80a557a701ddee18bd3bbe9afa8e851df64b999155c5e", "2c963f7c06bc1cead7e3b4759e1472383d4469fc3238dc42f8848190887b4775"), - "rule2" to TargetHash("Rule", "24f12d22ab247c5af32f954ca46dd4f6323ab2eef28455411b912aaf44a7c322", "bdc1abd0a07103cea34199a9c0d1020619136ff90fb88dcc3a8f873c811c1fe9"), - "rule3" to TargetHash("Rule", "be17f1e1884037b970e6b7c86bb6533b253a12d967029adc711e50d4662237e8", "91ea3015d4424bb8c1ecf381c30166c386c161d31b70967f3a021c1dc43c7774"), - "rule4" to TargetHash("Rule", "f3e5675e30fe25ff9b61a0c7f64c423964f886799407a9438e692fd803ecd47c", "bce09e1689cc7a8172653981582fea70954f8acd58985c92026582e4b75ec8d2"), + "rule1" to TargetHash("Rule", "7b3149cbd2219ca05bc80a557a701ddee18bd3bbe9afa8e851df64b999155c5e", "2c963f7c06bc1cead7e3b4759e1472383d4469fc3238dc42f8848190887b4775", emptyList()), + "rule2" to TargetHash("Rule", "24f12d22ab247c5af32f954ca46dd4f6323ab2eef28455411b912aaf44a7c322", "bdc1abd0a07103cea34199a9c0d1020619136ff90fb88dcc3a8f873c811c1fe9", emptyList()), + "rule3" to TargetHash("Rule", "be17f1e1884037b970e6b7c86bb6533b253a12d967029adc711e50d4662237e8", "91ea3015d4424bb8c1ecf381c30166c386c161d31b70967f3a021c1dc43c7774", listOf("rule1", "rule4")), + "rule4" to TargetHash("Rule", "f3e5675e30fe25ff9b61a0c7f64c423964f886799407a9438e692fd803ecd47c", "bce09e1689cc7a8172653981582fea70954f8acd58985c92026582e4b75ec8d2", listOf("rule1")), ) } @@ -250,6 +250,30 @@ class BuildGraphHasherTest : KoinTest { assertThat(newHash).isNotEqualTo(oldHash) } + @Test + fun testGeneratedTargetsDeps() = runBlocking { + // GeneratedSrcs do not have RuleInputs in the bazel query proto, + // so ensure that we are properly tracking their dependency edges to + // the targets that generate them. + val generator = createRuleTarget("rule1", emptyList(), "rule1Digest") + val target = createGeneratedTarget("rule0", "rule1") + val ruleInputs = listOf("rule0") + val rule3 = createRuleTarget("rule3", ruleInputs, "digest") + + whenever(bazelClientMock.queryAllTargets()).thenReturn(listOf(rule3, target, generator)) + var hash = hasher.hashAllBazelTargetsAndSourcefiles() + + val depsMapping = hash.mapValues{ + it.value.deps + } + + assertThat(depsMapping).containsOnly( + "rule3" to listOf("rule0"), + "rule0" to listOf("rule1"), + "rule1" to emptyList() + ) + } + private fun createRuleTarget(name: String, inputs: List, digest: String): BazelTarget.Rule { val target = mock() From 117e64ef054d69d545cd73c46d5ad910516fddd1 Mon Sep 17 00:00:00 2001 From: Alex Torok Date: Sun, 29 Sep 2024 14:42:54 +0000 Subject: [PATCH 05/14] Update get-impacted-targets Add e2e test for dump distances TODO: It would be nice if there was a better e2e test here -- Ask Maxwell how he generates the integration workspace data. --- .../bazel_diff/cli/GenerateHashesCommand.kt | 6 ++--- .../cli/GetImpactedTargetsCommand.kt | 24 +++++++++++++++---- .../interactor/DeserialiseHashesInteractor.kt | 5 ++++ .../interactor/GenerateHashesInteractor.kt | 6 ++--- .../test/kotlin/com/bazel_diff/e2e/E2ETest.kt | 13 +++++++--- .../impacted_targets_distances-1-2.txt | 8 +++++++ 6 files changed, 48 insertions(+), 14 deletions(-) create mode 100644 cli/src/test/resources/fixture/impacted_targets_distances-1-2.txt diff --git a/cli/src/main/kotlin/com/bazel_diff/cli/GenerateHashesCommand.kt b/cli/src/main/kotlin/com/bazel_diff/cli/GenerateHashesCommand.kt index 09ad385..04a4bec 100644 --- a/cli/src/main/kotlin/com/bazel_diff/cli/GenerateHashesCommand.kt +++ b/cli/src/main/kotlin/com/bazel_diff/cli/GenerateHashesCommand.kt @@ -142,7 +142,7 @@ class GenerateHashesCommand : Callable { scope = CommandLine.ScopeType.INHERIT, defaultValue = CommandLine.Parameters.NULL_VALUE ) - var depsFile: File? = null + var depsMappingJSONPath: File? = null @CommandLine.Option( names = ["-m", "--modified-filepaths"], @@ -167,7 +167,7 @@ class GenerateHashesCommand : Callable { cqueryCommandOptions, useCquery, keepGoing, - depsFile != null, + depsMappingJSONPath != null, fineGrainedHashExternalRepos, ), loggingModule(parent.verbose), @@ -175,7 +175,7 @@ class GenerateHashesCommand : Callable { ) } - return when (GenerateHashesInteractor().execute(seedFilepaths, outputPath, depsFile, ignoredRuleHashingAttributes, targetType, includeTargetType, modifiedFilepaths)) { + return when (GenerateHashesInteractor().execute(seedFilepaths, outputPath, depsMappingJSONPath, ignoredRuleHashingAttributes, targetType, includeTargetType, modifiedFilepaths)) { true -> CommandLine.ExitCode.OK false -> CommandLine.ExitCode.SOFTWARE }.also { stopKoin() } 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 7b1e90f..8f54fda 100644 --- a/cli/src/main/kotlin/com/bazel_diff/cli/GetImpactedTargetsCommand.kt +++ b/cli/src/main/kotlin/com/bazel_diff/cli/GetImpactedTargetsCommand.kt @@ -38,6 +38,14 @@ class GetImpactedTargetsCommand : Callable { ) lateinit var finalHashesJSONPath: File + @CommandLine.Option( + names = ["-d", "--depsFile"], + description = ["Path to the file where dependency edges are. If specified, build graph distance metrics will be computed from the given hash data."], + scope = CommandLine.ScopeType.INHERIT, + defaultValue = CommandLine.Parameters.NULL_VALUE + ) + var depsMappingJSONPath: File? = null + @CommandLine.Option( names = ["-tt", "--targetType"], split = ",", @@ -69,16 +77,22 @@ class GetImpactedTargetsCommand : Callable { val from = deserialiser.executeTargetHash(startingHashesJSONPath, targetType) val to = deserialiser.executeTargetHash(finalHashesJSONPath, targetType) - val impactedTargets = CalculateImpactedTargetsInteractor().execute(from, to) + val impactedTargetsStream = if (depsMappingJSONPath != null) { + val depsMapping = deserialiser.deserializeDeps(depsMappingJSONPath!!) + CalculateImpactedTargetsInteractor().executeWithDistances(from, to, depsMapping).map { (label, metrics) -> + "${label}~${metrics.targetDistance}~${metrics.packageDistance}" + }.stream() + } else { + CalculateImpactedTargetsInteractor().execute(from, to).stream() + } return try { - BufferedWriter(when (val path=outputPath) { + BufferedWriter(when (val path = outputPath) { null -> FileWriter(FileDescriptor.out) else -> FileWriter(path) }).use { writer -> - impactedTargets.forEach { - writer.write(it) - //Should not depend on OS + impactedTargetsStream.forEach { line -> + writer.write(line) writer.write("\n") } } diff --git a/cli/src/main/kotlin/com/bazel_diff/interactor/DeserialiseHashesInteractor.kt b/cli/src/main/kotlin/com/bazel_diff/interactor/DeserialiseHashesInteractor.kt index 1068cdb..994cf41 100644 --- a/cli/src/main/kotlin/com/bazel_diff/interactor/DeserialiseHashesInteractor.kt +++ b/cli/src/main/kotlin/com/bazel_diff/interactor/DeserialiseHashesInteractor.kt @@ -46,4 +46,9 @@ class DeserialiseHashesInteractor : KoinComponent { val shape = object : TypeToken>() {}.type return gson.fromJson(FileReader(file), shape) } + + fun deserializeDeps(file: File): Map> { + val shape = object : TypeToken>>() {}.type + return gson.fromJson(FileReader(file), shape) + } } diff --git a/cli/src/main/kotlin/com/bazel_diff/interactor/GenerateHashesInteractor.kt b/cli/src/main/kotlin/com/bazel_diff/interactor/GenerateHashesInteractor.kt index e98ba49..9adb082 100644 --- a/cli/src/main/kotlin/com/bazel_diff/interactor/GenerateHashesInteractor.kt +++ b/cli/src/main/kotlin/com/bazel_diff/interactor/GenerateHashesInteractor.kt @@ -18,7 +18,7 @@ class GenerateHashesInteractor : KoinComponent { private val logger: Logger by inject() private val gson: Gson by inject() - fun execute(seedFilepaths: File?, outputPath: File?, depsFile: File?, ignoredRuleHashingAttributes: Set, targetTypes:Set?, includeTargetType: Boolean = false, modifiedFilepaths: File?): Boolean { + fun execute(seedFilepaths: File?, outputPath: File?, depsMappingJSONPath: File?, ignoredRuleHashingAttributes: Set, targetTypes:Set?, includeTargetType: Boolean = false, modifiedFilepaths: File?): Boolean { return try { val epoch = Calendar.getInstance().getTimeInMillis() val seedFilepathsSet: Set = when { @@ -58,8 +58,8 @@ class GenerateHashesInteractor : KoinComponent { }.use { fileWriter -> fileWriter.write(gson.toJson(hashes.mapValues { it.value.toJson(includeTargetType) })) } - if (depsFile != null) { - FileWriter(depsFile).use { fileWriter -> + if (depsMappingJSONPath != null) { + FileWriter(depsMappingJSONPath).use { fileWriter -> fileWriter.write(gson.toJson(hashes.mapValues { it.value.deps })) } } 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 7dbfcad..47c4fc9 100644 --- a/cli/src/test/kotlin/com/bazel_diff/e2e/E2ETest.kt +++ b/cli/src/test/kotlin/com/bazel_diff/e2e/E2ETest.kt @@ -19,7 +19,7 @@ class E2ETest { private fun CommandLine.execute(args: List) = execute(*args.toTypedArray()) - private fun testE2E(extraGenerateHashesArgs: List, extraGetImpactedTargetsArgs: List, expectedResultFile: String) { + private fun testE2E(extraGenerateHashesArgs: List, extraGetImpactedTargetsArgs: List, expectedResultFile: String, computeDistances: Boolean = false) { val projectA = extractFixtureProject("/fixture/integration-test-1.zip") val projectB = extractFixtureProject("/fixture/integration-test-2.zip") @@ -29,6 +29,7 @@ class E2ETest { val outputDir = temp.newFolder() val from = File(outputDir, "starting_hashes.json") val to = File(outputDir, "final_hashes.json") + val depsFile = File(outputDir, "deps.json") val impactedTargetsOutput = File(outputDir, "impacted_targets.txt") val cli = CommandLine(BazelDiff()) @@ -38,11 +39,11 @@ class E2ETest { ) //To cli.execute( - listOf("generate-hashes", "-w", workingDirectoryB.absolutePath, "-b", bazelPath, to.absolutePath) + extraGenerateHashesArgs + listOf("generate-hashes", "-w", workingDirectoryB.absolutePath, "-b", bazelPath, to.absolutePath) + extraGenerateHashesArgs + if (computeDistances) listOf("-d", depsFile.absolutePath) else emptyList() ) //Impacted targets cli.execute( - listOf("get-impacted-targets", "-sh", from.absolutePath, "-fh", to.absolutePath, "-o", impactedTargetsOutput.absolutePath) + extraGetImpactedTargetsArgs + listOf("get-impacted-targets", "-sh", from.absolutePath, "-fh", to.absolutePath, "-o", impactedTargetsOutput.absolutePath) + extraGetImpactedTargetsArgs + if (computeDistances) listOf("-d", depsFile.absolutePath) else emptyList() ) val actual: Set = impactedTargetsOutput.readLines().filter { it.isNotBlank() }.toSet() @@ -57,6 +58,12 @@ class E2ETest { testE2E(emptyList(), emptyList(), "/fixture/impacted_targets-1-2.txt") } + @Test + fun testE2EDistances() { + testE2E(emptyList(), emptyList(), "/fixture/impacted_targets_distances-1-2.txt", computeDistances = true) + } + + @Test fun testE2EIncludingTargetType() { testE2E(listOf("-tt", "Rule,SourceFile"), emptyList(), "/fixture/impacted_targets-1-2-rule-sourcefile.txt") diff --git a/cli/src/test/resources/fixture/impacted_targets_distances-1-2.txt b/cli/src/test/resources/fixture/impacted_targets_distances-1-2.txt new file mode 100644 index 0000000..c3ae3fb --- /dev/null +++ b/cli/src/test/resources/fixture/impacted_targets_distances-1-2.txt @@ -0,0 +1,8 @@ +//test/java/com/integration:libbazel-diff-integration-test-lib-src.jar~0~0 +//test/java/com/integration:TestStringGenerator.java~0~0 +//test/java/com/integration:bazel-diff-integration-test-lib~0~0 +//test/java/com/integration:bazel-diff-integration-tests-src.jar~2~0 +//test/java/com/integration:libbazel-diff-integration-test-lib.jar~0~0 +//test/java/com/integration:bazel-diff-integration-tests~1~0 +//test/java/com/integration:bazel-diff-integration-tests_deploy-src.jar~2~0 +//test/java/com/integration:bazel-diff-integration-tests.jar~2~0 From 56a955ef278c458a9ed5810671352c16d3592cda Mon Sep 17 00:00:00 2001 From: Alex Torok Date: Sun, 29 Sep 2024 15:19:45 +0000 Subject: [PATCH 06/14] Update documentation --- README.md | 45 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/README.md b/README.md index c7bb1ca..753d633 100644 --- a/README.md +++ b/README.md @@ -61,6 +61,44 @@ Open `bazel-diff-example.sh` to see how this is implemented. This is purely an e * We run `bazel-diff` on the starting and final JSON hash filepaths to get our impacted set of targets. This impacted set of targets is written to a file. +## Build Graph Distance Metrics + +`bazel-diff` can optionally compute build graph distance metrics between two revisions. This is +useful for understanding the impact of a change on the build graph. Directly impacted targets are +targets that have had their rule attributes or source file dependencies changed. Indirectly impacted +targets are that are impacted only due to a change in one of their target dependencies. + +For each target, the following metrics are computed: + +* `target_distance`: The number of dependency hops that it takes to get from an impacted target to a directly impacted target. +* `package_distance`: The number of dependency hops that cross a package boundary to get from an impacted target to a directly impacted target. + +Build graph distance metrics can be used by downstream tools to power features such as: + +* Only running sanitizers on impacted tests that are in the same package as a directly impacted target. +* Only running large-sized tests that are within a few package hops of a directly impacted target. +* Only running computationally expensive jobs when an impacted target is within a certain distance of a directly impacted target. + +To enable this feature, you must generate a dependency mapping on your final revision when computing hashes, then pass it into the `get-impacted-targets` command. + +```bash +git checkout BASE_REV +bazel-diff generate-hashes [...] + +git checkout FINAL_REV +bazel-diff generate-hashes --depsFile deps.json [...] + +bazel-diff get-impacted-targets --depsFile deps.json [...] +``` + +This will produce an impacted targets list with target label, target distance, and package distance delimited by `~` characters: + +```text +//foo:bar~0~0 +//foo:baz~1~0 +//bar:qux~1~1 +``` + ## CLI Interface `bazel-diff` Command @@ -352,6 +390,13 @@ Now you can simply run `bazel-diff` from your project: bazel run @bazel_diff//:bazel-diff -- bazel-diff -h ``` +## Learn More + +Take a look at the following bazelcon talks to learn more about `bazel-diff`: + +* [BazelCon 2023: Improving CI efficiency with Bazel querying and bazel-diff](https://www.youtube.com/watch?v=QYAbmE_1fSo) +* BazelCon 2024: Not Going the Distance: Filtering Tests by Build Graph Distance: Coming Soon + ## Running the tests To run the tests simply run From 27b0b6874007f7ed62e274fdfca7d829fc7c5298 Mon Sep 17 00:00:00 2001 From: Alex Torok Date: Sun, 29 Sep 2024 16:59:04 +0000 Subject: [PATCH 07/14] Move target type filtering to happen after impacted targets are computed --- .../cli/GetImpactedTargetsCommand.kt | 15 ++++++++---- .../interactor/DeserialiseHashesInteractor.kt | 16 ++----------- .../bazel_diff/interactor/TargetTypeFilter.kt | 16 +++++++++++++ .../test/kotlin/com/bazel_diff/e2e/E2ETest.kt | 6 +++++ .../DeserialiseHashesInteractorTest.kt | 24 +------------------ .../impacted_targets-1-2-rule-sourcefile.txt | 2 +- ..._targets_distances-1-2-rule-sourcefile.txt | 3 +++ 7 files changed, 40 insertions(+), 42 deletions(-) create mode 100644 cli/src/main/kotlin/com/bazel_diff/interactor/TargetTypeFilter.kt create mode 100644 cli/src/test/resources/fixture/impacted_targets_distances-1-2-rule-sourcefile.txt 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 8f54fda..12ddace 100644 --- a/cli/src/main/kotlin/com/bazel_diff/cli/GetImpactedTargetsCommand.kt +++ b/cli/src/main/kotlin/com/bazel_diff/cli/GetImpactedTargetsCommand.kt @@ -4,6 +4,7 @@ import com.bazel_diff.di.loggingModule import com.bazel_diff.di.serialisationModule import com.bazel_diff.interactor.CalculateImpactedTargetsInteractor import com.bazel_diff.interactor.DeserialiseHashesInteractor +import com.bazel_diff.interactor.TargetTypeFilter import org.koin.core.context.startKoin import org.koin.core.context.stopKoin import picocli.CommandLine @@ -74,16 +75,22 @@ class GetImpactedTargetsCommand : Callable { validate() val deserialiser = DeserialiseHashesInteractor() - val from = deserialiser.executeTargetHash(startingHashesJSONPath, targetType) - val to = deserialiser.executeTargetHash(finalHashesJSONPath, targetType) + val from = deserialiser.executeTargetHash(startingHashesJSONPath) + val to = deserialiser.executeTargetHash(finalHashesJSONPath) + + val typeFilter = TargetTypeFilter(targetType, to) val impactedTargetsStream = if (depsMappingJSONPath != null) { val depsMapping = deserialiser.deserializeDeps(depsMappingJSONPath!!) - CalculateImpactedTargetsInteractor().executeWithDistances(from, to, depsMapping).map { (label, metrics) -> + CalculateImpactedTargetsInteractor().executeWithDistances(from, to, depsMapping) + .filterKeys { typeFilter.accepts(it) } + .map { (label, metrics) -> "${label}~${metrics.targetDistance}~${metrics.packageDistance}" }.stream() } else { - CalculateImpactedTargetsInteractor().execute(from, to).stream() + CalculateImpactedTargetsInteractor().execute(from, to) + .filter { typeFilter.accepts(it) } + .stream() } return try { diff --git a/cli/src/main/kotlin/com/bazel_diff/interactor/DeserialiseHashesInteractor.kt b/cli/src/main/kotlin/com/bazel_diff/interactor/DeserialiseHashesInteractor.kt index 994cf41..ad2bcd1 100644 --- a/cli/src/main/kotlin/com/bazel_diff/interactor/DeserialiseHashesInteractor.kt +++ b/cli/src/main/kotlin/com/bazel_diff/interactor/DeserialiseHashesInteractor.kt @@ -15,22 +15,10 @@ class DeserialiseHashesInteractor : KoinComponent { * @param file path to file that has been pre-validated * @param targetTypes the target types to filter. If null, all targets will be returned */ - fun executeTargetHash(file: File, targetTypes: Set? = null): Map { + fun executeTargetHash(file: File): Map { val shape = object : TypeToken>() {}.type val result: Map = gson.fromJson(FileReader(file), shape) - if (targetTypes == null) { - return result.mapValues { TargetHash.fromJson(it.value) } - } else { - return result.mapValues { - TargetHash.fromJson(it.value) - }.filter { (_, targetHash) -> - if (targetHash.hasType()) { - targetTypes.contains(targetHash.type) - } else { - throw IllegalStateException("No type info found in ${file}, please re-generate the JSON with --includeTypeTarget!") - } - } - } + return result.mapValues { TargetHash.fromJson(it.value) } } /** diff --git a/cli/src/main/kotlin/com/bazel_diff/interactor/TargetTypeFilter.kt b/cli/src/main/kotlin/com/bazel_diff/interactor/TargetTypeFilter.kt new file mode 100644 index 0000000..5f597a9 --- /dev/null +++ b/cli/src/main/kotlin/com/bazel_diff/interactor/TargetTypeFilter.kt @@ -0,0 +1,16 @@ +package com.bazel_diff.interactor +import com.bazel_diff.hash.TargetHash + +class TargetTypeFilter(private val targetTypes: Set?, private val targets: Map) { + + fun accepts(label: String): Boolean { + if (targetTypes == null) { + return true + } + val targetHash = targets[label]!! + if (!targetHash.hasType()) { + throw IllegalStateException("No target type info found, please re-generate the target hashes JSON with --includeTypeTarget!") + } + return targetTypes.contains(targetHash.type) + } +} 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 47c4fc9..c043c5d 100644 --- a/cli/src/test/kotlin/com/bazel_diff/e2e/E2ETest.kt +++ b/cli/src/test/kotlin/com/bazel_diff/e2e/E2ETest.kt @@ -74,6 +74,12 @@ class E2ETest { testE2E(listOf("--includeTargetType"), listOf("-tt", "Rule,SourceFile"), "/fixture/impacted_targets-1-2-rule-sourcefile.txt") } + @Test + fun testE2EWithTargetTypeAndDistance() { + testE2E(listOf("--includeTargetType"), listOf("-tt", "Rule,SourceFile"), "/fixture/impacted_targets_distances-1-2-rule-sourcefile.txt", computeDistances = true) + } + + @Test fun testFineGrainedHashExternalRepo() { // The difference between these two snapshots is simply upgrading the Guava version. diff --git a/cli/src/test/kotlin/com/bazel_diff/interactor/DeserialiseHashesInteractorTest.kt b/cli/src/test/kotlin/com/bazel_diff/interactor/DeserialiseHashesInteractorTest.kt index 3812d56..0e3485e 100644 --- a/cli/src/test/kotlin/com/bazel_diff/interactor/DeserialiseHashesInteractorTest.kt +++ b/cli/src/test/kotlin/com/bazel_diff/interactor/DeserialiseHashesInteractorTest.kt @@ -40,19 +40,6 @@ class DeserialiseHashesInteractorTest : KoinTest { )) } - @Test - fun testDeserialisatingFileWithoutType() { - val file = temp.newFile().apply { - writeText("""{"target-name":"hash#direct"}""") - } - - assertThat { interactor.executeTargetHash(file, setOf("Whatever"))} - .isFailure().apply { - messageContains("please re-generate the JSON with --includeTypeTarget!") - hasClass(IllegalStateException::class) - } - } - @Test fun testDeserialisationWithType() { val file = temp.newFile().apply { @@ -63,19 +50,10 @@ class DeserialiseHashesInteractorTest : KoinTest { |}""".trimMargin()) } - assertThat(interactor.executeTargetHash(file, null)).isEqualTo(mapOf( + assertThat(interactor.executeTargetHash(file)).isEqualTo(mapOf( "target-1" to TargetHash("GeneratedFile", "hash1", "direct1"), "target-2" to TargetHash("Rule", "hash2", "direct2"), "target-3" to TargetHash("SourceFile", "hash3", "direct3") )) - assertThat(interactor.executeTargetHash(file, setOf("GeneratedFile"))).isEqualTo(mapOf( - "target-1" to TargetHash("GeneratedFile", "hash1", "direct1") - )) - assertThat(interactor.executeTargetHash(file, setOf("Rule"))).isEqualTo(mapOf( - "target-2" to TargetHash("Rule", "hash2", "direct2") - )) - assertThat(interactor.executeTargetHash(file, setOf("SourceFile"))).isEqualTo(mapOf( - "target-3" to TargetHash("SourceFile", "hash3", "direct3") - )) } } diff --git a/cli/src/test/resources/fixture/impacted_targets-1-2-rule-sourcefile.txt b/cli/src/test/resources/fixture/impacted_targets-1-2-rule-sourcefile.txt index dc0cdc5..471c47d 100644 --- a/cli/src/test/resources/fixture/impacted_targets-1-2-rule-sourcefile.txt +++ b/cli/src/test/resources/fixture/impacted_targets-1-2-rule-sourcefile.txt @@ -1,3 +1,3 @@ //test/java/com/integration:bazel-diff-integration-test-lib //test/java/com/integration:bazel-diff-integration-tests -//test/java/com/integration:TestStringGenerator.java \ No newline at end of file +//test/java/com/integration:TestStringGenerator.java diff --git a/cli/src/test/resources/fixture/impacted_targets_distances-1-2-rule-sourcefile.txt b/cli/src/test/resources/fixture/impacted_targets_distances-1-2-rule-sourcefile.txt new file mode 100644 index 0000000..c8edfba --- /dev/null +++ b/cli/src/test/resources/fixture/impacted_targets_distances-1-2-rule-sourcefile.txt @@ -0,0 +1,3 @@ +//test/java/com/integration:bazel-diff-integration-test-lib~0~0 +//test/java/com/integration:bazel-diff-integration-tests~1~0 +//test/java/com/integration:TestStringGenerator.java~0~0 From 02efb3ebb731399558f175b7c034ad75a9b71295 Mon Sep 17 00:00:00 2001 From: Alex Torok Date: Mon, 30 Sep 2024 13:38:25 +0000 Subject: [PATCH 08/14] Address review feedback 1. Output detailed hash data as json 2. Use a different serialization format for targethashes to avoid ambiguity --- README.md | 10 +++-- .../bazel_diff/cli/GenerateHashesCommand.kt | 4 +- .../cli/GetImpactedTargetsCommand.kt | 41 ++++++++---------- .../com/bazel_diff/hash/TargetDigest.kt | 6 +-- .../kotlin/com/bazel_diff/hash/TargetHash.kt | 16 ++++--- .../CalculateImpactedTargetsInteractor.kt | 42 ++++++++++++++++++- .../test/kotlin/com/bazel_diff/e2e/E2ETest.kt | 22 ++++++++-- .../CalculateImpactedTargetsInteractorTest.kt | 18 ++++---- .../DeserialiseHashesInteractorTest.kt | 8 ++-- ..._targets_distances-1-2-rule-sourcefile.txt | 8 ++-- .../impacted_targets_distances-1-2.txt | 18 ++++---- 11 files changed, 125 insertions(+), 68 deletions(-) diff --git a/README.md b/README.md index 753d633..f4d3d23 100644 --- a/README.md +++ b/README.md @@ -91,12 +91,14 @@ bazel-diff generate-hashes --depsFile deps.json [...] bazel-diff get-impacted-targets --depsFile deps.json [...] ``` -This will produce an impacted targets list with target label, target distance, and package distance delimited by `~` characters: +This will produce an impacted targets json list with target label, target distance, and package distance: ```text -//foo:bar~0~0 -//foo:baz~1~0 -//bar:qux~1~1 +[ + {"label": "//foo:bar", "targetDistance": 0, "packageDistance": 0}, + {"label": "//foo:baz", "targetDistance": 1, "packageDistance": 0}, + {"label": "//bar:qux", "targetDistance": 1, "packageDistance": 1} +] ``` ## CLI Interface diff --git a/cli/src/main/kotlin/com/bazel_diff/cli/GenerateHashesCommand.kt b/cli/src/main/kotlin/com/bazel_diff/cli/GenerateHashesCommand.kt index 04a4bec..f60660a 100644 --- a/cli/src/main/kotlin/com/bazel_diff/cli/GenerateHashesCommand.kt +++ b/cli/src/main/kotlin/com/bazel_diff/cli/GenerateHashesCommand.kt @@ -137,8 +137,8 @@ class GenerateHashesCommand : Callable { var ignoredRuleHashingAttributes: Set = emptySet() @CommandLine.Option( - names = ["-d", "--depsFile"], - description = ["Path to the file where dependency edges are written to. If not specified, the dependency edges will not be written to a file."], + names = ["-d", "--depEdgesFile"], + description = ["Path to the file where dependency edges are written to. If not specified, the dependency edges will not be written to a file. Needed for computing build graph distance metrics. See bazel-diff docs for more details about build graph distance metrics."], scope = CommandLine.ScopeType.INHERIT, defaultValue = CommandLine.Parameters.NULL_VALUE ) 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 12ddace..5841f0e 100644 --- a/cli/src/main/kotlin/com/bazel_diff/cli/GetImpactedTargetsCommand.kt +++ b/cli/src/main/kotlin/com/bazel_diff/cli/GetImpactedTargetsCommand.kt @@ -40,7 +40,7 @@ class GetImpactedTargetsCommand : Callable { lateinit var finalHashesJSONPath: File @CommandLine.Option( - names = ["-d", "--depsFile"], + names = ["-d", "--depEdgesFile"], description = ["Path to the file where dependency edges are. If specified, build graph distance metrics will be computed from the given hash data."], scope = CommandLine.ScopeType.INHERIT, defaultValue = CommandLine.Parameters.NULL_VALUE @@ -58,7 +58,7 @@ class GetImpactedTargetsCommand : Callable { @CommandLine.Option( names = ["-o", "--output"], scope = CommandLine.ScopeType.LOCAL, - description = ["Filepath to write the impacted Bazel targets to, newline separated. If not specified, the targets will be written to STDOUT."], + description = ["Filepath to write the impacted Bazel targets to. If using depEdgesFile: formatted in json, otherwise: newline separated. If not specified, the output will be written to STDOUT."], ) var outputPath: File? = null @@ -78,30 +78,17 @@ class GetImpactedTargetsCommand : Callable { val from = deserialiser.executeTargetHash(startingHashesJSONPath) val to = deserialiser.executeTargetHash(finalHashesJSONPath) - val typeFilter = TargetTypeFilter(targetType, to) - - val impactedTargetsStream = if (depsMappingJSONPath != null) { - val depsMapping = deserialiser.deserializeDeps(depsMappingJSONPath!!) - CalculateImpactedTargetsInteractor().executeWithDistances(from, to, depsMapping) - .filterKeys { typeFilter.accepts(it) } - .map { (label, metrics) -> - "${label}~${metrics.targetDistance}~${metrics.packageDistance}" - }.stream() - } else { - CalculateImpactedTargetsInteractor().execute(from, to) - .filter { typeFilter.accepts(it) } - .stream() - } - - return try { - BufferedWriter(when (val path = outputPath) { + val outputWriter = BufferedWriter(when (val path = outputPath) { null -> FileWriter(FileDescriptor.out) else -> FileWriter(path) - }).use { writer -> - impactedTargetsStream.forEach { line -> - writer.write(line) - writer.write("\n") - } + }) + + return try { + if (depsMappingJSONPath != null) { + val depsMapping = deserialiser.deserializeDeps(depsMappingJSONPath!!) + CalculateImpactedTargetsInteractor().executeWithDistances(from, to, depsMapping, outputWriter, targetType) + } else { + CalculateImpactedTargetsInteractor().execute(from, to, outputWriter, targetType) } CommandLine.ExitCode.OK } catch (e: IOException) { @@ -122,5 +109,11 @@ class GetImpactedTargetsCommand : Callable { "Incorrect final hashes: file doesn't exist or can't be read." ) } + if (depsFile != null && !depsFile.canRead()) { + throw CommandLine.ParameterException( + spec.commandLine(), + "Incorrect dep edges file: file doesn't exist or can't be read." + ) + } } } diff --git a/cli/src/main/kotlin/com/bazel_diff/hash/TargetDigest.kt b/cli/src/main/kotlin/com/bazel_diff/hash/TargetDigest.kt index 24c86ee..36eb2d2 100644 --- a/cli/src/main/kotlin/com/bazel_diff/hash/TargetDigest.kt +++ b/cli/src/main/kotlin/com/bazel_diff/hash/TargetDigest.kt @@ -8,11 +8,11 @@ data class TargetDigest( val deps: List? = null, ) { fun clone(newDeps: List? = null): TargetDigest { - var newDeps = newDeps + var toUse = newDeps if (newDeps == null) { - newDeps = deps + toUse = deps } - return TargetDigest(overallDigest.clone(), directDigest.clone(), newDeps) + return TargetDigest(overallDigest.clone(), directDigest.clone(), toUse) } } diff --git a/cli/src/main/kotlin/com/bazel_diff/hash/TargetHash.kt b/cli/src/main/kotlin/com/bazel_diff/hash/TargetHash.kt index 1da4b29..9aa4ce4 100644 --- a/cli/src/main/kotlin/com/bazel_diff/hash/TargetHash.kt +++ b/cli/src/main/kotlin/com/bazel_diff/hash/TargetHash.kt @@ -7,11 +7,11 @@ data class TargetHash( val deps: List? = null ) { val hashWithType by lazy { - "${type}#${hash}#${directHash}" + "${type}#${hash}~${directHash}" } val totalHash by lazy { - "${hash}#${directHash}" + "${hash}~${directHash}" } fun toJson(includeTargetType: Boolean): String { @@ -30,9 +30,15 @@ data class TargetHash( fun fromJson(json: String): TargetHash { val parts = json.split("#") return when (parts.size) { - 2 -> TargetHash("", parts[0], parts[1]) - 3 -> TargetHash(parts[0], parts[1], parts[2]) - else -> throw IllegalArgumentException("Invalid JSON format") + 1 -> Pair("", parts[0]) + 2 -> Pair(parts[0], parts[1]) + else -> throw IllegalArgumentException("Invalid targetHash format: $json") + }.let { (type, hash) -> + val hashes = hash.split("~") + when (hashes.size) { + 2 -> TargetHash(type, hashes[0], hashes[1]) + else -> throw IllegalArgumentException("Invalid targetHash format: $json") + } } } } 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 1206769..0b6d4ae 100644 --- a/cli/src/main/kotlin/com/bazel_diff/interactor/CalculateImpactedTargetsInteractor.kt +++ b/cli/src/main/kotlin/com/bazel_diff/interactor/CalculateImpactedTargetsInteractor.kt @@ -2,8 +2,11 @@ package com.bazel_diff.interactor import com.bazel_diff.hash.TargetHash import com.google.common.collect.Maps +import com.google.gson.Gson import org.koin.core.component.KoinComponent +import org.koin.core.component.inject import java.io.File +import java.io.Writer import java.util.concurrent.ConcurrentMap import java.util.concurrent.ConcurrentHashMap import java.util.stream.Stream @@ -16,6 +19,7 @@ data class TargetDistanceMetrics( ) {} class CalculateImpactedTargetsInteractor : KoinComponent { + private val gson: Gson by inject() @VisibleForTesting class InvalidDependencyEdgesException(message: String) : Exception(message) @@ -25,10 +29,24 @@ class CalculateImpactedTargetsInteractor : KoinComponent { INDIRECT } - fun execute(from: Map, to: Map): Set { + fun execute(from: Map, to: Map, outputWriter: Writer, targetTypes: Set?) { /** * This call might be faster if end hashes is a sorted map */ + val typeFilter = TargetTypeFilter(targetTypes, to) + + computeSimpleImpactedTargets(from, to) + .filter { typeFilter.accepts(it) } + .let { impactedTargets -> + outputWriter.use { writer -> + impactedTargets.forEach { + writer.write("$it\n") + } + } + } + } + + fun computeSimpleImpactedTargets(from: Map, to: Map): Set { val difference = Maps.difference(to, from) val onlyInEnd: Set = difference.entriesOnlyOnLeft().keys val changed: Set = difference.entriesDiffering().keys @@ -39,7 +57,27 @@ class CalculateImpactedTargetsInteractor : KoinComponent { return impactedTargets } - fun executeWithDistances(from: Map, to: Map, depEdges: Map>): Map { + fun executeWithDistances(from: Map, to: Map, depEdges: Map>, outputWriter: Writer, targetTypes: Set?) { + val typeFilter = TargetTypeFilter(targetTypes, to) + + computeAllDistances(from, to, depEdges) + .filterKeys { typeFilter.accepts(it) } + .let { impactedTargets -> + outputWriter.use { writer -> + writer.write(gson.toJson( + impactedTargets.map{ + mapOf( + "label" to it.key, + "targetDistance" to it.value.targetDistance, + "packageDistance" to it.value.packageDistance + ) + } + )) + } + } + } + + fun computeAllDistances(from: Map, to: Map, depEdges: Map>): Map { val difference = Maps.difference(to, from) val newLabels = difference.entriesOnlyOnLeft().keys 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 c043c5d..7b9a150 100644 --- a/cli/src/test/kotlin/com/bazel_diff/e2e/E2ETest.kt +++ b/cli/src/test/kotlin/com/bazel_diff/e2e/E2ETest.kt @@ -3,6 +3,8 @@ package com.bazel_diff.e2e import assertk.assertThat import assertk.assertions.isEqualTo import com.bazel_diff.cli.BazelDiff +import com.google.gson.Gson +import com.google.gson.reflect.TypeToken import org.junit.Ignore import org.junit.Rule import org.junit.Test @@ -46,11 +48,23 @@ class E2ETest { listOf("get-impacted-targets", "-sh", from.absolutePath, "-fh", to.absolutePath, "-o", impactedTargetsOutput.absolutePath) + extraGetImpactedTargetsArgs + if (computeDistances) listOf("-d", depsFile.absolutePath) else emptyList() ) - val actual: Set = impactedTargetsOutput.readLines().filter { it.isNotBlank() }.toSet() - val expected: Set = - javaClass.getResourceAsStream(expectedResultFile).use { it.bufferedReader().readLines().filter { it.isNotBlank() }.toSet() } + if (!computeDistances) { + val actual: Set = impactedTargetsOutput.readLines().filter { it.isNotBlank() }.toSet() + val expected: Set = + javaClass.getResourceAsStream(expectedResultFile).use { it.bufferedReader().readLines().filter { it.isNotBlank() }.toSet() } + + assertThat(actual).isEqualTo(expected) + } else { + // When computing target distances, the output format is json. Read the files and assert the sorted contents. + val gson = Gson() + val shape = object : TypeToken>>() {}.type + val actual = gson.fromJson>>(impactedTargetsOutput.readText(), shape).sortedBy { it["label"] as String } + val expected = javaClass.getResourceAsStream(expectedResultFile).use { + gson.fromJson>>(it.bufferedReader().readText(), shape).sortedBy { it["label"] as String } + } - assertThat(actual).isEqualTo(expected) + assertThat(actual).isEqualTo(expected) + } } @Test 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 2a952fb..105db9f 100644 --- a/cli/src/test/kotlin/com/bazel_diff/interactor/CalculateImpactedTargetsInteractorTest.kt +++ b/cli/src/test/kotlin/com/bazel_diff/interactor/CalculateImpactedTargetsInteractorTest.kt @@ -34,7 +34,7 @@ class CalculateImpactedTargetsInteractorTest : KoinTest { end["3"] = "d" val endHashes = end.mapValues { TargetHash("", it.value, it.value) } - val impacted = interactor.execute(startHashes, endHashes) + val impacted = interactor.computeSimpleImpactedTargets(startHashes, endHashes) assertThat(impacted).containsExactlyInAnyOrder( "1", "3" ) @@ -53,7 +53,7 @@ class CalculateImpactedTargetsInteractorTest : KoinTest { makeIndirectlyChanged(endHashes, "//:3") val interactor = CalculateImpactedTargetsInteractor() - val impacted = interactor.executeWithDistances(startHashes, endHashes, depEdges) + val impacted = interactor.computeAllDistances(startHashes, endHashes, depEdges) assertThat(impacted).containsOnly( "//:1" to TargetDistanceMetrics(0, 0), @@ -72,7 +72,7 @@ class CalculateImpactedTargetsInteractorTest : KoinTest { ) val interactor = CalculateImpactedTargetsInteractor() - val impacted = interactor.executeWithDistances(startHashes, endHashes, mapOf()) + val impacted = interactor.computeAllDistances(startHashes, endHashes, mapOf()) assertThat(impacted).containsOnly( "//:1" to TargetDistanceMetrics(0, 0), @@ -91,7 +91,7 @@ class CalculateImpactedTargetsInteractorTest : KoinTest { makeIndirectlyChanged(endHashes, "//:2", "//:3") val interactor = CalculateImpactedTargetsInteractor() - val impacted = interactor.executeWithDistances(startHashes, endHashes, depEdges) + val impacted = interactor.computeAllDistances(startHashes, endHashes, depEdges) assertThat(impacted).containsOnly( "//:1" to TargetDistanceMetrics(0,0), @@ -111,7 +111,7 @@ class CalculateImpactedTargetsInteractorTest : KoinTest { makeIndirectlyChanged(endHashes, "//A:2", "//B:3", "//B:4", "//C:5") val interactor = CalculateImpactedTargetsInteractor() - val impacted = interactor.executeWithDistances(startHashes, endHashes, depEdges) + val impacted = interactor.computeAllDistances(startHashes, endHashes, depEdges) assertThat(impacted).containsOnly( "//A:1" to TargetDistanceMetrics(0,0), @@ -139,7 +139,7 @@ class CalculateImpactedTargetsInteractorTest : KoinTest { makeIndirectlyChanged(endHashes, "//A:target_1", "//A:target_2", "//A:target_3", "//A:target_4", "//B:target", "//C:target", "//D:target", "//final:final") val interactor = CalculateImpactedTargetsInteractor() - val impacted = interactor.executeWithDistances(startHashes, endHashes, depEdges) + val impacted = interactor.computeAllDistances(startHashes, endHashes, depEdges) assertThat(impacted["//final:final"]).isEqualTo(TargetDistanceMetrics(4,2)) } @@ -161,7 +161,7 @@ class CalculateImpactedTargetsInteractorTest : KoinTest { makeIndirectlyChanged(endHashes, "//:2", "//:3") val interactor = CalculateImpactedTargetsInteractor() - val impacted = interactor.executeWithDistances(startHashes, endHashes, depEdges) + val impacted = interactor.computeAllDistances(startHashes, endHashes, depEdges) assertThat(impacted).containsOnly( "//:1" to TargetDistanceMetrics(0,0), @@ -181,12 +181,12 @@ class CalculateImpactedTargetsInteractorTest : KoinTest { val interactor = CalculateImpactedTargetsInteractor() assertThat { - interactor.executeWithDistances(startHashes, endHashes, depEdges) + interactor.computeAllDistances(startHashes, endHashes, depEdges) }.isFailure().message().isEqualTo("//:2 was indirectly impacted, but has no impacted dependencies.") assertThat { // empty dep edges - interactor.executeWithDistances(startHashes, endHashes, mapOf()) + interactor.computeAllDistances(startHashes, endHashes, mapOf()) }.isFailure().message().isEqualTo("//:2 was indirectly impacted, but has no dependencies.") } diff --git a/cli/src/test/kotlin/com/bazel_diff/interactor/DeserialiseHashesInteractorTest.kt b/cli/src/test/kotlin/com/bazel_diff/interactor/DeserialiseHashesInteractorTest.kt index 0e3485e..3367c72 100644 --- a/cli/src/test/kotlin/com/bazel_diff/interactor/DeserialiseHashesInteractorTest.kt +++ b/cli/src/test/kotlin/com/bazel_diff/interactor/DeserialiseHashesInteractorTest.kt @@ -31,7 +31,7 @@ class DeserialiseHashesInteractorTest : KoinTest { @Test fun testDeserialisation() { val file = temp.newFile().apply { - writeText("""{"target-name":"hash#direct"}""") + writeText("""{"target-name":"hash~direct"}""") } val actual = interactor.executeTargetHash(file) @@ -44,9 +44,9 @@ class DeserialiseHashesInteractorTest : KoinTest { fun testDeserialisationWithType() { val file = temp.newFile().apply { writeText("""{ - | "target-1":"GeneratedFile#hash1#direct1", - | "target-2":"Rule#hash2#direct2", - | "target-3":"SourceFile#hash3#direct3" + | "target-1":"GeneratedFile#hash1~direct1", + | "target-2":"Rule#hash2~direct2", + | "target-3":"SourceFile#hash3~direct3" |}""".trimMargin()) } diff --git a/cli/src/test/resources/fixture/impacted_targets_distances-1-2-rule-sourcefile.txt b/cli/src/test/resources/fixture/impacted_targets_distances-1-2-rule-sourcefile.txt index c8edfba..87b8ff9 100644 --- a/cli/src/test/resources/fixture/impacted_targets_distances-1-2-rule-sourcefile.txt +++ b/cli/src/test/resources/fixture/impacted_targets_distances-1-2-rule-sourcefile.txt @@ -1,3 +1,5 @@ -//test/java/com/integration:bazel-diff-integration-test-lib~0~0 -//test/java/com/integration:bazel-diff-integration-tests~1~0 -//test/java/com/integration:TestStringGenerator.java~0~0 +[ +{"label": "//test/java/com/integration:bazel-diff-integration-test-lib", "targetDistance": 0, "packageDistance": 0}, +{"label": "//test/java/com/integration:bazel-diff-integration-tests", "targetDistance": 1, "packageDistance": 0}, +{"label": "//test/java/com/integration:TestStringGenerator.java", "targetDistance": 0, "packageDistance": 0} +] diff --git a/cli/src/test/resources/fixture/impacted_targets_distances-1-2.txt b/cli/src/test/resources/fixture/impacted_targets_distances-1-2.txt index c3ae3fb..2ea3920 100644 --- a/cli/src/test/resources/fixture/impacted_targets_distances-1-2.txt +++ b/cli/src/test/resources/fixture/impacted_targets_distances-1-2.txt @@ -1,8 +1,10 @@ -//test/java/com/integration:libbazel-diff-integration-test-lib-src.jar~0~0 -//test/java/com/integration:TestStringGenerator.java~0~0 -//test/java/com/integration:bazel-diff-integration-test-lib~0~0 -//test/java/com/integration:bazel-diff-integration-tests-src.jar~2~0 -//test/java/com/integration:libbazel-diff-integration-test-lib.jar~0~0 -//test/java/com/integration:bazel-diff-integration-tests~1~0 -//test/java/com/integration:bazel-diff-integration-tests_deploy-src.jar~2~0 -//test/java/com/integration:bazel-diff-integration-tests.jar~2~0 +[ +{"label": "//test/java/com/integration:libbazel-diff-integration-test-lib-src.jar", "targetDistance": 0, "packageDistance": 0}, +{"label": "//test/java/com/integration:TestStringGenerator.java", "targetDistance": 0, "packageDistance": 0}, +{"label": "//test/java/com/integration:bazel-diff-integration-test-lib", "targetDistance": 0, "packageDistance": 0}, +{"label": "//test/java/com/integration:bazel-diff-integration-tests-src.jar", "targetDistance": 2, "packageDistance": 0}, +{"label": "//test/java/com/integration:libbazel-diff-integration-test-lib.jar", "targetDistance": 0, "packageDistance": 0}, +{"label": "//test/java/com/integration:bazel-diff-integration-tests", "targetDistance": 1, "packageDistance": 0}, +{"label": "//test/java/com/integration:bazel-diff-integration-tests_deploy-src.jar", "targetDistance": 2, "packageDistance": 0}, +{"label": "//test/java/com/integration:bazel-diff-integration-tests.jar", "targetDistance": 2, "packageDistance": 0} +] From 8a59899e6097f65664a9b8a486dcd9bbbfd3163c Mon Sep 17 00:00:00 2001 From: Alex Torok Date: Fri, 4 Oct 2024 17:58:30 +0000 Subject: [PATCH 09/14] Fix typo in variable name --- MODULE.bazel.lock | 2 +- .../main/kotlin/com/bazel_diff/cli/GetImpactedTargetsCommand.kt | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/MODULE.bazel.lock b/MODULE.bazel.lock index b46e7f4..0483a99 100644 --- a/MODULE.bazel.lock +++ b/MODULE.bazel.lock @@ -127,7 +127,7 @@ "@@rules_jvm_external~//:extensions.bzl%maven": { "general": { "bzlTransitiveDigest": "aME0tyUxYd+PGhICmzT9zEnIgZNf05hZhsfDD5v0JXM=", - "usagesDigest": "AkZduefFr6Ydap5ccPDieRcZ7XCBlsl1d5R2hstogZc=", + "usagesDigest": "b+xab90Gm1rhKygTVWH8xsx8d1c7DeF8xj+sQm0dN9s=", "recordedFileInputs": { "@@rules_jvm_external~//rules_jvm_external_deps_install.json": "cafb5d2d8119391eb2b322ce3840d3352ea82d496bdb8cbd4b6779ec4d044dda", "@@//maven_install.json": "8ab3e635bbd52fcff5900e530933fbdc41c45d1d5b2a2aef8af1becf7a7d0784" 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 5841f0e..dd0fe8f 100644 --- a/cli/src/main/kotlin/com/bazel_diff/cli/GetImpactedTargetsCommand.kt +++ b/cli/src/main/kotlin/com/bazel_diff/cli/GetImpactedTargetsCommand.kt @@ -109,7 +109,7 @@ class GetImpactedTargetsCommand : Callable { "Incorrect final hashes: file doesn't exist or can't be read." ) } - if (depsFile != null && !depsFile.canRead()) { + if (depsMappingJSONPath != null && !depsMappingJSONPath!!.canRead()) { throw CommandLine.ParameterException( spec.commandLine(), "Incorrect dep edges file: file doesn't exist or can't be read." From aaa97714c82c31bc5e7ef376b9b673cacd69ba1b Mon Sep 17 00:00:00 2001 From: Alex Torok Date: Sat, 5 Oct 2024 14:42:41 +0000 Subject: [PATCH 10/14] Add test workspace and e2e test --- cli/BUILD | 8 +++ .../test/kotlin/com/bazel_diff/e2e/E2ETest.kt | 72 +++++++++++++++++++ .../workspaces/distance_metrics/A/BUILD | 27 +++++++ .../workspaces/distance_metrics/A/one.sh | 0 .../workspaces/distance_metrics/A/three.sh | 0 .../workspaces/distance_metrics/BUILD | 6 ++ .../workspaces/distance_metrics/WORKSPACE | 2 + .../workspaces/distance_metrics/lib.sh | 0 8 files changed, 115 insertions(+) create mode 100644 cli/src/test/resources/workspaces/distance_metrics/A/BUILD create mode 100755 cli/src/test/resources/workspaces/distance_metrics/A/one.sh create mode 100644 cli/src/test/resources/workspaces/distance_metrics/A/three.sh create mode 100644 cli/src/test/resources/workspaces/distance_metrics/BUILD create mode 100644 cli/src/test/resources/workspaces/distance_metrics/WORKSPACE create mode 100644 cli/src/test/resources/workspaces/distance_metrics/lib.sh diff --git a/cli/BUILD b/cli/BUILD index fb0248d..12e9d7c 100644 --- a/cli/BUILD +++ b/cli/BUILD @@ -143,6 +143,7 @@ kt_jvm_test( }), test_class = "com.bazel_diff.e2e.E2ETest", runtime_deps = [":cli-test-lib"], + data = [":workspaces"], ) kt_jvm_test( @@ -175,3 +176,10 @@ kt_jvm_library( "@bazel_diff_maven//:org_mockito_kotlin_mockito_kotlin", ], ) + +filegroup( + name = "workspaces", + srcs = [ + "src/test/resources/workspaces", + ], +) 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 7b9a150..de1de21 100644 --- a/cli/src/test/kotlin/com/bazel_diff/e2e/E2ETest.kt +++ b/cli/src/test/kotlin/com/bazel_diff/e2e/E2ETest.kt @@ -2,6 +2,7 @@ package com.bazel_diff.e2e import assertk.assertThat import assertk.assertions.isEqualTo +import assertk.assertions.containsExactlyInAnyOrder import com.bazel_diff.cli.BazelDiff import com.google.gson.Gson import com.google.gson.reflect.TypeToken @@ -176,6 +177,9 @@ class E2ETest { // the only impacted targets. val projectA = extractFixtureProject("/fixture/fine-grained-hash-bzlmod-test-1.zip") val projectB = extractFixtureProject("/fixture/fine-grained-hash-bzlmod-test-2.zip") + println("----------------") + println("Testing in workspace: $projectA") + println("----------------") val workingDirectoryA = projectA val workingDirectoryB = projectB @@ -206,6 +210,56 @@ class E2ETest { assertThat(actual).isEqualTo(expected) } +// @Ignore("foo") + @Test + fun testTargetDistanceMetrics() { + val workspace = copyTestWorkspace("distance_metrics") + println("Testing in workspace: $workspace") + + val outputDir = temp.newFolder() + val from = File(outputDir, "starting_hashes.json") + val to = File(outputDir, "final_hashes.json") + val depsFile = File(outputDir, "depEdges.json") + val impactedTargetsOutput = File(outputDir, "impacted_targets.txt") + + val cli = CommandLine(BazelDiff()) + + cli.execute("generate-hashes", "--includeTargetType", "-w", workspace.absolutePath, "-b", "bazel", from.absolutePath) + // Modify the workspace + File(workspace, "A/one.sh").appendText("foo") + cli.execute("generate-hashes", "--includeTargetType", "-w", workspace.absolutePath, "-d", depsFile.absolutePath, "-b", "bazel", to.absolutePath) + + //Impacted targets + cli.execute( + "get-impacted-targets", + "-sh", from.absolutePath, + "-fh", to.absolutePath, + "-d", depsFile.absolutePath, + "-tt", "Rule,GeneratedFile", + "-o", impactedTargetsOutput.absolutePath, + ) + + // When computing target distances, the output format is json. Read the files and assert the sorted contents. + val gson = Gson() + val shape = object : TypeToken>>() {}.type + val actual = gson.fromJson>>(impactedTargetsOutput.readText(), shape).sortedBy { it["label"] as String } + val expected: List> = listOf( + mapOf("label" to "//A:one", "targetDistance" to 0.0, "packageDistance" to 0.0), + mapOf("label" to "//A:gen_two", "targetDistance" to 1.0, "packageDistance" to 0.0), + mapOf("label" to "//A:two.sh", "targetDistance" to 2.0, "packageDistance" to 0.0), + mapOf("label" to "//A:two", "targetDistance" to 3.0, "packageDistance" to 0.0), + mapOf("label" to "//A:three", "targetDistance" to 4.0, "packageDistance" to 0.0), + mapOf("label" to "//:lib", "targetDistance" to 5.0, "packageDistance" to 1.0) + ) + + assertThat(actual.size).isEqualTo(expected.size) + + expected.forEach { expectedMap -> + val actualMap = actual.find { it["label"] == expectedMap["label"] } + assertThat(actualMap).isEqualTo(expectedMap) + } + } + // TODO: re-enable the test after https://github.com/bazelbuild/bazel/issues/21010 is fixed @Ignore("cquery mode is broken with Bazel 7 because --transition=lite is crashes due to https://github.com/bazelbuild/bazel/issues/21010") @Test @@ -414,6 +468,24 @@ class E2ETest { assertThat(actual).isEqualTo(expected) } + private fun copyTestWorkspace(path: String): File { + val testProject = temp.newFolder() + + // Copy all of the files in path into a new folder + // print the pwd + val filepath = File("cli/src/test/resources/workspaces", path) + filepath.walkTopDown().forEach { file -> + val destFile = File(testProject, file.relativeTo(filepath).path) + if (file.isDirectory) { + destFile.mkdirs() + } else { + file.copyTo(destFile) + } + } + return testProject + + } + private fun extractFixtureProject(path: String): File { val testProject = temp.newFolder() val fixtureCopy = temp.newFile() diff --git a/cli/src/test/resources/workspaces/distance_metrics/A/BUILD b/cli/src/test/resources/workspaces/distance_metrics/A/BUILD new file mode 100644 index 0000000..5269721 --- /dev/null +++ b/cli/src/test/resources/workspaces/distance_metrics/A/BUILD @@ -0,0 +1,27 @@ + +sh_binary( + name = "one", + srcs = ["one.sh"], +) + +# Put a generated file in the path to ensure that we properly handle deps that cross +# one. +genrule( + name = "gen_two", + outs = ["two.sh"], + cmd = "$(location :one) && echo two > $@", + tools = [":one"] +) +sh_library( + name = "two", + srcs = ["two.sh"], +) + +sh_library( + name = "three", + srcs = ["three.sh"], + deps =["two"], + visibility = ["//visibility:public"], +) + + diff --git a/cli/src/test/resources/workspaces/distance_metrics/A/one.sh b/cli/src/test/resources/workspaces/distance_metrics/A/one.sh new file mode 100755 index 0000000..e69de29 diff --git a/cli/src/test/resources/workspaces/distance_metrics/A/three.sh b/cli/src/test/resources/workspaces/distance_metrics/A/three.sh new file mode 100644 index 0000000..e69de29 diff --git a/cli/src/test/resources/workspaces/distance_metrics/BUILD b/cli/src/test/resources/workspaces/distance_metrics/BUILD new file mode 100644 index 0000000..5be336c --- /dev/null +++ b/cli/src/test/resources/workspaces/distance_metrics/BUILD @@ -0,0 +1,6 @@ + +sh_library( + name = "lib", + srcs = ["lib.sh"], + deps = ["//A:three"], +) diff --git a/cli/src/test/resources/workspaces/distance_metrics/WORKSPACE b/cli/src/test/resources/workspaces/distance_metrics/WORKSPACE new file mode 100644 index 0000000..0b53bec --- /dev/null +++ b/cli/src/test/resources/workspaces/distance_metrics/WORKSPACE @@ -0,0 +1,2 @@ + +workspace(name="distance_metrics_integration") diff --git a/cli/src/test/resources/workspaces/distance_metrics/lib.sh b/cli/src/test/resources/workspaces/distance_metrics/lib.sh new file mode 100644 index 0000000..e69de29 From a60e0064c3af70bec2c4ef88be7e7a2d8175c105 Mon Sep 17 00:00:00 2001 From: Alex Torok Date: Sat, 5 Oct 2024 15:49:45 +0000 Subject: [PATCH 11/14] remove printlns --- cli/src/test/kotlin/com/bazel_diff/e2e/E2ETest.kt | 7 ------- 1 file changed, 7 deletions(-) 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 de1de21..c7b5361 100644 --- a/cli/src/test/kotlin/com/bazel_diff/e2e/E2ETest.kt +++ b/cli/src/test/kotlin/com/bazel_diff/e2e/E2ETest.kt @@ -177,9 +177,6 @@ class E2ETest { // the only impacted targets. val projectA = extractFixtureProject("/fixture/fine-grained-hash-bzlmod-test-1.zip") val projectB = extractFixtureProject("/fixture/fine-grained-hash-bzlmod-test-2.zip") - println("----------------") - println("Testing in workspace: $projectA") - println("----------------") val workingDirectoryA = projectA val workingDirectoryB = projectB @@ -210,11 +207,9 @@ class E2ETest { assertThat(actual).isEqualTo(expected) } -// @Ignore("foo") @Test fun testTargetDistanceMetrics() { val workspace = copyTestWorkspace("distance_metrics") - println("Testing in workspace: $workspace") val outputDir = temp.newFolder() val from = File(outputDir, "starting_hashes.json") @@ -239,7 +234,6 @@ class E2ETest { "-o", impactedTargetsOutput.absolutePath, ) - // When computing target distances, the output format is json. Read the files and assert the sorted contents. val gson = Gson() val shape = object : TypeToken>>() {}.type val actual = gson.fromJson>>(impactedTargetsOutput.readText(), shape).sortedBy { it["label"] as String } @@ -472,7 +466,6 @@ class E2ETest { val testProject = temp.newFolder() // Copy all of the files in path into a new folder - // print the pwd val filepath = File("cli/src/test/resources/workspaces", path) filepath.walkTopDown().forEach { file -> val destFile = File(testProject, file.relativeTo(filepath).path) From 68bef937a0e40324b98266258ff784214f2b9326 Mon Sep 17 00:00:00 2001 From: Alex Torok Date: Sat, 5 Oct 2024 15:54:24 +0000 Subject: [PATCH 12/14] Missing bazelignore --- .bazelignore | 1 + 1 file changed, 1 insertion(+) create mode 100644 .bazelignore diff --git a/.bazelignore b/.bazelignore new file mode 100644 index 0000000..ce33f0a --- /dev/null +++ b/.bazelignore @@ -0,0 +1 @@ +cli/src/test/resources/workspaces From 16be74999b2d9a40e76ef115d606a50acbf8440a Mon Sep 17 00:00:00 2001 From: Alex Torok Date: Wed, 9 Oct 2024 23:48:58 +0000 Subject: [PATCH 13/14] Remove e2e test that is duplicated by other one --- .../test/kotlin/com/bazel_diff/e2e/E2ETest.kt | 39 ++++--------------- ..._targets_distances-1-2-rule-sourcefile.txt | 5 --- .../impacted_targets_distances-1-2.txt | 10 ----- 3 files changed, 7 insertions(+), 47 deletions(-) delete mode 100644 cli/src/test/resources/fixture/impacted_targets_distances-1-2-rule-sourcefile.txt delete mode 100644 cli/src/test/resources/fixture/impacted_targets_distances-1-2.txt 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 11f2004..09190ac 100644 --- a/cli/src/test/kotlin/com/bazel_diff/e2e/E2ETest.kt +++ b/cli/src/test/kotlin/com/bazel_diff/e2e/E2ETest.kt @@ -22,7 +22,7 @@ class E2ETest { private fun CommandLine.execute(args: List) = execute(*args.toTypedArray()) - private fun testE2E(extraGenerateHashesArgs: List, extraGetImpactedTargetsArgs: List, expectedResultFile: String, computeDistances: Boolean = false) { + private fun testE2E(extraGenerateHashesArgs: List, extraGetImpactedTargetsArgs: List, expectedResultFile: String) { val projectA = extractFixtureProject("/fixture/integration-test-1.zip") val projectB = extractFixtureProject("/fixture/integration-test-2.zip") @@ -32,7 +32,6 @@ class E2ETest { val outputDir = temp.newFolder() val from = File(outputDir, "starting_hashes.json") val to = File(outputDir, "final_hashes.json") - val depsFile = File(outputDir, "deps.json") val impactedTargetsOutput = File(outputDir, "impacted_targets.txt") val cli = CommandLine(BazelDiff()) @@ -42,30 +41,18 @@ class E2ETest { ) //To cli.execute( - listOf("generate-hashes", "-w", workingDirectoryB.absolutePath, "-b", bazelPath, to.absolutePath) + extraGenerateHashesArgs + if (computeDistances) listOf("-d", depsFile.absolutePath) else emptyList() + listOf("generate-hashes", "-w", workingDirectoryB.absolutePath, "-b", bazelPath, to.absolutePath) + extraGenerateHashesArgs ) //Impacted targets cli.execute( - listOf("get-impacted-targets", "-sh", from.absolutePath, "-fh", to.absolutePath, "-o", impactedTargetsOutput.absolutePath) + extraGetImpactedTargetsArgs + if (computeDistances) listOf("-d", depsFile.absolutePath) else emptyList() + listOf("get-impacted-targets", "-sh", from.absolutePath, "-fh", to.absolutePath, "-o", impactedTargetsOutput.absolutePath) + extraGetImpactedTargetsArgs ) - if (!computeDistances) { - val actual: Set = impactedTargetsOutput.readLines().filter { it.isNotBlank() }.toSet() - val expected: Set = - javaClass.getResourceAsStream(expectedResultFile).use { it.bufferedReader().readLines().filter { it.isNotBlank() }.toSet() } - - assertThat(actual).isEqualTo(expected) - } else { - // When computing target distances, the output format is json. Read the files and assert the sorted contents. - val gson = Gson() - val shape = object : TypeToken>>() {}.type - val actual = gson.fromJson>>(impactedTargetsOutput.readText(), shape).sortedBy { it["label"] as String } - val expected = javaClass.getResourceAsStream(expectedResultFile).use { - gson.fromJson>>(it.bufferedReader().readText(), shape).sortedBy { it["label"] as String } - } + val actual: Set = impactedTargetsOutput.readLines().filter { it.isNotBlank() }.toSet() + val expected: Set = + javaClass.getResourceAsStream(expectedResultFile).use { it.bufferedReader().readLines().filter { it.isNotBlank() }.toSet() } - assertThat(actual).isEqualTo(expected) - } + assertThat(actual).isEqualTo(expected) } @Test @@ -73,12 +60,6 @@ class E2ETest { testE2E(emptyList(), emptyList(), "/fixture/impacted_targets-1-2.txt") } - @Test - fun testE2EDistances() { - testE2E(emptyList(), emptyList(), "/fixture/impacted_targets_distances-1-2.txt", computeDistances = true) - } - - @Test fun testE2EIncludingTargetType() { testE2E(listOf("-tt", "Rule,SourceFile"), emptyList(), "/fixture/impacted_targets-1-2-rule-sourcefile.txt") @@ -89,12 +70,6 @@ class E2ETest { testE2E(listOf("--includeTargetType"), listOf("-tt", "Rule,SourceFile"), "/fixture/impacted_targets-1-2-rule-sourcefile.txt") } - @Test - fun testE2EWithTargetTypeAndDistance() { - testE2E(listOf("--includeTargetType"), listOf("-tt", "Rule,SourceFile"), "/fixture/impacted_targets_distances-1-2-rule-sourcefile.txt", computeDistances = true) - } - - @Test fun testFineGrainedHashExternalRepo() { // The difference between these two snapshots is simply upgrading the Guava version. diff --git a/cli/src/test/resources/fixture/impacted_targets_distances-1-2-rule-sourcefile.txt b/cli/src/test/resources/fixture/impacted_targets_distances-1-2-rule-sourcefile.txt deleted file mode 100644 index 87b8ff9..0000000 --- a/cli/src/test/resources/fixture/impacted_targets_distances-1-2-rule-sourcefile.txt +++ /dev/null @@ -1,5 +0,0 @@ -[ -{"label": "//test/java/com/integration:bazel-diff-integration-test-lib", "targetDistance": 0, "packageDistance": 0}, -{"label": "//test/java/com/integration:bazel-diff-integration-tests", "targetDistance": 1, "packageDistance": 0}, -{"label": "//test/java/com/integration:TestStringGenerator.java", "targetDistance": 0, "packageDistance": 0} -] diff --git a/cli/src/test/resources/fixture/impacted_targets_distances-1-2.txt b/cli/src/test/resources/fixture/impacted_targets_distances-1-2.txt deleted file mode 100644 index 2ea3920..0000000 --- a/cli/src/test/resources/fixture/impacted_targets_distances-1-2.txt +++ /dev/null @@ -1,10 +0,0 @@ -[ -{"label": "//test/java/com/integration:libbazel-diff-integration-test-lib-src.jar", "targetDistance": 0, "packageDistance": 0}, -{"label": "//test/java/com/integration:TestStringGenerator.java", "targetDistance": 0, "packageDistance": 0}, -{"label": "//test/java/com/integration:bazel-diff-integration-test-lib", "targetDistance": 0, "packageDistance": 0}, -{"label": "//test/java/com/integration:bazel-diff-integration-tests-src.jar", "targetDistance": 2, "packageDistance": 0}, -{"label": "//test/java/com/integration:libbazel-diff-integration-test-lib.jar", "targetDistance": 0, "packageDistance": 0}, -{"label": "//test/java/com/integration:bazel-diff-integration-tests", "targetDistance": 1, "packageDistance": 0}, -{"label": "//test/java/com/integration:bazel-diff-integration-tests_deploy-src.jar", "targetDistance": 2, "packageDistance": 0}, -{"label": "//test/java/com/integration:bazel-diff-integration-tests.jar", "targetDistance": 2, "packageDistance": 0} -] From 50b1609cb7ec9db2050bad00e518d6abad3b82dd Mon Sep 17 00:00:00 2001 From: Alex Torok Date: Mon, 14 Oct 2024 11:56:10 -0700 Subject: [PATCH 14/14] Update BUILD --- cli/BUILD | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/cli/BUILD b/cli/BUILD index c5e2619..6a84140 100644 --- a/cli/BUILD +++ b/cli/BUILD @@ -53,10 +53,7 @@ kt_jvm_test( kt_jvm_test( name = "TargetHashTest", - jvm_flags = select({ - ":macos": ["-Djava.security.manager=allow"], - "//conditions:default": [], - }), + jvm_flags = ["-Djava.security.manager=allow"], test_class = "com.bazel_diff.hash.TargetHashTest", runtime_deps = [":cli-test-lib"], )