You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Add support for incremental annotation processing in Gradle #1120
My understanding is that currently, projects that include annotation processors are unable to be incrementally compiled in Gradle; such projects always trigger a full rebuild. The forthcoming Gradle version 4.7 will contain improvements that will allow for incremental annotation processing.
Presuming Dagger fits into one of the two categories of annotation processor described in the above link, adding the required meta-data to the META-INF directory would likely significantly improve build times.
Even if not directly possible, would it be possible to split the processor into two parts? e.g. one that handles @Inject on constructors (to generate _Factory and other trivial generation, incremental) and another that handles the complex graph building (full regeneration). I might not have deep enough insight, but this should help a lot, if people have modules with constructor injection, and assemble the Dagger graph at top level (:app).
Reacted by Ben Sandee, Maciej Czekański, Dawid Hyży, Artem Hluhovskyi, Andrew Munn, David Medenjak, Chaitanya Pramod, Curtis Kroetsch, burningfireplace, Stojan Anastasov and 1 more
This is what we plan to do for toothpick, an alternative DI lib.
We already have 2 different APs.
To support Incap, we will have an AP that is incremental and another one
that is not, but that can actually be optional. So we plan to only use it
for release and not debug. Hence getting a fully incremental DI lib.
Le mer. 18 avr. 2018 07 h 32, Róbert Papp (TWiStErRob) <
notifications@github.com> a écrit :
Even if not directly possible, would it be possible to split the processor
into two parts? e.g. one that handles @Inject on constructors (to
generate _Factory, incremental) and another that handles the complex
graph building (full regeneration). I might not have deep enough insight,
but this should help a lot, if people have modules with constructor
injection, and assemble the Dagger graph at top level (:app).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1120 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABv33dS5c_G6esV1QEck_hqXvdCv2wA9ks5tp06OgaJpZM4S_Nif>
.
@TWiStErRob it would likely be very difficult to split the processors since they the different steps require ordering that would be hard to ensure if they're split.
@stephanenicolas Can I suggest that we add a flag or some other way to test incap without requiring every project in the wild to release a new artifact? I don't think this scales well, and it forces the effort on project maintainers. It would be great if someone could file a bug saying "Hey, I've been using Dagger with Incap and it works great, can you add default support so I don't need to keep this ugly flag in my repo?"
I don't love the idea of releasing an artifact with incap support without knowing that it works.
Can I suggest that we add a flag or some other way to test incap without requiring every project in the wild to release a new artifact? I don't think this scales well, and it forces the effort on project maintainers.
The effort has to be on the project maintainers, because you really have to think about what your processor does. You can't turn a switch in Gradle and say "Let's just pretend this is incremental". That would lead to broken builds and lost trust with users. Afaik @hungvietnguyen is already planning to work on Dagger after he's done with DataBinding.
You shouldn't rely on someone trying it out and saying "it works". There should be automated functional tests ensuring that it works. Functional testing can be done with the Gradle Tooling API.
I don't yet see why anything would have to be split. I'd like to understand why Dagger wouldn't fit the isolating or aggregating category.
Reacted by Andrey Mischenko, TechYourChance and Andrew Reitz
different steps require ordering that would be hard to ensure if they're split.
Do you need the Blah_Factory class to be generated before you start using it? If there's an @Inject on a ctor that class will have a _Factory, right? Couldn't you just assume it'll be there next to Blah by the time the real compilation gets to it? You control both processors, so you can assume that it'll do these steps. I'm sure there's more to it than I see currently, just throwing this out there.
I agree with @oehme that testing is the way to go more than letting users experiments. The amount of complexity to rely on users. And only maintainers can have a good understanding of their AP to declare their behaviour to gradle.
I don't think a release for a flag is so much of an issue, but I believe there is much more fine grained understanding of incap necessary to flag the AP properly.
Though, about testing, I don't like so much the idea of integration tests, because they can slow down running tests and also use a non standard testing framework. A better solution would be to modify the google testing lib that most APs use for unit tessting to check that the right elements are passed to the filer when creating files. This seems to be doable and would provide very little change to existing tests to make sure incremental behaviors are working fine.
I am not too sure about the current status of this issue, but it could be a key to faster tests of incremental annotation processing: google/compile-testing#58
Not sure what you mean by "non-standard" testing framework, you can use JUnit just like you would for any other test and call Gradle through the Tooling API. End-to-end tests are key to ensure that this actually works. Unit tests are of course better to test all kinds of corner cases. But if I had to choose a place to start, end-to-end always get my recommendation.
Reacted by Said Tahsin Dane, Andrew Reitz and Arunkumar
@oehme@stephanenicolas that's fine and your opinion is totally valid, but I think it's unlikely for project authors to write integration tests for a particular build system's compilation strategy. I don't think we'd ever trust a single voice saying that it's working, but if a larger group gives it a try and can all verify, that's a strong signal. There are many build systems available, and taking away the effort from project maintainers can help features take hold.
We can't take that effort away from the maintainer, they actually have to know what the processor is doing and ensure that complies with the constraints mentioned in the incremental annotation processing documentation. We put in a lot of work to make it as transparent as possible (i.e. no new APIs), but there are still lots of things a badly behaved processor could do to break it. Hence the opt in which only someone with deep knowledge like the maintainer can do.
This is the highest voted issue on both our and your issue tracker. For many Android users this is the only thing standing between them and fast incremental builds. How can I help move this forward?
Reacted by Stojan Anastasov, Milos Marinkovic and Andrew Reitz
I have reread the docs over and over... and I still don't understand what would make an annotation processor aggregating. All I know is what it can't do.
If Bar is changed to no longer implement RandomAccess, does that cause every other file to be recompiled? Or no because RandomAccess has no methods and/or Bar has no annotations that Dagger declares?
If I add the @javax.inject.Scope annotation to @MaybeScoped, do the other files get recompiled?
JSR 330 states that all scopes must be RUNTIME retention, but that's not enforced by dagger (we operate on whatever we can see). We don't declare javax.inject.Scope as an annotation that we process over because it's a meta-annotation.
Would this functionality still be considered in "aggregating"?
It should be fine to read the options before you say which you support. The latter is only there so javac can warn when an option was not used by anyone.
Reacted by Łukasz Wasylkowski, Giovanni Longatto Nazario Marques, Tony Robalik, Rooz Mohazzabi, Paul Merlin, Juan Diana, Yenchi Lin, Donát Csikós, Artur Artikov, Jeremy Tecson and 13 more
@eleventigers Is the aforementioned code block necessary for dagger version, 2.23.1? Currently, when scanning for dependencies that do not support incremental annotation processing without the code block, I do not get any warnings.
Reacted by cfl777, Luis Cortes, Luca Spinazzola and Yaroslav Berezanskyi
@eleventigers Is the aforementioned code block necessary for dagger version, 2.23.1? Currently, when scanning for dependencies that do not support incremental annotation processing without the code block, I do not get any warnings.
Activity
tbroyer commentedon Mar 28, 2018
I'm actually 99.9% certain Dagger does not fit. @stephanenicolas?
TWiStErRob commentedon Apr 18, 2018
Even if not directly possible, would it be possible to split the processor into two parts? e.g. one that handles
@Injecton constructors (to generate_Factoryand other trivial generation, incremental) and another that handles the complex graph building (full regeneration). I might not have deep enough insight, but this should help a lot, if people have modules with constructor injection, and assemble the Dagger graph at top level (:app).stephanenicolas commentedon Apr 19, 2018
oehme commentedon Apr 25, 2018
Why would Dagger not fit, can you please elaborate on that?
ronshapiro commentedon Apr 26, 2018
@TWiStErRob it would likely be very difficult to split the processors since they the different steps require ordering that would be hard to ensure if they're split.
@stephanenicolas Can I suggest that we add a flag or some other way to test incap without requiring every project in the wild to release a new artifact? I don't think this scales well, and it forces the effort on project maintainers. It would be great if someone could file a bug saying "Hey, I've been using Dagger with Incap and it works great, can you add default support so I don't need to keep this ugly flag in my repo?"
I don't love the idea of releasing an artifact with incap support without knowing that it works.
oehme commentedon Apr 26, 2018
The effort has to be on the project maintainers, because you really have to think about what your processor does. You can't turn a switch in Gradle and say "Let's just pretend this is incremental". That would lead to broken builds and lost trust with users. Afaik @hungvietnguyen is already planning to work on Dagger after he's done with DataBinding.
You shouldn't rely on someone trying it out and saying "it works". There should be automated functional tests ensuring that it works. Functional testing can be done with the Gradle Tooling API.
I don't yet see why anything would have to be split. I'd like to understand why Dagger wouldn't fit the isolating or aggregating category.
TWiStErRob commentedon Apr 26, 2018
Do you need the
Blah_Factoryclass to be generated before you start using it? If there's an@Injecton a ctor that class will have a_Factory, right? Couldn't you just assume it'll be there next toBlahby the time the real compilation gets to it? You control both processors, so you can assume that it'll do these steps. I'm sure there's more to it than I see currently, just throwing this out there.stephanenicolas commentedon Apr 27, 2018
I agree with @oehme that testing is the way to go more than letting users experiments. The amount of complexity to rely on users. And only maintainers can have a good understanding of their AP to declare their behaviour to gradle.
I don't think a release for a flag is so much of an issue, but I believe there is much more fine grained understanding of incap necessary to flag the AP properly.
Though, about testing, I don't like so much the idea of integration tests, because they can slow down running tests and also use a non standard testing framework. A better solution would be to modify the google testing lib that most APs use for unit tessting to check that the right elements are passed to the filer when creating files. This seems to be doable and would provide very little change to existing tests to make sure incremental behaviors are working fine.
I am not too sure about the current status of this issue, but it could be a key to faster tests of incremental annotation processing: google/compile-testing#58
oehme commentedon Apr 27, 2018
Not sure what you mean by "non-standard" testing framework, you can use JUnit just like you would for any other test and call Gradle through the Tooling API. End-to-end tests are key to ensure that this actually works. Unit tests are of course better to test all kinds of corner cases. But if I had to choose a place to start, end-to-end always get my recommendation.
ronshapiro commentedon Apr 27, 2018
@oehme @stephanenicolas that's fine and your opinion is totally valid, but I think it's unlikely for project authors to write integration tests for a particular build system's compilation strategy. I don't think we'd ever trust a single voice saying that it's working, but if a larger group gives it a try and can all verify, that's a strong signal. There are many build systems available, and taking away the effort from project maintainers can help features take hold.
oehme commentedon Apr 27, 2018
We can't take that effort away from the maintainer, they actually have to know what the processor is doing and ensure that complies with the constraints mentioned in the incremental annotation processing documentation. We put in a lot of work to make it as transparent as possible (i.e. no new APIs), but there are still lots of things a badly behaved processor could do to break it. Hence the opt in which only someone with deep knowledge like the maintainer can do.
oehme commentedon May 5, 2018
This is the highest voted issue on both our and your issue tracker. For many Android users this is the only thing standing between them and fast incremental builds. How can I help move this forward?
ronshapiro commentedon May 6, 2018
I have reread the docs over and over... and I still don't understand what would make an annotation processor aggregating. All I know is what it can't do.
What happens in this case?
If
Baris changed to no longer implementRandomAccess, does that cause every other file to be recompiled? Or no becauseRandomAccesshas no methods and/orBarhas no annotations that Dagger declares?ronshapiro commentedon May 6, 2018
Or, what about this:
If I add the
@javax.inject.Scopeannotation to@MaybeScoped, do the other files get recompiled?JSR 330 states that all scopes must be
RUNTIMEretention, but that's not enforced by dagger (we operate on whatever we can see). We don't declarejavax.inject.Scopeas an annotation that we process over because it's a meta-annotation.Would this functionality still be considered in "aggregating"?
tasomaniac commentedon May 6, 2018
I wonder if these question can be answered by setting up a Gradle functional test suite?
12 remaining items
oehme commentedon Aug 29, 2018
It should be fine to read the options before you say which you support. The latter is only there so javac can warn when an option was not used by anyone.
Apart from that the PR looks 👍
Add support for gradle's incremental annotation processing
Report originating elements to the Filer
Add support for gradle's incremental annotation processing
Report originating elements to the Filer
Add support for gradle's incremental annotation processing
Add support for gradle's incremental annotation processing
Add support for gradle's incremental annotation processing
autonomousapps commentedon Oct 18, 2018
@ronshapiro just wondering when we might expect a release with this feature?
ronshapiro commentedon Oct 19, 2018
Thanks for the reminder. Just released 2.18.
wbervoets commentedon May 17, 2019
Is this the correct way to pass this to Dagger from Gradle?:
android { defaultConfig { javaCompileOptions { annotationProcessorOptions { arguments ["-Adagger.gradle.incremental", "-Adagger.formatGeneratedSource=disabled"] } } }eleventigers commentedon May 17, 2019
@wbervoets to support both Kotlin and Java I have it configured (with other options) as:
kartikisharma-qz commentedon Jun 19, 2019
@eleventigers Is the aforementioned code block necessary for dagger version, 2.23.1? Currently, when scanning for dependencies that do not support incremental annotation processing without the code block, I do not get any warnings.
kahakai commentedon Sep 18, 2019
It's enabled by default since Dagger 2.24.