gnodet-bot commented on code in PR #566:
URL: https://github.com/apache/maven-jar-plugin/pull/566#discussion_r4176708010
##########
src/main/java/org/apache/maven/plugins/jar/AbstractJarMojo.java:
##########
@@ -297,38 +308,75 @@ public Path createArchive() throws MojoException {
/**
* Generates the JAR.
*
+ * <p>Uses the {@link BuildContext} aggregation pattern to register all
class files as
+ * inputs and associate them with the JAR output. The JAR is only rebuilt
when at least
+ * one input has changed since the last build (unless {@link
#forceCreation} is set).
+ * When inputs are removed, the build context automatically handles stale
output cleanup.</p>
+ *
* @throws MojoException in case of an error
*/
@Override
public void execute() throws MojoException {
if (skipIfEmpty && isEmpty(getClassesDirectory())) {
getLog().info(String.format("Skipping packaging of the %s.",
getType()));
+ buildContext.markSkipExecution();
+ return;
+ }
+
+ Path basedir = outputDirectory != null
+ ? outputDirectory
+ : Path.of(project.getBuild().getDirectory());
+ String resultFinalName =
+ finalName != null ? finalName :
project.getBuild().getFinalName();
+ Path jarFile = getJarFile(basedir, resultFinalName, getClassifier());
+
+ // Register all class files as inputs and aggregate them into the JAR
output.
+ // The aggregate() callback is only invoked when at least one input
has changed.
+ Path classesDir = getClassesDirectory();
+ if (!forceCreation && Files.isDirectory(classesDir)) {
+ InputSet inputSet = buildContext.newInputSet();
+ inputSet.registerInputs(classesDir, List.of("**/**"), List.of());
Review Comment:
🔴 **Input pattern mismatch.** `registerInputs()` uses hardcoded `"**/**"`
with no excludes, but `createArchive()` uses `getIncludes()`/`getExcludes()` —
which at minimum excludes `**/package.html` by default, and may exclude/include
user-configured patterns.
This means:
- A change to `package.html` (excluded from JAR) triggers a needless rebuild
- With custom `<includes>`, changes to files outside the include set still
trigger rebuilds
- The incremental detection tracks a **superset** of what actually enters
the JAR
```suggestion
InputSet inputSet = buildContext.newInputSet();
inputSet.registerInputs(classesDir,
Arrays.asList(getIncludes()), Arrays.asList(getExcludes()));
```
##########
src/main/java/org/apache/maven/plugins/jar/AbstractJarMojo.java:
##########
@@ -297,38 +308,75 @@ public Path createArchive() throws MojoException {
/**
* Generates the JAR.
*
+ * <p>Uses the {@link BuildContext} aggregation pattern to register all
class files as
+ * inputs and associate them with the JAR output. The JAR is only rebuilt
when at least
+ * one input has changed since the last build (unless {@link
#forceCreation} is set).
+ * When inputs are removed, the build context automatically handles stale
output cleanup.</p>
+ *
* @throws MojoException in case of an error
*/
@Override
public void execute() throws MojoException {
if (skipIfEmpty && isEmpty(getClassesDirectory())) {
getLog().info(String.format("Skipping packaging of the %s.",
getType()));
+ buildContext.markSkipExecution();
+ return;
+ }
+
+ Path basedir = outputDirectory != null
+ ? outputDirectory
+ : Path.of(project.getBuild().getDirectory());
+ String resultFinalName =
+ finalName != null ? finalName :
project.getBuild().getFinalName();
+ Path jarFile = getJarFile(basedir, resultFinalName, getClassifier());
+
+ // Register all class files as inputs and aggregate them into the JAR
output.
+ // The aggregate() callback is only invoked when at least one input
has changed.
+ Path classesDir = getClassesDirectory();
+ if (!forceCreation && Files.isDirectory(classesDir)) {
+ InputSet inputSet = buildContext.newInputSet();
+ inputSet.registerInputs(classesDir, List.of("**/**"), List.of());
+
+ boolean rebuilt = inputSet.aggregate(jarFile, (output, inputs) -> {
+ createArchive();
+ });
Review Comment:
⚠️ **Aggregate callback ignores both `output` and `inputs` parameters.**
`createArchive()` independently recomputes `basedir`/`finalName`/`jarFile` and
writes to its own path. The `Output` resource provided by the BuildContext
(which is the tracked output file) is never used.
If the two path computations ever diverge, the BuildContext would track one
file while the actual JAR lives at another. Consider either:
1. Refactoring `createArchive()` to accept a target `Path` parameter, or
2. Using the `Output` to get the canonical path and passing it through
Also, using `(output, inputs) ->` with unused params — if this is
intentional, a brief comment explaining why would help future readers.
##########
src/test/java/org/apache/maven/plugins/jar/JarMojoTest.java:
##########
@@ -47,4 +56,19 @@ void jarTestEnvironment(JarMojo mojo) throws Exception {
assertEquals("foo", mojo.getProject().getGroupId());
Review Comment:
💡 The existing test only verifies mojo wiring (`assertNotNull`, groupId
equality). There's no test for the new incremental `execute()` path — no
coverage for:
- `aggregate()` being called and creating a JAR on first build
- `markSkipExecution()` firing when inputs are unchanged
- `forceCreation = true` bypassing the incremental check
Given this is the core new behavior, at least one test exercising the
BuildContext integration would catch regressions early. (Acknowledged this is
experimental — flagging for when it graduates.)
--
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]
