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

Dependency on SLF4J - Migrate to System.Logger #156

Closed
XakepSDK opened this issue Oct 6, 2021 · 5 comments · Fixed by #190
Closed

Dependency on SLF4J - Migrate to System.Logger #156

XakepSDK opened this issue Oct 6, 2021 · 5 comments · Fixed by #190
Assignees
Milestone

Comments

@XakepSDK
Copy link

XakepSDK commented Oct 6, 2021

Can dependency on SLF4J be removed or library updated to 2.0-alpha? 1.7 is not modularized.

@rbygrave
Copy link
Contributor

rbygrave commented Oct 6, 2021

Can dependency on SLF4J be removed

Options:

  1. Change away from SLF4J API to Java logger (maybe this is ok'ish but does not feel like the right thing to do)
  2. Change the dependency to be scope provided (forces projects to explicitly specify the slf4j api dependency themselves)
  3. Document that projects can explicitly specify the SLF4J API before avaje-inject (in which case that version would trump the avaje-inject slf4j dependency)
  4. Update to 2.0-alpha (which is alpha? what is the impact of the api change, I don't myself?)
  5. Contribute to slf4j api to support module-info in the 1.7.x range

Sub-questions:

Why is 2.0-alpha still an alpha version?
Why should we not get a PR to 1.7.x to add module-info?

For me, I wonder if can we try and get a PR into slf4j-api 1.7.x to add module-info? If we managed to do that, that would help a LOT of projects and soften the upgrade path to module path. I don't know the details / differences with 2.0-alpha, what are the breaking changes from slf4j-api 1.7.x. to 2.0.x ? ... but getting 1.7.x to support module-info seems like a good thing to try and do.

Hmmm.

@XakepSDK
Copy link
Author

XakepSDK commented Oct 6, 2021

  1. 4 log events in test class, can be replaced with JUL (java.util.Logger)
    https://github.com/avaje/avaje-inject/blob/master/inject-test/src/main/java/io/avaje/inject/test/InjectExtension.java
  2. 1 debug log event here. Is it important?
    https://github.com/avaje/avaje-inject/blob/master/inject/src/main/java/io/avaje/inject/DBeanScopeBuilder.java
  3. 3 log events, 1 of them is important - logs exception
    https://github.com/avaje/avaje-inject/blob/master/inject/src/main/java/io/avaje/inject/spi/DBeanScope.java

Another option: Use multi-release jar to use SLF4J 1.7 for Java 8 and Platform Logger for Java 9+ (java.lang.System.Logger, in base module).

@rbygrave
Copy link
Contributor

rbygrave commented Dec 1, 2021

I have had a closer look at slf4j 2.x to see what was going on and why it's still alpha etc. Also just a little look at java.lang.System.Logger. For myself, I've been for so long just using slfj4-api that it had become a strong bias and I kind of missed java.lang.System.Logger arriving and yes it does look good to me.

That is, I'm starting to remove my bias away from slf4j-api and leaning towards the thought that java.lang.System.Logger might be the longer term better option when compared to slf4j-api 2.x. Given that avaje-inject has such little need for logging anyway it makes it more obvious for avaje-inject that we should probably push it towards the standard java logger(s)

Use multi-release jar to use SLF4J 1.7 for Java 8 and Platform Logger for Java 9+

We could do this or even use JUL for Java 8 and Platform Logger for 9+. At some point we also raise the question of moving our minimal java dependency from 8 to 11 which would mean we should just use the Java Platform Logger / java.lang.System.Logger and wouldn't need multi-release jar (but MR-jar should also be a pretty good and simple approach here).

Next steps
I think the next step is to try and prepare a PR that looks to take a multi-release jar approach with JUL and Platform Logger and see what that looks like.

rbygrave added a commit that referenced this issue Dec 14, 2021
@rbygrave
Copy link
Contributor

Note that I have the PR/branch https://github.com/avaje/avaje-inject/pull/172/files ... but I am somewhat unsatisfied with it at the moment.

My thought is that we almost certainly should change to System.Logger. If we bump to Java 11 then this is pretty simple and I am thinking that is maybe the best long term path going forward. To bump to Java 11 and at that time also change to use System.Logger.

https://www.reddit.com/r/java/comments/rdv98z/have_you_ever_wondered_how_javas_logging/ho49gp1/

@rbygrave rbygrave changed the title Dependency on SLF4J Dependency on SLF4J - Migrate to System.Logger Dec 14, 2021
@rbygrave rbygrave pinned this issue Dec 14, 2021
@rbygrave rbygrave added this to the 8.0 milestone Mar 15, 2022
@rbygrave rbygrave self-assigned this Mar 15, 2022
@rbygrave rbygrave linked a pull request Mar 21, 2022 that will close this issue
@rbygrave
Copy link
Contributor

PR #190 bumps to Java 11 and uses System.Logger. Closing this as now fixed.

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

Successfully merging a pull request may close this issue.

2 participants