Improved CDK Log4J interactions #878
Conversation
… package private so users don't create it in their own code (it should be via the factory). We also remove the compile time dependency on log4j-core, a use should include this them selves and may wish to use something like log4j-to-slf4j.
…me module so we can initialize it directly and don't need to use reflection.
…e explictly. We can switch on a system property in the XML now which I've done.
…bly throw an exception and is better controlled with Log Config
|
Not sure what is wrong with sonarcloud not finding the plugin |
seems to be a glitch on their side |
|
Maybe has to do with the update a few days ago? https://twitter.com/SonarQube/status/1560248339861577730 |
|
Reran and all fine, I will do some testing when I have to check it works correctly but if you have a look at changes you'll see the gist is to guard against errors @uli-f had. |
|
@johnmay, you merge it when you're done with your extra testing, right? |
|
Yep I’ll try and do that today |
|
Right testing things and sort of works but @egonw/@uli-f do we think it makes sense to provide a default logging config? Testing a simple Main like this: public class Main {
public static void main(String[] args) throws InvalidSmilesException {
SmilesParser smipar = new SmilesParser(SilentChemObjectBuilder.getInstance());
IAtomContainer mol = smipar.parseSmiles("CCO");
ILoggingTool loggingTool = LoggingToolFactory.createLoggingTool(Main.class);
loggingTool.warn("This is a warning!");
loggingTool.debug("This is a debug mesg!");
}
}By default you get STDERR messages... And following 391434c setting Now if I include
So include
The other option is we move it to "log4j2.xml" and it will be picked up by default. But as @uli-f normally you want the downstream application to set it's logging. My thoughts are we move Any other suggestions? |
|
To add.. if you don't specify the configuration Log4J default to Error. |
|
@johnmay Thank you so much for implementing those changes and summarizing your tests in the comment above. Best news first: The However, with the following dependencies in my (or better John's) downstream application I get the following stacktrace: My understanding is that we all agree that logging with all its APIs and libs is anything but easy to manage. By introducing another logging framework ( I'd opt for simplifying things by doing what most other libraries do:
A few more comments:
|
|
I had suggested this to Egon before but now I think I do like we have our even simpler API that doesn’t depend on anything. We don’t really use logging much anyways so the stderr isn’t much of an issue :). @dan2097 has noted before that the log4j2 api part has more downstream usages than slf4j. Also I believe log4j1, slf4j and logback are all the same author, and feels like rewarding bad behaviour. How are you running your application, I think your errors could be you have misspackaged it, property files may have gone missing. Also if you are using slf4j there is no need to include cdk-log4j/log4j-to-slf4j at all and instead just write your own wrapper that goes to slf4j? |
Okay. If that is what you agreed on, I am happy to come on board with the decision if it does not cause issues with any logging dependencies.
Good to know! I am not opinionated here as long as the logging is primarily done against a logging API and does not require a logging implementation.
Using IntelliJ which does a lot automagically, but is usually pretty good with maven builds. Checked out your branch, compiled, and installed in local repo. Then set up another maven project the the following And run your And now it does not crash anymore!
That is a good point and definitely something I might consider adding to the logging framework. @johnmay Would you prefer to have this added now or once the branch is merged? |
I think the conclusion was actually the opposite https://mvnrepository.com/artifact/org.slf4j/slf4j-api vs https://mvnrepository.com/artifact/org.apache.logging.log4j/log4j-api, but the other points still stand. [admitedly usage stats can be a bit misleading as legacy frameworks like log4j v1 and commons logging still rank very highly] |
That actually sounds good to me. |
I meant more that the v1.7 API doesn't appear to be able to bind to v2.0 implementations and vice versa, which sounds like itwould neccesitate exclusion of v1.7 artifacts if only some libraries have moved to v2.0 |
|
JWM Todo: may need to either add log4j-core to test or remove the cdk-log4j dependency. |
|
Kudos, SonarCloud Quality Gate passed! |
As per #873 depending on log4j-core causes issues. By removing some functionality e.g. runtime level set we don't need to depend on log4j-core and should make things work better overall.
WIP - need to do some testing on this in a down stream library