-
-
Notifications
You must be signed in to change notification settings - Fork 174
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
feat!: upgrade slf4j to 2.0.3 #247
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Okay, realized it was not working previously because it needed an implementation of If you want to try it out, you can use:
|
Hi, @compscidr, do you need some further testing support? I was wondering if this PR could afterwards be merged as our project relies on this library as well. |
So far I've been using the changes from the PR using the jitpack build since I pushed them and it seems to be working well. It would be great to hear how others are making out with it. I did some pretty janky quickfixes to just make it api compatible - I'm curious about the markers and whether they will work well. I think merging directly into this repo now will likely break backwards compatibility issues with pre-2.x slf4j, so it might take some careful thought if both are to be maintained together. I'm not sure on the status of this repo / whether it is being maintained (it seems like there were some recent pushes in the last year or so), but if its not, I can make a fork that's compatible with 2.x and start to maintain it. I'd likely drop older slf4j support entirely and focus only on the new stuff. In the meantime, I just discovered the test suite errors, I'll at least get it passing those things and get the green check when I get a chance. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Hey @tony19, Cheers |
Just tested the implementation with: It still prints out:
No logs in either appender. Seems to me, that the provider is still not loading properly. Was that part of your fix, @compscidr? |
Hm that's weird, I have it working in a pretty complicated project atm - when I get a chance I'll see if I can make it work with a minimal example or something to verify there isn't some other interaction happening. |
Thanks that would be great. Are you also using the following?
Also, thanks for your fast reply. Cheers |
Okay here's a pretty minimal sample project with it working: https://github.com/compscidr/hello-kotlin-android - it does use target sdk 33 and source-compat VERSION_11 This is what it output to androids logcat:
And then it also logged to papertrail which I also often use alongside. |
logback-android/src/main/java/ch/qos/logback/classic/util/LogbackMDCAdapter.java
Show resolved
Hide resolved
logback-android/src/main/java/ch/qos/logback/core/net/AbstractSocketAppender.java
Outdated
Show resolved
Hide resolved
Thanks for the updates! I'll merge this in 3.x, since it's technically a breaking change (as you pointed out). |
Found the issue: |
Not sure if this is quite the right way of going about it, but was noticing if I updated slf4j to 2.0.x I wasn't getting logs showing up in Android any longer.
It looks like the only breaking change was in a few places
Marker
turned toList<Marker>
so I updated throughout the code to take this wherever it was needed.Tried it out against an android project and logging is now working again, so seems to work.
I have a feeling this will probably make things not work any longer with older slf4j though, so you may also not want to merge it, or make a different version for slf4j 2.x or something.
fix #249