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

Merged
merged 1 commit into from
Feb 12, 2019

Conversation

technoir42
Copy link
Contributor

I've had to change the retention of GlideModule and GlideExtension to CLASS because aggregating annotation processors support only CLASS and RUNTIME retention.

Resolves #2983
cc @stephanenicolas

@bingranl
Copy link

bingranl commented Feb 6, 2019

Thanks for your PR, Sergey! I agree with you for changing the retention policy and make it aggregating. But I am wondering why are you adding originating element for those generated classes while you already make glide an aggregating annotation processor. As far as I know, aggregating AP doesn't need that and we are not able to know all the originating elements which is why we make it aggregating.

@sjudd
Copy link
Collaborator

sjudd commented Feb 7, 2019

Thanks! I'm working on the build failure, it seems to be an issue with travis setup.

I'm also curious about the originating elements. Isolating annotation processor documentation mentions originating elements: https://docs.gradle.org/current/userguide/java_plugin.html#example_an_isolated_annotation_processor. The aggregating documentation doesn't seem to. I've also found: gradle/gradle#4130

@sjudd
Copy link
Collaborator

sjudd commented Feb 7, 2019

Build error should be fixed if you rebase: b56dbab

@technoir42
Copy link
Contributor Author

I was not sure whether originating elements were required for aggregating AP so I added them anyway. Now, looking at the https://github.com/gradle/gradle/blob/7e2af8a93d4e8f21ff044cc971492717cc14ed7f/subprojects/language-java/src/main/java/org/gradle/api/internal/tasks/compile/processing/AggregatingProcessingStrategy.java#L67 I see that Gradle simply ignores them in case of an aggregating AP.

I've reverted everything related to originating elements and tested again, it seems that it works exactly the same. We can add them later if Gradle ever starts requiring them.

@technoir42
Copy link
Contributor Author

Not sure what's the reason for this failure:

com.bumptech.glide.load.data.HttpUrlFetcherServerTest > testReturnsInputStreamOnStatusOk FAILED
    Wanted but not invoked:
    callback.onDataReady(<Capturing argument>);
    -> at com.bumptech.glide.load.data.HttpUrlFetcherServerTest.testReturnsInputStreamOnStatusOk(HttpUrlFetcherServerTest.java:73)
    However, there were other interactions with this mock:
    -> at com.bumptech.glide.load.data.HttpUrlFetcher.loadData(HttpUrlFetcher.java:65)
        at com.bumptech.glide.load.data.HttpUrlFetcherServerTest.testReturnsInputStreamOnStatusOk(HttpUrlFetcherServerTest.java:73)

I don't see it when running ./gradlew clean build --parallel locally.

@bingranl
Copy link

I can't reproduce the error locally either. Could that be a flaky test? How about we run it again?

@sjudd
Copy link
Collaborator

sjudd commented Feb 12, 2019

I've restarted the job, I believe that is a flake.

Otherwise this looks good to me, I'll merge when I confirm the test passes. Thanks!

@bingranl
Copy link

Great! Thanks!

@sjudd sjudd merged commit a16a1ba into bumptech:master Feb 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants