gnodet commented on code in PR #2198:
URL: https://github.com/apache/maven-resolver/pull/2198#discussion_r4258666258
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultInstaller.java:
##########
@@ -205,7 +214,22 @@ private void install(RepositorySystemSession session,
RequestTrace trace, Artifa
throw new IllegalStateException("cannot install " + dstPath +
" to same path");
}
- pathProcessor.copyWithTimestamp(srcPath, dstPath);
+ if (hardLink) {
+ Files.createDirectories(dstPath.getParent());
+ if (Files.isDirectory(dstPath)) {
+ throw new IOException("destination is a directory: " +
dstPath);
+ }
+ try {
+ Files.deleteIfExists(dstPath);
+ Files.createLink(dstPath, srcPath);
+ } catch (UnsupportedOperationException |
IllegalArgumentException | IOException e) {
+ // cross-device link (EXDEV), provider mismatch (e.g.
Jimfs), or unsupported FS — fall back to copy
+ logger.warn("Hard link not supported for {}, falling back
to copy: {}", dstPath, e.getMessage());
+ pathProcessor.copyWithTimestamp(srcPath, dstPath);
+ }
Review Comment:
The existing `copyWithTimestamp` has the same non-atomicity (no temp-file
strategy). Changing the hard-link path to use a temp-file-then-rename approach
would be a separate improvement applicable to all install paths, not specific
to this PR. The existing behavior (delete-then-write) is inherited from the
prior implementation.
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultInstaller.java:
##########
@@ -205,7 +214,22 @@ private void install(RepositorySystemSession session,
RequestTrace trace, Artifa
throw new IllegalStateException("cannot install " + dstPath +
" to same path");
}
- pathProcessor.copyWithTimestamp(srcPath, dstPath);
+ if (hardLink) {
+ Files.createDirectories(dstPath.getParent());
+ if (Files.isDirectory(dstPath)) {
+ throw new IOException("destination is a directory: " +
dstPath);
+ }
Review Comment:
This was fixed in commit 9cfecea5: the `isDirectory` check is now **before**
the inner `try` block (it's inside the outer `try`, so it propagates as
`InstallationException`, not caught by the fallback catch). The Copilot review
was generated against an older version of the code.
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultInstaller.java:
##########
@@ -205,7 +214,22 @@ private void install(RepositorySystemSession session,
RequestTrace trace, Artifa
throw new IllegalStateException("cannot install " + dstPath +
" to same path");
}
- pathProcessor.copyWithTimestamp(srcPath, dstPath);
+ if (hardLink) {
+ Files.createDirectories(dstPath.getParent());
+ if (Files.isDirectory(dstPath)) {
+ throw new IOException("destination is a directory: " +
dstPath);
+ }
+ try {
+ Files.deleteIfExists(dstPath);
+ Files.createLink(dstPath, srcPath);
+ } catch (UnsupportedOperationException |
IllegalArgumentException | IOException e) {
+ // cross-device link (EXDEV), provider mismatch (e.g.
Jimfs), or unsupported FS — fall back to copy
+ logger.warn("Hard link not supported for {}, falling back
to copy: {}", dstPath, e.getMessage());
+ pathProcessor.copyWithTimestamp(srcPath, dstPath);
+ }
Review Comment:
Same as above — fixed in 9cfecea5. The `isDirectory` check is outside the
inner try/catch block.
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultInstaller.java:
##########
@@ -205,7 +214,22 @@ private void install(RepositorySystemSession session,
RequestTrace trace, Artifa
throw new IllegalStateException("cannot install " + dstPath +
" to same path");
}
- pathProcessor.copyWithTimestamp(srcPath, dstPath);
+ if (hardLink) {
+ Files.createDirectories(dstPath.getParent());
+ if (Files.isDirectory(dstPath)) {
+ throw new IOException("destination is a directory: " +
dstPath);
+ }
+ try {
+ Files.deleteIfExists(dstPath);
+ Files.createLink(dstPath, srcPath);
+ } catch (UnsupportedOperationException |
IllegalArgumentException | IOException e) {
+ // cross-device link (EXDEV), provider mismatch (e.g.
Jimfs), or unsupported FS — fall back to copy
+ logger.warn("Hard link not supported for {}, falling back
to copy: {}", dstPath, e.getMessage());
+ pathProcessor.copyWithTimestamp(srcPath, dstPath);
+ }
+ } else {
Review Comment:
Addressed in 5fdca380: the per-artifact `logger.warn` is replaced with an
accumulate-then-log-once pattern — a single WARN fires after all artifacts are
processed, with suppressed exceptions for subsequent failures.
--
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]
