-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Split and modernize JANSI support. #2916
Conversation
This commit: - Adds a new `ConsoleStreamProvider` plugin type that can decide how the Console Appender stream is built. - Moves JAnsi support to a new `log4j-jansi` module. - Upgrades the JAnsi library to version 2.x Fixes #1736.
This is blocked by #2691: if the |
JANSI is deprecated, see fusesource/jansi#294 for details.
We rewrite `JAnsiTextRenderer` to use our internal `AnsiEscape` util instead of the external JAnsi library.
I rewrite |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Module should not be named "log4j-jansi" since the PR no longer uses JAnsi. How about "log4j-ansi"?
The module uses The |
@ppkarwasz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my remark about removing the JAnsi support, completely.
This is blocked by #2691: if the
TextRenderer
interface is removed from the%ex
pattern converter, theJAnsiTextRenderer
can be removed, otherwise it should be kept.
#2691 cannot remove anything, since it is against 2.x
. But be my guest to completely remove it while porting it to main
. Hence, I see two paths we can take:
- Wait for Consolidate stack trace rendering in Pattern Layout #2691 to get merged to
2.x
, port it tomain
, and then removeTextRenderer
inmain
. - Remove
TextRenderer
inmain
without waiting for Consolidate stack trace rendering in Pattern Layout #2691.
[@alan0428a, I am CC'ing you, in case you're interested in.]
Of course you can not remove it, but as far as I can tell, you are bypassing it. Since |
Note that in this PR we are discussing 3 related, but distinct features: Enabling ANSI escapes on Windows terminalsThis feature is probably useless and can be dropped. It requires the JAnsi library. Detecting if the console is a terminalThe JAnsi library is very good at it. It can detect separately if In Log4j Core 2 we emulate this behavior by checking if Coloring of stack traces and messagesIn Log4j Core 2, JAnsi was also used to color stack traces and messages. This is enabled by adding This PR rewrites |
How do you feel about removing JAnsi (but not ANSI) support as a test? For this PR it would mean:
|
Hi @ppkarwasz ATM, I only care that my Log4j 2 XML configurations don't break and keep their current colorized output. However that happens should be an implementation detail. |
# Conflicts: # log4j-core-test/pom.xml # log4j-core/pom.xml # log4j-parent/pom.xml
@vy, Should I port the changes to |
@ppkarwasz, yes, please do so. Curious: Does that translate to no JAnsi dependency on |
Are you suggesting we drop an end-of-life dependency (Jansi 1.x) released by a project that does not exist any more (Jansi, which was replaced by JLine) and break backward compatibility for Windows platforms older than 2017? 😛 Windows 7 and previous releases have a 3.5% market share, so I think we could do it. |
@ppkarwasz, yes, let's please indeed drop the JAnsi dependency. |
Done in #3070 |
This PR:
ConsoleStreamProvider
plugin type that can decide how the Console Appender stream is built.log4j-jansi
module.Upgrades the JAnsi library to version 2.xReplaces JAnsi with JLine 3, since JAnsi is deprecated (cf. Archive this repository, Jansi is part of Jline3 now fusesource/jansi#294).JAnsiTextRenderer
to use our internalAnsiEscape
utility.Fixes #1736.