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

why was slf4j-api added as a dependency? #1094

Closed
zoff99 opened this issue Apr 14, 2024 · 21 comments · Fixed by #1178
Closed

why was slf4j-api added as a dependency? #1094

zoff99 opened this issue Apr 14, 2024 · 21 comments · Fixed by #1178
Labels
question Further information is requested released Issue has been released

Comments

@zoff99
Copy link

zoff99 commented Apr 14, 2024

i read: #997 (comment)
and: 32082c0
and: 89dbda1

but i can not see any reason this was done.

here it says "Read the Changelog", but there is nothing about slf4j-api in the changelog.
why was this added? can you link to documentation about the decision or some code in sqlite that requires it?

why was it not possible to make this optional?

@zoff99 zoff99 added the triage label Apr 14, 2024
@gotson
Copy link
Collaborator

gotson commented Apr 15, 2024

it was added because it's used in almost any library, and it lets the application control logging. You can check #802

@gotson gotson added question Further information is requested and removed triage labels Apr 15, 2024
@jinsihou19
Copy link

yes,it is trouble for me to add external dependencies. look it...

@gotson gotson closed this as completed May 27, 2024
@headius
Copy link
Contributor

headius commented Jul 30, 2024

I must also raise a concern about this. JRuby does not ship with slf4j and we are very reluctant to add new dependencies. As a result, when we support SQLite for Ruby on Rails we will also have to ship an slf4j library.

Most users of slf4j I know of allow their code to fall back on j.u.logging so that upstream users do not have to introduce a new dependency. This also conflicts with users running on other logging backends, forcing the use of two different logging frameworks for no good reason.

Can we please reopen this discussion and figure out a way to remove the slf4j hard dependency from sqlite-jdbc?

See jruby/activerecord-jdbc-adapter#1149 (comment)

@headius
Copy link
Contributor

headius commented Jul 30, 2024

In #802 (comment) @gotson mentions that two other drivers also use slf4j: MySQL and H2. However on both of these databases, the slf4j usage and depedency is optional:

Both databases default to a simple logger without external dependencies.

@headius
Copy link
Contributor

headius commented Jul 30, 2024

Neither MySQL nor H1 maven artifacts depend on slf4j directly.

@KweezyCode
Copy link

remove the slf4j1 dependency, as this causes conflicts between different versions (for example i can't use JDA which uses slf4j2)

@headius
Copy link
Contributor

headius commented Sep 10, 2024

@gotson It seems like this hard slf4j dependency is still causing trouble for people. Can we please have a discussion about making it optional? As I pointed out in #1094 (comment), none of the databases you used as examples actually have a hard dependency on slf4j.

The only option for users that want to avoid the dependency is to fork the project and push their own artifacts. Nobody wants to have to do that, especially me!

@KweezyCode
Copy link

@gotson It seems like this hard slf4j dependency is still causing trouble for people. Can we please have a discussion about making it optional? As I pointed out in #1094 (comment), none of the databases you used as examples actually have a hard dependency on slf4j.

The only option for users that want to avoid the dependency is to fork the project and push their own artifacts. Nobody wants to have to do that, especially me!

i have made a fork, check my profile

@gotson
Copy link
Collaborator

gotson commented Sep 11, 2024

@gotson It seems like this hard slf4j dependency is still causing trouble for people. Can we please have a discussion about making it optional? As I pointed out in #1094 (comment), none of the databases you used as examples actually have a hard dependency on slf4j.

The only option for users that want to avoid the dependency is to fork the project and push their own artifacts. Nobody wants to have to do that, especially me!

if someone can send a PR where SLF4J is a soft-dependency, with a fallback on JUL, i will have a look at it.

@headius
Copy link
Contributor

headius commented Sep 11, 2024

if someone can send a PR where SLF4J is a soft-dependency, with a fallback on JUL, i will have a look at it.

@gotson Thank you for your flexibility! 🙇‍♂️

@KweezyCode You have commits that remove SLF4J altogether, do you want to take a stab at making it optional? We might be able to get this resolved quickly!

@KweezyCode
Copy link

if someone can send a PR where SLF4J is a soft-dependency, with a fallback on JUL, i will have a look at it.

@gotson Thank you for your flexibility! 🙇‍♂️

@KweezyCode You have commits that remove SLF4J altogether, do you want to take a stab at making it optional? We might be able to get this resolved quickly!

I don't support the idea of bloating the software, so it's better to remove something from the library than to make something optional (which will make the code grow even more)

@zoff99
Copy link
Author

zoff99 commented Sep 11, 2024

i hope we can meet on middle ground. making it optional sounds like a reasonable way to solve this issue.

@headius
Copy link
Contributor

headius commented Sep 11, 2024

@KweezyCode Optionally using slf4j has become pretty standard as a way to allow users to configure other kinds of loggers without the original author having to add support for them. It is in most cases the last logging framework you ever need to add, because it acts as a shim on top of all the other ones. This pattern is quite common and adds very little bloat.

I think we also need to be pragmatic here. The two other database drivers mentioned both include slf4j as an optional logging backend and @gotson is meeting us halfway here by accepting a patch to make it optional in this driver. I for one just want to get this patched and released so my users can upgrade.

@michael-o
Copy link
Contributor

michael-o commented Sep 12, 2024

Here is one serious problem I have hit today while upgrading to latest version: If you are running in a multi-classloader container like Tomcat and you have the JDBC driver in the server or common classloader to make it available to all webapps and the webapp uses SLF4J and its own SLF4J binding you will end with such a fuckup:

2024-09-12T19:55:45.438 INFORMATION [main] org.apache.jasper.servlet.TldScanner.scanJars At least one JAR was scanned for TLDs yet contained no TLDs. Enable debug logging for this logger for a complete list of JARs that were scanned but no TLDs were found in them. Skipping unneeded JARs during scanning can improve startup time and JSP compilation time.
SLF4J: Class path contains multiple SLF4J bindings.
SLF4J: Found binding in [jar:file:/var/opt/tomcat-smartld/webapps/smartld%23%23029/WEB-INF/lib/logback-classic-1.2.12.jar!/org/slf4j/impl/StaticLoggerBinder.class]
SLF4J: Found binding in [jar:file:/opt/ports/apache-tomcat-9.0.94/lib/jdbc/slf4j-simple-1.7.36.jar!/org/slf4j/impl/StaticLoggerBinder.class]
SLF4J: See http://www.slf4j.org/codes.html#multiple_bindings for an explanation.
SLF4J: Actual binding is of type [ch.qos.logback.classic.util.ContextSelectorStaticBinder]

This is not funny and there is not way to solve. The only workaround is to move the JDBC driver into the webapp classloader.

So adding SLF4J the way it is now wasn't good after all.

I support @headius' position.

I forgot to mention: The JAR installed has all native libraries stripped because they aren't suited. SQLite3 is globally installed, the JNI shim is linked against it and installed globally as well. No size overhead, no extraction overhead. Since it is a stripped-down version I cannot use the one from Maven Central as a dep in my webapp POM. The other one comes with a custom-tailed Tomcat. Loss-loss situation here.

headius added a commit to headius/sqlite-jdbc that referenced this issue Sep 18, 2024
In order to avoid the hard dependency on slf4j, this patch
abstracts the Logger interface and LoggerFactory factory to fall
back on a java.util.logging-based implementation if slf4j is not
present.

Fixes xerial#1094
@headius
Copy link
Contributor

headius commented Sep 18, 2024

I have pushed #1178 which makes slf4j optional. It is no longer a hard dependency, and when not present the implementation will fall back on java.util.logging, similar to other JDBC drivers.

I will work with the xerial/sqlite-jdbc maintainers to get that PR merged. Could this issue be reopened please?

headius added a commit to headius/sqlite-jdbc that referenced this issue Sep 18, 2024
In order to avoid the hard dependency on slf4j, this patch
abstracts the Logger interface and LoggerFactory factory to fall
back on a java.util.logging-based implementation if slf4j is not
present.

Fixes xerial#1094
@headius
Copy link
Contributor

headius commented Sep 18, 2024

@michael-o:

The JAR installed has all native libraries stripped because they aren't suited.

I think you should file a separate bug for this.

@gotson
Copy link
Collaborator

gotson commented Sep 19, 2024

I have pushed #1178 which makes slf4j optional. It is no longer a hard dependency, and when not present the implementation will fall back on java.util.logging, similar to other JDBC drivers.

I will work with the xerial/sqlite-jdbc maintainers to get that PR merged. Could this issue be reopened please?

this issue will remain closed, it was a question. The PR being opened is enough.

headius added a commit to headius/sqlite-jdbc that referenced this issue Sep 19, 2024
In order to avoid the hard dependency on slf4j, this patch
abstracts the Logger interface and LoggerFactory factory to fall
back on a java.util.logging-based implementation if slf4j is not
present.

Fixes xerial#1094
headius added a commit to headius/sqlite-jdbc that referenced this issue Sep 19, 2024
In order to avoid the hard dependency on slf4j, this patch
abstracts the Logger interface and LoggerFactory factory to fall
back on a java.util.logging-based implementation if slf4j is not
present.

Fixes xerial#1094
headius added a commit to headius/sqlite-jdbc that referenced this issue Sep 21, 2024
In order to avoid the hard dependency on slf4j, this patch
abstracts the Logger interface and LoggerFactory factory to fall
back on a java.util.logging-based implementation if slf4j is not
present.

Fixes xerial#1094
gotson added a commit that referenced this issue Sep 23, 2024
@headius
Copy link
Contributor

headius commented Sep 24, 2024

#1178 was merged! We just need a release now.

@zoff99
Copy link
Author

zoff99 commented Sep 24, 2024

thanks for everybody who helped solve this.

Copy link
Contributor

🎉 This issue has been resolved in 3.46.1.1 (Release Notes)

@github-actions github-actions bot added the released Issue has been released label Sep 25, 2024
@headius
Copy link
Contributor

headius commented Oct 2, 2024

Thank you for the release!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested released Issue has been released
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants