Design doc for better/built-in Java annotation processing support #456

Closed
wants to merge 8 commits into
from

Conversation

Projects
None yet
4 participants
@tbroyer
Contributor

tbroyer commented May 15, 2015

Superseeds #431

/cc @alkemist

@@ -0,0 +1,363 @@
+<?xml version="1.0" encoding="UTF-8" standalone="no"?>

This comment has been minimized.

@tbroyer

tbroyer May 15, 2015

Contributor

This image is intended to eventually replace subprojects/docs/src/docs/userguide/img/javaPluginConfigurations.graphml in the user guide.

@tbroyer

tbroyer May 15, 2015

Contributor

This image is intended to eventually replace subprojects/docs/src/docs/userguide/img/javaPluginConfigurations.graphml in the user guide.

@mark-vieira mark-vieira added the cla:yes label Nov 2, 2015

@pioterj pioterj added this to the waiting-for-commiter milestone Nov 12, 2015

@breskeby

This comment has been minimized.

Show comment
Hide comment
@breskeby

breskeby Feb 19, 2016

Member

Hey Thomas,
We finally have the bandwith to deal better with community contributions like this. With 2.12 there will be a 'compileOnly' dependency that can be used for annotation processing. If you're still interested to work on this, please let us know.

Member

breskeby commented Feb 19, 2016

Hey Thomas,
We finally have the bandwith to deal better with community contributions like this. With 2.12 there will be a 'compileOnly' dependency that can be used for annotation processing. If you're still interested to work on this, please let us know.

@tbroyer

This comment has been minimized.

Show comment
Hide comment
@tbroyer

tbroyer Feb 19, 2016

Contributor

With 2.12 there will be a 'compileOnly' dependency that can be used for annotation processing.

Oh great! Looks a lot like what I do in net.ltgt.apt plugin

If you're still interested to work on this, please let us know.

Yes sure. How should we start? Do you want me to first update this PR taking into account the compileOnly/testCompileOnly that are already in? (possibly just marking them as already committed).

Contributor

tbroyer commented Feb 19, 2016

With 2.12 there will be a 'compileOnly' dependency that can be used for annotation processing.

Oh great! Looks a lot like what I do in net.ltgt.apt plugin

If you're still interested to work on this, please let us know.

Yes sure. How should we start? Do you want me to first update this PR taking into account the compileOnly/testCompileOnly that are already in? (possibly just marking them as already committed).

@breskeby

This comment has been minimized.

Show comment
Hide comment
@breskeby

breskeby Feb 19, 2016

Member

For now I would update the design doc to reflect that we have now compileOnly dependencies. The test coverage should also contain integration tests to ensure we don't break anything regarding the current behaviour. Would you mind prefixing the different stories in the spec with Story -. That makes it easier to follow in my opinion. I think we can all agree that having these additional compiler options available for dealing with annotations. I'm just not sure yet if we should have an additional default configuration coming with the java plugin for annotation processors. This brings some overhead to EVERY java projects.
With having compileOnly now available compile time only annotation processing does work with reasonable defaults.
I saw you have also provided a first implementation with #450. I'll comment on this separately.

Member

breskeby commented Feb 19, 2016

For now I would update the design doc to reflect that we have now compileOnly dependencies. The test coverage should also contain integration tests to ensure we don't break anything regarding the current behaviour. Would you mind prefixing the different stories in the spec with Story -. That makes it easier to follow in my opinion. I think we can all agree that having these additional compiler options available for dealing with annotations. I'm just not sure yet if we should have an additional default configuration coming with the java plugin for annotation processors. This brings some overhead to EVERY java projects.
With having compileOnly now available compile time only annotation processing does work with reasonable defaults.
I saw you have also provided a first implementation with #450. I'll comment on this separately.

@tbroyer

This comment has been minimized.

Show comment
Hide comment
@tbroyer

tbroyer Feb 19, 2016

Contributor

Updated, also added a few things (used separate commits to make reviewing changes easier; tell me if you'd prefer I squash commits instead)

I'm just not sure yet if we should have an additional default configuration coming with the java plugin for annotation processors. This brings some overhead to EVERY java projects.

The reason is that you don't want annotation processor dependencies to mess with your project's dependencies, and end up with things that compile but don't run (hopefully fail tests, but that depends on test coverage).
There are annotation processors that look at what's in the classpath too to decide what to do (e.g. Immutables.org would default to using Guava collections if available in the classpath, and JDK collections with Collections.unmodifiableXxx otherwise; if another annotation processor brings Guava, that would affect the behavior of Immutables.org).
This can be avoided by clearly separating the processorpath from the compile classpath.

See google/dagger#228 and google/auto#268

That could probably be left to plugins though (such as android-apt and net.ltgt.apt); in this case, that could still be described here with a note that it won't be implemented, right?

Contributor

tbroyer commented Feb 19, 2016

Updated, also added a few things (used separate commits to make reviewing changes easier; tell me if you'd prefer I squash commits instead)

I'm just not sure yet if we should have an additional default configuration coming with the java plugin for annotation processors. This brings some overhead to EVERY java projects.

The reason is that you don't want annotation processor dependencies to mess with your project's dependencies, and end up with things that compile but don't run (hopefully fail tests, but that depends on test coverage).
There are annotation processors that look at what's in the classpath too to decide what to do (e.g. Immutables.org would default to using Guava collections if available in the classpath, and JDK collections with Collections.unmodifiableXxx otherwise; if another annotation processor brings Guava, that would affect the behavior of Immutables.org).
This can be avoided by clearly separating the processorpath from the compile classpath.

See google/dagger#228 and google/auto#268

That could probably be left to plugins though (such as android-apt and net.ltgt.apt); in this case, that could still be described here with a note that it won't be implemented, right?

@breskeby

This comment has been minimized.

Show comment
Hide comment
@breskeby

breskeby Feb 22, 2016

Member

This looks quite good to me. Yes I think we can just put a note on the stories which we probably no want in gradle core. Two minor remarks before I'll merge your stuff

  1. is there any integration test missing from your point of view? I saw you have a todo in the spec
  2. could you add a dedicated API section about the changes on the compile options. see https://github.com/gradle/gradle/tree/master/design-docs/features/ide-integration/source-and-target-jvm#story---expose-natures-and-builders-for-projects for example. many thanks!
Member

breskeby commented Feb 22, 2016

This looks quite good to me. Yes I think we can just put a note on the stories which we probably no want in gradle core. Two minor remarks before I'll merge your stuff

  1. is there any integration test missing from your point of view? I saw you have a todo in the spec
  2. could you add a dedicated API section about the changes on the compile options. see https://github.com/gradle/gradle/tree/master/design-docs/features/ide-integration/source-and-target-jvm#story---expose-natures-and-builders-for-projects for example. many thanks!
@tbroyer

This comment has been minimized.

Show comment
Hide comment
@tbroyer

tbroyer Feb 23, 2016

Contributor

is there any integration test missing from your point of view? I saw you have a todo in the spec

Well, actually, no. I believe described unit tests would be enough for the rest here (but, well, I also believed they were enough by themselves, without even integration tests, so…)

could you add a dedicated API section about the changes on the compile options. see https://github.com/gradle/gradle/tree/master/design-docs/features/ide-integration/source-and-target-jvm#story---expose-natures-and-builders-for-projects for example. many thanks!

Done.

PTAL

Contributor

tbroyer commented Feb 23, 2016

is there any integration test missing from your point of view? I saw you have a todo in the spec

Well, actually, no. I believe described unit tests would be enough for the rest here (but, well, I also believed they were enough by themselves, without even integration tests, so…)

could you add a dedicated API section about the changes on the compile options. see https://github.com/gradle/gradle/tree/master/design-docs/features/ide-integration/source-and-target-jvm#story---expose-natures-and-builders-for-projects for example. many thanks!

Done.

PTAL

@breskeby

This comment has been minimized.

Show comment
Hide comment
@breskeby

breskeby Feb 23, 2016

Member

Hey,
I merged this pull request manually and did some minor tweaks to the spec. thanks for the contribution!

Member

breskeby commented Feb 23, 2016

Hey,
I merged this pull request manually and did some minor tweaks to the spec. thanks for the contribution!

@breskeby breskeby closed this Feb 23, 2016

@breskeby breskeby self-assigned this Feb 23, 2016

@tbroyer

This comment has been minimized.

Show comment
Hide comment
@tbroyer

tbroyer Feb 23, 2016

Contributor

Thanks. What would be the next step then? Updating #450?

Contributor

tbroyer commented Feb 23, 2016

Thanks. What would be the next step then? Updating #450?

@tbroyer tbroyer deleted the tbroyer:apt-design-doc branch Feb 23, 2016

@breskeby

This comment has been minimized.

Show comment
Hide comment
@breskeby

breskeby Feb 23, 2016

Member

yes. I think so.

Member

breskeby commented Feb 23, 2016

yes. I think so.

@tbroyer tbroyer referenced this pull request Dec 13, 2017

Merged

Add annotationProcessor configurations for each SourceSet #3786

11 of 11 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment