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

Clean logging dependencies #11148

Closed
jhoeller opened this issue Nov 25, 2017 · 9 comments
Closed

Clean logging dependencies #11148

jhoeller opened this issue Nov 25, 2017 · 9 comments
Assignees
Labels
type: task A general task
Milestone

Comments

@jhoeller
Copy link

The Spring Boot 2.0 starters seem to have a few oddities in master still:

  • The core starter excludes commons-logging from spring-core (which isn't necessary anymore with Spring Framework 5.0's spring-jcl arrangement).

  • spring-boot-starter-logging brings in log4j-over-slf4j which is a legacy Log4J 1.2 API binding (EOL'ed). Isn't it just meant to bring in log4j-to-slf4j for the Log4J 2.x API binding?

  • spring-boot-starter-log4j2 brings in log4j-core as well as log4j-api but the latter is a transitive dependency of the former anyway, at least in all recent versions.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Nov 25, 2017
@wilkinsona
Copy link
Member

wilkinsona commented Nov 25, 2017

The core starter excludes commons-logging from spring-core

Thanks. That's an oversight and we should tidy it up.

spring-boot-starter-logging brings in log4j-over-slf4j which is a legacy Log4J 1.2 API binding

This was intentional, but perhaps it's becoming unnecessary? The reason behind keeping the dependency was that we still want to be able to bridge the logging from libraries that use Log4J 1 into SLF4J.

spring-boot-starter-log4j2 brings in log4j-core as well as log4j-api

Thanks. We should tidy this up too.

@wilkinsona wilkinsona added priority: normal type: task A general task and removed status: waiting-for-triage An issue we've not yet triaged labels Nov 25, 2017
@wilkinsona wilkinsona added this to the 2.0.0.M7 milestone Nov 25, 2017
@jhoeller
Copy link
Author

Since Log4J 1.x has been EOL'ed in 2015, I doubt that there any relevant libraries using it still... in particular through its native API. There might just be the odd setup out there using it behind Commons Logging still. From that perspective, I would simply drop that Log4J 1.2 API bridge for good... also removing an inconsistency between the Logback vs Log4J 2 starters (where the similarly purposed log4j-1.2-api jar was never declared to begin with, as far as I can tell).

@wilkinsona wilkinsona self-assigned this Nov 27, 2017
@wilkinsona
Copy link
Member

wilkinsona commented Nov 27, 2017

We still have a number of Spring projects that depend upon it either directly or transitively. Those appear to be:

  1. Spring Batch (BATCH-2656)
  2. Spring Kafka (transitively, so out of our control)
  3. Spring Retry (Drop use of Log4J 1.x spring-retry#95)

Spring Batch seems to be the most dependent on Log4J 1.x. Hopefully @mminella can tidy that up rather than carrying support into the new 4.x major line.

Spring Kafka's transitive dependency is via Kafka and, I think, a ZooKeeper client library that only affects spring-kafka-test. I don't think this should prevent us from dropping the bridge.

Spring Retry seems to build happily without its optional Log4j 1.2 dependency, so I think we're in the clear there.

@wilkinsona
Copy link
Member

It was only a test that was actually using Log4j in Batch so I think we can go ahead and tidy all of this up in M7.

@wilkinsona
Copy link
Member

I missed one in spring-boot-starter-parent.

@isopov
Copy link
Contributor

isopov commented Nov 29, 2017

@wilkinsona According to https://freemarker.apache.org/docs/pgui_misc_logging.html freemarker 2.3.* needs log4j-over-slf4j removed in this issue (still this version in https://github.com/spring-projects/spring-boot/blob/master/spring-boot-project/spring-boot-dependencies/pom.xml )
while 2.4.* does not. So this still library needs it also

@wilkinsona
Copy link
Member

Thanks, @isopov. By my reading of the FreeMarker documentation, we're ok here. It'll now use JUL (rather than SLF4J/Log4J) which should still be routed into Logback.

@wilkinsona
Copy link
Member

wilkinsona commented Nov 29, 2017

Our FreeMarker sample, with debug logging enabled, produces output similar to the following:

2017-11-29 13:37:12.898 DEBUG 89595 --- [nio-8080-exec-2] freemarker.cache                         : Couldn't find template in cache for "welcome.ftl"("en_GB", UTF-8, parsed); will try to load it.
2017-11-29 13:37:12.900 DEBUG 89595 --- [nio-8080-exec-2] freemarker.cache                         : TemplateLoader.findTemplateSource("welcome_en_GB.ftl"): Not found
2017-11-29 13:37:12.901 DEBUG 89595 --- [nio-8080-exec-2] freemarker.cache                         : TemplateLoader.findTemplateSource("welcome_en.ftl"): Not found
2017-11-29 13:37:12.901 DEBUG 89595 --- [nio-8080-exec-2] freemarker.cache                         : TemplateLoader.findTemplateSource("welcome.ftl"): Found
2017-11-29 13:37:12.901 DEBUG 89595 --- [nio-8080-exec-2] freemarker.cache                         : Loading template for "welcome.ftl"("en_GB", UTF-8, parsed) from "/Users/awilkinson/dev/spring/spring-boot/master/spring-boot-samples/spring-boot-sample-web-freemarker/target/classes/templates/welcome.ftl"
2017-11-29 13:37:13.045 DEBUG 89595 --- [nio-8080-exec-2] freemarker.cache                         : "welcome.ftl"("en_GB", UTF-8, parsed) cached copy not yet stale; using cached.

I think we're fine to drop the old Log4J 1 bridge. We'll keep dependency management for it (as it's part of SLF4J which we manage) so it's easy for people to add in themselves if they really need to do so.

@jhoeller
Copy link
Author

Alternatively, one could set -Dorg.freemarker.loggerLibrary=CommonsLogging (or =SLF4J) to enforce the same kind of routing, in particular in plain Spring Framework 5 setups. The Log4J 1.2 API really needs to disappear from our runtime environments for good... two years after its EOL.

Also, Boot's Log4J 2 starter never included such a Log4J 1.2 API bridge. So I suppose FreeMarker was always logging through JUL there anyway? In any case, this is also about consistency between those logging starters, so even just for that reason we should push forward here.

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

No branches or pull requests

4 participants