Skip to content

Support mixed Java/Scala modules using Java annotation processors #159

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

Merged
merged 2 commits into from
Aug 11, 2021

Conversation

grollinger
Copy link
Contributor

We use an annotation processor (lombok) on some Java files in a combined Java/Scala module.

When we tried running scoverage on that module, we noticed that the compileScoverage* tasks failed because the annotation processor wasn't being run, even though the non-scoverage compile* tasks succeed.

This PR changes the scoverage compile task to inherit the annotationProcessorPath from the original compile task, in addition to compileClasspath and runtimeClasspath.

That fixes the compilation problem we observed.

I'm not sure this is the best solution to this problem, so please let me know if you can think of a better one.

@eyalroth
Copy link
Contributor

eyalroth commented Aug 5, 2021

Thank you for the contribution!

I'm a bit hesitant to add this to the project, for a few reasons:

  1. The more custom integrations with other tools/plugins, the harder it would become to maintain this project and its compatibility with the other tools.

  2. I'm not exactly sure about this, but It seems as though Lombok is reliant on java-compile tasks rather than scala-compile ones. The thing is, mixed java-scala projects preferably should not use the java-compile tasks at all, but rather defer all compilation to the scala-compile tasks. You can see this configured in the relevant functional test.

Given that, I'm not even sure that Lombok is compatible with the preferred way of compiling mixed projects, regardless of the usage of scoverage. I would perhaps seek their help in first making it compatible, and that would perhaps solve the issue here as well.

  1. I'm not seeing the Lombok gradle plugin being configured in the PR functional tests, so I'm not sure it actually works.

@grollinger
Copy link
Contributor Author

  1. I'm only using lombok as an example of a Java annotation processor. Any other Java annotation processor will have the same problem with gradle-scoverage tasks.
  2. Yes, Java annotation processors rely on the Java compile task. Given that there are some very nice libraries and frameworks that rely on Java annotation processors, I don't think it's too much to ask that scoverage should not break projects depending on them.
  3. Java annotation processors are natively supported by Gradle. There is no need for a lombok gradle plugin to make it work.

Given that, I'm not even sure that Lombok is compatible with the preferred way of compiling mixed projects, regardless of the usage of scoverage.

As you say, there are some things one needs to consider when using Java annotation processors.
Because running the annotation processor is done by the Java compile task, you have to use that task to compile any source file that needs to use an annotation processor. For these source files, you won't be able to refer to classes implemented in Scala in the same module.
That doesn't mean that you have to compile all Java sources in this module using the Java compile task.
In fact, you can have the best of both worlds. We configure our build with a dedicated source directory for sources requiring an annotation processor src/main/java_lombok. All other Java sources in the module are still compiled using the compileScala task.

The default configuration of the Gradle Scala plugin is to compile Java with the compileJava task and Scala with the compileScala task. Otherwise, the scala-java-multi-module project wouldn't have to explicitly override the srcDirs for Java and Scala. So I think it's a bit of a stretch to say that what scala-java-multi-module does is the preferred way of configuring mixed modules. But even if we grant that most people will want to set up their mixed modules that way, I don't see why that should break the compile for people with different requirements.

@eyalroth
Copy link
Contributor

eyalroth commented Aug 5, 2021

  1. Java annotation processors are natively supported by Gradle. There is no need for a lombok gradle plugin to make it work.

I missed that part in their documentation. In any case, they do recommend using the gradle plugin (though I'm not sure that matters).

  1. I'm only using lombok as an example of a Java annotation processor. Any other Java annotation processor will have the same problem with gradle-scoverage tasks.

I'm not familiar with Java annotation processing, but from what I gather, it is a Java compiler feature which is not supported by the Scala compiler (without even considering scoverage). See this question in the Gradle forums (though it's an old question and things might have changed) and this relatively recent issue in the gradle repostiory.

Furthermore, I see here a mentioning of Lombok being an exceptional annotation processor (I'm not sure it matters):

An important thing to note is the limitation of the annotation processing API — it can only be used to generate new files, not to change existing ones.

The notable exception is the Lombok library which uses annotation processing as a bootstrapping mechanism to include itself into the compilation process and modify the AST via some internal compiler APIs. This hacky technique has nothing to do with the intended purpose of annotation processing and therefore is not discussed in this article.

In any case, I'm not sure how does coverage breaks your build if you have a separate source directory with all the sources that require annotation processing. If I'm not mistaken, you could define a custom java compilation task to compile these source files, and make the scala compilation task (not even scoverage) depend on it. Would this not work?

@grollinger
Copy link
Contributor Author

In any case, I'm not sure how does coverage breaks your build if you have a separate source directory with all the sources that require annotation processing.

It does that, because it copies the javaCompile task to javaScoverageCompile but doesn't copy over all the necessary configuration to make it work (i.e. the annotationProcessorPath) so the new scoverage task javaScoverageCompile is now broken.
When scoverage compilation is run, therefore, it tries to compile the Java sources using the java compiler but without running the annotation processor.

You can see that in action when you revert the line I added to copy over the annotationProcessorPath and run the new functionalTest:

./gradlew functionalTest --tests "*ScalaJavaAnnotationProcessorTest*"

it fails with the following error:

  > Task :mixed_scala_java:compileJava
    > Task :mixed_scala_java:compileScala
    > Task :mixed_scala_java:compileScoverageJava
    > Task :mixed_scala_java:compileScoverageScala     FAILED

    [Error] /gradle-scoverage/build/resources/functionalTest/projects/scala-java-annotation-processor/mixed_scala_java/src/main/scala/org/hello/WorldScala.scala:6: value foo is not a member of org.hello.WorldJava

As you can see, the non-scoverage tasks compileJava and compileScala worked fine.
But when compileScoverageScala is run, it tries to access a method foo() that should have been produced by compileScoverageJava by running the annotation processor. That method is not present because the annotation processor wasn't run.

If I'm not mistaken, you could define a custom java compilation task to compile these source files, and make the scala compilation task (not even scoverage) depend on it. Would this not work?

That might well work, but it would be a workaround.
If you say that we shouldn't rely on the Java compile task, then gradle-scoverage shouldn't either, and it shouldn't try to (incompletely) re-create that task.
Why should we have to create a custom task instead of using the Java compile task as it was intended to be used?

@eyalroth
Copy link
Contributor

eyalroth commented Aug 7, 2021

I see now. Thank you for being patience and working hard on explaining the situation at hand. Overall I'm happy that we have some sort of documentation for this issue.

May I add two requests to the PR?

  1. Can you confirm that the project passes all tests (./gradlew check) if you haven't already? It seems like Travis CI is no longer working for the project.
  2. In src/functionalTest/resources/projects/scala-java-annotation-processor/mixed_scala_java/build.gradle - could you add the configureSources part like in the scala-java-multi-module project, but commented out and with a comment explaining that this practice is incompatible with Java's annotation processing?

@grollinger grollinger force-pushed the annotation-processors branch from e3c162e to d48486e Compare August 9, 2021 06:46
@grollinger
Copy link
Contributor Author

Can you confirm that the project passes all tests (./gradlew check) if you haven't already? It seems like Travis CI is no longer working for the project.

Already done. But it seems CI is working again.

In src/functionalTest/resources/projects/scala-java-annotation-processor/mixed_scala_java/build.gradle - could you add the configureSources part like in the scala-java-multi-module project, but commented out and with a comment explaining that this practice is incompatible with Java's annotation processing?

Done. If you'd like to phrase the comment differently, just let me know.

@grollinger grollinger force-pushed the annotation-processors branch from d48486e to cc77b81 Compare August 9, 2021 07:01
@eyalroth
Copy link
Contributor

Done. If you'd like to phrase the comment differently, just let me know.

It's looking good :)

Copy link
Contributor

@eyalroth eyalroth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good!

As I said before, I really appreciate your patience and willingness to explain the reasoning behind this PR.

@maiflai maiflai merged commit ee291c2 into scoverage:master Aug 11, 2021
@maiflai
Copy link
Contributor

maiflai commented Aug 11, 2021

deployed 6.1.0 with this PR - thanks both

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants