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

Migrate Glide from kapt to ksp, and remove kapt from project #13381

Closed
wants to merge 5 commits into from

Conversation

jamesonwilliams
Copy link
Contributor

@jamesonwilliams jamesonwilliams commented Jan 24, 2024

Contributor checklist

  • Virtual device: Emulator, Pixel 7 API 34
  • My contribution is fully baked and ready to be merged as is
  • I ensure that all the open issues my contribution fixes are mentioned in the commit message of my first commit using the Fixes #1234 syntax

Description

The goal of this PR is to replace kapt with ksp across the project. kapt is a legacy tool that slows down the build process.

The single user of kapt in the codebase right now is Glide. Glide supports KSP, but the KSP support does not extend to the deprecated Glide Code Generation API. So, the bulk of this PR is in migrating to the new Glide APIs as per the table below:

Old Code Gen API New API Remark
GlideApp Glide GlideApp was a code-generated class, via kapt
GlideRequests RequestManager RequestManager is a new return type of Glide
GlideRequest RequestBuilder RequestBuilder is a new return type of RequestManager

The work is broken up into three key commits:

Commit sequence Commit hash Description
1 010285d Migrate away from deprecated code-generation APIs
2 84e86f7 Remove kapt from this project
3 c4f89b3 Add ksp support into the project

@jamesonwilliams jamesonwilliams changed the title Remove kapt Migrate deprecated Glide APIs & Remove all kapt from project Jan 24, 2024
@greyson-signal
Copy link
Contributor

Thank you for this! Unfortunately it seems like this breaks reading pre-existing images. I believe this is because while the codegen stuff you linked to is deprecated, Glide still does officially use annotations for configuration. And we have lots of configuration stuff for storing images encrypted on disk and such.

Glide has a ksp annotation processor, so I think we'd want to switch to that. I was messing around with it for a bit but wasn't quite able to get it to work and I didn't want to sink too much time into it, but I think that'd be the next step.

@jamesonwilliams
Copy link
Contributor Author

Thank you for this! Unfortunately it seems like this breaks reading pre-existing images. I believe this is because while the codegen stuff you linked to is deprecated, Glide still does officially use annotations for configuration. And we have lots of configuration stuff for storing images encrypted on disk and such.

Glide has a ksp annotation processor, so I think we'd want to switch to that. I was messing around with it for a bit but wasn't quite able to get it to work and I didn't want to sink too much time into it, but I think that'd be the next step.

Hey, @greyson-signal - thanks for checking this out! My original version of this PR did do the kapt -> ksp update, and I think I can add the ksp bit back in fairly easily. I had missed the annotations in the codebase. I do now see a couple of @GlideModule being used.

To align with you, what test did you perform to validate image functionality? I can do the same. I had run the image-editor-app sample app, but that may not have been enough.

@jamesonwilliams jamesonwilliams changed the title Migrate deprecated Glide APIs & Remove all kapt from project Migrate Glide from kapt to ksp, and remove kapt from project Jan 24, 2024
@jamesonwilliams
Copy link
Contributor Author

jamesonwilliams commented Jan 24, 2024

Thank you for this! Unfortunately, it seems like this breaks reading pre-existing images.

I was able to confirm this; simply removing kapt does break pre-existing images. To test this, I sent myself a picture on a vanilla build and then installed the "removes kapt" build. When running that build, it looks like this - not good:

But, adding ksp back into the project fixes the problem. When I then installed the ksp-enabled build, I can see the source image as expected, again:

@greyson-signal
Copy link
Contributor

This is great! I verified it works as well, thank you very much! Had a bit of a facepalm as well because I realized I failed to get ksp working earlier because I had accidentally added it to the app module instead of glide-config :p

This will go out in the 6.46 release 👍

@greyson-signal
Copy link
Contributor

(Also don't worry about the CI failure, I fixed that in the 6.46 branch)

./gradlew qa --parallel passes locally now.
@jamesonwilliams
Copy link
Contributor Author

(Also don't worry about the CI failure, I fixed that in the 6.46 branch)

@greyson-signal Ah you know what, I'm pretty sure this build error is my fault. (Sorry, I've just realized I should be running ./gradlew qa to validate my work locally before pushing.)

Fixed in a34bd86

(Incidentally, I have to apply a small patch (#13382) to get the qa target to succeed at all on my ARM MacBook)

TwoLeaves pushed a commit to TwoLeaves/Signal-antisocial that referenced this pull request Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants