Copilot commented on code in PR #541:
URL: https://github.com/apache/maven-ear-plugin/pull/541#discussion_r3673826302
##########
src/main/java/org/apache/maven/plugins/ear/EarMojo.java:
##########
@@ -665,7 +665,7 @@ private void changeManifestClasspath(
EarModule module, File original, JavaEEVersion javaEEVersion,
Collection<String> outdatedResources)
throws MojoFailureException {
final String moduleLibDir = module.getLibDir();
- if (!((moduleLibDir == null) || skinnyModules || (skinnyWars && module
instanceof WebModule))) {
+ if (!(skinnyModules || (skinnyWars && (module instanceof WebModule ||
moduleLibDir == null)))) {
return;
}
Review Comment:
The boolean condition changes semantics vs. the prior logic: previously
`moduleLibDir == null` alone was sufficient to proceed (even when `skinnyWars`
is false), but now `moduleLibDir == null` only allows proceeding when
`skinnyWars` is true. This can skip manifest classpath handling for modules
with a null libDir in non-skinny modes. Consider restoring the original intent
explicitly (e.g., `moduleLibDir == null || skinnyModules || (skinnyWars &&
module instanceof WebModule)`) or otherwise aligning this condition to the
expected behavior for `moduleLibDir == null`.
##########
src/main/java/org/apache/maven/plugins/ear/EarMojo.java:
##########
@@ -713,12 +713,9 @@ private void changeManifestClasspath(
Attribute classPath =
mf.getMainSection().getAttribute("Class-Path");
List<String> classPathElements = new ArrayList<>();
- boolean classPathExists;
if (classPath != null) {
- classPathExists = true;
classPathElements.addAll(Arrays.asList(classPath.getValue().split(" ")));
} else {
- classPathExists = false;
classPath = new Attribute("Class-Path", "");
}
Review Comment:
When `Class-Path` is absent, this code still creates a new
`Attribute("Class-Path", "")` even if `skipClassPathModification` is enabled
(and the attribute will not be added later). Combined with the fact the
manifest is always rewritten, this increases the risk of “touching” a
previously untouched manifest (e.g., normalization/reformatting) despite the
skip flag’s intent. A concrete fix is to avoid constructing a new `Class-Path`
attribute unless you’re actually going to set/add it (i.e., only create it
inside the `!skipClassPathModification` path, or gate this `else` on
`!skipClassPathModification`).
##########
src/main/java/org/apache/maven/plugins/ear/EarMojo.java:
##########
@@ -776,50 +773,47 @@ private void changeManifestClasspath(
}
// Modify the classpath entries in the manifest
- final boolean forceClassPathModification =
- javaEEVersion.lt(JavaEEVersion.FIVE) ||
defaultLibBundleDir == null;
- final boolean classPathExtension = !skipClassPathModification ||
forceClassPathModification;
- for (EarModule otherModule : getModules()) {
- if (module.equals(otherModule)) {
- continue;
- }
- final int moduleClassPathIndex =
findModuleInClassPathElements(classPathElements, otherModule);
- if (moduleClassPathIndex != -1) {
- if (otherModule.isClassPathItem()) {
- classPathElements.set(moduleClassPathIndex,
otherModule.getUri());
- } else {
- classPathElements.remove(moduleClassPathIndex);
+ if (!skipClassPathModification) {
+ for (EarModule otherModule : getModules()) {
+ if (module.equals(otherModule)) {
+ continue;
+ }
+ final int moduleClassPathIndex =
findModuleInClassPathElements(classPathElements, otherModule);
+ if (moduleClassPathIndex != -1) {
+ if (otherModule.isClassPathItem()) {
+ classPathElements.set(moduleClassPathIndex,
otherModule.getUri());
+ } else {
+ classPathElements.remove(moduleClassPathIndex);
+ }
+ } else if (otherModule.isClassPathItem()) {
+ classPathElements.add(otherModule.getUri());
}
- } else if (otherModule.isClassPathItem() &&
classPathExtension) {
- classPathElements.add(otherModule.getUri());
}
- }
- // Remove provided modules from classpath
- for (EarModule otherModule : getProvidedEarModules()) {
- final int moduleClassPathIndex =
findModuleInClassPathElements(classPathElements, otherModule);
- if (moduleClassPathIndex != -1) {
- classPathElements.remove(moduleClassPathIndex);
+ // Remove provided modules from classpath
+ for (EarModule otherModule : getProvidedEarModules()) {
+ final int moduleClassPathIndex =
findModuleInClassPathElements(classPathElements, otherModule);
+ if (moduleClassPathIndex != -1) {
+ classPathElements.remove(moduleClassPathIndex);
+ }
}
- }
- if (!skipClassPathModification || !classPathElements.isEmpty() ||
classPathExists) {
classPath.setValue(StringUtils.join(classPathElements.iterator(), " "));
mf.getMainSection().addConfiguredAttribute(classPath);
+ }
- // Write the manifest to disk, preserve timestamp
- FileTime lastModifiedTime =
Files.getLastModifiedTime(manifestFile);
- try (BufferedWriter writer = Files.newBufferedWriter(
- manifestFile,
- StandardCharsets.UTF_8,
- StandardOpenOption.WRITE,
- StandardOpenOption.CREATE,
- StandardOpenOption.TRUNCATE_EXISTING)) {
- mf.write(writer);
- }
- Files.setLastModifiedTime(manifestFile, lastModifiedTime);
- removeFromOutdatedResources(manifestFile, outdatedResources);
+ // Write the manifest to disk, preserve timestamp
+ FileTime lastModifiedTime =
Files.getLastModifiedTime(manifestFile);
+ try (BufferedWriter writer = Files.newBufferedWriter(
+ manifestFile,
+ StandardCharsets.UTF_8,
+ StandardOpenOption.WRITE,
+ StandardOpenOption.CREATE,
+ StandardOpenOption.TRUNCATE_EXISTING)) {
+ mf.write(writer);
}
+ Files.setLastModifiedTime(manifestFile, lastModifiedTime);
+ removeFromOutdatedResources(manifestFile, outdatedResources);
Review Comment:
With this change, the manifest is always rewritten via `mf.write(writer)`
even when `skipClassPathModification=true`. This can still alter the manifest
content (e.g., line wrapping, ordering, whitespace, newline normalization)
despite “skipping” modifications, which can impact reproducible builds and
consumers expecting byte-identical manifests. If the requirement is “write the
original manifest unchanged,” consider short-circuiting the rewrite path when
`skipClassPathModification=true` by copying the original manifest bytes (or
skipping the write when no logical changes occurred) rather than serializing
through the Manifest writer.
--
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]
