search

Ibotta / gradle-aspectj-pipeline-plugin

A Gradle plugin for Android projects which performs AspectJ weaving using Android's bytcode manipulation pipeline.
Apache License 2.0
75 stars 15 forks source link

AGP 8.0.0 + Investigation #34

Closed jdvp closed 1 year ago

jdvp commented 1 year ago

Hey guys, I see that the plugin is no longer being maintained, etc. but did have a couple questions

For just a little bit of background info, once I had seen your update I had decided to write an article about why I personally wouldn't use AOP in enterprise apps in 2023 : Why I Don't Recommend Aspect-Oriented Programming in Android in 2023 (let me know if you read it and think it's not right or you have additional points I might add)

Part of the article talks about how support for plugins can is community-driven and how they are prone to break due to AGP or Kotlin changes , which appeared to be the case here. Once I published it, I did want to make sure that the plugin was actually broken to kind of wrap up everything nicely lol

But to my surprise, updating my sample app worked on both AGP 8.0.0 (beta) and AGP 8.1.0 (alpha) without changes to the plugin version.

I figured that maybe the removed Transform API might only be impacting compilation of the actual plugin, so I moved on to try running that. However, it also worked for both AGP 8.0.0 and 8.1.0, although it did require some routine changes such as version bumping for compatibility etc. See the changes required in this sample MR

-- I took a look in the source and realized that the plugin doesn't even appear to use the Transform API that is being removed. This leads me to a couple of questions.

Does it make sense to remove the warning ? It appears that the plugin will 'just work' for at least the next major AGP version and likely beyond. I do understand not wanting to support it from a corporate point of view, especially considering I even just published an article about not using AOP in Android. But, since it's open source maybe it's better to just let it be until issues or a new maintainer comes along?

Documentation on registerPostJavacGeneratedBytecode and related methods in the BaseVariant API is practically non-existent so IDK if/when it would be deprecated but it does appear that some popular libs such as Moshi do appear to use it, so should be alright for a bit? lol

--

Anyway, maybe I am missing something so let me know haha this is also kind of just me rambling etc.

eschlenz commented 1 year ago

This is interesting. It was my understanding that registerPostJavacGeneratedBytecode was part of the Transform API, in some way. This has been our understanding from the beginning, however we never had a need to investigate that further to determine the accuracy. We basically got this project working and, other than maintenance, that was that.

Read your article and here are my thoughts:

  1. Completely agree on the esoteric nature of AOP and the problems that can lead to if, for example, a subject matter expert left a company.
  2. Writing tests for code that leverages AOP was generally very clunky.
  3. There was an excessive need to use reflection within the advice classes, which is generally frowned upon for mobile because of the performance hit.

Our AOP solution originally started off as code that lived in our gradle files. It would tap into certain gradle tasks, like the compile task, to trigger the weaving. It was constantly breaking our gradle cache and/or not re-weaving when we'd expect it to. We were lucky enough to be working with the Gradle team while evaluating Gradle Enterprise. And through their help we recrafted the AOP logic into the plugin you see today. Ever time we've experienced a weird issue with gradle caches, however, there remains this uncertainty as to whether it is our plugin or something else going on. We gained some trust over time, but never fully trusted it.

The lack of documentation on registerPostJavacGeneratedBytecode, which you pointed out, was one of the biggest reasons we didn't have a ton of confidence. Also, Gradle itself I don't feel is super easy to understand for most people. The fact that we even had a somewhat functional solution for AOP prior to working with Gradle experts was kind of a miracle.

Actually, this whole conversation further highlights the cloudiness and uncertainty that exists with registerPostJavacGeneratedBytecode, Gradle, and the Transform API. Documentation is scant.

To your main question, "Does it make sense to remove the warning ?", yeah, probably. Ibotta has moved on from AOP and we won't be maintaining this project any more. As you called out, we are happy to transfer ownership to new maintainers. And if our understanding of the Transform API and its possible affect on registerPostJavacGeneratedBytecode is just wrong, that should be remedied.

I do feel like this project is just a ticking time bomb waiting for one major change in Gradle or AGP that will fully cripple its utility.

jdvp commented 1 year ago

Thanks for the follow up and additional info! That totally makes sense.

And with Kotlin becoming more prevalent and diverging syntax over time, it would require first party support for some new AspeKt lib for me to totally trust it. Like how would you even begin to write a breakpoint for Kotlin Context Receivers?

Thanks for updating the README and for all of the additional context here! Maybe the lib will just work for a while longer and if no new maintainers show up, maybe it really does just mean the paradigm is outdated and Android has outgrown it!