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 annotationProcessor configurations for each SourceSet #3786

Merged
merged 4 commits into from Jan 29, 2018

Conversation

Projects
None yet
5 participants
@tbroyer
Contributor

tbroyer commented Dec 13, 2017

Context

See #2300 and related issues.
It was also discussed a couple years ago and there was a design doc: #456 #616

Contributor Checklist

  • Review Contribution Guidelines
  • Make sure that all commmits are signed off to indicate that you agree to the terms of Developer Certificate of Origin.
  • Check "Allow edit from maintainers" option in pull request so that additional changes can be pushed by Gradle team
  • Provide integration tests (under <subproject>/src/integTest) to verify changes from a user perspective
  • Provide unit tests (under <subproject>/src/test) to verify logic
  • Update User Guide, DSL Reference, and Javadoc for public-facing changes
  • Ensure that tests pass locally: ./gradlew <changed-subproject>:check

Gradle Core Team Checklist

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation
  • Recognize contributor in release notes
@tbroyer

This comment has been minimized.

Contributor

tbroyer commented Dec 13, 2017

This is a first shot; documentation needs to be updated, and there should possibly be more integration tests (though the existing tests don't break, and the new behavior is tested by refactoring existing tests to take advantage of it)

I'm not sure the way I "default" annotationProcessorPath is good, which is the reason I'm sending this PR now, as I'm concerned that it might resolve the configuration too early. Maybe the configuration should be set in another property and AnnotationProcessorDetector needs to use it (i.e. annotationProcessorPath being null would mean use the “related” configuration, or the compile classpath rather than the current use the compile classpath)

Fwiw, some (existing, untouched) tests apparently require an Oracle JDK (e.g. some tests use javafx) and break with OpenJDK.

/cc @oehme

@oehme oehme self-requested a review Dec 13, 2017

@oehme oehme self-assigned this Dec 13, 2017

@oehme

oehme requested changes Dec 13, 2017 edited

Thanks @tbroyer, that was quick :)

I think we'll need to add the deprecation of processors on the compile classpath in the same go to make this change self-contained.

Regarding test coverage there should be one integration test for each scenario:

  • user adds processors to the processorPath configuration - only they are used, no warnings
  • user has no processors, but some 3rd party library leaks as processor - user is warned that these will be ignored in 5.0
  • user has leaking processors, but explicitly sets the processorPath option to an empty collection - no warning is displayed
  • user has leaking processors, but explicitly sets the proc:none option - no warning is displayed
  • user has no processors and no processors are leaking - no warning is displayed
@@ -66,6 +66,9 @@ public FileCollection getEffectiveAnnotationProcessorClasspath(final CompileOpti
if (pos == compileOptions.getCompilerArgs().size() - 1) {
throw new InvalidUserDataException("No path provided for compiler argument -processorpath in requested compiler args: " + Joiner.on(" ").join(compileOptions.getCompilerArgs()));
}
DeprecationLogger.nagUserOfDeprecated(

This comment has been minimized.

@oehme

oehme Dec 13, 2017

Member

If we do this, we should un-incubate the annotationProcessorPath property (which is a good idea anyway, as the Android plugin already depends on it.

This comment has been minimized.

@tbroyer

tbroyer Dec 13, 2017

Contributor

It was suggested in #3058 (comment) that maybe annotation processor-related compiler options could be refactored into a nested object (similar to what I did in net.ltgt.apt plugin, except at the task level, but I didn't have the choice has compile options has only become an extensible DSL in 4.3; with this specific PR linked above actually), so maybe it's a bit premature to un-incubate annotationProcessorPath? (your call)

This comment has been minimized.

@oehme

oehme Dec 13, 2017

Member

It's already used by one of the most important plugins out there, so we can't remove it without a deprecation period anyway. So it is not incubating in practice. We can deprecate it later if we move to a nested object.

This comment has been minimized.

@tbroyer

tbroyer Dec 13, 2017

Contributor

It's already used by one of the most important plugins out there

I'm assuming you're talking about the Android plugin here (from earlier comment).
Fwiw, I've been told net.ltgt.apt is in the top 5 most downloaded plugins from the plugin portal; so that makes 2 “most important plugins” using annotationProcessorPath 😉
(but net.ltgt.apt is versioned in a 0.x scheme, specifically because the goal was to have those things in Gradle proper eventually)

This comment has been minimized.

@oehme

oehme Dec 13, 2017

Member

Even more reason then :)

new DslObject(compileTask.getOptions()).getConventionMapping().map("annotationProcessorPath", new Callable<Object>() {
@Override
public Object call() throws Exception {
return sourceSet.getAnnotationProcessorPath().isEmpty() ? null : sourceSet.getAnnotationProcessorPath();

This comment has been minimized.

@oehme

oehme Dec 13, 2017

Member

I think we shouldn't do an empty check here, but make the detector a little smarter. It can check whether the user passed a simple empty file collection or whether it's looking at a Configuration. If it's a Configuration, we fall back to the compileClasspath and emit a warning if there are processors in there. If it's a plain old file collection, we know that the user explicitly wanted us to ignore the compileClasspath. The warning could then instruct users to explicitly set the processor path to empty if they want no processing and tell them that this will be the default behavior in 5.0.

@tbroyer

This comment has been minimized.

Contributor

tbroyer commented Dec 14, 2017

Regarding test coverage there should be one integration test for each scenario:

  • user adds processors to the processorPath configuration - only they are used, no warnings

Done.
I also updated the groovy and scala plugins (through SourceSetUtils) to setup the new configuration in CompilerOptions.

  • user has no processors, but some 3rd party library leaks as processor - user is warned that these will be ignored in 5.0

Does not check for “third party library” specifically, but “processor present in the compile classpath”.

  • user has leaking processors, but explicitly sets the processorPath option to an empty collection - no warning is displayed
  • user has leaking processors, but explicitly sets the proc:none option - no warning is displayed

Done

  • user has no processors and no processors are leaking - no warning is displayed

This the default, and is tested automatically by the GradleExecuter. I had to modify a few tests to specifically disable the warning, or check for its presence.


I think we shouldn't do an empty check here, but make the detector a little smarter.

Done. That was a fun ride in Gradle internals BTW ;-)

@tbroyer

This comment has been minimized.

Contributor

tbroyer commented Dec 15, 2017

Fwiw, I verified with :plugins:check, :languageJava:check, :languageJvm:check, :languageGroovy:check, :languageScala:check, :scala:check, and :core:check; and grepped for -processorpath, javax.annotation.processing, and variations of annotation processor (which is how I found JansiEndUserIntegrationTest)

Documentation still needs a bit more work (including release notes), but I'd rather have feedback on the code first (and possibly suggestions for the documentation).

@oehme

This comment has been minimized.

Member

oehme commented Dec 20, 2017

This looks really nice, I'll do a more thorough review after the holidays. Thanks a lot @tbroyer!

@sean-brandt

This comment has been minimized.

sean-brandt commented Jan 16, 2018

Any progress here? Would be really great to see this go in.

@oehme oehme added this to the 4.6 RC1 milestone Jan 24, 2018

@oehme

This comment has been minimized.

Member

oehme commented Jan 24, 2018

@tbroyer I rebased your changes on master and did a bit of polishing, does this look good to you?

@oehme oehme requested a review from melix Jan 24, 2018

@tbroyer

This comment has been minimized.

Contributor

tbroyer commented Jan 24, 2018

Looks good, and thanks for the additional userguide content! (note: you have a typo in that commit's message 😉 also your last commit contains a small reformat for the previous commit)

@oehme

This comment has been minimized.

Member

oehme commented Jan 25, 2018

I fixed the typos and improved the deprecation message. There were also a few more test expectations I had to adjust.

I made the fallback behavior a bit more specific, so we only fall back when the new default configuration is used. That way users won't get a deprecation warning when they use the apt plugin, but their processor path happens to be empty.

@oehme

This comment has been minimized.

Member

oehme commented Jan 25, 2018

@melix This is good to go from my side, would you mind giving it a second look?

@oehme

oehme approved these changes Jan 25, 2018

@melix

This is very nice and long awaited, thanks for the contribution!

I added a minor thing to fix, but I think we need to be explicit in the release notes that it's a potential breaking change (especially if you already have a annotationProcessor configuration).

I'm also curious to see how it impacts larger projects in terms of memory usage (because of the creation of additional configurations).

Regarding the name of the configuration, I think I would have preferred annotationProcessing. In other words, I prefer that the name of the configuration describes the intent (I'm for annotation processing), rather than the consumer (I'm for the annotation processor), because, who knows, other consumers could be interested in using it too. This would also be in line with the names of the new configuration we create (api, implementation, ...).

@@ -330,6 +330,10 @@ class ResolveConfigurationDependenciesBuildOperationIntegrationTest extends Abst
dependencies {
compile 'org.foo:app-dep:1.0'
}
tasks.withType(JavaCompile) {
options.annotationProcessorPath = files()

This comment has been minimized.

@melix

melix Jan 25, 2018

Member

⭕️ Isn't the fact you had to add this a hint that this PR is potentially a breaking change?

This comment has been minimized.

@oehme

oehme Jan 25, 2018

Member

This is only here because this test asserts a number of resolve operations. It's not a breaking change, I just didn't care about asserting on the processor path, so I made it empty.

This comment has been minimized.

@oehme

oehme Jan 25, 2018

Member

The other tests were similarly adjusted to silence the deprecation warning. E.g. Antlr4 contains an annotation processor, which we don't actually want to use in that test.

@@ -97,6 +113,10 @@ buildCache {
}
```
### Putting annotation processors on the compile classpath or explicit `-processorpath` compiler argument
Putting processors on the compile classpath or using an explicit `-processorpath` compiler argument has been deprecated and will be removed in Gradle 5.0. Annotation processors should be added to the `annotationProcessor` Configuration instead. If you don't want any processing, but your compile classpath contains a processor unintentionally (e.g. as part of some library you use), use the `-proc:none` compiler argument to ignore it.

This comment has been minimized.

@melix

melix Jan 25, 2018

Member

Typo Configuration -> configuration

This comment has been minimized.

@oehme

oehme Jan 25, 2018

Member

I changed it, though it was intentional, since it's the class name. Do we use lower case in our docs for this?

This comment has been minimized.

@melix

melix Jan 25, 2018

Member

We use backticks if we want to refer to a class name.

@oehme

This comment has been minimized.

Member

oehme commented Jan 25, 2018

We'll see the performance impact soon once the branch build is complete. The name annotationProcessing makes sense, but it would create an asymmetry with the Android plugin, which has already standardized on annotationProcessor. That might confuse users when they have mixed Java/Android projects.

I'll add a note about the new Configuration potentially clashing with one the user already created.

@melix

This comment has been minimized.

Member

melix commented Jan 25, 2018

mmm, thanks. About:

I'll add a note about the new Configuration potentially clashing with one the user already created.

Since you just said that the name annotationProcessor was chosen because Android already uses it, then we need to make sure it doesn't break Android first.

@oehme

This comment has been minimized.

Member

oehme commented Jan 25, 2018

It won't, since the Android plugin doesn't use SourceSets. They also have api and implementation, which don't clash with ours of the same name.

@melix

This comment has been minimized.

Member

melix commented Jan 25, 2018

I'm actually more thinking about configuration.create, that could trigger the following error message:

Cannot add a configuration with name 'annotationProcessor' as a configuration with that name already exists

@oehme

This comment has been minimized.

Member

oehme commented Jan 25, 2018

I know, but that won't happen with Android, because the Android plugin doesn't add SourceSets and forbids applying the java plugin on the same project. They use the same names for everything for consistency, but they don't actually build on our Java infrastructure.

It could happen with other plugins/custom build scripts, so I'll still add a notice.

@melix

melix approved these changes Jan 25, 2018

@oehme

This comment has been minimized.

Member

oehme commented Jan 26, 2018

Performance tests look good too. I'll hold off with merging until CI on master is green again, there seem to be 2 flaky tests at the moment.

tbroyer and others added some commits Dec 12, 2017

Deprecates specifying a processorpath in the compilerArgs
We don't want users doing that so that we can correctly cache
their tasks. But, we also don't want to break them as they migrate
from passing the processorpath command line argument via the
annotationProcessorPath property on CompileOptions.

Part of #2300

Signed-off-by: Thomas Broyer <t.broyer@ltgt.net>
Update release notes wrt annotation processing promotion/deprecation
Part of #2300

Signed-off-by: Thomas Broyer <t.broyer@ltgt.net>
Add annotationProcessor configurations for each SourceSet
And configure the compileJava.options.annotationProcessorPath
to use the configuration when not empty (and use 'null' when
the configuration is empty to preserve the current behavior).

Part of #2300

Signed-off-by: Thomas Broyer <t.broyer@ltgt.net>

@oehme oehme merged commit 95dcf58 into gradle:master Jan 29, 2018

2 checks passed

DCO All commits have a DCO sign-off from the author
WIP ready for review
Details
@oehme

This comment has been minimized.

Member

oehme commented Jan 29, 2018

Thanks again for this great piece of work @tbroyer! Let's continue the discussion of the next steps on #2300

tbroyer added a commit to tbroyer/gradle-apt-plugin that referenced this pull request Feb 11, 2018

Add annotationProcessor configuration(s), deprecate apt one(s)
This aligns the API with the upcoming Gradle 4.6, added in
gradle/gradle#3786.
For backwards compatibility, the annotationProcessor configuration(s)
extend from the apt one(s).
@childnode

This comment has been minimized.

shouldn't we link to the blog post instead of "hiding details" and let users "need to google for"?

@tbroyer tbroyer deleted the tbroyer:issue-2300-first-class-annotation-processors branch Mar 1, 2018

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