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

The class GeneratedAppGlideModuleImpl generated by ksp does not register the LibraryGlideModule subclass. #5043

Closed
lyh8577 opened this issue Mar 7, 2023 · 6 comments
Labels

Comments

@lyh8577
Copy link

lyh8577 commented Mar 7, 2023

Glide Version:

dependencies {
    implementation("com.github.bumptech.glide:glide:4.15.0")
    ksp("com.github.bumptech.glide:ksp:4.15.0")
    implementation("com.github.bumptech.glide:okhttp3-integration:4.15.0")
}

Integration libraries:

plugins {
    id("org.jetbrains.kotlin.android") version "1.8.10" apply false
    id("com.google.devtools.ksp") version "1.8.10-1.0.9" apply false
}

Device/Android Version:
Android Studio Electric Eel | 2022.1.1 Patch 2

Issue details / Repro steps / Use case background:
Make project,then view the generated classes: GeneratedAppGlideModuleImpl

Kapt generated class

final class GeneratedAppGlideModuleImpl extends GeneratedAppGlideModule {
  private final KaptAppGlideModule appGlideModule;

  public GeneratedAppGlideModuleImpl(Context context) {
    appGlideModule = new KaptAppGlideModule();
    if (Log.isLoggable("Glide", Log.DEBUG)) {
      Log.d("Glide", "Discovered AppGlideModule from annotation: com.example.kaptdemo.KaptAppGlideModule");
      Log.d("Glide", "Discovered LibraryGlideModule from annotation: com.bumptech.glide.integration.okhttp3.OkHttpLibraryGlideModule");
    }
  }

  @Override
  public void applyOptions(@NonNull Context context, @NonNull GlideBuilder builder) {
    appGlideModule.applyOptions(context, builder);
  }

  @Override
  public void registerComponents(@NonNull Context context, @NonNull Glide glide,
      @NonNull Registry registry) {
    new OkHttpLibraryGlideModule().registerComponents(context, glide, registry);
    appGlideModule.registerComponents(context, glide, registry);
  }

  @Override
  public boolean isManifestParsingEnabled() {
    return appGlideModule.isManifestParsingEnabled();
  }

  @Override
  @NonNull
  public Set<Class<?>> getExcludedModuleClasses() {
    return Collections.emptySet();
  }

  @Override
  @NonNull
  GeneratedRequestManagerFactory getRequestManagerFactory() {
    return new GeneratedRequestManagerFactory();
  }
}

Ksp generated class

internal class [GlideDemo.zip](https://github.com/bumptech/glide/files/10907488/GlideDemo.zip)(
  @Suppress("UNUSED_PARAMETER")
  context: Context,
) : GeneratedAppGlideModule() {
  private val appGlideModule: KspAppGlideModule
  init {
    appGlideModule = KspAppGlideModule()
  }

  public override fun registerComponents(
    context: Context,
    glide: Glide,
    registry: Registry,
  ): Unit {
    appGlideModule.registerComponents(context, glide, registry)
  }

  public override fun applyOptions(context: Context, builder: GlideBuilder): Unit {
    appGlideModule.applyOptions(context, builder)
  }

  public override fun isManifestParsingEnabled(): Boolean = false
}

GlideDemo.zip

@sjudd
Copy link
Collaborator

sjudd commented Mar 7, 2023

Any chance you could push the project to a repo on Github rather than attaching a zip file?

@lyh8577
Copy link
Author

lyh8577 commented Mar 8, 2023

@sjudd
Copy link
Collaborator

sjudd commented Mar 8, 2023

Welp that's a pretty significant bug in the ksp library - namely it doesn't read library glide modules compiled with the java annotation processor. All library modules are compiled with the java annotation processor, so effectively ksp does not work with library glide modules.

I'll do a dot release to fix this in the short term.

Really there's no good reason why we need a separate Index annotation just for ksp, it really ought to just use the existing Java one. I'll make that as a separate follow-up change.

@sjudd sjudd added the bug label Mar 8, 2023
@sjudd
Copy link
Collaborator

sjudd commented Mar 8, 2023

Also thank you for filing the issue and attaching the sample app. I appreciate the report.

sjudd added a commit to sjudd/glide that referenced this issue Mar 8, 2023
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.

Unfortunately this is difficult to test. We can (and already have)
verifified across compilations that the KSP processor can correctly
read its own Indexes. However the android and Java plugins in gradle
seem to be incompatible so I can't find a way to use
kotlin-compile-testing to compile the library using the Java processor
and then run the KSP processor on the result. I'll keep looking, but
I'm not super optimistic.
sjudd added a commit to sjudd/glide that referenced this issue Mar 8, 2023
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.

TODO: Add tests before submitting.
sjudd added a commit to sjudd/glide that referenced this issue Mar 8, 2023
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.
sjudd added a commit to sjudd/glide that referenced this issue Mar 8, 2023
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.
sjudd added a commit to sjudd/glide that referenced this issue Mar 8, 2023
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.
@sjudd
Copy link
Collaborator

sjudd commented Mar 8, 2023

Fixed and released in 4.15.1, tests added in 9fd7a84

@sjudd sjudd closed this as completed Mar 8, 2023
@projectdelta6
Copy link

Does the library also need to update?
I'm using https://github.com/qoqa/glide-svg to render SVGs from URLs, but after updating my project to use KSP the SVGs no linger work at all?
I'm using Glide v4.15.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants