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

Add a Log4j2 build listener #192

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ppkarwasz
Copy link
Contributor

@ppkarwasz ppkarwasz commented Aug 6, 2022

This PR adds a Log4j2 build listener as a modern alternative to the obsolete Log4j 1.x listener.

The Log4j2 listener differs in some choices from the Log4j counterpart. Most notably:

  • the name of the loggers follows the pattern project_name.target_name.task_name,
  • the class name of the Java object that performs the logging is recorded in the className property of the location info (%C in the pattern layout of Log4j2 Core),
  • the name of the build file and the current line number are recorded in the corresponding properties of the location info (%F and %L),
  • VERBOSE is not mapped to DEBUG, but a custom level is used.

The Log4j2 listener can be used as BuildLogger, but the level change feature works only if the underlying Log4j2 API implementation is Log4j2 Core.

This PR adds a Log4j2 build listener as a modern alternative to the
obsolete Log4j 1.x listener.
@bodewig
Copy link
Member

bodewig commented Aug 7, 2022

Many thank @ppkarwasz in particular since you found all the places that need to be touched when adding a new dependency. :-)

TBH I don't really see any reason why this should be part of the core Ant distribution. Publishing the logger as a separate artifact with a s release cycle separate from Ant itself may be a better idea than adding another (even if optional) dependency to Ant.

@vlsi
Copy link

vlsi commented Aug 7, 2022

I agree, it does not make sense adding dependency on log4j2.
It would be better adding a slf4j build listener.

@ppkarwasz
Copy link
Contributor Author

@bodewig, I understand the reluctance of adding yet another component with external dependencies to maintain, but I believe this should be a low maintenance listener. If you consider adding it to the Ant project, I can help maintaining it.

Otherwise I'll publish it as a separate artifact.

@visi: I don't believe we are in the position to impartially decide, which API between Log4j2 and SLF4J is better, :-). SLF4J (even the recent 2.0.0-beta1) does not provide a way to set the location (file name/line number) programmatically, but that could probably be provided as MDC entries.

@vlsi
Copy link

vlsi commented Aug 7, 2022

I can help maintaining it.

Well, it would be nice to avoid dependency on log4j2 if possible since any new CVE would trigger cases like "Ant is using vulnerable log4j"

@ppkarwasz
Copy link
Contributor Author

This would be just a marketing problem: Log4j2 Core's vulnerabilities never affected the Log4j2 API as well as Logback's vulnerabilities never affected SLF4J. In the case of a new CVE against Log4j2 Core, some users will inquire if it affects Ant anyway (as they did for Log4Shell). In the panic that followed last December's events probably wondered if their old HP calculator was affected. :-)

This listener does not depend on Log4j Core. The setMessageOutputLevel method uses Log4j Core if it is present, but I can also add support for Logback if needed.

@rgoers
Copy link
Member

rgoers commented Aug 7, 2022

@vlsi What a pointless comment. ANY software can have a CVE. For the record, the Log4j 2 API has never had any CVE's reported against it. From a functionality point of view it is exactly the same as using SLF4J except it has more features.

What you are doing is equivalent to saying "Don't use SLF4J because Logback has had CVE's reported against it:.

TBH we've considered renaming the Log4j API to something like LA4J (Logging API for Java) or JLA (Java Logging API) just to avoid arguments like yours. But we figure most people with average intelligence will understand if they simply read the documentation. Perhaps we will make a new web site for JLA and the download will just be log4j-api.

All that said, I am not weighing on on what should be done with Piotr's contribution. That is up to the Ant PMC to decide.

@bodewig
Copy link
Member

bodewig commented Aug 8, 2022

I'd say the same thing about an SLF4J based logger implementation. My reluctance to adding new dependencies for things that could be add-ons - maybe just a separate antlib under the umbrella of the Ant project - rather than Ant's core has nothing to do with the dependency being log4j 2.x at all.

If we add something the existing dev team is not really interested in - otherwise the logger would exist already - maintaining it becomes a chore that either requires somebody of the dev team has to take care of things they are not really familiar with or relies on somebody external taking over maintenance. Here I feel it would be easier if the artifact was provided by somebody who cares for it.

I do hear the "this is a low-maintenance contribution" argument and am not saying NO here, just voicing my opinion.

We are maintaining things we accepted more than twentytwo years ago (<netrexxc> anyone :-) ) just to give a perspective on what maintenance involves.

@vlsi
Copy link

vlsi commented Aug 8, 2022

My reluctance to adding new dependencies for things that could be add-ons

+1

I'd say the same thing about an SLF4J based logger implementation

Well, at least slf4j allows plugging several implementations, so if slf4j is ever added, then users could add whatever logging impl they want via the relevant slf4j-.. bridge.
That does not mean slf4j should be added to Ant, however, I mean that if log4j2 appender is added, then it incurs extra dependency and risk, while it is useful only for log4j2 users.
It might be that in a couple of years, it would require adding ant-log4j3, and so on.

this is a low-maintenance contribution

Well, I could easily imagine how users might want to make the logging level to be configurable.
For instance, users might want to log private targets at a lower level than public ones, so the default output becomes more user-friendly.

Then, the contribution does not clarify the intended use case.
Is it intended to be used by humans? If so, why the default Ant output is not enough?

Is it supposed to be used for machine integration? (e.g. a CI server parsing log output) If so, why the default Ant's BuildListeners are not enough?

This does not look like a low-maintenance contribution to me 🤷‍♂️ . Replacing log4j2 with slf4j would not answer "what are the use-cases" questions.

It is more like low-unmaintenance contribution.

@ppkarwasz
Copy link
Contributor Author

We are maintaining things we accepted more than twentytwo years ago (<netrexxc> anyone :-) ) just to give a perspective on what maintenance involves.

@bodewig, I perfectly understand the problem, since the Apache Logging project has the same problem with components nobody has been using for years. As Ralph said, it is to the Ant PMC to decide, if you are interested in this contribution.

Well, at least slf4j allows plugging several implementations, so if slf4j is ever added, then users could add whatever logging impl they want via the relevant slf4j-.. bridge.

@vlsi, you perfectly know that the Log4j2 API has also several implementations (cf. API separation).

Well, I could easily imagine how users might want to make the logging level to be configurable.
For instance, users might want to log private targets at a lower level than public ones, so the default output becomes more user-friendly.

If such a need occurs I can modify the listener implementation to log a custom Message type that will wrap the BuildEvent object. This way the user will be able to write all sorts of filters. The Message interface provides a way to convert it to a list (String, Object...), hence this listener could also be used with the Log4j2-to-SLF4J bridge.

Then, the contribution does not clarify the intended use case.
Is it intended to be used by humans? If so, why the default Ant output is not enough?

Is it supposed to be used for machine integration? (e.g. a CI server parsing log output) If so, why the default Ant's BuildListeners are not enough?

I plan to use this listener at work to store the build logs in a structured way.

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 this pull request may close these issues.

4 participants