Skip to content

Fixes #1028, try to make compiler summary message usableinstead of obscur aggregates #1027

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.

Sign up for GitHub

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

Open
rmannibucau wants to merge 2 commits into
base: master
Choose a base branch
from
+351 −109
Open

Fixes #1028, try to make compiler summary message usableinstead of obscur aggregates #1027

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fb9b27e
try to make compiler summary message usable
rmannibucau Feb 2, 2026
38f6bba
avoid two calls to useOrCreateListener
rmannibucau Feb 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 24 additions & 2 deletions src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -885,6 +885,16 @@ final void amendincrementalCompilation(EnumSet<IncrementalBuild.Aspect> aspects,
@Parameter(property = "maven.compiler.maxmem")
protected String maxmem;

/**
* Should maven log a summary of compiler messages (if any) by type.
* When there are a lot of messages of the same type (unchecked, deprecation etc),
* it can make the output more readable.
*
* @since 4.0.0-beta-5
*/
@Parameter(property = "maven.compiler.messageLogType", defaultValue = "COMPILER")
protected MessageLogType messageLogType;

Copy link
Contributor

@desruisseaux desruisseaux Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we use logger configuration instead of introducing a new plugin parameter? Currently, we use dependency injection like below, where Log is org.apache.maven.api.plugin.Log:

@Inject
protected Log logger;

I think that each plugin should use a named logger. With apache/maven#11685 recently merged, I would check if using System.getLogger("org.apache.maven.compiler") produces the desired output. If it works, then I suggest to drop org.apache.maven.api.plugin.Log and use system loggers or java.util.logging instead. Then:

  • "org.apache.maven" would be the parent logger in Maven core and all plugins.
  • "org.apache.maven.compiler" would be the logger for Maven Compiler Plugin.
  • "org.apache.maven.compiler.summary" (or something else, proposal welcome) could be the logger for summary.

Logging can be configured in a logging framework specific way, but we could also add a <logging> section in the POM 4.1 model. We need to address Maven logging verbosity anyway, not just for the compiler plugin.

Copy link
Author

@rmannibucau rmannibucau Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about that and rational was that this is a behavior you want to have and not logger related, this is closer to an "output format" than a channel with a log level. Also think it would be a bad practise to abuse logger as we did abuse type in terms of design cause loggers are intended for output management and not for output formatting/content selection primarily - even if we can always workaround things.

so think we are good this way

Copy link
Contributor

@desruisseaux desruisseaux Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not output formatting, but output filtering, which is in scope of logger's configuration.

Copy link
Author

@rmannibucau rmannibucau Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

filtering for a logger is only the level which doesn't apply well at all there

agree "formatting" is poor from a semantic point of view but still, it doesn't match logger design

also think we should stick to logger=class until we abuse MDC globally which is not needed there and would require a lot of rework

Copy link
Contributor

@desruisseaux desruisseaux Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

filtering for a logger is only the level

In java.util.logging, filtering is also by logger name. Filtering is also by handler (different filtering for logs sent to the console or logs sent to a file). I never use SLF4J, but I presume that it has the same capability than java.util.logging. If we use system logging, it will delegate either to java.util.logging (default), or to SLF4J, therefore is would have this capability as well.

also think we should stick to logger=class

java.util.logging uses a more generic term, "subsystem", with a recommendation to base it on the package name or class name. In other words, the logger name should indeed starts at least with the package name, but there is some flexibility for what come after the package name.

// ----------------------------------------------------------------------
// Read-only parameters
// ----------------------------------------------------------------------
Expand Down Expand Up @@ -1227,7 +1237,7 @@ public void execute() throws MojoException {
* Therefore, the executor can safely be executed in a background thread,
* provided that the {@link #logger} is thread-safe.
*
* @param listener where to send compilation warnings, or {@code null} for the Maven logger
* @param listener where to send compilation messages, or {@code null} for the Maven logger
* @return the task to execute for compiling the project using the configuration in this <abbr>MOJO</abbr>
* @throws MojoException if this method identifies an invalid parameter in this <abbr>MOJO</abbr>
* @throws IOException if an error occurred while creating the output directory or scanning the source directories
Expand All @@ -1248,6 +1258,18 @@ public ToolExecutor createExecutor(DiagnosticListener<? super JavaFileObject> li
return executor;
}

/**
* Simple utility to enforce a listener.
* @param listener caller provided listener.
* @return user listener if not null else maven internal one.
*/
private DiagnosticListener<? super JavaFileObject> useOrCreateListener(
final DiagnosticListener<? super JavaFileObject> listener) {
return listener == null
Copy link
Contributor

@desruisseaux desruisseaux Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer that we keep this construction inside ToolExecutor as today, in order to have a single place where the default listener is constructed. It would avoid my confusion in this and this comment.

Copy link
Author

@rmannibucau rmannibucau Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just tried to deduplicate the useOrCreateListener call

? new DiagnosticLogger(logger, messageBuilderFactory, null, project.getRootDirectory(), messageLogType)
: listener;
}

/**
* {@return the compiler to use for compiling the code}
* If {@link #fork} is {@code true}, the returned compiler will be a wrapper for a command line.
Expand Down Expand Up @@ -1370,7 +1392,7 @@ public Options parseParameters(final OptionChecker compiler) {
*/
@SuppressWarnings("UseSpecificCatch")
private void compile(final JavaCompiler compiler, final Options configuration) throws IOException {
final ToolExecutor executor = createExecutor(null);
final ToolExecutor executor = createExecutor(useOrCreateListener(null));
Copy link
Contributor

@desruisseaux desruisseaux Feb 2, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This call to useOrCreateListener duplicates the same call done in createExecutor.

EDIT: with a second though, it is a duplication for the main code, but not for TestCompilerMojo.

if (!executor.applyIncrementalBuild(this, configuration)) {
return;
}
Expand Down
253 changes: 158 additions & 95 deletions src/main/java/org/apache/maven/plugin/compiler/DiagnosticLogger.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,18 +21,22 @@
import javax.tools.Diagnostic;
import javax.tools.DiagnosticListener;
import javax.tools.JavaFileObject;
import javax.tools.ToolProvider;

import java.nio.file.Path;
import java.util.Arrays;
import java.util.LinkedHashMap;
import java.util.Locale;
import java.util.Map;
import java.util.Optional;
import java.util.ResourceBundle;
import java.util.function.Consumer;

import org.apache.maven.api.plugin.Log;
import org.apache.maven.api.services.MessageBuilder;
import org.apache.maven.api.services.MessageBuilderFactory;

import static java.util.Optional.ofNullable;

/**
* A Java compiler diagnostic listener which send the messages to the Maven logger.
*
Expand All @@ -59,6 +63,11 @@ final class DiagnosticLogger implements DiagnosticListener<JavaFileObject> {
*/
private final Path directory;

/**
* Type of output of the logger.
*/
private final MessageLogType logType;

/**
* Number of errors or warnings.
*/
Expand All @@ -77,23 +86,30 @@ final class DiagnosticLogger implements DiagnosticListener<JavaFileObject> {
/**
* Creates a listener which will send the diagnostics to the given logger.
*
* @param logger the logger where to send diagnostics
* @param logger the logger where to send diagnostics
* @param messageBuilderFactory the factory for creating message builders
* @param locale the locale for compiler message
* @param directory the base directory with which to relativize the paths to source files
* @param locale the locale for compiler message
* @param directory the base directory with which to relativize the paths to source files
* @param logType output flavor
*/
DiagnosticLogger(Log logger, MessageBuilderFactory messageBuilderFactory, Locale locale, Path directory) {
DiagnosticLogger(
Log logger,
MessageBuilderFactory messageBuilderFactory,
Locale locale,
Path directory,
MessageLogType logType) {
this.logger = logger;
this.messageBuilderFactory = messageBuilderFactory;
this.locale = locale;
this.directory = directory;
this.logType = logType;
codeCount = new LinkedHashMap<>();
}

/**
* Makes the given file relative to the base directory.
*
* @param file the path to make relative to the base directory
* @param file the path to make relative to the base directory
* @return the given path, potentially relative to the base directory
*/
private String relativize(String file) {
Expand All @@ -118,63 +134,67 @@ public void report(Diagnostic<? extends JavaFileObject> diagnostic) {
if (message == null || message.isBlank()) {
return;
}
MessageBuilder record = messageBuilderFactory.builder();
record.a(message);
JavaFileObject source = diagnostic.getSource();
Diagnostic.Kind kind = diagnostic.getKind();
String style;
switch (kind) {
case ERROR:
style = ".error:-bold,f:red";
break;
case MANDATORY_WARNING:
case WARNING:
style = ".warning:-bold,f:yellow";
break;
default:
style = ".info:-bold,f:blue";
if (diagnostic.getLineNumber() == Diagnostic.NOPOS) {
source = null; // Some messages are generic, e.g. "Recompile with -Xlint:deprecation".
}
break;
}
if (source != null) {
record.newline().a(" at ").a(relativize(source.getName()));
long line = diagnostic.getLineNumber();
long column = diagnostic.getColumnNumber();
if (line != Diagnostic.NOPOS || column != Diagnostic.NOPOS) {
record.style(style).a('[');
if (line != Diagnostic.NOPOS) {
record.a(line);
}
if (column != Diagnostic.NOPOS) {
record.a(',').a(column);
}
record.a(']').resetStyle();
if (logType == MessageLogType.COMPILER || logType == MessageLogType.ALL) {
MessageBuilder record = messageBuilderFactory.builder();
record.a(message);
JavaFileObject source = diagnostic.getSource();
Diagnostic.Kind kind = diagnostic.getKind();
String style;
switch (kind) {
case ERROR:
style = ".error:-bold,f:red";
break;
case MANDATORY_WARNING:
case WARNING:
style = ".warning:-bold,f:yellow";
break;
default:
style = ".info:-bold,f:blue";
if (diagnostic.getLineNumber() == Diagnostic.NOPOS) {
source = null; // Some messages are generic, e.g. "Recompile with -Xlint:deprecation".
}
break;
}
}
String log = record.toString();
switch (kind) {
case ERROR:
if (firstError == null) {
firstError = message;
if (source != null) {
record.newline().a(" at ").a(relativize(source.getName()));
long line = diagnostic.getLineNumber();
long column = diagnostic.getColumnNumber();
if (line != Diagnostic.NOPOS || column != Diagnostic.NOPOS) {
record.style(style).a('[');
if (line != Diagnostic.NOPOS) {
record.a(line);
}
if (column != Diagnostic.NOPOS) {
record.a(',').a(column);
}
record.a(']').resetStyle();
}
logger.error(log);
numErrors++;
break;
case MANDATORY_WARNING:
case WARNING:
logger.warn(log);
numWarnings++;
break;
default:
logger.info(log);
break;
}
String log = record.toString();
switch (kind) {
case ERROR:
if (firstError == null) {
firstError = message;
}
logger.error(log);
numErrors++;
break;
case MANDATORY_WARNING:
case WARNING:
logger.warn(log);
numWarnings++;
break;
default:
logger.info(log);
break;
}
}
// Statistics
String code = diagnostic.getCode();
if (code != null) {
codeCount.merge(code, 1, (old, initial) -> old + 1);
if (logType == MessageLogType.SUMMARY || logType == MessageLogType.ALL) {
// Statistics
String code = diagnostic.getCode();
if (code != null) {
codeCount.merge(code, 1, (old, initial) -> old + 1);
}
}
}

Expand All @@ -191,50 +211,93 @@ Optional<String> firstError(Throwable cause) {
* Reports summary after the compilation finished.
*/
void logSummary() {
MessageBuilder message = messageBuilderFactory.builder();
final String patternForCount;
if (!codeCount.isEmpty()) {
@SuppressWarnings("unchecked")
Map.Entry<String, Integer>[] entries = codeCount.entrySet().toArray(Map.Entry[]::new);
Arrays.sort(entries, (a, b) -> Integer.compare(b.getValue(), a.getValue()));
patternForCount = patternForCount(Math.max(entries[0].getValue(), Math.max(numWarnings, numErrors)));
message.strong("Summary of compiler messages:").newline();
for (Map.Entry<String, Integer> entry : entries) {
final var bundle = tryGetCompilerBundle();
for (final var entry : codeCount.entrySet().stream()
// sort occurrence then key to have an absolute ordering
.sorted(Map.Entry.<String, Integer>comparingByValue()
.reversed()
.thenComparing(Map.Entry.comparingByKey()))
.toList()) {
int count = entry.getValue();
message.format(patternForCount, count, entry.getKey()).newline();
String key = entry.getKey();
if (bundle != null) {
try {
// not great but the code is worse when you read it
key = key + " (" + bundle.getString(key) + ")";
} catch (final RuntimeException re) {
// ignore, use the plain key
}
}
Consumer<String> log;
if (entry.getKey().startsWith("compiler.")) {
final var sub = entry.getKey().substring("compiler.".length());
if (sub.startsWith("err.")) {
log = logger::error;
} else if (sub.startsWith("warn.")) {
log = logger::warn;
} else {
log = logger::info;
}
} else {
log = logger::info;
}
log.accept(messageBuilderFactory
.builder()
.strong(key + ": ")
.append(String.valueOf(count))
.build());
}
} else {
patternForCount = patternForCount(Math.max(numWarnings, numErrors));
}

if ((numWarnings | numErrors) != 0) {
MessageBuilder message = messageBuilderFactory.builder();
message.strong("Total:");
if (numWarnings != 0) {
message.append(' ').append(String.valueOf(numWarnings)).append(" warning");
if (numWarnings > 1) {
message.append('s');
}
}
if (numErrors != 0) {
message.append(' ').append(String.valueOf(numErrors)).append(" error");
if (numErrors > 1) {
message.append('s');
}
}
logger.info(message.build());
}
if (numWarnings != 0) {
writeCount(message, patternForCount, numWarnings, "warning");
}
if (numErrors != 0) {
writeCount(message, patternForCount, numErrors, "error");
}
logger.info(message.toString());
}

/**
* {@return the pattern for formatting the specified number followed by a label}
* The given number should be the widest number to format.
* A margin of 4 spaces is added at the beginning of the line.
*/
private static String patternForCount(int n) {
return " %" + Integer.toString(n).length() + "d %s";
}

/**
* Appends the count of warnings or errors, making them plural if needed.
*/
private static void writeCount(MessageBuilder message, String patternForCount, int count, String name) {
message.newline();
message.format(patternForCount, count, name);
if (count > 1) {
message.append('s');
// we mainly know the one for javac as of today, this impl is best effort
private ResourceBundle tryGetCompilerBundle() {
Copy link
Contributor

@desruisseaux desruisseaux Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very specific to the implementation details of the OpenJDK compiler, and I don't think that it will work in JPMS context (access to non-exported resources are blocked by the JVM). It is not exactly the desired properties file anyway, because the values are for example "{0} uses or overrides a deprecated API" and we have nothing to provide for the {0} part. We may have to provide our own .properties file. It would cover only a subset of the messages, but we could cover the most common ones.

Copy link
Author

@rmannibucau rmannibucau Feb 2, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that it will work in JPMS context

It does in our context, give it a try

It is not exactly the desired properties file anyway

Not sure what you mean cause it is literally the pattern message for the code you used 🤔 , so the intent and since it is an aggregation we don't want to fill the placeholders there, but at least the user sees something readable

Copy link
Contributor

@desruisseaux desruisseaux Feb 2, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does in our context, give it a try

Yes but this is not a JPMS context. While Maven is not yet modularized, this is one of my long term goal.

Not sure what you mean cause it is literally the pattern message for the code you used

Yes but with parameters that we don't have when writing a summary. The "{0} uses or overrides a deprecated API" is a message reported when one particular class or method use a deprecated API, and "{0}" is the name of that class or method. But when we write the summary, we are saying that (for example) this warnings occurred 100 times. We have no particular class or method that we can insert in the "{0}" part.

Copy link
Author

@rmannibucau rmannibucau Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While Maven is not yet modularized, this is one of my long term goal.

hmm, there is no consensus on that so tempted to say it will not happen so it is ok, worse case it would be easy to make it working too so not a blocker IMHO

Copy link
Contributor

@desruisseaux desruisseaux Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, there is no consensus on that so tempted to say it will not happen so it is ok, worse case it would be easy to make it working too

Agree that modularizing Maven is not on the agenda soon. However, it will not be easy at all to make the pull request works in that case. It will be blocked by the JVM, unless we put --add-opens options in the Maven launch script, which is strongly discouraged except for testing. Generally, using internal API is discouraged even in a non-JPMS world, and it is precisely for avoiding such practices that JPMS makes it hard.

Furthermore, we still have the issue of resource strings not applicable (the problem of having no value to supply in "{0}" emplacements).

// ignore the locale since we do log everything in english,
// use only default bundle to avoid mixed outputs
final var bundleName = "com.sun.tools.javac.resources.compiler";
try {
final var clazz = ToolProvider.getSystemJavaCompiler().getClass();
final var resources = bundleName.replace('.', '/') + ".class";
final var is = clazz.getModule() == null
? ofNullable(clazz.getClassLoader())
.orElseGet(ClassLoader::getSystemClassLoader)
.getResourceAsStream(resources)
: clazz.getModule().getResourceAsStream(resources);
if (is == null) {
return null;
}
try (is) {
final var bytes = is.readAllBytes();
final var rbClass = new ClassLoader() {
{
super.defineClass(bundleName, bytes, 0, bytes.length);
}
}.loadClass(bundleName);
final var cons = rbClass.getConstructor();
cons.setAccessible(true);
return (ResourceBundle) cons.newInstance();
}
} catch (final Exception e) {
return null;
}
}
}
Loading
Loading