[fix][build] Switch slf4j scope to provided from compile #22607
Conversation
Signed-off-by: Zixuan Liu <[email protected]>
github-actions
bot
added
the
doc-required
nodece
force-pushed
the
slf4j-scope-provided
branch
from
65d23e6 to
e7aecef
Compare
Signed-off-by: Zixuan Liu <[email protected]>
nodece
force-pushed
the
slf4j-scope-provided
branch
from
e7aecef to
0b7f55d
Compare
|
/pulsarbot rerun-failure-checks |
| @@ -765,18 +765,21 @@ flexible messaging model and an intuitive client API.</description> | |||
| <groupId>org.slf4j</groupId> | |||
| <artifactId>slf4j-api</artifactId> | |||
| <version>${slf4j.version}</version> | |||
| <scope>provided</scope> | |||
There was a problem hiding this comment.
I think that slf4j-api should be kept in default (compile) scope. Users of the library can use dependency management to enforce a specific version of slf4j-api. Dependency resolution picks the "closest" dependency by default when there are version conflicts. It's better to rely on this for slf4j-api.
There was a problem hiding this comment.
I prefer slf4j-api keep compile scope too. We can make this update a minor update. It's not a breaking change.
There was a problem hiding this comment.
The default compile scope breaks many downstream applications, as I've said in https://lists.apache.org/thread/4omvl62k4jhntj3rsywp14zzgh1l3l9q
With the compile scope, we should revert #22391.
There was a problem hiding this comment.
The default
compilescope breaks many downstream applications, as I've said in https://lists.apache.org/thread/4omvl62k4jhntj3rsywp14zzgh1l3l9q
I agree that it's fine for all other dependencies, but not for slf4j-api. please check the reasoning in #22607 (comment)
There was a problem hiding this comment.
I see your reason. But as the conclusion, did you mean all downstream projects need to import the
slf4j-apidependency explicitly to get it around?
In the future, since there are both SLF4J 1.7.x and 2.x, any project would have to use dependency management to select the desired version for that project. Obviously if a project is using 2.x features, it will require 2.x version at runtime.
I think that the correct solution is to keep slf4j-api in compile scope. It's simply a bad solution to ignore the dependency. For slf4j-simple and jcl-over-slf4j, the correct scope is provided since they are runtime only type of dependencies. The same argumentation doesn't apply to slf4j-api.
We must avoid introducing any breaking change, or at least, document the breaking change and how should users get it around.
My assumption is that slf4j-api is binary compatible for all 1.7.x interfaces in 2.x. . However, it will be necessary to document that Pulsar has switched to use slf4j 2.x . All Java based projects will have to deal with this eventually so this challenge isn't specific to Pulsar.
There was a problem hiding this comment.
@nodece Please make the scope compile (it's default, so drop provided) for slf4j-api.
There was a problem hiding this comment.
Another point is that #22391 will be released at 3.3.0. When users upgrade to the pulsar version, they must pay attention to the version of slf4j. We need to mention this in the release note.
In the future, since there are both SLF4J 1.7.x and 2.x, any project would have to use dependency management to select the desired version for that project.
In the mailing list, I said:
Unifying SLF4J is quite difficult, many libraries include the slf4j 1.x or 2.x, and when the slf4j version is inconsistent, we still need to use <exclude>.
This PR ensures that downstream users can use either slf4j 1. x or 2. x, once the pulsar uses the slf4j 2.x api(https://www.slf4j.org/manual.html#fluent), this will force users to upgrade to slf4j 2.x.
For me, using scope compile is correct.
All Java based projects will have to deal with this eventually so this challenge isn't specific to Pulsar.
+1, just like what I did in #22391, unify the slf4j version.
There was a problem hiding this comment.
@nodece please make this compile scope so that we can resolve this review
|
It seems some tests failed even in the 5th rerun, could you take a look? |
|
Based on the review results, let's continue to use compile scope. When using the pulsar dependency, the downstream users must unify the slf4j version in the project. |
Motivation
#22391 upgrades the slf4j version to 2.0.13.
When the users upgrade the broker/client library to the latest version, #22391 breaks users who are using slf4j 1.x, and their application is unable to print logs.
Maven has a scope feature, that can help us fix this issue. Import the slf4j dependency with
<scope>provided</scope>in the pulsar, and then the users use the broker/client library, users can use any version of slf4j.There is a small issue here, users must include slf4j-api in their projects. The slf4j-api is commonly used anywhere, and I don't think we need to worry about this issue.
Modifications
providedfromcompilein thepom.xml, and then the libraries don't include the slf4j.compilefromprovidedin thedistribution/server/pom.xmlanddistribution/shell/pom.xml, and then the tar includes the slf4j.Documentation
docdoc-requireddoc-not-neededdoc-complete