feat: make slf4j optional with fallback on JUL

[headius]

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 #1094

[headius]

Note I have only implemented the bare minimum interface necessary to allow this patch to drop into existing code. Additional log levels and log level methods can be implemented in the future if necessary.

[gotson]

Hi, thanks for the PR. Can you check the failing CI tests ?

[gotson]

looks like you may have forgotten to commit the new Logger class

[headius]

looks like you may have forgotten to commit the new Logger class

Classic! Re-pushed with the missing files.

[headius]

I pushed two changes to address failures in a previous job run:

• Apply spotless formatting to the two small javadocs in Logger and LoggerFactory.
• Revert the changes to Makefile and Makefile.common as the slf4j jar file appears to be needed in those builds.

[gotson]

Revert the changes to Makefile and Makefile.common as the slf4j jar file appears to be needed in those builds.

it shouldn't be, but it's been a while since i looked at it

[headius]

it shouldn't be, but it's been a while since i looked at it

@gotson Originally the slf4j jar was added to those files during the addition of slf4j logging. Without the jar in CLASSPATH, the "amalgamation" job failed to run its javac targets. It is green now with the jar restored.

[headius]

Everything is green now. The job "test aarch64 ubuntu_latest jdk21" took a very long time, but I assume that is because it is running aarch64 tests in an emulator on an x86_64 job runner.

[gotson]

Without the jar in CLASSPATH, the "amalgamation" job failed to run its javac targets. It is green now with the jar restored.

we need to fix this, it should not be required. I pulled your PR code and tested locally, and the make native fails because of some issues in NativeDB.java:

"$JAVA_HOME/bin/javac"  -d target/common-lib -sourcepath src/main/java -h target/common-lib src/main/java/org/sqlite/core/NativeDB.java
src/main/java/org/sqlite/util/LoggerFactory.java:89: error: package org.slf4j does not exist
        final org.slf4j.Logger logger;
                       ^
src/main/java/org/sqlite/util/LoggerFactory.java:92: error: package org.slf4j does not exist
            logger = org.slf4j.LoggerFactory.getLogger(hostClass);
                              ^

I would say it's because you define this field in SLF4JLogger: final org.slf4j.Logger logger;

You can achieve the same behaviour by commenting the slf4j compile only block in pom.xml.

[gotson]

Not sure we can work around that though, maybe using reflection ?

[jcgay]

compile is already the default scope for a dependency with Maven. Adding it doesn't change anything : https://maven.apache.org/guides/introduction/introduction-to-dependency-mechanism.html#dependency-scope

I guess if we want to make it optional it should not be automatically added transitively as a dependency and we should use the <optional>true</optional> configuration ? https://maven.apache.org/guides/introduction/introduction-to-optional-and-excludes-dependencies.html

[gotson]

i am not very familiar with Maven, but when we reference the SLF4J package, java needs to know the package, even if it is not included in the build, to compile the code.

i think we can leave it as is, and leave slf4j in the makefile to compile NativeDB.java

[jcgay]

The <optional> configuration does not exclude the dependency for the sqlite-jdbc build. It will still be added to the classpath when compiling.

The <optional> configuration will only affect builds which depends on sqlite-jdbc. The slf4j-api will not be added as a transitive dependency. It should make the consumption easier like:

• one that don't want to use SLF4J would add:

<dependencies>
    <dependency>
      <groupId>org.xerial</groupId>
      <artifactId>sqlite-jdbc</artifactId>
      <version>3.46.1.0</version>
    </dependency>
</dependencies>

instead of:

<dependencies>
    <dependency>
      <groupId>org.xerial</groupId>
      <artifactId>sqlite-jdbc</artifactId>
      <version>3.46.1.0</version>
    </dependency>
    <exclusions>
       <exclusion>
         <groupId>org.slf4j</groupId>
         <artifactId>slf4j-api</artifactId>
       </exclusion>
     </exclusions>
</dependencies>

• one that want to use SLF4J would add:

<dependencies>
    <dependency>
      <groupId>org.xerial</groupId>
      <artifactId>sqlite-jdbc</artifactId>
      <version>3.46.1.0</version>
    </dependency>
<dependency>
      <groupId>org.slf4j</groupId>
      <artifactId>slf4j-api</artifactId>
      <version>1.7.36</version>
    </dependency>
</dependencies>

But if we want to use SLF4J the dependency should be already declared somewhere 😇.

It just triggers me because we say SLF4J is optional but it will still be added in the dependency graph if we don't exclude it explicitly. It may be not what people expect when using sqlite-jdbc but this is not necessarily a big deal if this is documented 😇.

[gotson]

i guess optional would be better than compile then ?

[jcgay]

In fact this is an addition to the compile scope, it would be:

<dependency>
            <groupId>org.slf4j</groupId>
            <artifactId>slf4j-api</artifactId>
            <version>1.7.36</version>
            <scope>compile</scope>
            <optional>true</optional>
</dependency>

or just (which is equivalent):

<dependency>
            <groupId>org.slf4j</groupId>
            <artifactId>slf4j-api</artifactId>
            <version>1.7.36</version>
            <optional>true</optional>
</dependency>

Because with Maven, when a scope is not set, it is compile by default.

The other way to reproduce the same transitive mechanism behavior (not adding slf4j-api automatically in the classpath when you depend on sqlite-jdbc) would be to replace the compile scope by provided. But it does change a little the meaning because it would indicate that we really need slf4j-api at runtime and that it should be provided by something else (an application server, etc): https://stackoverflow.com/questions/40393098/when-to-use-optional-dependencies-and-when-to-use-provided-scope

My 2 cents is to make the dependency <optional> in sqlite-jdbc which will not resolve slf4j-api transitively when we depend on sqlite-jdbc and let the consumer use this logging facade or not 😇.

[gotson]

My 2 cents is to make the dependency <optional> in sqlite-jdbc which will not resolve slf4j-api transitively when we depend on sqlite-jdbc and let the consumer use this logging facade or not 😇.

i am OK with that

[headius]

My 2 cents is to make the dependency <optional>

I did not know this! I will make the change and re-push.

[headius]

I have updated the build to make slf4j optional, as @jcgay suggested. This should resolve the last few concerns about this PR, but let me know if anything else needs to change!

[gotson]

@headius thanks for the PR. I have added one commit to update the coding tests for loggers. Thanks @jcgay for the maven insights.

[headius]

@gotson Thank you! Looking forward to the release.

[rmraya]

Hi,
It looks like you forgot to remove the dependency on org.slf4j from src/main/java9/module-info.java
Regards,
Rodolfo

[headius]

@rmraya D'oh! Could you push a PR? I believe it should be requires static.

From https://www.oracle.com/corporate/features/understanding-java-9-modules.html:

There is also a requires static directive to indicate that a module is required at compile time, but is optional at runtime. This is known as an optional dependency and won’t be discussed in this introduction.

[rmraya]

Marking as static means that it is needed at compile time and what I need is to get rid of slf4j jars.

[headius]

I'm not sure what the right option is then. Won't removing it altogether mean it cannot access those APIs at all at runtime?

[headius]

It still seems to me like requires static is correct. It marks the module as required only at compile time for that module. I don't think it would force you to have slf4j at compile time as a user of sqlite-jdbc.

https://www.baeldung.com/java-modularity#2-requires-static

[rmraya]

OK, let me build from source and test. I'll post a PR when I have it working.

[rmraya]

Created new PR #1205