Skip to content

Commit

Permalink
Read library glide module names from Java indexes
Browse files Browse the repository at this point in the history
Progress for bumptech#5043

Annotation processors (including ksp) only run on newly compiled code.
Libraries have been previously compiled, so an annotation processor will
not be run on them. To find any LibraryGlideModules included in those
libraries, we look for specific generated classes in those libraries
that we call Indexes. Indexes contain an annotation listing the class
names of any LibraryGlideModules. Indexes are generated by Glide's
annotation processors for each library and are exported as part of the
library.

To preserve the package private visibility of the existing Java Index
annotation, I created a new Index annotation for the KSP processor. This
lets us reference the KSP Index annotation directly.

Unfortunately it also means that the Java and KSP Index classes are not
the same. While it's only a small amount of duplicated code, it's a
significant compatibility issue because the KSP and Java processors no
longer produce the same Index class.

In particular the KSP processor looks only for its Index class and not
for the Java processor's Index class. This is unfortunate because
Glide's libraries are always processed by the Java annotation processor,
not the KSP processor. In turn this means that Glide's KSP processor
effectively ignores any of Glide's integration libraries.

To fix this in the short term I've made the KSP processor look for both
its Indexes and the Java annotation processors Indexes. A more robust
fix would be to merge the two Index processors so that the annotation
processors are mutually compatible. I'll do that in a follow-up.

I've written tests, but they're somewhat involved so I'll send them as
a follow-up.
  • Loading branch information
sjudd committed Mar 8, 2023
1 parent 462416b commit c600e4b
Show file tree
Hide file tree
Showing 3 changed files with 71 additions and 15 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -191,23 +191,58 @@ internal class AppGlideModuleParser(
private fun getIndexesAndLibraryGlideModuleNames(): IndexFilesAndLibraryModuleNames {
val allIndexFiles: MutableList<KSDeclaration> = mutableListOf()
val allLibraryGlideModuleNames: MutableList<String> = mutableListOf()
resolver.getDeclarationsFromPackage(GlideSymbolProcessorConstants.PACKAGE_NAME).forEach {
index: KSDeclaration ->
val libraryGlideModuleNames = extractGlideModulesFromIndexAnnotation(index)
if (libraryGlideModuleNames.isNotEmpty()) {
allIndexFiles.add(index)
allLibraryGlideModuleNames.addAll(libraryGlideModuleNames)
}

val allIndexesAndLibraryModules =
getAllLibraryNamesFromJavaIndexes() + getAllLibraryNamesFromKspIndexes()
for ((index, libraryGlideModuleNames) in allIndexesAndLibraryModules) {
allIndexFiles.add(index)
allLibraryGlideModuleNames.addAll(libraryGlideModuleNames)
}

return IndexFilesAndLibraryModuleNames(allIndexFiles, allLibraryGlideModuleNames)
}

private fun extractGlideModulesFromIndexAnnotation(
internal data class IndexAndLibraryModuleNames(
val index: KSDeclaration, val libraryModuleNames: List<String>
)

private fun getAllLibraryNamesFromKspIndexes(): List<IndexAndLibraryModuleNames> =
getAllLibraryNamesFromIndexes(GlideSymbolProcessorConstants.PACKAGE_NAME) { index ->
extractGlideModulesFromKspIndexAnnotation(index)
}

private fun getAllLibraryNamesFromJavaIndexes(): List<IndexAndLibraryModuleNames> =
getAllLibraryNamesFromIndexes(GlideSymbolProcessorConstants.JAVA_ANNOTATION_PACKAGE_NAME) {
index -> extractGlideModulesFromJavaIndexAnnotation(index)
}

@OptIn(KspExperimental::class)
private fun getAllLibraryNamesFromIndexes(
packageName: String, extractLibraryModuleNamesFromIndex: (KSDeclaration) -> List<String>
) = buildList {
resolver.getDeclarationsFromPackage(packageName)
.forEach { index: KSDeclaration ->
val libraryGlideModuleNames = extractLibraryModuleNamesFromIndex(index)
if (libraryGlideModuleNames.isNotEmpty()) {
environment.logger.info(
"Found index annotation: $index with modules: $libraryGlideModuleNames"
)
add(IndexAndLibraryModuleNames(index, libraryGlideModuleNames))
}
}
}

private fun extractGlideModulesFromJavaIndexAnnotation(
index: KSDeclaration,
): List<String> {
val indexAnnotation: KSAnnotation = index.atMostOneIndexAnnotation() ?: return emptyList()
environment.logger.info("Found index annotation: $indexAnnotation")
val indexAnnotation: KSAnnotation = index.atMostOneJavaIndexAnnotation() ?: return emptyList()
return indexAnnotation.getModuleArgumentValues().toList()
}

private fun extractGlideModulesFromKspIndexAnnotation(
index: KSDeclaration,
): List<String> {
val indexAnnotation: KSAnnotation = index.atMostOneKspIndexAnnotation() ?: return emptyList()
return indexAnnotation.getModuleArgumentValues().toList()
}

Expand All @@ -220,29 +255,36 @@ internal class AppGlideModuleParser(
throw InvalidGlideSourceException("Found an invalid internal Glide index: $this")
}

private fun KSDeclaration.atMostOneIndexAnnotation() = atMostOneAnnotation(Index::class)
private fun KSDeclaration.atMostOneJavaIndexAnnotation() =
atMostOneAnnotation("com.bumptech.glide.annotation.compiler.Index")
private fun KSDeclaration.atMostOneKspIndexAnnotation() = atMostOneAnnotation(Index::class)

private fun KSDeclaration.atMostOneExcludesAnnotation() = atMostOneAnnotation(Excludes::class)

private fun KSDeclaration.atMostOneAnnotation(
annotation: KClass<out Annotation>,
): KSAnnotation? = atMostOneAnnotation(annotation.qualifiedName)

private fun KSDeclaration.atMostOneAnnotation(
annotationQualifiedName: String?,
): KSAnnotation? {
val matchingAnnotations: List<KSAnnotation> =
annotations
.filter {
annotation.qualifiedName?.equals(
annotationQualifiedName?.equals(
it.annotationType.resolve().declaration.qualifiedName?.asString()
)
?: false
}
.toList()
if (matchingAnnotations.size > 1) {
throw InvalidGlideSourceException(
"""Expected 0 or 1 $annotation annotations on $qualifiedName, but found:
"""Expected 0 or 1 $annotationQualifiedName annotations on $qualifiedName, but found:
${matchingAnnotations.size}"""
)
}
return matchingAnnotations.singleOrNull()

}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ object GlideSymbolProcessorConstants {
// This variable is visible only for testing
// TODO(b/174783094): Add @VisibleForTesting when internal is supported.
val PACKAGE_NAME: String = GlideSymbolProcessor::class.java.`package`.name
val JAVA_ANNOTATION_PACKAGE_NAME: String = "com.bumptech.glide.annotation.compiler"
const val SINGLE_APP_MODULE_ERROR = "You can have at most one AppGlideModule, but found: %s"
const val DUPLICATE_LIBRARY_MODULE_ERROR =
"LibraryGlideModules %s are included more than once, keeping only one!"
Expand Down
17 changes: 15 additions & 2 deletions glide/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ static def getAndroidPathsForJavadoc() {
]
}

static def getAndroidPathsForJar() {
[':library', ':third_party:disklrucache', ':third_party:gif_decoder']
}

// The paths of Java projects that should be included only in Javadoc, not in the jar.
static def getJavaPathsForJavadoc() {
[':annotation']
Expand All @@ -43,12 +47,21 @@ def getAndroidProjectsForJavadoc() {
asProjects(getAndroidPathsForJavadoc())
}

def getAndroidLibraryVariantsForJar() {
getAndroidLibraryVariantsForProjects(asProjects(getAndroidPathsForJar()))
}

def getAndroidLibraryVariantsForJavadoc() {
getAndroidProjectsForJavadoc().collect { project ->
getAndroidLibraryVariantsForProjects(getAndroidProjectsForJavadoc())
}

def getAndroidLibraryVariantsForProjects(projects) {
projects.collect { project ->
project.android.libraryVariants.findAll { type ->
type.buildType.name.equalsIgnoreCase("release")
}
}.sum()

}

def getSourceFilesForJavadoc() {
Expand Down Expand Up @@ -114,7 +127,7 @@ javadocJarTask.dependsOn(javadocTask)

jar {
from files(
getAndroidLibraryVariantsForJavadoc().collect { LibraryVariant variant ->
getAndroidLibraryVariantsForJar().collect { LibraryVariant variant ->
variant.getJavaCompileProvider().get().destinationDirectory
}
)
Expand Down

0 comments on commit c600e4b

Please sign in to comment.