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

Fix: properly detect the @Generated annotation on JDK9 to avoid package splits #882

Closed
wants to merge 3 commits into from

Conversation

zyxist
Copy link

@zyxist zyxist commented Sep 24, 2017

Solution to #880.

The original issue was closed with a statement that the changes in 772374b would fix it. However, they do not. The JDK9 modular application would still have to deal with the package split, but instead of patching java.xml.ws.annotation, it would need to patch jsr250. In addition, adding this dependency pollutes the application dependencies with (mostly unnecessary) JavaEE annotations.

The solution:

  • removes the import from javax.annotation.Generated, which removes the need to add any extra dependencies, and effectively removes the package split.
  • properly detects the new javax.annotation.processing.Generated annotation.

I tested the solution on my experimental application, where I added Jigsaw modules: https://github.com/zyxist/dagger-example-app

Use case 1:

module com.zyxist.example.dagger.app {
    requires dagger;
    requires javax.inject;
    requires java.compiler;
}

Result: the generated classes are annotated with javax.annotation.processing.Generated.

Use case 2:

module com.zyxist.example.dagger.app {
    requires dagger;
    requires javax.inject;
    requires java.xml.ws.annotation;
}

Result: the generated classes are annotated with javax.annotation.Generated.

Use case 3:

module com.zyxist.example.dagger.app {
    requires dagger;
    requires javax.inject;
}

Result: the generated classes are not annotated.

Use case 4: using Dagger together with Guava (a library that depends on jsr305):

module com.zyxist.example.dagger.app {
    requires dagger;
    requires javax.inject;
    requires guava; // note: this is not the 'correct' module name
    requires java.compiler;
}

Result: build successful, no patching needed.

For JDK8, the annotation processor will always detect javax.annotation.Generated.

Please check if I correctly removed the jsr250 dependency from the build files. I'm new to Bazel, and I had some issues with running the full build due to the issues with Android SDK.

CLA is signed.

@zyxist
Copy link
Author

zyxist commented Oct 12, 2017

@ronshapiro -> did you find time to take a look at the Pull Request related to javax.annotation.Generated on JDK9? I'd be really grateful for feedback, because this issue is really problematic, and it looks like other project have fallen into it, too.

ronshapiro added a commit that referenced this pull request Dec 13, 2017
Fixes #880
Closes #882

RELNOTES=Remove dependency on JSR 305 annotations

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=178784068
ronshapiro added a commit that referenced this pull request Dec 13, 2017
…the checker framework's annotations.

Fixes https://github.com/google/dagger/issue/880
Closes #882

RELNOTES=Fix JPMS issue with javax.annotation types

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=178799529
ronshapiro added a commit that referenced this pull request Dec 14, 2017
…the checker framework's annotations.

Fixes https://github.com/google/dagger/issue/880
Closes #882

RELNOTES=Fix JPMS issue with javax.annotation types

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=178799529
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.

2 participants