Skip to content

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

Open
wants to merge 4 commits into
base: 2.x
Choose a base branch
from

Conversation

ppkarwasz
Copy link
Contributor

@ppkarwasz ppkarwasz commented Nov 22, 2024

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 with RUNTIME retention and is not covered by 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 3819 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.

[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).
@rgoers
Copy link
Member

rgoers commented Nov 27, 2024

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.

@ppkarwasz
Copy link
Contributor Author

ppkarwasz commented Nov 27, 2024

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.

I don't consider a compiler warning (if the -Xlint:all linter option is enabled) to be a real problem. Besides, users can always disable the classfile category in the linter: it contains only two warnings (see this comment) and both of them are pretty harmless.

If I understand correctly, your -1 only applies to the additional dependency in log4j-api, while you are OK with adding it to the other artifacts?

@rgoers
Copy link
Member

rgoers commented Nov 27, 2024

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.

@ppkarwasz
Copy link
Contributor Author

I believe we made some effort in 3.0 to remove many dependencies but I don't believe we got rid of them all.

Version 3.0.0-beta3 of Log4j Core has no external dependencies (see MvnRepository). Most importantly, it has no optional dependencies, which are always a problem. Even if we exclude JPMS and OSGi, normal Java users pin their optional Log4j Core dependency in the POM file and they forget to upgrade them, when they upgrade Log4j Core. This will no longer be an issue.

@ppkarwasz
Copy link
Contributor Author

@rgoers,

I moved jspecify into the provided scope for log4j-api. Can you review this PR again?

Note: The build is expected to fail until #3601 is fixed. I opened #3602 that awaits a review.

@ppkarwasz ppkarwasz requested a review from rgoers April 13, 2025 09:05
@ppkarwasz ppkarwasz added this to the 2.x milestone Apr 13, 2025
@vy
Copy link
Member

vy commented Apr 15, 2025

@ppkarwasz, will this have any [backward compatibility] implications on the fat JAR users?

@ppkarwasz
Copy link
Contributor Author

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 requires static org.jspecify with requires org.jspecify, so if JPMS or OSGi user don't want it, they can exclude it. Fat JAR users of course are not JPMS users.

@vy
Copy link
Member

vy commented Apr 16, 2025

I am not sure what implications are you thinking about. Yes, their fat JARs will be a couple of KiB bigger.

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 compile-scoped dependency of log4j-core. Will that have any implications on such users?

@ppkarwasz
Copy link
Contributor Author

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 compile-scoped dependency of log4j-core. Will that have any implications on such users?

The annotation is only used at compile time. It can be absent at runtime.

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

Successfully merging this pull request may close these issues.

3 participants