-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Move JSpecify from provided
to compile
scope
#3228
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
base: 2.x
Are you sure you want to change the base?
Conversation
[JSpecify](https://jspecify.dev/) is a nullability annotation library with a [large list of supporters](https://jspecify.dev/about/). It is the recommended library to solve [LOG4J2-1477](https://issues.apache.org/jira/browse/LOG4J2-1477). The usage of JSpecify as `provided` library has however a drawback: if users don't have JSpecify on their stack, they'll get a Javac Linter warning whenever they use a class with nullability annotations (see [#3110](#3110) for example. Since JSpecify is an annotation with `RUNTIME` retention and is not covered by [JDK-8342833](https://bugs.openjdk.org/browse/JDK-8342833), the easiest solution provided by this PR is: - Move JSpecify from the `provided` to the `compile` scope. This will inflate users dependency stack by a whooping 4000 bytes. - Mark JSpecify as optional for OSGi and JPMS, so users can exclude the dependency if they wish to do it. In practice, this change should be neutral for users and will allow us to add nullability annotations to important class, such as `Logger` without breaking the build of users that use the [`-Werror` compiler flag](https://docs.oracle.com/en/java/javase/17/docs/specs/man/javac.html#option-Werror).
Yes, I am aware of all these arguments as they were raised when the dependency was first added. I made the same objection then but was assured that by using provided scope users would have no problems if the dependency is missing. If that really isn’t the case then the dependency should be removed and a different solution should be found. Note that even using Jakarta.annotations would be a problem as we also have a policy to limit the dependencies to Java.base in JPMS, which is work I believe you worked on as part of 3.0. |
I don't consider a compiler warning (if the If I understand correctly, your |
Yes, the -1 is for the API only. While we have always tried to minimize the dependencies on Log4j-core that has never been as strict. I believe we made some effort in 3.0 to remove many dependencies but I don't believe we got rid of them all. You would probably know that better than me since you have been actively working on it recently. |
Version |
# Conflicts: # log4j-taglib/pom.xml # pom.xml
@ppkarwasz, will this have any [backward compatibility] implications on the fat JAR users? |
I am not sure what implications are you thinking about. Yes, their fat JARs will be a couple of KiB bigger. Note: I didn't replace the JPMS |
No, what I mean is, there are users creating Fat JARs with an explicit list of dependencies. With the next minor Log4j 2 version, they will be missing a |
The annotation is only used at compile time. It can be absent at runtime. |
JSpecify is a nullability annotation library with a large list of supporters. It is the recommended library to solve LOG4J2-1477.
The usage of JSpecify as
provided
library has however a drawback: if users don't have JSpecify on their stack, they'll get a Javac Linter warning whenever they use a class with nullability annotations (see #3110 for example). Since JSpecify is an annotation withRUNTIME
retention and is not covered by JDK-8342833, the easiest solution provided by this PR is:provided
to thecompile
scope. This will inflate users dependency stack by a whooping 3819 bytes.In practice, this change should be neutral for users and will allow us to add nullability annotations to important class, such as
Logger
without breaking the build of users that use the-Werror
compiler flag.