gnodet commented on code in PR #2198:
URL: https://github.com/apache/maven-resolver/pull/2198#discussion_r4258657925
##########
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:
Good question. The hard-link attempt is specific to the _installer's_
strategy (install = optimistically create hard link, fall back to copy), rather
than a general path operation. `PathProcessor` is a lower-level IO abstraction
shared across the codebase (resolver, deploy, etc.). Placing the
try-link-then-copy logic there would leak installer-specific semantics into a
general SPI — callers that don't want hard-link fallback behavior would need to
opt out. So I've kept it in `DefaultInstaller` for now.
Your broader point about `PathProcessorSupport`/`PathProcessor` having
unnecessary abstraction is noted — happy to address that in a separate issue/PR
if you think it's worthwhile.
##########
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());
Review Comment:
Implemented a variant of your suggestion: instead of a field, the private
`install(..., hardLink)` method now returns the link failure `Exception` (or
`null` on success), and the outer loop accumulates them via `addSuppressed()`;
a single `logger.warn()` fires after the loop with all suppressed causes
attached. This keeps the per-artifact method stateless (no shared mutable
field) while still logging only once. Commit 5fdca380.
--
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]
