You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "gnodet (via GitHub)" <gi...@apache.org> on 2026/10/10 12:31:18 UTC
[PR] feat: use hard links for artifact installation by default [maven-resolver]
gnodet opened a new pull request, #2198:
URL: https://github.com/apache/maven-resolver/pull/2198
## Summary
Add `aether.installer.hardLink` configuration property (default `true`) to make `DefaultInstaller` create a hard link from the build output file to its destination in the local repository, instead of copying it.
This saves significant disk I/O for large JARs when building locally: instead of reading and writing the full file, the OS simply adds a new directory entry pointing to the same inode.
## Motivation
This was raised as a follow-up to [apache/maven-install-plugin#468](https://github.com/apache/maven-install-plugin/pull/468#pullrequestreview-5478638729). The install plugin only assembles the `ArtifactInstallerRequest` — the right place to implement hard-link installation is here in `DefaultInstaller`, which owns the file-copy step.
The feature is analogous to `FileTransporter.WriteOp.HARDLINK` which already exists for `file://` remote repositories.
## Behaviour
- **Default: `aether.installer.hardLink=true`** — hard link attempted first.
- **Fallback to copy** — when `Files.createLink` throws `UnsupportedOperationException` (unsupported filesystem) or `IOException` (cross-device link, `EXDEV`), the installer silently falls back to a regular file copy. The build is never broken.
- **Opt out: `aether.installer.hardLink=false`** — always copy, same as the previous behaviour.
## Design notes
- Property defined on `ConfigurationProperties` with full `@configurationSource` / `@configurationType` / `@configurationDefaultValue` / `@configurationRepoIdSuffix` annotations following the established pattern.
- `PREFIX_INSTALLER = "aether.installer."` added alongside the existing prefix constants.
- Hard links share an inode. Any tool that overwrites build output **in-place** (not via atomic rename) would silently corrupt the cached artifact. Standard Maven tooling always uses atomic rename for `target/`, so this is safe in practice.
- When destination exists as a directory, the code explicitly checks and falls through to `copyWithTimestamp` (which will fail as before), preserving existing error semantics.
## Testing
- `testHardLinkInstallCreatesHardLink` — verifies that install succeeds and (on same-device filesystems) produces a hard link (same inode). Gracefully skips the inode assertion on cross-device environments (common in CI containers).
- `testHardLinkDisabledFallsThroughToCopy` — sets `aether.installer.hardLink=false` and verifies install still succeeds with correct content.
- All 585 existing tests in `maven-resolver-impl` pass unchanged.
## Configuration
To opt out globally, add to `~/.m2/settings.xml`:
```xml
<!-- in Maven 4: via project properties or system property -->
```
Or pass on the command line:
```
mvn install -Daether.installer.hardLink=false
```
---
_Hermès Agent (Claude Sonnet 4.6) on behalf of Guillaume Nodet_
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: use hard links for artifact installation by default [maven-resolver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2198:
URL: https://github.com/apache/maven-resolver/pull/2198#discussion_r4250755962
##########
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:
The explicit directory check throws an `IOException` with a clear message, but that exception is immediately caught by the broad fallback catch (which also logs a misleading ‘Hard link not supported’ warning) and then overwritten by whatever `copyWithTimestamp` throws. If the intent is to preserve previous error semantics for directory destinations, avoid routing this case through the hard-link fallback catch (e.g., handle the directory case separately and proceed directly to `copyWithTimestamp` without warning, or rethrow without swallowing the more specific message).
##########
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:
Deleting the destination before the hard-link attempt can cause data loss if both the hard-link and the subsequent copy fail (e.g., permissions/disk-full during copy): the previously-installed artifact at `dstPath` is removed and not restored. To avoid leaving the local repository in a worse state on failure, use a temp destination strategy (create link/copy to a temp path, then move/replace) or defer removal of the existing destination until after a successful replacement.
##########
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:
The PR description states the installer will ‘silently’ fall back to copy, but the implementation logs a WARN on every hard-link failure. This can create noisy builds (especially in CI with cross-device mounts) and is inconsistent with the documented behavior. Consider removing the log entirely, or downgrading it to DEBUG/TRACE (and ideally include the throwable when enabled) so fallback remains silent by default.
##########
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 explicit directory check throws an `IOException` with a clear message, but that exception is immediately caught by the broad fallback catch (which also logs a misleading ‘Hard link not supported’ warning) and then overwritten by whatever `copyWithTimestamp` throws. If the intent is to preserve previous error semantics for directory destinations, avoid routing this case through the hard-link fallback catch (e.g., handle the directory case separately and proceed directly to `copyWithTimestamp` without warning, or rethrow without swallowing the more specific message).
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: use hard links for artifact installation by default [maven-resolver]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: use hard links for artifact installation by default [maven-resolver]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #2198:
URL: https://github.com/apache/maven-resolver/pull/2198#discussion_r4327559156
##########
maven-resolver-api/src/main/java/org/eclipse/aether/ConfigurationProperties.java:
##########
@@ -689,6 +696,37 @@ 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, or a custom in-memory filesystem such as
+ * Jimfs is used for testing), the JVM throws {@link java.nio.file.UnsupportedOperationException},
Review Comment:
```suggestion
* Jimfs is used for testing), the JVM throws {@link java.lang.UnsupportedOperationException},
```
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: use hard links for artifact installation by default [maven-resolver]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas merged PR #2198:
URL: https://github.com/apache/maven-resolver/pull/2198
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: use hard links for artifact installation by default [maven-resolver]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on PR #2198:
URL: https://github.com/apache/maven-resolver/pull/2198#issuecomment-6130646231
## Interaction with Windows IDE-lock fix (PR #2205)
When `hardLink=true` and IntelliJ (or another IDE) has the destination `.jar` open on Windows, `Files.deleteIfExists(dstPath)` inside the try block throws `AccessDeniedException` (a subclass of `IOException`). That is correctly caught and falls back to `pathProcessor.copyWithTimestamp()`.
However, `copyWithTimestamp()` internally uses `retryingMove()`, which also fails with `AccessDeniedException` on the same locked target — so by itself, this PR's degradation path still ends with a build failure on Windows+IDE.
PR #2205 fixes `retryingMove()` to add a last-resort `classicCopy()` fallback after exhausting all rename attempts. Together the two PRs compose cleanly: hard-link fails → copyWithTimestamp → retryingMove retries → classicCopy fallback → build succeeds.
The degradation chain in `hardLink=true` mode on Windows with an IDE lock:
1. `Files.deleteIfExists(dstPath)` → `IOException` (caught ✅)
2. Falls back to `pathProcessor.copyWithTimestamp(srcPath, dstPath)`
3. `retryingMove()` → `AccessDeniedException` × N retries
4. **With PR #2205**: `classicCopy(source, target)` → stream write succeeds ✅
So this PR's fallback degradation is correct but incomplete without #2205.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: use hard links for artifact installation by default [maven-resolver]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: use hard links for artifact installation by default [maven-resolver]
Posted by "desruisseaux (via GitHub)" <gi...@apache.org>.
desruisseaux commented on code in PR #2198:
URL: https://github.com/apache/maven-resolver/pull/2198#discussion_r4241732078
##########
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:
Should this block be moved to `PathProcessor` for consistency with `copyWithTimestamp` method?
Note: I don't think that the level of abstraction of `PathProcessor` (with a separation between an interface and a single implementation class with no use case I could see for more classes) is necessary. I would suggest to retrofit `PathProcessorSupport` into `PathProcessor`, but this is another story.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: use hard links for artifact installation by default [maven-resolver]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: use hard links for artifact installation by default [maven-resolver]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: use hard links for artifact installation by default [maven-resolver]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #2198:
URL: https://github.com/apache/maven-resolver/pull/2198#discussion_r4327544154
##########
maven-resolver-api/src/main/java/org/eclipse/aether/ConfigurationProperties.java:
##########
@@ -689,6 +696,37 @@ 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, or a custom in-memory filesystem such as
+ * Jimfs is used for testing), the JVM throws {@link java.nio.file.UnsupportedOperationException},
Review Comment:
[ERROR] /home/cstamas/Worx/apache-maven/maven-resolver/maven-resolver-api/src/main/java/org/eclipse/aether/ConfigurationProperties.java:798: error: reference not found
[ERROR] * Jimfs is used for testing), the JVM throws {@link java.nio.file.UnsupportedOperationException},
Seems Javadoc fails on this link, site cannot be built with this change. (I had locally merged multiple PRs, line count is off)
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: use hard links for artifact installation by default [maven-resolver]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #2198:
URL: https://github.com/apache/maven-resolver/pull/2198#discussion_r4327544154
##########
maven-resolver-api/src/main/java/org/eclipse/aether/ConfigurationProperties.java:
##########
@@ -689,6 +696,37 @@ 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, or a custom in-memory filesystem such as
+ * Jimfs is used for testing), the JVM throws {@link java.nio.file.UnsupportedOperationException},
Review Comment:
[ERROR] /home/cstamas/Worx/apache-maven/maven-resolver/maven-resolver-api/src/main/java/org/eclipse/aether/ConfigurationProperties.java:798: error: reference not found
[ERROR] * Jimfs is used for testing), the JVM throws {@link java.nio.file.UnsupportedOperationException},
Seems Javadoc fails on this link, site cannot be built with this change.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: use hard links for artifact installation by default [maven-resolver]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: use hard links for artifact installation by default [maven-resolver]
Posted by "desruisseaux (via GitHub)" <gi...@apache.org>.
desruisseaux commented on code in PR #2198:
URL: https://github.com/apache/maven-resolver/pull/2198#discussion_r4241741162
##########
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:
Indeed, we may need a mechanism for avoiding duplication. But I would suggest a private `Exception linkFailures` field of something like that, and a some like below in the `catch` block:
```java
if (linkFailures == null) {
linkFailure = e;
} else {
linkFailures.addSuppressed(e);
}
```
and log `linkFailures` at the end, after all JAR have been installed.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: use hard links for artifact installation by default [maven-resolver]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on code in PR #2198:
URL: https://github.com/apache/maven-resolver/pull/2198#discussion_r4258744399
##########
maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultInstallerTest.java:
##########
@@ -377,4 +379,90 @@ 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
+ //
+ // Use a counting PathProcessor to detect whether the fallback copy path was taken.
+ // On same-device filesystems: hard link is created (copyWithTimestamp NOT called).
+ // On cross-device (common in CI containers): Files.createLink throws, copy IS taken.
+ CountingPathProcessor countingProcessor = new CountingPathProcessor();
+ installer = new DefaultInstaller(
+ countingProcessor,
+ new StubRepositoryEventDispatcher(),
+ Collections.emptyMap(),
+ Collections.emptyMap(),
+ Collections.emptyMap(),
+ new StubSyncContextFactory());
+
+ 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, no copy).
+ // 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, copy NOT taken
+ 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");
+ assertEquals(
+ 0,
+ countingProcessor.copyWithTimestampCount,
+ "copyWithTimestamp must NOT be called on same-device FS");
+ } else {
+ // cross-device: fallback to copy must have been taken
+ assertEquals(
+ 1,
+ countingProcessor.copyWithTimestampCount,
+ "copyWithTimestamp must be called as fallback on cross-device FS");
+ }
+ } catch (UnsupportedOperationException | IllegalArgumentException ignored) {
Review Comment:
⚠️ **Counting assertions silently skipped on non-POSIX FS (re-raised — partially addressed)**
The `CountingPathProcessor` counting assertions (both the `assertEquals(0, count...)` and `assertEquals(1, count...)`) are inside this `try` block. When `Files.getAttribute(srcPath, "unix:dev")` throws `UnsupportedOperationException` on a non-POSIX filesystem (Windows, Jimfs), the catch fires and all counting assertions are silently skipped.
Maven-resolver CI is Linux-only, so the cross-device `else` branch now fires correctly there ✅. But a Windows developer running locally gets zero behavioral coverage from this test.
Fix: add a counting assertion in the `catch` block too — `countingProcessor.copyWithTimestampCount` should be 0 or 1 regardless of FS type:
```suggestion
} catch (UnsupportedOperationException | IllegalArgumentException ignored) {
// non-POSIX filesystem — inode/device check not available
// but we can still assert that at most one copy was attempted
assertTrue(
countingProcessor.copyWithTimestampCount == 0
|| countingProcessor.copyWithTimestampCount == 1,
"expected 0 copies (hard link) or 1 copy (fallback), got: "
+ countingProcessor.copyWithTimestampCount);
}
```
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org