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

logback-android and logback-classic ILoggingEvent binary incompatibility getMarkers vs getMarkerList #365

Open
3 tasks done
ulfandersson opened this issue Apr 10, 2024 · 1 comment
Assignees

Comments

@ulfandersson
Copy link

ulfandersson commented Apr 10, 2024

Describe the bug

As stated in the readme, one option for unit tests is to use logback-classic as a replacement for logback-android.

Since the upgrade to sl4j 2.0 (#247) the ILoggingEvent class is no longer binary compatible with logback-classic.

Specifically, and where this matters, is with regards to markers.

logback-android has:
List getMarkers

logback-classic has:
Marker getMarker
List getMarkerList

Reproduction

Call getMarkers() on a ILoggingEvent in a custom encoder (extending EncoderBase<ILoggingEvent>), use this custom encoder from a test where logback-android has been replaced with logback-classic

Example:

class CustomEncoder : EncoderBase<ILoggingEvent>() {
   
    override fun headerBytes(): ByteArray? = null

    override fun footerBytes(): ByteArray? = null

    override fun encode(event: ILoggingEvent): ByteArray {
        return "markers: ${event.markers}".toByteArray()
    }
}

Logs

No response

logback-android version

3.0.0

OS Version

14

What logback configuration are you using? (logback.xml or Java/Kotlin code)

No response

Validations

  • Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
  • Check that this is a concrete bug. For Q&A, please open a GitHub Discussion instead.
  • The provided reproduction is a minimal reproducible of the bug.
@azabost
Copy link

azabost commented Jul 22, 2024

There are a few more differences compared to logback-classic 1.5.6, e.g. it added public default java.time.Instant getInstant() to ILoggingEvent.

What's worse, it requires Java 8 API for two reasons:

  1. because Date/Time API was added in Java 8
  2. because they now use default implementations in interfaces

Unfortunately, logback-android uses Java 6 compatibility level:

compileOptions {
    sourceCompatibility JavaVersion.VERSION_1_6
    targetCompatibility JavaVersion.VERSION_1_6
}

Additionally, logback-classic uses Instant.ofEpochMilli(getTimeStamp()) in the default implementation of getInstant(). When I changed the compatibility level of logback-android to Java 8 on my machine for testing purposes, I've got an error from IDE saying that

Call requires API level 26 (current min is 9): java.time.Instant#ofEpochMilli

I hope it could be implemented differently in logback-android but I'm not sure.

EDIT: it worked when I added coreLibraryDesugaringEnabled true and coreLibraryDesugaring "com.android.tools:desugar_jdk_libs:2.0.4" to build.gradle

By the way, Android Gradle Plugin sets Java 8 compatibility level by default since 4.2.0.

That being said, could logback-android also migrate to Java 8 and enable core library desugaring? @tony19 @mrcsxsiq

Oh, and one more thing I noticed recently. When I used JDK 21 to build a project, I noticed this warning:

Java compiler version 21 has deprecated support for compiling with source/target version 8.
Try one of the following options:

  1. [Recommended] Use Java toolchain with a lower language version
  2. Set a higher source/target version
  3. Use a lower version of the JDK running the build (if you're not using Java toolchain)

For more details on how to configure these settings, see https://developer.android.com/build/jdks.
To suppress this warning, set android.javaCompile.suppressSourceTargetDeprecationWarning=true in gradle.properties.

warning: [options] source value 8 is obsolete and will be removed in a future release
warning: [options] target value 8 is obsolete and will be removed in a future release

That might be yet another reason not to stay with such a low compatibility level.

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

No branches or pull requests

3 participants