Skip to content

Fix transitive proto_library dependencies for scala_proto_library #368

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.

Sign up for GitHub

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

Open
evis wants to merge 1 commit into
base: 252
Choose a base branch
from
Open

Fix transitive proto_library dependencies for scala_proto_library #368

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
110 changes: 96 additions & 14 deletions .../main/kotlin/org/jetbrains/bazel/sync/workspace/mapper/normal/AspectBazelProjectMapper.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,11 +129,11 @@ class AspectBazelProjectMapper(
}
val interfacesAndBinariesFromTargetsToImport =
measure("Collect interfaces and classes from targets to import") {
collectInterfacesAndClasses(targetsToImport)
collectInterfacesAndClasses(targetsToImport, allTargets)
Copy link
Author

@evis evis Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it OK to pass allTargets through several method calls like this?

}
val outputJarsLibraries =
measure("Create output jars libraries") {
calculateOutputJarsLibraries(targetsToImport)
calculateOutputJarsLibraries(targetsToImport, allTargets)
}
val annotationProcessorLibraries =
measure("Create AP libraries") {
Expand Down Expand Up @@ -163,7 +163,7 @@ class AspectBazelProjectMapper(
}
val librariesFromDepsAndTargets =
measure("Libraries from targets and deps") {
createLibraries(targetsAsLibraries, repoMapping) +
createLibraries(targetsAsLibraries, repoMapping, allTargets) +
librariesFromDeps.values
.flatten()
.distinct()
Expand All @@ -176,6 +176,7 @@ class AspectBazelProjectMapper(
librariesFromDeps,
librariesFromDepsAndTargets,
interfacesAndBinariesFromTargetsToImport,
allTargets,
)
}
val librariesToImport =
Expand Down Expand Up @@ -256,6 +257,7 @@ class AspectBazelProjectMapper(

private fun calculateOutputJarsLibraries(
targetsToImport: Sequence<TargetInfo>,
allTargets: Map<Label, TargetInfo>,
): Map<Label, List<Library>> =
targetsToImport
.filter { shouldCreateOutputJarsLibrary(it) }
Expand All @@ -265,6 +267,7 @@ class AspectBazelProjectMapper(
target,
onlyOutputJars = true,
isInternalTarget = true,
allTargets,
)?.let { library ->
target.label() to listOf(library)
}
Expand Down Expand Up @@ -456,9 +459,10 @@ class AspectBazelProjectMapper(
libraryDependencies: Map<Label, List<Library>>,
librariesToImport: Map<Label, Library>,
interfacesAndBinariesFromTargetsToImport: Map<Label, Set<Path>>,
allTargets: Map<Label, TargetInfo>,
): Map<Label, List<Library>> {
val targetsToJdepsJars =
getAllJdepsDependencies(targetsToImport, libraryDependencies, librariesToImport)
getAllJdepsDependencies(targetsToImport, libraryDependencies, librariesToImport, allTargets)
val libraryNameToLibraryValueMap = HashMap<Label, Library>()
return targetsToJdepsJars.mapValues { target ->
val interfacesAndBinariesFromTarget =
Expand Down Expand Up @@ -490,6 +494,7 @@ class AspectBazelProjectMapper(
targetsToImport: Map<Label, TargetInfo>,
libraryDependencies: Map<Label, List<Library>>,
librariesToImport: Map<Label, Library>,
allTargets: Map<Label, TargetInfo>,
): Map<Label, Set<Path>> {
val jdepsJars =
withContext(Dispatchers.IO) {
Expand Down Expand Up @@ -521,6 +526,7 @@ class AspectBazelProjectMapper(
librariesToImport,
outputJarsFromTransitiveDepsCache,
allJdepsJars,
allTargets,
)
targetLabel to jarsFromJdeps - transitiveJdepsJars
}
Expand All @@ -537,10 +543,11 @@ class AspectBazelProjectMapper(
librariesToImport: Map<Label, Library>,
outputJarsFromTransitiveDepsCache: ConcurrentHashMap<Label, Set<Path>>,
allJdepsJars: Set<Path>,
allTargets: Map<Label, TargetInfo>,
): Set<Path> =
outputJarsFromTransitiveDepsCache.getOrPut(targetOrLibrary) {
val jarsFromTargets =
targetsToImport[targetOrLibrary]?.let { getTargetOutputJarsSet(it) + getTargetInterfaceJarsSet(it) }.orEmpty()
targetsToImport[targetOrLibrary]?.let { getTargetOutputJarsSet(it, allTargets) + getTargetInterfaceJarsSet(it) }.orEmpty()
val jarsFromLibraries =
librariesToImport[targetOrLibrary]?.let { it.outputs + it.interfaceJars }.orEmpty()
val outputJars =
Expand All @@ -563,6 +570,7 @@ class AspectBazelProjectMapper(
librariesToImport,
outputJarsFromTransitiveDepsCache,
allJdepsJars,
allTargets,
)
}
outputJars
Expand Down Expand Up @@ -630,7 +638,11 @@ class AspectBazelProjectMapper(
)
}

private suspend fun createLibraries(targets: Map<Label, TargetInfo>, repoMapping: RepoMapping): Map<Label, Library> =
private suspend fun createLibraries(
targets: Map<Label, TargetInfo>,
repoMapping: RepoMapping,
allTargets: Map<Label, TargetInfo>,
): Map<Label, Library> =
withContext(Dispatchers.Default) {
targets
.map { (targetId, targetInfo) ->
Expand All @@ -640,6 +652,7 @@ class AspectBazelProjectMapper(
targetInfo = targetInfo,
onlyOutputJars = false,
isInternalTarget = isTargetTreatedAsInternal(targetId.assumeResolved(), repoMapping),
allTargets = allTargets,
)?.let { library ->
targetId to library
}
Expand All @@ -654,8 +667,9 @@ class AspectBazelProjectMapper(
targetInfo: TargetInfo,
onlyOutputJars: Boolean,
isInternalTarget: Boolean,
allTargets: Map<Label, TargetInfo>,
): Library? {
val outputs = getTargetOutputJarPaths(targetInfo) + getIntellijPluginJars(targetInfo)
val outputs = getTargetOutputJarPaths(targetInfo, allTargets) + getIntellijPluginJars(targetInfo)
val sources = getSourceJarPaths(targetInfo)
val interfaceJars = getTargetInterfaceJarsSet(targetInfo).toSet()
val dependencies: List<BspTargetInfo.Dependency> = if (!onlyOutputJars) targetInfo.dependenciesList else emptyList()
Expand Down Expand Up @@ -700,8 +714,8 @@ class AspectBazelProjectMapper(

private fun List<FileLocation>.resolvePaths() = map { bazelPathsResolver.resolve(it) }.toSet()

private fun getTargetOutputJarPaths(targetInfo: TargetInfo) =
getTargetOutputJarsList(targetInfo)
private fun getTargetOutputJarPaths(targetInfo: TargetInfo, allTargets: Map<Label, TargetInfo>) =
getTargetOutputJarsList(targetInfo, allTargets)
.toSet()

private fun getIntellijPluginJars(targetInfo: TargetInfo): Set<Path> {
Expand All @@ -718,25 +732,93 @@ class AspectBazelProjectMapper(
.flatMap { it.sourceJarsList }
.resolvePaths()

private fun getTargetOutputJarsSet(targetInfo: TargetInfo) = getTargetOutputJarsList(targetInfo).toSet()
private fun getTargetOutputJarsSet(targetInfo: TargetInfo, allTargets: Map<Label, TargetInfo>) =
getTargetOutputJarsList(targetInfo, allTargets).toSet()

private fun getTargetOutputJarsList(targetInfo: TargetInfo) =
targetInfo.jvmTargetInfo.jarsList
private fun getTargetOutputJarsList(targetInfo: TargetInfo, allTargets: Map<Label, TargetInfo>): List<Path> {
val directJars = targetInfo.jvmTargetInfo.jarsList
.flatMap { it.binaryJarsList }
.map { bazelPathsResolver.resolve(it) }

return if (targetInfo.kind == "scala_proto_library") {
directJars + getTransitiveProtoScalapbJars(targetInfo, allTargets)
} else {
directJars
}
}

private fun getTargetInterfaceJarsSet(targetInfo: TargetInfo) = getTargetInterfaceJarsList(targetInfo).toSet()

private fun getTargetInterfaceJarsList(targetInfo: TargetInfo) =
targetInfo.jvmTargetInfo.jarsList
.flatMap { it.interfaceJarsList }
.map { bazelPathsResolver.resolve(it) }

private fun collectInterfacesAndClasses(targets: Sequence<TargetInfo>) =
/**
* Finds scalapb JAR files for transitive proto_library dependencies of a scala_proto_library.
* This ensures that imports from transitive proto dependencies are resolved correctly.
*/
private fun getTransitiveProtoScalapbJars(scalaProtoTarget: TargetInfo, allTargets: Map<Label, TargetInfo>): List<Path> {
val transitiveProtoTargets = findTransitiveProtoLibraries(scalaProtoTarget, allTargets)
return transitiveProtoTargets.mapNotNull { protoTarget ->
findScalapbJarForProtoTarget(protoTarget)
}
}

/**
* Finds all transitive proto_library dependencies of a scala_proto_library target.
* Excludes direct scala_proto_library dependencies to avoid duplicating JARs that are already
* included by the plugin's standard processing.
*/
private fun findTransitiveProtoLibraries(target: TargetInfo, allTargets: Map<Label, TargetInfo>): Set<TargetInfo> {
Copy link
Author

@evis evis Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does extracting the transitive proto_library dependency graph for each scala_proto_library target seem reasonable? Or would you prefer a different approach to fix the original problem?

val visitedTargets = mutableSetOf<Label>()
val protoTargets = mutableSetOf<TargetInfo>()

fun collectTransitiveProtoTargets(currentTarget: TargetInfo) {
val currentLabel = currentTarget.label()
if (currentLabel in visitedTargets) return
visitedTargets.add(currentLabel)

currentTarget.dependenciesList.forEach { dep ->
val depLabel = Label.parse(dep.id)
allTargets[depLabel]?.let { depTarget ->
protoTargets.add(depTarget)
collectTransitiveProtoTargets(depTarget)
}
}
}

target.dependenciesList.forEach { dep ->
val depLabel = Label.parse(dep.id)
allTargets[depLabel]?.let { depTarget ->
if (depTarget.kind == "proto_library") {
collectTransitiveProtoTargets(depTarget)
}
}
}

return protoTargets
}

private fun findScalapbJarForProtoTarget(protoTarget: TargetInfo): Path? {
val protoLabel = protoTarget.label()
val expectedJarName = "${protoLabel.targetName}_scalapb.jar"

val bazelBinPath =
bazelPathsResolver
.workspaceRoot()
.resolve("bazel-bin")
Copy link
Author

@evis evis Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I rely on the bazel-bin symlink here? I noticed that other JAR files are resolved using direct paths.

Should I use a consistent approach? How would you recommend doing it?

.resolve(protoLabel.packagePath.toString())
.resolve(expectedJarName)

return if (bazelBinPath.exists()) bazelBinPath else null
}

private fun collectInterfacesAndClasses(targets: Sequence<TargetInfo>, allTargets: Map<Label, TargetInfo>) =
targets
.associate { target ->
target.label() to
(getTargetInterfaceJarsList(target) + getTargetOutputJarsList(target))
(getTargetInterfaceJarsList(target) + getTargetOutputJarsList(target, allTargets))
.toSet()
}

Expand Down