gnodet commented on code in PR #2198:
URL: https://github.com/apache/maven-resolver/pull/2198#discussion_r4237887966
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultInstaller.java:
##########
@@ -205,7 +209,26 @@ private void install(RepositorySystemSession session,
RequestTrace trace, Artifa
throw new IllegalStateException("cannot install " + dstPath +
" to same path");
}
- pathProcessor.copyWithTimestamp(srcPath, dstPath);
+ boolean hardLink = ConfigUtils.getBoolean(
+ session,
+ ConfigurationProperties.DEFAULT_INSTALLER_HARD_LINK,
+ ConfigurationProperties.INSTALLER_HARD_LINK);
+ if (hardLink) {
+ try {
+ Files.createDirectories(dstPath.getParent());
+ if (Files.isDirectory(dstPath)) {
+ throw new IOException("destination is a directory: " +
dstPath);
Review Comment:
🔴 **IOException guard is immediately swallowed by the enclosing catch** —
`throw new IOException("destination is a directory: " + dstPath)` at line 220
is inside the `try` block whose `catch` at line 224 catches `IOException`. The
intentional guard is silently caught, causing a fallback to
`copyWithTimestamp(srcPath, dstPath)` on a directory — which also fails, but
with a confusing, less actionable error.
Move the directory check *before* the `try` block so it can propagate
properly:
```suggestion
if (Files.isDirectory(dstPath)) {
throw new IOException("destination is a directory: "
+ dstPath);
}
try {
```
Or alternatively, re-throw when the caught `IOException` was the one we
threw intentionally (check `e.getMessage().startsWith("destination is a
directory")` — brittle, so the first option is cleaner).
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultInstaller.java:
##########
@@ -205,7 +209,26 @@ private void install(RepositorySystemSession session,
RequestTrace trace, Artifa
throw new IllegalStateException("cannot install " + dstPath +
" to same path");
}
- pathProcessor.copyWithTimestamp(srcPath, dstPath);
+ boolean hardLink = ConfigUtils.getBoolean(
Review Comment:
💡 **`ConfigUtils.getBoolean()` called per-artifact** — for a multi-module
build, this reads the same immutable session property on every artifact.
Consider hoisting it to the outer `install(RepositorySystemSession session,
InstallRequest request)` and passing the boolean down to the private method.
##########
maven-resolver-api/src/main/java/org/eclipse/aether/ConfigurationProperties.java:
##########
@@ -88,6 +88,13 @@ public final class ConfigurationProperties {
*/
public static final String PREFIX_UTIL = PREFIX_AETHER + "util.";
+ /**
+ * Prefix for installer related configurations. <em>For internal use
only.</em>
+ *
+ * @since 2.0.9
Review Comment:
⚠️ **Wrong `@since` version** — `2.0.9` but the current POM is
`2.0.25-SNAPSHOT`. Same issue on the `INSTALLER_HARD_LINK` (line 715) and
`DEFAULT_INSTALLER_HARD_LINK` (line 726) entries.
##########
maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultInstallerTest.java:
##########
@@ -377,4 +379,47 @@ void testSetArtifactTimestamps() throws
InstallationException {
localArtifactFile.lastModified(),
"artifact timestamp was not set to src file");
}
+
+ @Test
+ void testHardLinkInstallCreatesHardLink() throws InstallationException,
IOException {
+ // default: hardLink=true — installer attempts a hard link, falls back
to copy on cross-device
+ request.addArtifact(artifact);
+ installer.install(session, request);
+
+ assertTrue(localArtifactFile.exists());
+ assertEquals("artifact", TestFileUtils.readString(localArtifactFile));
+
+ // On same-device filesystems, verify a hard link was created (same
inode).
+ // On cross-device (common in CI containers where /tmp and project dir
are on different mounts),
+ // Files.createLink throws IOException and the installer falls back to
copy — both are correct.
+ Path srcPath = artifact.getFile().toPath();
+ Path dstPath = localArtifactFile.toPath();
+ try {
+ boolean sameDevice =
+ Files.getAttribute(srcPath,
"unix:dev").equals(Files.getAttribute(dstPath, "unix:dev"));
+ if (sameDevice) {
+ // hard link must have been created — same inode expected
+ assertEquals(
+ Files.getAttribute(srcPath, "unix:ino"),
+ Files.getAttribute(dstPath, "unix:ino"),
+ "installed artifact should be a hard link to the
source on same-device FS");
+ }
+ // cross-device: fallback to copy is correct — content already
asserted above
+ } catch (UnsupportedOperationException | IllegalArgumentException
ignored) {
Review Comment:
💬 **Test degeneracy on cross-device CI** — on any CI where `/tmp` and the
project directory are on different mounts (very common with containers),
`Files.createLink` throws `IOException`, the installer falls back to copy, and
this test reduces to content-equality only — which is indistinguishable from a
plain copy. The hard-link path is effectively untested in CI.
Consider adding a `@EnabledOnOs(OS.LINUX)` + a same-device `tmpfs` fixture
that forces the same-device scenario, or at minimum add a log/counter spy to
assert the hard-link branch was taken.
##########
maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultInstallerTest.java:
##########
@@ -377,4 +379,47 @@ void testSetArtifactTimestamps() throws
InstallationException {
localArtifactFile.lastModified(),
"artifact timestamp was not set to src file");
}
+
+ @Test
+ void testHardLinkInstallCreatesHardLink() throws InstallationException,
IOException {
+ // default: hardLink=true — installer attempts a hard link, falls back
to copy on cross-device
+ request.addArtifact(artifact);
+ installer.install(session, request);
+
+ assertTrue(localArtifactFile.exists());
+ assertEquals("artifact", TestFileUtils.readString(localArtifactFile));
+
+ // On same-device filesystems, verify a hard link was created (same
inode).
+ // On cross-device (common in CI containers where /tmp and project dir
are on different mounts),
+ // Files.createLink throws IOException and the installer falls back to
copy — both are correct.
+ Path srcPath = artifact.getFile().toPath();
+ Path dstPath = localArtifactFile.toPath();
+ try {
+ boolean sameDevice =
+ Files.getAttribute(srcPath,
"unix:dev").equals(Files.getAttribute(dstPath, "unix:dev"));
+ if (sameDevice) {
+ // hard link must have been created — same inode expected
+ assertEquals(
+ Files.getAttribute(srcPath, "unix:ino"),
+ Files.getAttribute(dstPath, "unix:ino"),
+ "installed artifact should be a hard link to the
source on same-device FS");
+ }
+ // cross-device: fallback to copy is correct — content already
asserted above
+ } catch (UnsupportedOperationException | IllegalArgumentException
ignored) {
+ // non-POSIX filesystem — inode/device check not available;
content assertion is sufficient
+ }
+ }
+
+ @Test
+ void testHardLinkDisabledFallsThroughToCopy() throws
InstallationException, IOException {
+ // set hardLink=false — installer must copy, not hard-link
+ session.setConfigProperty(ConfigurationProperties.INSTALLER_HARD_LINK,
false);
+ request.addArtifact(artifact);
+ installer.install(session, request);
+
+ assertTrue(localArtifactFile.exists());
+ assertEquals("artifact", TestFileUtils.readString(localArtifactFile));
+ // With copy the content is correct; we cannot assert different inodes
portably
+ // but we verify the install succeeds and produces the correct content
+ }
Review Comment:
💬 **`testHardLinkDisabledFallsThroughToCopy` only asserts content, not
behavior** — there is no assertion that `Files.createLink` was *not* called. A
spy/mock on `Files.createLink` (or a counter on the fallback-copy path) would
make the test actually verify the `hardLink=false` code path is taken.
--
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]
