Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Use consistent labels for cquery #244

Merged
merged 1 commit into from
Oct 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion MODULE.bazel.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 4 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,10 @@ workspace.
targets. For example, one wants to specify `maven`
here if they user rules_jvm_external so that
individual third party dependency change won't
invalidate all targets in the mono repo.
invalidate all targets in the mono repo. Note that
when `--useCquery` is used, the canonical repo name
must be provided but with single `@`, e.g.
`@rules_jvm_external~~maven~maven`
-h, --help Show this help message and exit.
--ignoredRuleHashingAttributes=<ignoredRuleHashingAttributes>
Attributes that should be ignored when hashing rule
Expand Down
20 changes: 3 additions & 17 deletions cli/src/main/kotlin/com/bazel_diff/bazel/BazelQueryService.kt
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ class BazelQueryService(
val targets = outputFile.inputStream().buffered().use { proto ->
if (useCquery) {
val cqueryResult = AnalysisProtosV2.CqueryResult.parseFrom(proto)
cqueryResult.resultsList.filter { inCompatibleTargetSet(it, compatibleTargetSet) }.map { it.target }
cqueryResult.resultsList.filter { it.target.rule.name in compatibleTargetSet }.map { it.target }
} else {
mutableListOf<Build.Target>().apply {
while (true) {
Expand All @@ -60,15 +60,6 @@ class BazelQueryService(
return targets
}

private fun inCompatibleTargetSet(
target: AnalysisProtosV2.ConfiguredTarget,
compatibleTargetSet: Set<String>
): Boolean {
val name = target.target.rule.name
return name in compatibleTargetSet ||
name.startsWith("@") && !name.startsWith("@@") && "@${name}" in compatibleTargetSet
}

@OptIn(ExperimentalCoroutinesApi::class)
private suspend fun runQuery(
query: String,
Expand Down Expand Up @@ -111,13 +102,7 @@ class BazelQueryService(
# printed
return ""
if "IncompatiblePlatformProvider" not in providers(target):
label = str(target.label)
# normalize label to be consistent with content inside proto
if label.startswith("@//"):
return label[1:]
if label.startswith("@@//"):
return label[2:]
return label
return str(target.label)
return ""
""".trimIndent())
add(cqueryOutputFile.toString())
Expand All @@ -137,6 +122,7 @@ class BazelQueryService(
}
if (useCquery) {
addAll(cqueryOptions)
add("--consistent_labels")
} else {
addAll(commandOptions)
}
Expand Down
6 changes: 5 additions & 1 deletion cli/src/main/kotlin/com/bazel_diff/bazel/BazelRule.kt
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ class BazelRule(private val rule: Build.Rule) {
val name: String = rule.name

private fun transformRuleInput(fineGrainedHashExternalRepos: Set<String>, ruleInput: String): String {
if (ruleInput.startsWith("@") && fineGrainedHashExternalRepos.none { ruleInput.startsWith("@$it") || ruleInput.startsWith("@@${it}") }) {
if (isNotMainRepo(ruleInput) && ruleInput.startsWith("@") && fineGrainedHashExternalRepos.none { ruleInput.startsWith("@$it") || ruleInput.startsWith("@@${it}") }) {
val splitRule = ruleInput.split("//".toRegex()).dropLastWhile { it.isEmpty() }.toTypedArray()
if (splitRule.size == 2) {
var externalRule = splitRule[0]
Expand All @@ -47,4 +47,8 @@ class BazelRule(private val rule: Build.Rule) {
}
return ruleInput
}

private fun isNotMainRepo(ruleInput: String): Boolean {
return !ruleInput.startsWith("//") && !ruleInput.startsWith("@//") && !ruleInput.startsWith("@@//")
}
}
23 changes: 19 additions & 4 deletions cli/src/main/kotlin/com/bazel_diff/hash/SourceFileHasher.kt
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,9 @@ class SourceFileHasher : KoinComponent {
): ByteArray {
return sha256 {
val name = sourceFileTarget.name
val filenamePath = if (name.startsWith("//")) {
val filenameSubstring = name.substring(2)
val index = isMainRepo(name);
val filenamePath = if (index != -1) {
val filenameSubstring = name.substring(index)
Paths.get(filenameSubstring.removePrefix(":").replace(':', '/'))
} else if (name.startsWith("@")) {
val parts = if (name.startsWith("@@")) {
Expand Down Expand Up @@ -95,14 +96,28 @@ class SourceFileHasher : KoinComponent {

fun softDigest(sourceFileTarget: BazelSourceFileTarget, modifiedFilepaths: Set<Path> = emptySet()): ByteArray? {
val name = sourceFileTarget.name
if (!name.startsWith("//")) return null
val index = isMainRepo(name)
if (index == -1) return null

val filenameSubstring = name.substring(2)
val filenameSubstring = name.substring(index)
val filenamePath = filenameSubstring.replaceFirst(":".toRegex(), "/")
val absoluteFilePath = Paths.get(workingDirectory.toString(), filenamePath)
val file = absoluteFilePath.toFile()
if (!file.exists() || !file.isFile) return null

return digest(sourceFileTarget, modifiedFilepaths)
}

private fun isMainRepo(name: String): Int {
if (name.startsWith("//")) {
return 2;
}
if (name.startsWith("@//")) {
return 3;
}
if (name.startsWith("@@//")) {
return 4;
}
return -1;
}
}
19 changes: 14 additions & 5 deletions cli/src/test/kotlin/com/bazel_diff/e2e/E2ETest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -123,8 +123,7 @@ class E2ETest {
assertThat(actual).isEqualTo(expected)
}

@Test
fun testFineGrainedHashBzlMod() {
private fun testFineGrainedHashBzlMod(extraGenerateHashesArgs: List<String>, fineGrainedHashExternalRepo: String, expectedResultFile: String) {
// The difference between these two snapshots is simply upgrading the Guava version.
// Following is the diff. (The diff on maven_install.json is omitted)
//
Expand Down Expand Up @@ -161,11 +160,11 @@ class E2ETest {
val cli = CommandLine(BazelDiff())
//From
cli.execute(
"generate-hashes", "-w", workingDirectoryA.absolutePath, "-b", bazelPath, "--fineGrainedHashExternalRepos", "bazel_diff_maven", from.absolutePath
listOf("generate-hashes", "-w", workingDirectoryA.absolutePath, "-b", bazelPath, "--fineGrainedHashExternalRepos", fineGrainedHashExternalRepo, from.absolutePath) + extraGenerateHashesArgs
)
//To
cli.execute(
"generate-hashes", "-w", workingDirectoryB.absolutePath, "-b", bazelPath, "--fineGrainedHashExternalRepos", "bazel_diff_maven", to.absolutePath
listOf("generate-hashes", "-w", workingDirectoryB.absolutePath, "-b", bazelPath, "--fineGrainedHashExternalRepos", fineGrainedHashExternalRepo, to.absolutePath) + extraGenerateHashesArgs
)
//Impacted targets
cli.execute(
Expand All @@ -174,11 +173,21 @@ class E2ETest {

val actual: Set<String> = impactedTargetsOutput.readLines().filter { it.isNotBlank() }.toSet()
val expected: Set<String> =
javaClass.getResourceAsStream("/fixture/fine-grained-hash-bzlmod-test-impacted-targets.txt").use { it.bufferedReader().readLines().filter { it.isNotBlank() }.toSet() }
javaClass.getResourceAsStream(expectedResultFile).use { it.bufferedReader().readLines().filter { it.isNotBlank() }.toSet() }

assertThat(actual).isEqualTo(expected)
}

@Test
fun testFineGrainedHashBzlMod() {
testFineGrainedHashBzlMod(emptyList(), "bazel_diff_maven", "/fixture/fine-grained-hash-bzlmod-test-impacted-targets.txt")
}

@Test
fun testFineGrainedHashBzlModCquery() {
testFineGrainedHashBzlMod(listOf("--useCquery"), "@rules_jvm_external~~maven~maven", "/fixture/fine-grained-hash-bzlmod-cquery-test-impacted-targets.txt")
}

// 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
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
@@//src/main/java/com/integration:guava-user
@@rules_jvm_external~~maven~com_google_errorprone_error_prone_annotations_2_18_0//file:file
@@rules_jvm_external~~maven~com_google_guava_guava_32_0_0_jre//file:file
@@rules_jvm_external~~maven~com_google_j2objc_j2objc_annotations_2_8//file:file
@@rules_jvm_external~~maven~maven//:com_google_errorprone_error_prone_annotations
@@rules_jvm_external~~maven~maven//:com_google_errorprone_error_prone_annotations_2_18_0_extension
@@rules_jvm_external~~maven~maven//:com_google_guava_guava
@@rules_jvm_external~~maven~maven//:com_google_guava_guava_32_0_0_jre_extension
@@rules_jvm_external~~maven~maven//:com_google_j2objc_j2objc_annotations
@@rules_jvm_external~~maven~maven//:com_google_j2objc_j2objc_annotations_2_8_extension
@@rules_jvm_external~~maven~maven//:org_checkerframework_checker_qual
@@rules_jvm_external~~maven~maven//:org_checkerframework_checker_qual_3_33_0_extension
@@rules_jvm_external~~maven~org_checkerframework_checker_qual_3_33_0//file:file
Binary file modified cli/src/test/resources/fixture/fine-grained-hash-bzlmod-test-1.zip
Binary file not shown.
Binary file modified cli/src/test/resources/fixture/fine-grained-hash-bzlmod-test-2.zip
Binary file not shown.
Loading