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

Remove dependency on javax.annotation.Generated #6833

Closed
ulfjack opened this issue Mar 16, 2020 · 9 comments · Fixed by #6990
Closed

Remove dependency on javax.annotation.Generated #6833

ulfjack opened this issue Mar 16, 2020 · 9 comments · Fixed by #6990
Milestone

Comments

@ulfjack
Copy link
Contributor

ulfjack commented Mar 16, 2020

A similar issue was previously reported in #5343, which I cannot reopen. The problem with the solution proposed there is that the upstream library that is pulled this way is licensed under the CDDL. You should probably talk to a lawyer about the implications of that, and whether they apply here.

In any case, given that gRPC seems to be intended to be licensed as Apache 2, it seems wiser to remove the dependency entirely or at least provide an option to avoid it.

@ulfjack ulfjack added the bug label Mar 16, 2020
@voidzcy
Copy link
Contributor

voidzcy commented Mar 16, 2020

/cc @srini100

@ejona86
Copy link
Member

ejona86 commented Mar 22, 2020

A quick note that org.apache.tomcat:annotations-api is available and under Apache 2. It appears that long-term the annotation is being renamed to jakarta.annotation.Generated.

The biggest problem with the annotation is there are so many different artifacts containing it, which causes classpath collisions.

(I'll also note that the dependency is compileOnly. You don't need to distribute the annotation, which would be best since it avoids classpath collisions.)

@ulfjack
Copy link
Contributor Author

ulfjack commented Mar 22, 2020

Java 9 has javax.annotation.processing.Generated, which seems to be the proper replacement?

You're right that the Generated annotation is not necessary at runtime, but the "default" (?) maven artifact also contains other annotations that are not compile-only, so you'd have to be very careful not to accidentally use one of them elsewhere.

@ejona86
Copy link
Member

ejona86 commented Mar 22, 2020

See #3633 concerning javax.annotation.processing.

@ulfjack
Copy link
Contributor Author

ulfjack commented Mar 23, 2020

Ok, I'll try hard not to reopen that discussion. Would it be a hardship for you to add a flag that allows the user to select the annotation (or disable it)?

The mentioned org.apache.tomcat:annotations-api contains a source file jakarta/annotation/Generated.java. Misread?

I've worked around the issue for us, but it seems risky to recommend using CDDL-licensed code.

@ejona86
Copy link
Member

ejona86 commented Mar 23, 2020

There is org.apache.tomcat:annotations-api (javax.annotation) and org.apache.tomcat:tomcat-annotations-api (jakarta.annotation). Apparently all of J2EE moved to the Eclipse Foundation and is being renamed Jakarta EE and moving packages.

Normally we can't have options for the generated code because we need there to be a canonical version used by everyone (otherwise we start seeing diamond dependency issues). Since this annotation is source-level only, it would be possible to have an option for it. But in general it would be best for everyone to resist options. For example, in this case we may change our recommendation to use org.apache.tomcat or, depending how the tools are implemented, make a io.grpc.Generated.

@ejona86 ejona86 added enhancement and removed bug labels Mar 27, 2020
@creamsoup
Copy link
Contributor

closing it due to inactivity, please feel free to reopen.

@ulfjack
Copy link
Contributor Author

ulfjack commented Apr 17, 2020

I can't reopen bugs in this project, so that statement feels a bit passive-aggressive. I think having a dependency on CDDL-licensed code is a problem - even if only a burden on your users to make sure that they don't accidentally include it in whatever they're distributing -, and I'm happy to send a patch if there's agreement on what to do.

Personally I would use the javax.annotation.processing.Generated annotation in the JDK; I understand the concern that it's intended for annotation processing, but I personally think that concern is academic. I'd also be happy with removing the annotation, but I don't know if anyone's relying on it.

@ejona86 ejona86 reopened this Apr 17, 2020
@ejona86
Copy link
Member

ejona86 commented Apr 17, 2020

(@ulfjack, @creamsoup was not aware you couldn't reopen issues. All the same, we normally would say "comment with more information and we can reopen")

I saw this had been closed, but I didn't realize it was this. Yeah, this should remain open. The inactivity is on our side (i.e., me).

@ejona86 ejona86 added this to the Next milestone Apr 29, 2020
ejona86 added a commit to ejona86/grpc-java that referenced this issue Apr 29, 2020
javax.annotation-api is licensed CDDL, which was not noticed when it was
introduced. Tomcat provides an Apache 2 version of the same annotation. Note
that this annotation is only used when compiling with Java 9+.

Unfortunately this may cause classpath collisions since there are _many_ copies
of this annotation on Maven Central; we wanted one canonical source and
javax.annotation-api seemed like that source. We hope this won't impact many
users since we have always suggested using it only for compilation. But it will
probably impact some users. However, we didn't create this mess, this seems to
be "standard practice" for J2EE, which this annotation is now part of, so we're
just impacted by it.

Fixes grpc#6833
ejona86 added a commit to ejona86/grpc-java that referenced this issue Apr 29, 2020
javax.annotation-api is licensed CDDL, which was not noticed when it was
introduced. Tomcat provides an Apache 2 version of the same annotation. Note
that this annotation is only used when compiling with Java 9+.

Unfortunately this may cause classpath collisions since there are _many_ copies
of this annotation on Maven Central; we wanted one canonical source and
javax.annotation-api seemed like that source. We hope this won't impact many
users since we have always suggested using it only for compilation. But it will
probably impact some users. However, we didn't create this mess, this seems to
be "standard practice" for J2EE, which this annotation is now part of, so we're
just impacted by it.

Fixes grpc#6833
ejona86 added a commit that referenced this issue Apr 30, 2020
javax.annotation-api is licensed CDDL, which was not noticed when it was
introduced. Tomcat provides an Apache 2 version of the same annotation. Note
that this annotation is only used when compiling with Java 9+.

Unfortunately this may cause classpath collisions since there are _many_ copies
of this annotation on Maven Central; we wanted one canonical source and
javax.annotation-api seemed like that source. We hope this won't impact many
users since we have always suggested using it only for compilation. But it will
probably impact some users. However, we didn't create this mess, this seems to
be "standard practice" for J2EE, which this annotation is now part of, so we're
just impacted by it.

Fixes #6833
@ejona86 ejona86 modified the milestones: Next, 1.30 May 1, 2020
QIvan added a commit to QIvan/benchmarks that referenced this issue Sep 11, 2020
that's a known issue fix grpc grpc/grpc-java#3633
why is not javax.annotation-api see here grpc/grpc-java#6833
QIvan added a commit to QIvan/benchmarks that referenced this issue Sep 11, 2020
that's a known issue with grpc grpc/grpc-java#3633
why is not javax.annotation-api see here grpc/grpc-java#6833
mjpt777 pushed a commit to real-logic/benchmarks that referenced this issue Sep 12, 2020
that's a known issue with grpc grpc/grpc-java#3633
why is not javax.annotation-api see here grpc/grpc-java#6833
dfawley pushed a commit to dfawley/grpc-java that referenced this issue Jan 15, 2021
javax.annotation-api is licensed CDDL, which was not noticed when it was
introduced. Tomcat provides an Apache 2 version of the same annotation. Note
that this annotation is only used when compiling with Java 9+.

Unfortunately this may cause classpath collisions since there are _many_ copies
of this annotation on Maven Central; we wanted one canonical source and
javax.annotation-api seemed like that source. We hope this won't impact many
users since we have always suggested using it only for compilation. But it will
probably impact some users. However, we didn't create this mess, this seems to
be "standard practice" for J2EE, which this annotation is now part of, so we're
just impacted by it.

Fixes grpc#6833
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants