[MCOMPILER-397] Use relative paths in jpms.args to make the builds reproducible #193
Conversation
|
This is incomplete, the path separators have to be normalized too. |
slawekjaranowski
added
the
waiting-for-feedback
|
Done on a clone of the master branch (not yet merged). The fix in this pull request could not be applied on master, but could be applied on the 3.x branch. Should we redirect the target branch to 3.x, or close as "fixed" (after merge of aa4509b)? |
github-actions
bot
removed
the
waiting-for-feedback
This comment was marked as resolved.
This comment was marked as resolved.
|
ok, was merged with #320 - one big PR 😄 |
|
Resolve #626 |
slawekjaranowski
added
the
waiting-for-feedback
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Pull request overview
This PR aims to make Maven compiler plugin builds reproducible by converting absolute paths to relative paths in the generated jpms.args file, addressing JIRA issue MCOMPILER-397.
Changes:
- Modified path handling in JPMS runtime arguments to replace absolute paths with relative paths using "." as the base directory
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (value == null) { | ||
| entry = entryIter.next(); | ||
| value = entry.getKey(); | ||
| value = entry.getKey().replaceAll(new File("").getAbsolutePath(), "."); |
There was a problem hiding this 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.
| if (value == null) { | ||
| entry = entryIter.next(); | ||
| value = entry.getKey(); | ||
| value = entry.getKey().replaceAll(new File("").getAbsolutePath(), "."); |
There was a problem hiding this 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.
| if (value == null) { | ||
| entry = entryIter.next(); | ||
| value = entry.getKey(); | ||
| value = entry.getKey().replaceAll(new File("").getAbsolutePath(), "."); |
There was a problem hiding this 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().
| if (value == null) { | ||
| entry = entryIter.next(); | ||
| value = entry.getKey(); | ||
| value = entry.getKey().replaceAll(new File("").getAbsolutePath(), "."); |
There was a problem hiding this 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.
| value = entry.getKey().replaceAll(new File("").getAbsolutePath(), "."); | |
| value = entry.getKey().replace(new File("").getAbsolutePath(), "."); |
5 participants
PR for https://issues.apache.org/jira/browse/MCOMPILER-397