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

Add support for incremental annotation processing in Gradle #423

Closed
jeremy-techson opened this issue Apr 18, 2018 · 20 comments · Fixed by #972
Closed

Add support for incremental annotation processing in Gradle #423

jeremy-techson opened this issue Apr 18, 2018 · 20 comments · Fixed by #972

Comments

@jeremy-techson
Copy link

With the release of Gradle 4.7, it now supports incremental annotation processing.

I'm really not sure if Epoxy falls into the two categories supported but assuming it is, it would likely help improve build times.

@elihart
Copy link
Contributor

elihart commented Apr 18, 2018

Thanks for bringing this up! It has been on my radar and I would love to support it.

Some notes:

In the common cases Epoxy is an isolated processor

  • Generates one model file per class with @ModelView or @EpoxyModelClass/EpoxyAttribute

At other times Epoxy is aggregating

  • The kotlin extensions for a package are created by looking at all models and writing a single file for them. We could potentially break this into multiple files to make it isolated
  • EpoxyController's @AutoModel is mostly isolated, but it is a bit of a hack because the model types it depends on aren't available until after annotation processing so it does some reflection to monitor that process

Other places Epoxy breaks the rules for incremental:

  • com.sun.source.util.Trees is used to get the name of layout resources in annotation params (such as view holder layouts)
  • Databinding is a bit of a hacky case as well since the generated binding class is looked up reflectively.

In summary, it looks like this could almost work, but there are some gotchas to sort out and those will slow down the work. It may take a large refactor to make this possible (possibly splitting into separate processors). It's possible we could support just the basic case to start with. I'll add notes to this as I investigate more, but this probably won't happen too soon.

@elihart
Copy link
Contributor

elihart commented Mar 13, 2019

Our team builds with Buck so we have little incentive to prioritize this at the moment. If somebody would like to contribute the change that would be great

@josh-burton
Copy link

Kapt will support incremental annotation processing in the next release. Unfortunately, any annotation processor that is not incremental will disable incremental across all processors.

Definitely need this for Epoxy!

@elihart
Copy link
Contributor

elihart commented Apr 6, 2019

Incremental processing has this limitation

They must not depend on compiler-specific APIs like com.sun.source.util.Trees. Gradle wraps the processing APIs, so attempts to cast to compiler-specific types will fail. If your processor does this, it cannot be incremental, unless you have some fallback mechanism.

Epoxy uses com.sun.source.util.Trees to parse the layout resource ids inside annotations, so from my understanding this makes it ineligible.

However, the upcoming Android change to namespace resources will also make placing resources in annotations impossible, so my understanding is Epoxy will have to be changed to not allow resources in annotations. At the point we make a breaking change to the library to do that we can get rid of com.sun.source.util.Trees usage and support aggregating incremental support

@elihart
Copy link
Contributor

elihart commented Apr 6, 2019

In the meantime, it may be possible to use the "dynamic" mode, where the Epoxy annotation processor can at runtime declare itself as aggregating if none of the annotations it processes need to use com.sun.source.util.Trees

If anybody knows more about this I'd love some opinions on whether that would work.

@DanielNovak
Copy link

Anything new? :-)

@markchristopherng
Copy link

a lot of people will need this to improve build speed

@elihart
Copy link
Contributor

elihart commented Jun 14, 2019

That's right - we still can't use incremental because we are on buck, but if anybody out there wants to contribute a PR to do this please do.

@markchristopherng
Copy link

Your company has another library https://github.com/airbnb/DeepLinkDispatch that is in the same boat but seems a lot less active. I am just wondering if that library is a lot easier to convert than epoxy?

@elihart
Copy link
Contributor

elihart commented Jun 16, 2019

Deeplink dispatch should be very easy in comparison. Its been a while since I looked at the code, but it should be a standard aggregating processor - it might just need to be marked as aggregating with no actual code changes needed

@markchristopherng
Copy link

that would be awesome if you could take a look, very much appreciated

@plnice
Copy link
Contributor

plnice commented Oct 29, 2019

In the meantime, it may be possible to use the "dynamic" mode, where the Epoxy annotation processor can at runtime declare itself as aggregating if none of the annotations it processes need to use com.sun.source.util.Trees

If anybody knows more about this I'd love some opinions on whether that would work.

Something like this was recently done in Butterknife: JakeWharton/butterknife#1546, would it be possible to include a similar solution in Epoxy? I guess that would fix the issue.

@elihart
Copy link
Contributor

elihart commented Oct 29, 2019

@plnice thank you for sharing, that does seem like it would help a lot.

That should get us most of the way there, and seems like it would be fairly simple.

However, I am not sure about how the reflection cases I mentioned in my first comment play out with incremental processing. If those aren't a concern then great! this should be easy.

@kaushalyap
Copy link

Epoxy is awesome!, but the problem is this.

@elihart any plans on this? most people use gradle, although you guys use buck, having this fixed would be a great help!

As @athornz mentioned the problem is this thing disabling other incremental annotation processing.

@elihart
Copy link
Contributor

elihart commented Nov 1, 2019

We will probably get to it in the next few months - if anybody else would like to do it, keeping in mind all the points above, I'd be happy to review.

@ZacSweers
Copy link

I took a shot at this - PR #902

@kaushalyap
Copy link

@ZacSweers Thanks a lot!

@elihart Please look at the mentioned PR.

@markchristopherng
Copy link

@elihart Is there any update on when this will be released?

@emartynov
Copy link

emartynov commented Mar 17, 2020

Just found that Zac deleted his PR with implementation. So release is postponed a bit more.

@elihart
Copy link
Contributor

elihart commented Sep 10, 2020

This has been in beta for a while, and is now fully release in the new 4.0.0

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