maven icon indicating copy to clipboard operation
maven copied to clipboard

[MNG-7083] - introduce lazy Log message evaluation

Open McFoggy opened this issue 4 years ago • 10 comments

as discussed in the dev mailing list [1][2] this PR enhances org.apache.maven.plugin.logging.Log by adding a lazy log method with a java.util.function.Supplier for each log level. For each level the supplier will be called only if the corresponding log level is active. Using java 8 interface default method ensures backward compatibility with other potential existing implementations.

Following this checklist to help us incorporate your contribution quickly and easily:

  • [X] Make sure there is a JIRA issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
  • [X] Each commit in the pull request should have a meaningful subject line and body.
  • [X] Format the pull request title like [MNG-XXX] - Fixes bug in ApproximateQuantiles, where you replace MNG-XXX with the appropriate JIRA issue. Best practice is to use the JIRA issue title in the pull request title and in the first line of the commit message.
  • [X] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • [X] Run mvn clean verify to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
  • [X] You have run the Core IT

Still two unrelatd ITs tests are failing locally

[ERROR] Errors:
[ERROR]   MavenITmng5669ReadPomsOnce>AbstractMavenIntegrationTestCase.runTest:255->testWithBuildConsumer:119 » Verification
[ERROR]   MavenITmng5669ReadPomsOnce>AbstractMavenIntegrationTestCase.runTest:255->testWithoutBuildConsumer:65 » Verification

If your pull request is about ~20 lines of code you don't need to sign an Individual Contributor License Agreement if you are unsure please ask on the developers list.

To make clear that you license your contribution under the Apache License Version 2.0, January 2004 you have to acknowledge this by using the following check-box.

PS: I'll verify by asking on the mailing list if I have an ICLA already, or fill one if not.

McFoggy avatar Jan 26 '21 11:01 McFoggy

Initially I did not wanted to pollute stdout & stderr that's why I mocked them in SystemStreamLogTest. Thinking more about it, it can be a bad idea to have mocked them. If required I'll remove the mock and let the SystemStreamLogTest test output messages.

McFoggy avatar Jan 26 '21 11:01 McFoggy

I really can't believe we are reinventing the wheel by implementing SLF4J Light.

michael-o avatar Jan 26 '21 12:01 michael-o

I really can't believe we are reinventing the wheel by implementing SLF4J Light.

that's not what I understood from the different discussions. I really thought that it was expected to leverage the org.apache.maven.plugin.logging.Log abstraction which represents the public interface that plugins should see and use.

McFoggy avatar Jan 26 '21 13:01 McFoggy

@McFoggy it is the outcome of the discussions because slf4j is 1. unstable, 2. has no correct binding impl (it is a singleton leading to a bunch of issues in mojo when transparently exposed) 3. does not enable to switch the binding until you reimpl slf4j by design 4. is not contextual with most available impl and 5. we already reimplement slf4j and just inherit from a small API part but not the rest. I fully understand it can be surprising but it is the outcome of most vendors in most projects caring of the context and to have a pluggable logging impl so it is not that shocking after some thoughts as seen in the discussion.

rmannibucau avatar Jan 26 '21 13:01 rmannibucau

@McFoggy it is the outcome of the discussions because slf4j is 1. unstable, 2. has no correct binding impl (it is a singleton leading to a bunch of issues in mojo when transparently exposed) 3. does not enable to switch the binding until you reimpl slf4j by design 4. is not contextual with most available impl and 5. we already reimplement slf4j and just inherit from a small API part but not the rest. I fully understand it can be surprising but it is the outcome of most vendors in most projects caring of the context and to have a pluggable logging impl so it is not that shocking after some thoughts as seen in the discussion.

I would expect such discussions to be held with @ceki instead with just us.

michael-o avatar Jan 26 '21 14:01 michael-o

FYI ICLA received & handled by Apache secretary, signed CLAs list updated (https://people.apache.org/unlistedclas.html)

McFoggy avatar Jan 26 '21 18:01 McFoggy

If asked for, I will squash the commits.

McFoggy avatar Jan 26 '21 18:01 McFoggy

I'd like to see how much we can improve our core code with these changes

slachiewicz avatar May 30 '21 14:05 slachiewicz

I really don't think we want to invent new logging API. Let me explain:

  • I do agree with @rmannibucau et al that "slf4j-api should not leak into plugins", but IMHO none of the reasons listed above is the real issue.
  • IMHO, the real issue is that slf4j-api is "like Guava", is literally everywhere, and (in short term future) will be really easy to end up (in complex plugins, kinda edge case) with a class-path that contains 1.5, 1.7 and 2.x versions of slf4j-api (and let's assume they will be incompatible).
  • This is true for complex plugins that usually drag in components from different dependencies/projects. OTOH, simple plugins (dependency wise, not functionality or logic wise) that has only components "rolled by it's own" CAN control which ~~slf4j-api~~ Logging API it expects and uses.
  • Hence, IMHO Maven should not be an impediment (or trouble source) to developers of "complex plugins", by adding a "yet another" slf4j-api version to their (possibly already complex) equation to solve.

Still, as plugin realms are "self first", I think plugin developers are able to solve Mojo (and their component, rolled by own or imported from foreign projects a dependencies) using any logging API they want.

Considering all this above, I'd rather expect to make Mojo Logger "closer" to slf4j-api, not only to simplify our work (to adapt slf4j logger to Mojo Logger), but also as sl4j logger is lazy as well, without any extra code. As in case of Mojo Logger, our wrapper will protect users of Mojo logger from sl4j-api incompatibilities (is provided at runtime by Maven Core).

cstamas avatar Oct 02 '21 08:10 cstamas

Proposal to supersede this https://github.com/apache/maven/pull/565

cstamas avatar Oct 05 '21 20:10 cstamas