gnodet commented on code in PR #2198:
URL: https://github.com/apache/maven-resolver/pull/2198#discussion_r4237645604
##########
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**: the current POM snapshot is
`2.0.25-SNAPSHOT`, so this should be `@since 2.0.25`, not `@since 2.0.9`.
##########
maven-resolver-api/src/main/java/org/eclipse/aether/ConfigurationProperties.java:
##########
@@ -689,6 +696,36 @@ public enum HttpVersion {
*/
public static final boolean DEFAULT_HTTP_SEND_RFC9457_ACCEPT = true;
+ /**
+ * A flag indicating whether artifact installation should use hard links
instead of copying files. When enabled,
+ * the installer creates a hard link from the source artifact (build
output) to its destination in the local
+ * repository, avoiding a full file copy and saving disk I/O and space for
large JARs.
+ * <p>
+ * Hard links require that the source and target paths reside on the
<strong>same filesystem volume</strong>.
+ * When they do not (e.g. {@code ~/.m2} is on a separate mount), the JVM
throws
+ * {@link java.nio.file.UnsupportedOperationException} or {@link
java.nio.file.AtomicMoveNotSupportedException}
Review Comment:
⚠️ **Inaccurate exception reference**: `AtomicMoveNotSupportedException` is
only thrown by `Files.move()` with `ATOMIC_MOVE`. `Files.createLink()` throws
`UnsupportedOperationException` (unsupported FS) or `IOException` (e.g. EXDEV
on cross-device). Please replace with:
```suggestion
* When they do not (e.g. {@code ~/.m2} is on a separate mount), the JVM
throws
* {@link java.nio.file.UnsupportedOperationException} or {@link
java.io.IOException}
```
##########
maven-resolver-api/src/main/java/org/eclipse/aether/ConfigurationProperties.java:
##########
@@ -689,6 +696,36 @@ public enum HttpVersion {
*/
public static final boolean DEFAULT_HTTP_SEND_RFC9457_ACCEPT = true;
+ /**
+ * A flag indicating whether artifact installation should use hard links
instead of copying files. When enabled,
+ * the installer creates a hard link from the source artifact (build
output) to its destination in the local
+ * repository, avoiding a full file copy and saving disk I/O and space for
large JARs.
+ * <p>
+ * Hard links require that the source and target paths reside on the
<strong>same filesystem volume</strong>.
+ * When they do not (e.g. {@code ~/.m2} is on a separate mount), the JVM
throws
+ * {@link java.nio.file.UnsupportedOperationException} or {@link
java.nio.file.AtomicMoveNotSupportedException}
+ * and the installer automatically falls back to a regular file copy, so
the build is never broken.
+ * <p>
+ * <strong>Note:</strong> hard links share an inode. Any tool that
overwrites the build output file
+ * <em>in-place</em> (rather than via an atomic rename) would silently
corrupt the cached artifact.
+ * Standard Maven build tooling always replaces output via rename, so this
is generally safe. Set this
+ * property to {@code false} if your environment performs in-place writes
to build output files.
+ *
+ * @since 2.0.9
Review Comment:
⚠️ **Wrong `@since` version**: should be `@since 2.0.25` (current snapshot
is `2.0.25-SNAPSHOT`).
```suggestion
* @since 2.0.25
```
##########
maven-resolver-api/src/main/java/org/eclipse/aether/ConfigurationProperties.java:
##########
@@ -689,6 +696,36 @@ public enum HttpVersion {
*/
public static final boolean DEFAULT_HTTP_SEND_RFC9457_ACCEPT = true;
+ /**
+ * A flag indicating whether artifact installation should use hard links
instead of copying files. When enabled,
+ * the installer creates a hard link from the source artifact (build
output) to its destination in the local
+ * repository, avoiding a full file copy and saving disk I/O and space for
large JARs.
+ * <p>
+ * Hard links require that the source and target paths reside on the
<strong>same filesystem volume</strong>.
+ * When they do not (e.g. {@code ~/.m2} is on a separate mount), the JVM
throws
+ * {@link java.nio.file.UnsupportedOperationException} or {@link
java.nio.file.AtomicMoveNotSupportedException}
+ * and the installer automatically falls back to a regular file copy, so
the build is never broken.
+ * <p>
+ * <strong>Note:</strong> hard links share an inode. Any tool that
overwrites the build output file
+ * <em>in-place</em> (rather than via an atomic rename) would silently
corrupt the cached artifact.
+ * Standard Maven build tooling always replaces output via rename, so this
is generally safe. Set this
+ * property to {@code false} if your environment performs in-place writes
to build output files.
+ *
+ * @since 2.0.9
+ * @configurationSource {@link
RepositorySystemSession#getConfigProperties()}
+ * @configurationType {@link java.lang.Boolean}
+ * @configurationDefaultValue {@link #DEFAULT_INSTALLER_HARD_LINK}
+ * @configurationRepoIdSuffix No
+ */
+ public static final String INSTALLER_HARD_LINK = PREFIX_INSTALLER +
"hardLink";
+
+ /**
+ * The default value for {@link #INSTALLER_HARD_LINK}: {@code true}.
+ *
+ * @since 2.0.9
Review Comment:
⚠️ **Wrong `@since` version**: should be `@since 2.0.25`.
```suggestion
* @since 2.0.25
```
##########
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);
+ }
+ Files.deleteIfExists(dstPath);
+ Files.createLink(dstPath, srcPath);
+ } catch (UnsupportedOperationException | IOException e) {
Review Comment:
🐛 **`IOException` from directory check is silently swallowed**: `throw new
IOException("destination is a directory: " + dstPath)` at line 220 is inside
the `try` block, so it's caught by `catch (UnsupportedOperationException |
IOException e)` at line 224 and treated as a "hard link not supported"
condition — the fallback copies to the directory path, which either silently
creates a file named by the directory or succeeds incorrectly.
The `isDirectory` check should be moved **before** the `try` block (or
rethrown as an `IllegalStateException`) so it isn't masked:
```suggestion
if (Files.isDirectory(dstPath)) {
throw new IllegalStateException("destination is a
directory: " + dstPath);
}
Files.deleteIfExists(dstPath);
Files.createLink(dstPath, srcPath);
} catch (UnsupportedOperationException | IOException e) {
```
##########
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);
Review Comment:
💡 **Config read per artifact**: `ConfigUtils.getBoolean()` is called inside
the per-artifact `install()` method, so it's invoked once per artifact per
session. This property is session-scoped and never changes mid-install.
Consider reading it once in the caller and passing it in (or reading it at the
top of the outer `install(RepositorySystemSession, InstallRequest)` method).
##########
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);
+ }
+ Files.deleteIfExists(dstPath);
+ Files.createLink(dstPath, srcPath);
+ } catch (UnsupportedOperationException | IOException e) {
+ // cross-device link (EXDEV) or unsupported FS — fall back
to copy
+ logger.debug("Hard link not supported for {}, falling back
to copy: {}", dstPath, e.getMessage());
Review Comment:
💡 **Log level**: `DEBUG` means users won't see when the fallback is
triggered unless they enable debug logging. Since the fallback changes IO
behavior (copy vs. hard-link), this could matter for diagnosing slow builds or
unexpected disk usage. Consider `WARN` so it's visible by default, or at least
`INFO`.
```suggestion
logger.warn("Hard link not supported for {}, falling
back to copy: {}", dstPath, e.getMessage());
```
##########
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 degenerates in CI**: in most CI environments (Docker containers,
GitHub Actions), `/tmp` and the project directory are on different mounts
(`tmpfs` vs. overlay), so `Files.createLink()` throws and the installer falls
back to copy. In that case, `sameDevice` is `false`, the inode assertion is
skipped, and the test only checks content — identical to
`testSetArtifactTimestamps`. This means CI won't catch a regression that
removes the `Files.createLink()` call entirely.
Consider using a `@TempDir` on the same filesystem as the project, or spying
on `Files.createLink` via a test subclass of `DefaultInstaller` that overrides
the link attempt to record whether it was called.
##########
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:
💡 **Test coverage gap**: this test only asserts the install succeeds and
produces correct content when `hardLink=false`. It doesn't assert that
`Files.createLink` was NOT called. Without a spy or mock, this test would still
pass even if `hardLink=false` was completely ignored and a hard link was
created. Consider adding a test subclass that overrides or intercepts the link
attempt to verify the copy 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]
