Copilot commented on code in PR #193:
URL:
https://github.com/apache/maven-compiler-plugin/pull/193#discussion_r2680383545
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1063,7 +1063,7 @@ public void execute() throws MojoExecutionException,
CompilationFailureException
String value = entry.getValue();
if (value == null) {
entry = entryIter.next();
- value = entry.getKey();
+ value = entry.getKey().replaceAll(new
File("").getAbsolutePath(), ".");
Review Comment:
The new path replacement logic for making builds reproducible lacks test
coverage. Consider adding a test case that verifies absolute paths in JPMS
arguments are correctly converted to relative paths in the generated jpms.args
file.
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1063,7 +1063,7 @@ public void execute() throws MojoExecutionException,
CompilationFailureException
String value = entry.getValue();
if (value == null) {
entry = entryIter.next();
- value = entry.getKey();
+ value = entry.getKey().replaceAll(new
File("").getAbsolutePath(), ".");
Review Comment:
On Windows systems, File.getAbsolutePath() returns paths with backslashes
which are regex special characters. When used with replaceAll (which treats the
first parameter as a regex), this will cause a PatternSyntaxException.
Additionally, dots in the path (which are common, e.g., in version numbers in
parent directories) are also regex metacharacters that will cause incorrect
matching.
```suggestion
value = entry.getKey().replace(new
File("").getAbsolutePath(), ".");
```
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1063,7 +1063,7 @@ public void execute() throws MojoExecutionException,
CompilationFailureException
String value = entry.getValue();
if (value == null) {
entry = entryIter.next();
- value = entry.getKey();
+ value = entry.getKey().replaceAll(new
File("").getAbsolutePath(), ".");
Review Comment:
Creating a new File object on every iteration is inefficient. Consider
computing the absolute path once outside the loop and storing it in a variable
for reuse.
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1063,7 +1063,7 @@ public void execute() throws MojoExecutionException,
CompilationFailureException
String value = entry.getValue();
if (value == null) {
entry = entryIter.next();
- value = entry.getKey();
+ value = entry.getKey().replaceAll(new
File("").getAbsolutePath(), ".");
Review Comment:
The path replacement approach is fragile. This assumes that entry.getKey()
always contains absolute paths starting with the current directory's absolute
path, which may not be true in all cases. If entry.getKey() doesn't contain the
absolute path prefix, this replacement will have no effect. Consider validating
that the replacement actually occurred or using a more robust approach like
converting absolute paths to relative paths using Path.relativize().
--
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]
