gnodet-bot commented on code in PR #13695:
URL: https://github.com/apache/maven/pull/13695#discussion_r4365693628
##########
impl/maven-logging/src/main/java/module-info.java:
##########
@@ -19,15 +19,14 @@
module org.apache.maven.logging {
requires org.apache.maven.api;
+ requires org.apache.maven.api.logging;
requires org.apache.maven.jline;
- requires org.slf4j;
- exports org.apache.maven.logging.api;
- exports org.apache.maven.slf4j to
+ exports org.apache.maven.logging.impl to
+ org.apache.maven.logging.slf4j,
+ org.apache.maven.logging.jul,
+ org.apache.maven.core,
org.apache.maven.cling,
org.apache.maven.embedder,
- org.apache.maven.core;
-
- provides org.slf4j.spi.SLF4JServiceProvider with
- org.apache.maven.slf4j.MavenServiceProvider;
+ org.apache.maven.testing;
Review Comment:
🟠**Wrong fix: `maven-logging` exports an internal package to
`maven-testing`.**
`maven-testing` uses `DefaultLogger` directly via `new
DefaultLogger("anonymous")` in `MojoExtension.createLog()`. The JPMS fix
(exporting `org.apache.maven.logging.impl` to `org.apache.maven.testing`)
papers over the real issue: test infrastructure should not depend on
implementation internals.
The root cause is that `MojoExtension` binds `Log.class` — a type that is
`@Deprecated(since = "4.1.0", forRemoval = true)`. The correct migration is to
bind `Logger.class` instead, using the public API:
```suggestion
org.apache.maven.embedder;
}
```
And in `MojoExtension.java`, change the DI binding:
```java
// Before (impl coupling):
@Provides @Singleton @Priority(-10)
private Log createLog() {
return new DefaultLogger("anonymous");
}
// After (API-only):
@Provides @Singleton @Priority(-10)
private Logger createLogger() {
return LoggerManager.get().getLogger("anonymous");
}
```
This requires updating callers of `Log` injection in `maven-testing` to use
`Logger` — but that migration is the stated goal of this PR (`Log` is being
phased out). Since `Logger` does not extend `Log` (they are distinct
interfaces), `maven-testing/pom.xml` would also need `maven-api-log` as an
explicit dependency, and the `@Provides` binding changed to `Logger.class`.
If the migration of `MojoExtension` to `Logger` is deferred to a follow-up
(acceptable since it's a separate concern), the minimal correct fix for this
commit is to at minimum add an **explicit** `<dependency>` on `maven-logging`
in `maven-testing/pom.xml` so the transitive dependency is declared, not silent.
##########
impl/maven-testing/src/main/java/org/apache/maven/testing/plugin/MojoExtension.java:
##########
@@ -529,7 +528,7 @@ private MojoExecution createMojoExecution() {
@Singleton
@Priority(-10)
private Log createLog() {
- return new DefaultLog(LoggerFactory.getLogger("anonymous"));
+ return new DefaultLogger("anonymous");
Review Comment:
🟡 **Stale commented code at line 354 uses
`DefaultLogger(LoggerFactory.getLogger(...))` — wrong constructor.**
Line 354 in the dead comment block reads:
```java
// binder.bind(Log.class).toInstance(new
DefaultLogger(LoggerFactory.getLogger("anonymous")));
```
But `DefaultLogger(org.slf4j.Logger)` is not a valid constructor —
`DefaultLogger` only has `DefaultLogger(String name)` and `DefaultLogger(String
name, String projectId, String mojoId)`. If this comment were ever uncommented,
it would fail to compile. Clean up the stale comment or correct the constructor
call.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
