gnodet commented on code in PR #2198:
URL: https://github.com/apache/maven-resolver/pull/2198#discussion_r4238567211
##########
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 in CI (re-raised from prior review — still open):** The
`catch (UnsupportedOperationException | IllegalArgumentException ignored)`
block silently swallows the inode check on non-POSIX or cross-device
filesystems, which covers virtually every CI environment (Docker, GitHub
Actions). On those systems, this test reduces to the same content-equality
assertion as every other install test — it provides zero behavioral coverage of
the hard-link path.
The fix requires either:
1. Use a `@TempDir` on the same filesystem as the local repo, forcing
same-device layout, then unconditionally assert inode equality.
2. Or subclass `DefaultInstaller` in the test to override the
`Files.createLink` call with a counter/spy, then assert the counter was
incremented.
##########
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:
💡 **Per-artifact WARN spam on cross-device environments:** `logger.warn` is
called inside the per-artifact loop — in a multi-module build where `~/.m2` and
the project are on different mounts (common in Docker containers), every
artifact install produces a separate WARN line. A 50-module build emits 50+
warnings saying the same thing.
Consider logging the first fallback at WARN and subsequent ones at DEBUG
within the same install request. One approach: pass a `boolean[] warnedOnce =
{false}` array into the private method, or check a local flag in the outer loop:
```suggestion
logger.debug("Hard link not supported for {}, falling
back to copy: {}", dstPath, e.getMessage());
pathProcessor.copyWithTimestamp(srcPath, dstPath);
```
Or alternatively, cache the fallback decision per session and only warn on
first encounter.
##########
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());
Review Comment:
⚠️ **Test behavioral gap (re-raised from prior review — still open):**
`testHardLinkDisabledFallsThroughToCopy` only asserts that the installed file
exists and has the right content — which is identical to the happy path of any
other install test. It does not verify that `Files.createLink` was NOT called,
so this test would pass even if the `hardLink=false` branch were never taken.
To actually test the `hardLink=false` code path, inject a spy on
`PathProcessor` and assert `copyWithTimestamp` was called, or check that the
installed file has a different inode from the source on same-device filesystems.
--
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]
