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 #1120

Closed
wrotte opened this issue Mar 28, 2018 · 33 comments

Comments

@wrotte
Copy link

commented Mar 28, 2018

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.

https://docs.gradle.org/nightly/userguide/java_plugin.html#sec: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.

@tbroyer

This comment has been minimized.

Copy link

commented Mar 28, 2018

I'm actually 99.9% certain Dagger does not fit. @stephanenicolas?

@TWiStErRob

This comment has been minimized.

Copy link

commented Apr 18, 2018

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).

@stephanenicolas

This comment has been minimized.

Copy link

commented Apr 19, 2018

@oehme

This comment has been minimized.

Copy link

commented Apr 25, 2018

Why would Dagger not fit, can you please elaborate on that?

@ronshapiro

This comment has been minimized.

Copy link

commented 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

This comment has been minimized.

Copy link

commented Apr 26, 2018

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.

@TWiStErRob

This comment has been minimized.

Copy link

commented Apr 26, 2018

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.

@stephanenicolas

This comment has been minimized.

Copy link

commented 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

This comment has been minimized.

Copy link

commented 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

This comment has been minimized.

Copy link

commented 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

This comment has been minimized.

Copy link

commented 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

This comment has been minimized.

Copy link

commented 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

This comment has been minimized.

Copy link

commented 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?

class Foo implements Bar {
  @Inject Foo() {}
}

interface Bar extends RandomAccess {}

@Module
abstract class FooModule {
  @Binds abstract RandomAccess from(Foo foo);
}

@Component(modules = FooModule.class)
interface TestComponent {
  RandomAccess r();
}

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?

@ronshapiro

This comment has been minimized.

Copy link

commented May 6, 2018

Or, what about this:

@Retention(SOURCE)
@interface MaybeScoped {}

@MaybeScoped
class Blue {
  @Inject Blue {}
}

@Component
@MaybeScoped
interface BlueComponent {
  Blue blue();
}  

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"?

@tasomaniac

This comment has been minimized.

Copy link

commented May 6, 2018

I wonder if these question can be answered by setting up a Gradle functional test suite?

@ronshapiro

This comment has been minimized.

Copy link

commented May 6, 2018

@tasomaniac we care not only that they work now, because that may be by accident. We don't want to rely on aspects of the implementation that aren't in the spec.

@oehme

This comment has been minimized.

Copy link

commented May 6, 2018

@ronshapiro Dagger doesn't look aggregating to me, but isolating, which is more efficient.

As far as I can see, you don't create a module by searching the round environment for possible implementors of an interface (that would be aggregating), but instead the user has to put the implementation class right there in the Module's signature. So the @Module-annotated class is the sole entry point for the generated module implementation and everything else is reachable from its AST. That's the definition of isolating.

The first example is simple:

  • Bar is changed, so Bar will be recompiled
  • Foo references Bar, so Foo will be recompiled
  • FooModule references Foo, so FooModule will be recompiled
  • TestComponent references FooModule, so TestComponent will be recompiled

The second example is not much harder:

  • Changes to source rentention annotations lead to a full recompile (because we can't find them in the byte code and we don't yet use a compiler plugin to collect that data. This may become more efficient in the future.)
  • Changes to class/runtime retention annotations follow the same rules as every other class - A class is recompiled if it was changed or if it transitively references a changed class

Both of these examples are not specific to annotation processing, they are just about basic correctness of incremental compilation. The incremental processing documentation only mentions what you can't do in processors, because those are the only restrictions. You can do everything else and rely on a correct incremental compiler in Gradle.

@ronshapiro

This comment has been minimized.

Copy link

commented May 6, 2018

Ok, thanks for that explanation @oehme! I definitely did not get that from the docs; "These look at each annotated element in isolation" didn't give me the sense that these actions could be chained.

I think the safest thing is to still probably have a separate artifact that mirrors dagger-compiler (maybe com.google.dagger:dagger-compiler-experimental-incap:<version?) before we release this to the masses. We can advertise that users can try it out and report any issues, and assuming we don't hear problems back then we can remove that artifact and merge things into the main one.

SGTY?

@oehme

This comment has been minimized.

Copy link

commented May 7, 2018

The chaining (or transitive) aspect is explained in the chapter on incremental compilation above the one about incremental annotation processing. That's because it's not specific to annotation processing, but necessary for correct incremental compilation in general.

Actually you might not even need a separate artifact, because in Gradle 4.8 the registration mechanism was changed to be based on processor options. So your processor can now dynamically answer whether it is incremental or not. That means you could check a system property and tell your users to set that property if they want to try it out.

@tbroyer

This comment has been minimized.

Copy link

commented May 8, 2018

@oehme What about the following case: based on @ronshapiro's above (#1120 (comment)), let's assume I have some Baz class with an @Inject constructor already; what if I add an @Inject Baz baz; field to the Foo class?

Nothing yet references that field, will Gradle recompile anything besides Foo?

For Dagger to work correctly, the TestComponent would have to be reprocessed (not necessarily recompiled), how would Gradle know that it has to do it? And more importantly do you guarantee that it'll do so? (particularly as, in case everything about Foo is generated inline in the DaggerTestComponent whose single originatingElement would be TestComponent; nothing would reference Foo as an originatingElement)

@ronshapiro

This comment has been minimized.

Copy link

commented May 8, 2018

@tbroyer my understanding of what @oehme said is that:

  • Foo being compiled causes FooModule, to be recompiled because it references Foo, and Foo's ABI has changed.
  • FooModule being recompiled causes TestComponent to be recompiled because TestComponent references FooModule, whose ABI has also changed.
@oehme

This comment has been minimized.

Copy link

commented May 8, 2018

@tbroyer Ron is correct. You always need to take into account that Gradle already has a fully working incremental compiler. TestComponent transitively references Foo, so it would have to be recompiled even if we take Dagger completely out of the picture.

The originatingElement is just to let us know the connection between TestComponent and DaggerTestComponent, so we can delete the latter if the former is deleted.

@lwasylkowski

This comment has been minimized.

Copy link

commented Aug 2, 2018

What is the status of the issue? Is this something the community can help with, is it still in consideration or it's actively worked on (or in the backlog)? Is there any kind of estimate here? I wanted to bump this up since Dagger is usually one of the relatively heavier annotation processors in Android projects

@ronshapiro

This comment has been minimized.

Copy link

commented Aug 2, 2018

I think we just need to have someone implement the switch that @oehme mentioned in #1120 (comment) (as long as gradle/gradle#5498 (comment) is also in, which I presume it is)

@oehme

This comment has been minimized.

Copy link

commented Aug 2, 2018

Yes, you could register Dagger as a dynamic processor and then have a feature toggle (-Aincremental.dagger=true) that controls whether Dagger will report itself as incremental or non-incremental.

ronshapiro added a commit that referenced this issue Aug 29, 2018
@ronshapiro

This comment has been minimized.

Copy link

commented Aug 29, 2018

@oehme I think we need a system property and not a -A flag because that will require reading the supported options before we report back what options are supported.

I've made a quick PR to implement this in #1266, does that look reasonable?

@oehme

This comment has been minimized.

Copy link

commented 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 👍

ronshapiro added a commit that referenced this issue Sep 7, 2018
Add support for gradle's incremental annotation processing
Fixes #1120

RELNOTES=Add support for gradle's incremental annotation processing. This can be enabled by passing `-Adagger.gradle.incremental` to `javac`

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=211815034
ronshapiro added a commit that referenced this issue Sep 7, 2018
Report originating elements to the Filer
For certain types (like @ContributesAndroidInjector generated types, which don't refer to their originating element) this can be critical for implementing an incremental processor. See #1120

RELNOTES=Originating elements are now reported to the `Filer`

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=211809425
ronshapiro added a commit that referenced this issue Sep 7, 2018
Add support for gradle's incremental annotation processing
Fixes #1120

RELNOTES=Add support for gradle's incremental annotation processing. This can be enabled by passing `-Adagger.gradle.incremental` to `javac`

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=211815034
@ronshapiro ronshapiro referenced this issue Sep 7, 2018
ronshapiro added a commit that referenced this issue Sep 12, 2018
Report originating elements to the Filer
For certain types (like @ContributesAndroidInjector generated types, which don't refer to their originating element) this can be critical for implementing an incremental processor. See #1120

RELNOTES=Originating elements are now reported to the `Filer`

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=211809425
ronshapiro added a commit that referenced this issue Sep 12, 2018
Add support for gradle's incremental annotation processing
Fixes #1120

RELNOTES=Add support for gradle's incremental annotation processing. This can be enabled by passing `-Adagger.gradle.incremental` to `javac`

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=211815034
ronshapiro added a commit that referenced this issue Sep 12, 2018
Add support for gradle's incremental annotation processing
Fixes #1120

RELNOTES=Add support for gradle's incremental annotation processing. This can be enabled by passing `-Adagger.gradle.incremental` to `javac`

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=211815034
@ronshapiro ronshapiro referenced this issue Sep 12, 2018
ronshapiro added a commit that referenced this issue Sep 12, 2018
Add support for gradle's incremental annotation processing
Fixes #1120

RELNOTES=Add support for gradle's incremental annotation processing. This can be enabled by passing `-Adagger.gradle.incremental` to `javac`

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=211815034
@autonomousapps

This comment has been minimized.

Copy link

commented Oct 18, 2018

@ronshapiro just wondering when we might expect a release with this feature?

@ronshapiro

This comment has been minimized.

Copy link

commented Oct 19, 2018

Thanks for the reminder. Just released 2.18.

@wbervoets

This comment has been minimized.

Copy link

commented 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

This comment has been minimized.

Copy link

commented May 17, 2019

@wbervoets to support both Kotlin and Java I have it configured (with other options) as:

// Enables Dagger fastInit mode, which reduces component building latency by loading less classes,
// see: https://google.github.io/dagger/compiler-options.html#fastinit-mode
def optionFastInitEnabled = '-Adagger.fastInit=enabled'
// Disables Dagger code formatting to speed builds,
// see: https://google.github.io/dagger/compiler-options.html#turning-off-code-formatting
def optionFormattingDisabled = '-Adagger.formatGeneratedSource=disabled'
// Enabled incremental annotation processing support, since Kotlin 1.3.30
// See: https://kotlinlang.org/docs/reference/kapt.html#incremental-annotation-processing-since-1330
def argumentIncremental = 'dagger.gradle.incremental'

subprojects {
    pluginManager.withPlugin('kotlin-kapt') {
        kapt {
            javacOptions {
                option(optionFastInitEnabled)
                option(optionFormattingDisabled)
            }
            arguments {
                arg(argumentIncremental, 'true')
            }
        }
    }

    afterEvaluate {
        tasks.withType(JavaCompile.class) {
            options.compilerArgs << optionFastInitEnabled << optionFormattingDisabled
        }

        if (pluginManager.hasPlugin('com.android.library') || pluginManager.hasPlugin('com.android.application')) {
            android {
                defaultConfig {
                    javaCompileOptions {
                        annotationProcessorOptions {
                            arguments[argumentIncremental] = 'true'
                        }
                    }
                }
            }
        }
    }
}
@kartikisharma-qz

This comment has been minimized.

Copy link

commented 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.

@artnest

This comment has been minimized.

Copy link

commented Sep 18, 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.

It's enabled by default since Dagger 2.24.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.