[MNG-7532] Log shouldn't have been deprecated so ensure it is not

[rmannibucau]

Follow up of dev discussions and to avoid to reduplicate the API in the new plugin API, restoring Log official status.

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

• 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.
• Each commit in the pull request should have a meaningful subject line and body.
• Format the pull request title like [MNG-XXX] SUMMARY, where you replace MNG-XXX
  and SUMMARY 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.
• Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
• Run mvn clean verify to make sure basic checks pass. A more thorough check will
  be performed on your pull request automatically.
• You have run the Core IT successfully.

[michael-o]

I still don't understand why we need a wrapper around a facade...

[rmannibucau]

@michael-o I don't have all the threads handy but we have several dev@ threads explaining it has several technical pitfalls (the way the API loads the impl - v1 or v2) and that we don't want to bind ourselves to an implementation we can't sustain in time. This PR just avoids to have to redefine yet another package and API for plugin (work Guillaume is doing) since we already have it and can enhance it with Supplier or alike for formatter like features - more efficient as of today.

[michael-o]

@michael-o I don't have all the threads handy but we have several dev@ threads explaining it has several technical pitfalls (the way the API loads the impl - v1 or v2) and that we don't want to bind ourselves to an implementation we can't sustain in time. This PR just avoids to have to redefine yet another package and API for plugin (work Guillaume is doing) since we already have it and can enhance it with Supplier or alike for formatter like features - more efficient as of today.

I see, what I would like to avoid is to add features to this wrapper which will duplicate SLF4J at the end. We have this: Plexus Logging.

[rmannibucau]

@michael-o right but it is not clear what we do of plexus - we are between being based on it and dropping it. Personally I tend to think we should rely on the abstraction we want to expose long term in a very stable way so either JUL (which can be used as an abstraction) or an org.apache.maven.somethingLogging but if we import plexus @asf it sounds like another valid option.

[michael-o]

@michael-o right but it is not clear what we do of plexus - we are between being based on it and dropping it. Personally I tend to think we should rely on the abstraction we want to expose long term in a very stable way so either JUL (which can be used as an abstraction) or an org.apache.maven.somethingLogging but if we import plexus @asf it sounds like another valid option.

Do you expect SLF4J 2.x not to live 10 years?

[michael-o]

I'd be interested to head @ceki's opinion on this.

[rmannibucau]

Do you expect SLF4J 2.x not to live 10 years?

I expect it to not be usable within 5 years at least due to the current usage of 1.x and SLF4J is a good example of why we shouldnt rely on a 3rd party for our own API (once again, point is used by users), it broke several times and we can't help.
Only facade we can leverage would be Java Util Logging if you really want to kill our existing logging abstraction and use something existing IMHO.

[ceki]

The client facing API of SLF4J is stable and has been stable since version 1.0. You can compile an application using the API in SLF4J 1.0 (released in 2005) and it will run just fine with SLF4J version 2.0 on the classpath.

As for Maven creating its own logging API, there is a FAQ entry treating this question.

Anyway, thank you all for your awesome work on Maven.

[rmannibucau]

The client facing API of SLF4J is stable and has been stable since version 1.0.

Not really 1.5->1.6 were not compat for ex (LocationAwareLogger#log, MessageFormatter, etc...). It hurt several apps already.
So even I agree it is generally stable, it is not something we can assume for our api.
It also increase the work needed to stay compatible since we have our own binding.

It is really not "is slf4j bad" topic - in particular since it is fine to use it in @apache/maven core modules - but simply a design issue (we could workaround technical details if we would like to use it on a pure technical side), we should own our API to ensure mojo and extensions are reliable in time and not highly unstable (like gradle had been and was forced to introduce the wrapper to workaround it).

[ceki]

@rmannibucau Since you mention LocationAwareLogger, the person who has been been publicly and loudly complaining about LocationAwareLogger for the last 12 years is the same person who asked for that breaking change in the first place. I am sure that is a bit hard to believe but please have a look at SLF4J-118. The ticket is rather short and quite informative in its own right.

As for stability, perhaps Maven has a higher standard regarding API compatibility than SLF4J, as Maven really shines in that regard.

[michael-o]

The client facing API of SLF4J is stable and has been stable since version 1.0.

Not really 1.5->1.6 were not compat for ex (LocationAwareLogger#log, MessageFormatter, etc...). It hurt several apps already. So even I agree it is generally stable, it is not something we can assume for our api. It also increase the work needed to stay compatible since we have our own binding.

It is really not "is slf4j bad" topic - in particular since it is fine to use it in @apache/maven core modules - but simply a design issue (we could workaround technical details if we would like to use it on a pure technical side), we should own our API to ensure mojo and extensions are reliable in time and not highly unstable (like gradle had been and was forced to introduce the wrapper to workaround it).

You are mostly talking about impl, not API not. For the plugin dev.SLF4J simply works.

[rmannibucau]

You are mostly talking about impl, not API not. For the plugin dev.SLF4J simply works.

No @michael-o this is 100% the API sadly. We went through that several times on several apache projects as mentionned and as mentionned it is not only due to slf4j quality but also our own design so let's just do our homework and align on the dev@ discussion our code.

@ceki the people I saw complaining about it was not this one so guess it was not alone bitten by this change. I also think the "lifecycles" of both projects are not aligned there and to be honest, exposing SLF4J creates several issues in MOJO - once again, nothing linked to SLF4J, 100% linked to Maven classloading and design even if for the good - so in terms of design and technically Maven should really aim at abstracting it even if I agree with Michael, it means another abstraction but it is highly justified by years of experience and no real good fixes without it as proven by the past.

[slachiewicz]

it's incomplete - we would also update maven site and say that slf4j introduced for logging in 3.1 is now obsolete?

[rmannibucau]

@slachiewicz there are some discussions about the related pages since there was never a consensus nor a consistent update of that on our website.
Short term goal of that pr was to avoid to reintroduce the same api in another package for mvn 4 plugin api since we already have a first citizen api we mis-deprecated.
Guess we can split the work and keep the doc topic for another pr/the already open thread.

[gnodet]

it's incomplete - we would also update maven site and say that slf4j introduced for logging in 3.1 is now obsolete?

It seems to me that https://maven.apache.org/maven-logging.html is mostly about the logging system in general, not really the api to use for mojos.

[rmannibucau]

yes, slf4j was never validated for mojo, there was an excess of enthusiasm in the javadoc but it was never validated nor ack-ed by the community so no big deal.

[slawekjaranowski]

For Maven plugins we can provide own API or abstraction ... but users can use third party library for implementing plugin code.

We can not expect that all code in plugins only use Maven and JDK API, thirt party library used in plugins code can uses any logging framework.

It should not be important what logging framework is used in Maven core.
Technically we can forward logging message from one framework to another and it can be transparent for plugins code.

So I'm for not providing next abstraction but allow users use some of commonly available frameworks, like spring-boot does: https://docs.spring.io/spring-boot/docs/current/reference/html/features.html#features.logging

[rmannibucau]

We can not expect that all code in plugins only use Maven and JDK API, thirt party library used in plugins code can uses any logging framework.

Sure and we comply to that, only thing is that end user can only expect maven logging framework to be integrated with maven ecosystem (think CI and maven customizations) so if the plugin provider wants something well integrated it has to use that.
frontend maven plugin forwarded node logs to maven logging system for that to give an example.

It should not be important what logging framework is used in Maven core.

Probably a good wish but factually it is.

Technically we can forward logging message from one framework to another and it can be transparent for plugins code.

Good wish too but there are several cases which are not trivial. Personally I see it as limitations when deployed inside maven and most plugins have work arounds for that so all good, but we are not neutral enough (as the JVM is) to support anything.
It is more or less the same than we are not 100% isolated between plugins - classloader is partially only, env is almost not isolated etc...

Not sure about the spring reference but it is the same, they work in some env but not always and this is ok.

The only thing maven guarantees is that the plugin API will go through maven logging system so if integration is customized it will get the logs, anything else is not guaranteed - except JUL by construction indeed.

[gnodet]

We can not expect that all code in plugins only use Maven and JDK API, thirt party library used in plugins code can uses any logging framework.

Sure and we comply to that, only thing is that end user can only expect maven logging framework to be integrated with maven ecosystem (think CI and maven customizations) so if the plugin provider wants something well integrated it has to use that. frontend maven plugin forwarded node logs to maven logging system for that to give an example.

It should not be important what logging framework is used in Maven core.

Probably a good wish but factually it is.

Technically we can forward logging message from one framework to another and it can be transparent for plugins code.

Good wish too but there are several cases which are not trivial. Personally I see it as limitations when deployed inside maven and most plugins have work arounds for that so all good, but we are not neutral enough (as the JVM is) to support anything. It is more or less the same than we are not 100% isolated between plugins - classloader is partially only, env is almost not isolated etc...

Not sure about the spring reference but it is the same, they work in some env but not always and this is ok.

The only thing maven guarantees is that the plugin API will go through maven logging system so if integration is customized it will get the logs, anything else is not guaranteed - except JUL by construction indeed.

If we:

• provide a simple abstraction for mojo (the current Log)
• export major APIs (commons-logging and slf4j
• redirect those, in addition to JUL
  This should cover most basic cases.

If course, libraries that do setup specifically a logging framework, it will most certainly not be integrated, but we can't do much about that.

[slachiewicz]

Then we can also think how to support slf4j 1 and 2. More and more libs switch to v2

[gnodet]

Then we can also think how to support slf4j 1 and 2. More and more libs switch to v2

Afaik, the client api is compatible, if maven exports slf4j 2.x and provides an implementation, client should be fine.
See https://www.slf4j.org/faq.html#compatibility

[rmannibucau]

@slachiewicz if we switch we should just go to JUL IMHO (see the list for details) but not sure it would be for v4, hopefully v5. Anyway, does not change anything to the fact we don't want to show our impl to end users since one feature we provide is the hability to integrator to handle it as they want. All log libs have pitfalls and we can't make everything working (typically there are some cases with some bridge which will fail), so let's stay simple and 1. drop slf4j from consumers view in code, 2. if we need to change current impl move to JUL which enables to be dep free, abstract any impl (several asf projects do like cxf) and work in all integrator env without hacks.

[slachiewicz]

For Maven 5 sounds good, hope we someone will deliver new better API and bridges to existing frameworks.

[gnodet]

So the problem with exporting the slf4j unmodified is that things may break if a plugin depends on a library which itself imposes constraints on the slf4j binding used (by configuring it programmatically for example).
By exporting the maven logging api, we ensure plugins won't be affected, whatever libraries they have in their classloader.
It would be nice to have a project that reproduce the issues, as it's not yet clear what kind of failure would happen. @rmannibucau ?

[rmannibucau]

@gnodet let's ignore a second the classloading errors and take a simple functional case: you want logger X to be logged particularly and the app knows the provider it uses so can configure it directly - this is a common case as we discussed on slack. I'll use JUL binding for the example but it is not rare it is logback or log4j2 too.
Do we want to say to users "no, will never work until you implement another classloading abstraction to isolate yourself from plugin ecosystem"? Not on my side.

Here is a simple mojo showing it:

<dependency>
  <groupId>org.slf4j</groupId>
  <artifactId>slf4j-api</artifactId>
  <version>1.7.36</version>
</dependency>
<dependency>
  <groupId>org.slf4j</groupId>
  <artifactId>slf4j-jdk14</artifactId>
  <version>1.7.36</version>
</dependency>

@Mojo(name = "demo", defaultPhase = NONE)
public class MyMojo extends AbstractMojo {
    @Override
    public void execute() {
        run();
    }

    private void run() {
        final var loggerName = getClass().getName();
        final var logger = Logger.getLogger(loggerName);
        final var delegate = Logger.getLogger("");
        logger.addHandler(new Handler() {
            @Override
            public void publish(final LogRecord record) { // just a hack to get console logger but not close it
                delegate.getHandlers()[0].publish(record);
            }

            @Override
            public void flush() {
                // no-op
            }

            @Override
            public void close() throws SecurityException {
                // no-op
            }
        });
        LoggerFactory.getLogger(loggerName)
                .info("I am here");
    }
}

Indeed you have to assume the logging configuration is not part of the execute method but a transitive one - a bit like spring logging system.

Running this mojo we get:

[INFO] --- demo:0.0.1-SNAPSHOT:demo (run) @ consumer ---
[INFO] I am here

Which is not really the expectation:

juil. 12, 2023 8:24:12 AM com.github.rmannibucau.maven.reproducer.Main main
INFO: I am in xml
juil. 12, 2023 8:24:12 AM com.github.rmannibucau.maven.reproducer.Main main
INFO: I am in xml

So we can see that the format is wrong - which can be an issue for downside parsing, the plugin will not enable to test grokking for ex, but the setup is not even respected in maven (the duplicated log is an intentional coding bug but does not surface with maven). Worse, maven ignores logger.setUseParentHandlers(false); code - which is intended to fix the duplicated line.
So maven does not enable to do any log related work there.

So I really think we should give the control of that to mojo:

• slf4j can be imported or not in the limit of the strict API - not the SPI
• JUL should likely be isolated like in tomcat per mojo - default = inherit but a toggle would enable to let the plugin use its own config/custom logging manager if the system prop is set there (by delegation we can do it)

The last trick we'll see - since we spoke of slf4j 2 - is that it does not use its classloading custom lookup anymore for the logger factory but a plain ServiceLoader JPMS based so ClassRealm is not ready for that at all. Guess we'll need to work on that at some point so I don't think we'll need a workaround in our SLF4J impl to delegate to plugin impl if any but means we will still have some issues with slf4j.

Hope it helps

[rmannibucau]

Up, seems we are still un a broken state where we say to use slf4j for plugins and I didn't find the mvn4 API (pretty sure you did something @gnodet but didnt see it in mvn4 branch).

[gnodet]

Up, seems we are still un a broken state where we say to use slf4j for plugins and I didn't find the mvn4 API (pretty sure you did something @gnodet but didnt see it in mvn4 branch).

https://github.com/apache/maven/blob/master/api/maven-api-core/src/main/java/org/apache/maven/api/plugin/Log.java

I'm actually working on fixing some parts in the v4 plugins, including the class loader which should only include the Maven 4 api, so basically org.apache.maven.api.* and jakarta.inject packages.

[gnodet]

Up, seems we are still un a broken state where we say to use slf4j for plugins and I didn't find the mvn4 API (pretty sure you did something @gnodet but didnt see it in mvn4 branch).

https://github.com/apache/maven/blob/master/api/maven-api-core/src/main/java/org/apache/maven/api/plugin/Log.java

I'm actually working on fixing some parts in the v4 plugins, including the class loader which should only include the Maven 4 api, so basically org.apache.maven.api.* and jakarta.inject packages.

However, not exposing slf4j would mean we should somehow provide a bridge from slf4j to org.apache.maven.api.plugin.Log so that plugins can use other libraries but still log to the maven infrastructure.

[rmannibucau]

Ok so guess we should:

1. merge this PR to not encourage people to use slf4j since we agreed it was wrong a long time ago now
2. encourage to move to maven 4 when possible (until then keep the old bridge solution plugins were using)
3. probably document (enhance if needed) the classloading tuning to enable plugins to inherit or not from slf4j (think we discussed that)?

[cstamas]

This thread is full of misconceptions. Slf4j API worked and works. I bet 99.999 of our users use logger just for that, for logging. What Romain is talking about is setting up your own logger implementation and so on.

First, BEFORE we do anything about this, can we see some numbers (like Mojos "just logging" vs "setting up their own loggers as Romain explains")? As I feel we have the situation of "it would be nice to do it", but the actual benefit would be beneficial only for 1-2 mojo coders?

Take away simplicity and availability of SLF4J API only due someone somewhere fiddles with logging implementations? Would be like forcing left-handed scissors on everyone, because there are left handed people out there (I am one of them).

As always, and since eons, if you want something super-duper-uber-smart stuff, go for it, just isolate it in your own classloader, no problem here.

For me this PR is bicycle shedding at it's best.

[cstamas]

Ok so guess we should:

1. merge this PR to not encourage people to use slf4j since we agreed it was wrong a long time ago now

I find this silly and self contradicting: "we agreed to not use it" while it is you who said "there was never consensus". So which one? You cannot have both....

[rmannibucau]

@cstamas

bet 99.999 of our users use logger just for that

99.99% uses Log facade (most of plugins just wire the logs to this interface since it is the only one in maven api today).
Just review github and our own mojo for ex.
Very few use slf4j since it was done in a very recent maven release and most of plugin still comply to older api too.

Take away simplicity and availability of SLF4J API only due someone somewhere fiddles with logging implementations

Think you miss the fact SLF4J is an implementation for us and a detail we don't want to support for years cause as explained in the related threads slf4J is not stable and already bite several asf project so we must only promote as api something we control, this is a pure design and experience perspective.
If I'm following your reasoning we should just expose maven/lib to mojo and I agree it is trivial to agree this is bad but all libs are available and I don't think we use anything hard to consume so I don't think this reasoning can last very long.

merge this PR to not encourage people to use slf4j since we agreed it was wrong a long time ago now

Ok, let me rephrase: "since using slf4j was agreed to be wrong, let's not deprecate the only API we have and control and we'll bridge to mvn4 api".
The "there was never consensus" was on "moving to slf4j as api", this was never discussed nor agreed so revert is the minimum we can do.

For me this PR is bicycle shedding at it's best.

As explained this deprecation is a bug and was not intended so we should just fix it, after we can rediscuss what we do if you feel it was right but as of today this merge shouldn't have happent.
Agreement was on maven core code which can use slf4j as primary API, never ever on mojo - agreement was actually the opposite.

[cstamas]

Very few use slf4j since it was done in a very recent maven release and most of plugin still comply to older api too.

Not buying this: MANY of our plugins are under maintained, are even coded against Maven2 principles (as I noted in Slack). So, the fact they use something old, is nothing new 😄

OTOH, look at all the takari or polyglot stuff.... and many other well-done and well maintained plugins...

First mention of use of slf4j was around 2008 in dev list (regarding Wagon implementations, shading and all), so unsure what "recent" thing we talk about. Maybe deprecation was "recent" yes, like many other Maven internal issues there were stuck for over 10+ years... but that is NOT a reason to not make them through.

[rmannibucau]

@cstamas please just check out "slf4j plugin api" in your mailbox and check the related 5-6 threads, there is NO consensus in the community about using slf4j in the api so this is not something we can promote. Agree the community is split - also why we must not promote slf4j yet for plugins.
So let's just drop the deprecation for now, and if you want we can reopen the debate but I kind of think maven 4 will close it by design, we all agreed maven 4 will abstract anything an user can rely on so there is no real point debating if slf4j is good or not post maven 4.
I also understand you put slf4j everywhere but on my side of the fence, slf4j is almost nowhere, so status is simple:

• yes slf4j works by a bug in extensions.xml for simple plugin
• slf4j does not generally works as a state
• we agreed to not aim at continuing slf4j (or any maven lib) support (by default - until the parent fist flag is configurable) in mojo

So not sure what is really to discuss to be honest, look like you fight for an already finished battle 🤔

[gnodet]

IIRC, the example that was given by @rmannibucau was when a library actually requires a given implementation of slf4j. This happens sometimes when a library is configuring the underlying implementation programmatically.
@rmannibucau you should really come up with an IT so that we can investigate actual problems rather than never-ending discussions on principles.

Proposal for v4 plugins. In those case, I'm not sure what happens if we expose even the slf4j implementation.
https://github.com/apache/maven/pull/1336/files#diff-9600f93ce04acabe017b026dc1ebbb445e8cb363bb2677a764a783ab85cf804aR105-R116

I've also hit some problems with plexus logger which is now hidden, so it needs to be configured manually:
https://github.com/apache/maven-compiler-plugin/pull/147/files#diff-d4bac42d8f4c68d397ddbaa05c1cbbed7984ef6dc0bb9ea60739df78997e99eeR597-R598

[michael-o]

This is a Draculan discussion. It sucks out everything.

[rmannibucau]

@gnodet well code was shared but you can review https://github.com/rmannibucau/mojo-logging-demo which is only about 2 cases (but there are more like using alpha version of slf4j or the opposite). Overall the outcome of discussions is that even if it would be simpler to leak all our internals (it is not only logging there), it is not really possible because we adopted an isolated architecture (realms) cause plugins have to do their stuff and we should just support it so we should isolated everything but the mvn4 api (status quo for mvn3).

side note: if this is no more assumed by most of community - I don't think it is the case but just highlighting what it means - we would have to drop the classrealm which wouldn't make much sense anymore and just request any plugin writer to fork for their work which rely on any other dependency than the ones we provide - don't think it is good but it is ultimately what we mean if we leak more and more.

[gnodet]

@gnodet well code was shared but you can review https://github.com/rmannibucau/mojo-logging-demo which is only about 2 cases (but there are more like using alpha version of slf4j or the opposite). Overall the outcome of discussions is that even if it would be simpler to leak all our internals (it is not only logging there), it is not really possible because we adopted an isolated architecture (realms) cause plugins have to do their stuff and we should just support it so we should isolated everything but the mvn4 api (status quo for mvn3).

side note: if this is no more assumed by most of community - I don't think it is the case but just highlighting what it means - we would have to drop the classrealm which wouldn't make much sense anymore and just request any plugin writer to fork for their work which rely on any other dependency than the ones we provide - don't think it is good but it is ultimately what we mean if we leak more and more.

SLF4j api has been exported for more than 13 years, I haven't traced back to the very beginning though...

[rmannibucau]

@gnodet well yes and no, original export was partial and broken but makes 6-7 years it is bothering, but anyway does not change we don't want it to surface as a maven provided API from the previous discussions so deprecated was a missed 3 years ago IMHO. Until we get maven 4 "1.0.0" we should just deprecate Log and then switch to the provider Log of mvn 4 API, looks quite straight forward to me as path, no?

[jira-importer]

Resolve #8918

[jira-importer]

Resolve #8918

[elharo]

Is this still in play or should we close this PR?

[rmannibucau]

̀Use SLF4J directly is still totally wrong since we want to promote APILog` interface now. I'm tempted to keep it not deprecated in the compat module and promote api one bu thappy to get others feedback