Remove apache commons logging in favor of slf4j + log4j for JAVA 9 #3653
Conversation
Siedlerchr
added
status: ready-for-review
There was a problem hiding this comment.
I'll admit that I haven't looked through every single statement, since most of them are practically identical. Nevertheless, I think this is good to go. Updating/removing ancient dependencies is always appreciated and here it is especially nice that there are so little changes except for the logger declarations.
The build failure is just down to the fetcher that you've fixed in a different PR, as far as I can see.
|
The breaking build is still from the test I fixed in the other pr |
There was a problem hiding this comment.
Looks good.
Maybe, we could also Apache Log4J2 and use LogBack completely. Currently alpha only... (https://logback.qos.ch/download.html)
| @@ -0,0 +1,31 @@ | |||
| # Use slf4j together with log4j2 for logging | |||
|
|
|||
| * Currently JabRef uses apache commons logging 1.2 for logging errors and messages. However, this is not compatible with java 9 and is superseeded by log4j. | |||
|
|
||
|
|
||
| ## Decision Outcome | ||
|
|
There was a problem hiding this comment.
Please place the decision here. :-)
The next subsection is for a detailed elaboration.
| * `+` *Already defined* | ||
| * `+` *Java 9 support* | ||
| * `-` *Direct dependency* | ||
| * *[...]* <!-- numbers of pros and cons can vary --> |
|
|
||
| ## Decision Outcome | ||
|
|
||
| ## Pros and Cons of the Alternatives <!-- optional --> |
|
|
||
| ### *Log4j2* | ||
|
|
||
| * `+` *Already defined* |
There was a problem hiding this comment.
Please remove the stars. They are only meant as placeholders...
| ## Considered Alternatives | ||
|
|
||
| * [Log4j2](https://logging.apache.org/log4j/2.x/) | ||
| * [SLF4J](https://www.slf4j.org/) |
There was a problem hiding this comment.
I think, the options are:
- Log4J2
- SLF4J with Logback binding
- SLF4J with Log4J binding
Because of https://logging.apache.org/log4j/2.0/log4j-slf4j-impl/, it seemed, you chose the Log4J binding
|
Thank you for using MADR 🎉. |
|
Switching to logback could maybe solve #3511? |
add some more info to ADR
|
|
||
| ### Log4j2 | ||
|
|
||
| * `+` Already defined |
There was a problem hiding this comment.
Do you mean "already defined as dependency"? --> Maybe rephrase to "Dependency already exists"
* upstream/master: fix isbn fetcher test as Joshua Bloch has published a new revivsion of Effetive Java convert to junit5 jupiter
make coday happy
|
Codacy still complains about an unused import, but this is a false positive, right? Otherwise, checkstyle would complain. If this is not a problem, then this PR can be merged. |
|
Interesting case. the import was only used because it was referenced in an javadoc comment. So I removed it. |
* upstream/master: (46 commits) Replace openoffice jars with libreoffice jars (#3662) Export no empty lines in RIS format (#3661) Remove apache commons logging in favor of slf4j + log4j for JAVA 9 (#3653) fix isbn fetcher test as Joshua Bloch has published a new revivsion of Effetive Java convert to junit5 jupiter Extend RIS import with multiple fields (#3642) Fix ICAR fetcher test which resulted in build failure (#3654) New translations JabRef_en.properties (French) (#3650) Add link to MADR and fix typo [WIP] Add "Convert to BibTeX format" cleanup (#3541) Fix typo Fix typo Quickfix to get build running on all platforms (#3638) New translations JabRef_en.properties (French) New translations JabRef_en.properties (Vietnamese) New translations JabRef_en.properties (Chinese Simplified) New translations JabRef_en.properties (Dutch) New translations JabRef_en.properties (French) New translations JabRef_en.properties (German) New translations JabRef_en.properties (Greek) New translations JabRef_en.properties (Indonesian) ...
While testing with Java 9, I noticed that commons logging infrastructure we use is not compatible.
However, sfl4j + log4j2 are already java 9 compatible.
Added ADR