Skip to content

Commit f121539

Browse files
alex-torokalex-torok
committed
Address review feedback
1. Output detailed hash data as json 2. Use a different serialization format for targethashes to avoid ambiguity
1 parent 27b0b68 commit f121539

File tree

10 files changed

+119
-64
lines changed

10 files changed

+119
-64
lines changed

‎cli/src/main/kotlin/com/bazel_diff/cli/GenerateHashesCommand.kt

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -137,8 +137,8 @@ class GenerateHashesCommand : Callable<Int> {
137137
var ignoredRuleHashingAttributes: Set<String> = emptySet()
138138

139139
@CommandLine.Option(
140-
names = ["-d", "--depsFile"],
141-
description = ["Path to the file where dependency edges are written to. If not specified, the dependency edges will not be written to a file."],
140+
names = ["-d", "--depEdgesFile"],
141+
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."],
142142
scope = CommandLine.ScopeType.INHERIT,
143143
defaultValue = CommandLine.Parameters.NULL_VALUE
144144
)

‎cli/src/main/kotlin/com/bazel_diff/cli/GetImpactedTargetsCommand.kt

Lines changed: 17 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ class GetImpactedTargetsCommand : Callable<Int> {
4040
lateinit var finalHashesJSONPath: File
4141

4242
@CommandLine.Option(
43-
names = ["-d", "--depsFile"],
43+
names = ["-d", "--depEdgesFile"],
4444
description = ["Path to the file where dependency edges are. If specified, build graph distance metrics will be computed from the given hash data."],
4545
scope = CommandLine.ScopeType.INHERIT,
4646
defaultValue = CommandLine.Parameters.NULL_VALUE
@@ -58,7 +58,7 @@ class GetImpactedTargetsCommand : Callable<Int> {
5858
@CommandLine.Option(
5959
names = ["-o", "--output"],
6060
scope = CommandLine.ScopeType.LOCAL,
61-
description = ["Filepath to write the impacted Bazel targets to, newline separated. If not specified, the targets will be written to STDOUT."],
61+
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."],
6262
)
6363
var outputPath: File? = null
6464

@@ -78,30 +78,17 @@ class GetImpactedTargetsCommand : Callable<Int> {
7878
val from = deserialiser.executeTargetHash(startingHashesJSONPath)
7979
val to = deserialiser.executeTargetHash(finalHashesJSONPath)
8080

81-
val typeFilter = TargetTypeFilter(targetType, to)
82-
83-
val impactedTargetsStream = if (depsMappingJSONPath != null) {
84-
val depsMapping = deserialiser.deserializeDeps(depsMappingJSONPath!!)
85-
CalculateImpactedTargetsInteractor().executeWithDistances(from, to, depsMapping)
86-
.filterKeys { typeFilter.accepts(it) }
87-
.map { (label, metrics) ->
88-
"${label}~${metrics.targetDistance}~${metrics.packageDistance}"
89-
}.stream()
90-
} else {
91-
CalculateImpactedTargetsInteractor().execute(from, to)
92-
.filter { typeFilter.accepts(it) }
93-
.stream()
94-
}
95-
96-
return try {
97-
BufferedWriter(when (val path = outputPath) {
81+
val outputWriter = BufferedWriter(when (val path = outputPath) {
9882
null -> FileWriter(FileDescriptor.out)
9983
else -> FileWriter(path)
100-
}).use { writer ->
101-
impactedTargetsStream.forEach { line ->
102-
writer.write(line)
103-
writer.write("\n")
104-
}
84+
})
85+
86+
return try {
87+
if (depsMappingJSONPath != null) {
88+
val depsMapping = deserialiser.deserializeDeps(depsMappingJSONPath!!)
89+
CalculateImpactedTargetsInteractor().executeWithDistances(from, to, depsMapping, outputWriter, targetType)
90+
} else {
91+
CalculateImpactedTargetsInteractor().execute(from, to, outputWriter, targetType)
10592
}
10693
CommandLine.ExitCode.OK
10794
} catch (e: IOException) {
@@ -122,5 +109,11 @@ class GetImpactedTargetsCommand : Callable<Int> {
122109
"Incorrect final hashes: file doesn't exist or can't be read."
123110
)
124111
}
112+
if (depsFile != null && !depsFile.canRead()) {
113+
throw CommandLine.ParameterException(
114+
spec.commandLine(),
115+
"Incorrect dep edges file: file doesn't exist or can't be read."
116+
)
117+
}
125118
}
126119
}

‎cli/src/main/kotlin/com/bazel_diff/hash/TargetDigest.kt

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,11 @@ data class TargetDigest(
88
val deps: List<String>? = null,
99
) {
1010
fun clone(newDeps: List<String>? = null): TargetDigest {
11-
var newDeps = newDeps
11+
var toUse = newDeps
1212
if (newDeps == null) {
13-
newDeps = deps
13+
toUse = deps
1414
}
15-
return TargetDigest(overallDigest.clone(), directDigest.clone(), newDeps)
15+
return TargetDigest(overallDigest.clone(), directDigest.clone(), toUse)
1616
}
1717
}
1818

‎cli/src/main/kotlin/com/bazel_diff/hash/TargetHash.kt

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,11 @@ data class TargetHash(
77
val deps: List<String>? = null
88
) {
99
val hashWithType by lazy {
10-
"${type}#${hash}#${directHash}"
10+
"${type}#${hash}~${directHash}"
1111
}
1212

1313
val totalHash by lazy {
14-
"${hash}#${directHash}"
14+
"${hash}~${directHash}"
1515
}
1616

1717
fun toJson(includeTargetType: Boolean): String {
@@ -30,9 +30,15 @@ data class TargetHash(
3030
fun fromJson(json: String): TargetHash {
3131
val parts = json.split("#")
3232
return when (parts.size) {
33-
2 -> TargetHash("", parts[0], parts[1])
34-
3 -> TargetHash(parts[0], parts[1], parts[2])
35-
else -> throw IllegalArgumentException("Invalid JSON format")
33+
1 -> Pair("", parts[0])
34+
2 -> Pair(parts[0], parts[1])
35+
else -> throw IllegalArgumentException("Invalid targetHash format: $json")
36+
}.let { (type, hash) ->
37+
val hashes = hash.split("~")
38+
when (hashes.size) {
39+
2 -> TargetHash(type, hashes[0], hashes[1])
40+
else -> throw IllegalArgumentException("Invalid targetHash format: $json")
41+
}
3642
}
3743
}
3844
}

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

Lines changed: 40 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,11 @@ package com.bazel_diff.interactor
22

33
import com.bazel_diff.hash.TargetHash
44
import com.google.common.collect.Maps
5+
import com.google.gson.Gson
56
import org.koin.core.component.KoinComponent
7+
import org.koin.core.component.inject
68
import java.io.File
9+
import java.io.Writer
710
import java.util.concurrent.ConcurrentMap
811
import java.util.concurrent.ConcurrentHashMap
912
import java.util.stream.Stream
@@ -16,6 +19,7 @@ data class TargetDistanceMetrics(
1619
) {}
1720

1821
class CalculateImpactedTargetsInteractor : KoinComponent {
22+
private val gson: Gson by inject()
1923

2024
@VisibleForTesting
2125
class InvalidDependencyEdgesException(message: String) : Exception(message)
@@ -25,10 +29,24 @@ class CalculateImpactedTargetsInteractor : KoinComponent {
2529
INDIRECT
2630
}
2731

28-
fun execute(from: Map<String, TargetHash>, to: Map<String, TargetHash>): Set<String> {
32+
fun execute(from: Map<String, TargetHash>, to: Map<String, TargetHash>, outputWriter: Writer, targetTypes: Set<String>?) {
2933
/**
3034
* This call might be faster if end hashes is a sorted map
3135
*/
36+
val typeFilter = TargetTypeFilter(targetTypes, to)
37+
38+
computeSimpleImpactedTargets(from, to)
39+
.filter { typeFilter.accepts(it) }
40+
.let { impactedTargets ->
41+
outputWriter.use { writer ->
42+
impactedTargets.forEach {
43+
writer.write("$it\n")
44+
}
45+
}
46+
}
47+
}
48+
49+
fun computeSimpleImpactedTargets(from: Map<String, TargetHash>, to: Map<String, TargetHash>): Set<String> {
3250
val difference = Maps.difference(to, from)
3351
val onlyInEnd: Set<String> = difference.entriesOnlyOnLeft().keys
3452
val changed: Set<String> = difference.entriesDiffering().keys
@@ -39,7 +57,27 @@ class CalculateImpactedTargetsInteractor : KoinComponent {
3957
return impactedTargets
4058
}
4159

42-
fun executeWithDistances(from: Map<String, TargetHash>, to: Map<String, TargetHash>, depEdges: Map<String, List<String>>): Map<String, TargetDistanceMetrics> {
60+
fun executeWithDistances(from: Map<String, TargetHash>, to: Map<String, TargetHash>, depEdges: Map<String, List<String>>, outputWriter: Writer, targetTypes: Set<String>?) {
61+
val typeFilter = TargetTypeFilter(targetTypes, to)
62+
63+
computeAllDistances(from, to, depEdges)
64+
.filterKeys { typeFilter.accepts(it) }
65+
.let { impactedTargets ->
66+
outputWriter.use { writer ->
67+
writer.write(gson.toJson(
68+
impactedTargets.map{
69+
mapOf(
70+
"label" to it.key,
71+
"targetDistance" to it.value.targetDistance,
72+
"packageDistance" to it.value.packageDistance
73+
)
74+
}
75+
))
76+
}
77+
}
78+
}
79+
80+
fun computeAllDistances(from: Map<String, TargetHash>, to: Map<String, TargetHash>, depEdges: Map<String, List<String>>): Map<String, TargetDistanceMetrics> {
4381
val difference = Maps.difference(to, from)
4482

4583
val newLabels = difference.entriesOnlyOnLeft().keys

‎cli/src/test/kotlin/com/bazel_diff/e2e/E2ETest.kt

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@ package com.bazel_diff.e2e
33
import assertk.assertThat
44
import assertk.assertions.isEqualTo
55
import com.bazel_diff.cli.BazelDiff
6+
import com.google.gson.Gson
7+
import com.google.gson.reflect.TypeToken
68
import org.junit.Ignore
79
import org.junit.Rule
810
import org.junit.Test
@@ -46,11 +48,23 @@ class E2ETest {
4648
listOf("get-impacted-targets", "-sh", from.absolutePath, "-fh", to.absolutePath, "-o", impactedTargetsOutput.absolutePath) + extraGetImpactedTargetsArgs + if (computeDistances) listOf("-d", depsFile.absolutePath) else emptyList()
4749
)
4850

49-
val actual: Set<String> = impactedTargetsOutput.readLines().filter { it.isNotBlank() }.toSet()
50-
val expected: Set<String> =
51-
javaClass.getResourceAsStream(expectedResultFile).use { it.bufferedReader().readLines().filter { it.isNotBlank() }.toSet() }
51+
if (!computeDistances) {
52+
val actual: Set<String> = impactedTargetsOutput.readLines().filter { it.isNotBlank() }.toSet()
53+
val expected: Set<String> =
54+
javaClass.getResourceAsStream(expectedResultFile).use { it.bufferedReader().readLines().filter { it.isNotBlank() }.toSet() }
55+
56+
assertThat(actual).isEqualTo(expected)
57+
} else {
58+
// When computing target distances, the output format is json. Read the files and assert the sorted contents.
59+
val gson = Gson()
60+
val shape = object : TypeToken<List<Map<String, Any>>>() {}.type
61+
val actual = gson.fromJson<List<Map<String, Any>>>(impactedTargetsOutput.readText(), shape).sortedBy { it["label"] as String }
62+
val expected = javaClass.getResourceAsStream(expectedResultFile).use {
63+
gson.fromJson<List<Map<String, Any>>>(it.bufferedReader().readText(), shape).sortedBy { it["label"] as String }
64+
}
5265

53-
assertThat(actual).isEqualTo(expected)
66+
assertThat(actual).isEqualTo(expected)
67+
}
5468
}
5569

5670
@Test

‎cli/src/test/kotlin/com/bazel_diff/interactor/CalculateImpactedTargetsInteractorTest.kt

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ class CalculateImpactedTargetsInteractorTest : KoinTest {
3434
end["3"] = "d"
3535
val endHashes = end.mapValues { TargetHash("", it.value, it.value) }
3636

37-
val impacted = interactor.execute(startHashes, endHashes)
37+
val impacted = interactor.computeSimpleImpactedTargets(startHashes, endHashes)
3838
assertThat(impacted).containsExactlyInAnyOrder(
3939
"1", "3"
4040
)
@@ -53,7 +53,7 @@ class CalculateImpactedTargetsInteractorTest : KoinTest {
5353
makeIndirectlyChanged(endHashes, "//:3")
5454

5555
val interactor = CalculateImpactedTargetsInteractor()
56-
val impacted = interactor.executeWithDistances(startHashes, endHashes, depEdges)
56+
val impacted = interactor.computeAllDistances(startHashes, endHashes, depEdges)
5757

5858
assertThat(impacted).containsOnly(
5959
"//:1" to TargetDistanceMetrics(0, 0),
@@ -72,7 +72,7 @@ class CalculateImpactedTargetsInteractorTest : KoinTest {
7272
)
7373

7474
val interactor = CalculateImpactedTargetsInteractor()
75-
val impacted = interactor.executeWithDistances(startHashes, endHashes, mapOf())
75+
val impacted = interactor.computeAllDistances(startHashes, endHashes, mapOf())
7676

7777
assertThat(impacted).containsOnly(
7878
"//:1" to TargetDistanceMetrics(0, 0),
@@ -91,7 +91,7 @@ class CalculateImpactedTargetsInteractorTest : KoinTest {
9191
makeIndirectlyChanged(endHashes, "//:2", "//:3")
9292

9393
val interactor = CalculateImpactedTargetsInteractor()
94-
val impacted = interactor.executeWithDistances(startHashes, endHashes, depEdges)
94+
val impacted = interactor.computeAllDistances(startHashes, endHashes, depEdges)
9595

9696
assertThat(impacted).containsOnly(
9797
"//:1" to TargetDistanceMetrics(0,0),
@@ -111,7 +111,7 @@ class CalculateImpactedTargetsInteractorTest : KoinTest {
111111
makeIndirectlyChanged(endHashes, "//A:2", "//B:3", "//B:4", "//C:5")
112112

113113
val interactor = CalculateImpactedTargetsInteractor()
114-
val impacted = interactor.executeWithDistances(startHashes, endHashes, depEdges)
114+
val impacted = interactor.computeAllDistances(startHashes, endHashes, depEdges)
115115

116116
assertThat(impacted).containsOnly(
117117
"//A:1" to TargetDistanceMetrics(0,0),
@@ -139,7 +139,7 @@ class CalculateImpactedTargetsInteractorTest : KoinTest {
139139
makeIndirectlyChanged(endHashes, "//A:target_1", "//A:target_2", "//A:target_3", "//A:target_4", "//B:target", "//C:target", "//D:target", "//final:final")
140140

141141
val interactor = CalculateImpactedTargetsInteractor()
142-
val impacted = interactor.executeWithDistances(startHashes, endHashes, depEdges)
142+
val impacted = interactor.computeAllDistances(startHashes, endHashes, depEdges)
143143

144144
assertThat(impacted["//final:final"]).isEqualTo(TargetDistanceMetrics(4,2))
145145
}
@@ -161,7 +161,7 @@ class CalculateImpactedTargetsInteractorTest : KoinTest {
161161
makeIndirectlyChanged(endHashes, "//:2", "//:3")
162162

163163
val interactor = CalculateImpactedTargetsInteractor()
164-
val impacted = interactor.executeWithDistances(startHashes, endHashes, depEdges)
164+
val impacted = interactor.computeAllDistances(startHashes, endHashes, depEdges)
165165

166166
assertThat(impacted).containsOnly(
167167
"//:1" to TargetDistanceMetrics(0,0),
@@ -181,12 +181,12 @@ class CalculateImpactedTargetsInteractorTest : KoinTest {
181181

182182
val interactor = CalculateImpactedTargetsInteractor()
183183
assertThat {
184-
interactor.executeWithDistances(startHashes, endHashes, depEdges)
184+
interactor.computeAllDistances(startHashes, endHashes, depEdges)
185185
}.isFailure().message().isEqualTo("//:2 was indirectly impacted, but has no impacted dependencies.")
186186

187187
assertThat {
188188
// empty dep edges
189-
interactor.executeWithDistances(startHashes, endHashes, mapOf())
189+
interactor.computeAllDistances(startHashes, endHashes, mapOf())
190190
}.isFailure().message().isEqualTo("//:2 was indirectly impacted, but has no dependencies.")
191191

192192
}

‎cli/src/test/kotlin/com/bazel_diff/interactor/DeserialiseHashesInteractorTest.kt

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ class DeserialiseHashesInteractorTest : KoinTest {
3131
@Test
3232
fun testDeserialisation() {
3333
val file = temp.newFile().apply {
34-
writeText("""{"target-name":"hash#direct"}""")
34+
writeText("""{"target-name":"hash~direct"}""")
3535
}
3636

3737
val actual = interactor.executeTargetHash(file)
@@ -44,9 +44,9 @@ class DeserialiseHashesInteractorTest : KoinTest {
4444
fun testDeserialisationWithType() {
4545
val file = temp.newFile().apply {
4646
writeText("""{
47-
| "target-1":"GeneratedFile#hash1#direct1",
48-
| "target-2":"Rule#hash2#direct2",
49-
| "target-3":"SourceFile#hash3#direct3"
47+
| "target-1":"GeneratedFile#hash1~direct1",
48+
| "target-2":"Rule#hash2~direct2",
49+
| "target-3":"SourceFile#hash3~direct3"
5050
|}""".trimMargin())
5151
}
5252

‎cli/src/test/resources/fixture/impacted_targets_distances-1-2-rule-sourcefile.txt

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1-
//test/java/com/integration:bazel-diff-integration-test-lib~0~0
2-
//test/java/com/integration:bazel-diff-integration-tests~1~0
3-
//test/java/com/integration:TestStringGenerator.java~0~0
1+
[
2+
{"label": "//test/java/com/integration:bazel-diff-integration-test-lib", "targetDistance": 0, "packageDistance": 0},
3+
{"label": "//test/java/com/integration:bazel-diff-integration-tests", "targetDistance": 1, "packageDistance": 0},
4+
{"label": "//test/java/com/integration:TestStringGenerator.java", "targetDistance": 0, "packageDistance": 0}
5+
]

‎cli/src/test/resources/fixture/impacted_targets_distances-1-2.txt

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,10 @@
1-
//test/java/com/integration:libbazel-diff-integration-test-lib-src.jar~0~0
2-
//test/java/com/integration:TestStringGenerator.java~0~0
3-
//test/java/com/integration:bazel-diff-integration-test-lib~0~0
4-
//test/java/com/integration:bazel-diff-integration-tests-src.jar~2~0
5-
//test/java/com/integration:libbazel-diff-integration-test-lib.jar~0~0
6-
//test/java/com/integration:bazel-diff-integration-tests~1~0
7-
//test/java/com/integration:bazel-diff-integration-tests_deploy-src.jar~2~0
8-
//test/java/com/integration:bazel-diff-integration-tests.jar~2~0
1+
[
2+
{"label": "//test/java/com/integration:libbazel-diff-integration-test-lib-src.jar", "targetDistance": 0, "packageDistance": 0},
3+
{"label": "//test/java/com/integration:TestStringGenerator.java", "targetDistance": 0, "packageDistance": 0},
4+
{"label": "//test/java/com/integration:bazel-diff-integration-test-lib", "targetDistance": 0, "packageDistance": 0},
5+
{"label": "//test/java/com/integration:bazel-diff-integration-tests-src.jar", "targetDistance": 2, "packageDistance": 0},
6+
{"label": "//test/java/com/integration:libbazel-diff-integration-test-lib.jar", "targetDistance": 0, "packageDistance": 0},
7+
{"label": "//test/java/com/integration:bazel-diff-integration-tests", "targetDistance": 1, "packageDistance": 0},
8+
{"label": "//test/java/com/integration:bazel-diff-integration-tests_deploy-src.jar", "targetDistance": 2, "packageDistance": 0},
9+
{"label": "//test/java/com/integration:bazel-diff-integration-tests.jar", "targetDistance": 2, "packageDistance": 0}
10+
]

0 commit comments

Comments
 (0)