You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "goutamadwant (via GitHub)" <gi...@apache.org> on 2026/09/15 06:03:02 UTC
[PR] Include transitive SNAPSHOT inputs in project checksums [maven-build-cache-extension]
goutamadwant opened a new pull request, #534:
URL: https://github.com/apache/maven-build-cache-extension/pull/534
Fixes #407.
For `app -> bridge -> leaf-SNAPSHOT`, changing `leaf` can leave the reproducible `bridge` JAR byte-identical. The application checksum currently includes only declared mutable dependencies, so a clean application build can restore stale compiled constants from cache.
Collect Maven's mediated dependency graph before cache lookup and hash additional external SNAPSHOT artifacts. Use project checksums for transitive reactor inputs. If graph resolution is unavailable, skip cache restoration and saving, then let normal Maven execution enforce dependency requirements.
The integration tests cover SNAPSHOT and release bridges, unchanged-input cache hits, exclusions, dependency management, transitive reactor inputs, and offline `validate`. Both bridge cases fail on unchanged master (`8bec537`); their fixed cached outputs match uncached rebuilds. Graph collection happens before lookup; only additional external SNAPSHOT artifacts are resolved.
Validation on JDK 17:
- `mvn -Prun-its verify javadoc:jar`: passed with Maven 3.9.16. The run reported 97 unit tests and 354 integration tests, with zero failures or errors. Four schema-dependent unit tests and one macOS permission test were skipped.
- `mvn -Prun-its,maven3.10 -Dtest='MavenProjectInput*' -Dit.test=TransitiveSnapshotTest verify`: passed with Maven 3.10.0-rc-1 for the integration builds.
Following this checklist to help us incorporate your
contribution quickly and easily:
- [x] Your pull request should address just one issue, without pulling in other changes.
- [x] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [x] Each commit in the pull request should have a meaningful subject line and body.
Note that commits might be squashed by a maintainer on merge.
- [ ] Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied.
This may not always be possible but is a best-practice.
This change uses integration tests against Maven's real dependency resolver.
- [x] Run `mvn verify` to make sure basic checks pass.
A more thorough check will be performed on your pull request automatically.
- [x] You have run the integration tests successfully (`mvn -Prun-its verify`).
If your pull request is about ~20 lines of code you don't need to sign an
[Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf) if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
you have to acknowledge this by using the following check-box.
- [ ] I hereby declare this contribution to be licenced under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
- [ ] In any other case, please file an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
--
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] Include transitive SNAPSHOT inputs in project checksums [maven-build-cache-extension]
Posted by "olamy (via GitHub)" <gi...@apache.org>.
olamy commented on code in PR #534:
URL: https://github.com/apache/maven-build-cache-extension/pull/534#discussion_r4013637641
##########
src/main/java/org/apache/maven/buildcache/checksum/MavenProjectInput.java:
##########
@@ -665,7 +676,59 @@ private static boolean isReadable(Path entry) throws IOException {
}
private SortedMap<String, String> getMutableDependencies() throws IOException {
- return getMutableDependenciesHashes("", project.getDependencies());
+ SortedMap<String, String> hashes = getMutableDependenciesHashes("", project.getDependencies());
+ if (project.getDependencies().isEmpty()) {
+ return hashes;
+ }
+
+ // Collect through Maven so dependency management, exclusions and scope mediation match the build.
+ // Resolve only additional external snapshots; reactor dependencies use project checksums.
+ DefaultDependencyResolutionRequest request =
+ new DefaultDependencyResolutionRequest(project, session.getRepositorySession());
+ request.setResolutionFilter((node, parents) -> {
Review Comment:
there should be filtering here. for main source we do not need to check `test` scope dependencies. Any change in test dependencies will invalidate the cache for src/main
--
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] Include transitive SNAPSHOT inputs in project checksums [maven-build-cache-extension]
Posted by "olamy (via GitHub)" <gi...@apache.org>.
olamy commented on code in PR #534:
URL: https://github.com/apache/maven-build-cache-extension/pull/534#discussion_r4013623561
##########
src/main/java/org/apache/maven/buildcache/CacheControllerImpl.java:
##########
@@ -199,7 +200,14 @@ public CacheResult findCachedBuild(
String projectName = getVersionlessProjectKey(project);
- ProjectsInputInfo inputInfo = projectInputCalculator.calculateInput(project);
+ final ProjectsInputInfo inputInfo;
+ try {
+ inputInfo = projectInputCalculator.calculateInput(project);
+ } catch (DependencyGraphResolutionException e) {
+ LOGGER.warn("Skipping build cache for {}: {}", projectName, e.getMessage());
Review Comment:
Well, if any remote repositories check for snapshots, we will now skip a module that has not been restored or saved. Caching is becoming subject to remote repo availability; this sounds fragile to me.
--
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] Include transitive SNAPSHOT inputs in project checksums [maven-build-cache-extension]
Posted by "olamy (via GitHub)" <gi...@apache.org>.
olamy commented on code in PR #534:
URL: https://github.com/apache/maven-build-cache-extension/pull/534#discussion_r4013604256
##########
src/main/java/org/apache/maven/buildcache/checksum/MavenProjectInput.java:
##########
@@ -665,7 +676,59 @@ private static boolean isReadable(Path entry) throws IOException {
}
private SortedMap<String, String> getMutableDependencies() throws IOException {
- return getMutableDependenciesHashes("", project.getDependencies());
+ SortedMap<String, String> hashes = getMutableDependenciesHashes("", project.getDependencies());
+ if (project.getDependencies().isEmpty()) {
+ return hashes;
+ }
+
+ // Collect through Maven so dependency management, exclusions and scope mediation match the build.
+ // Resolve only additional external snapshots; reactor dependencies use project checksums.
+ DefaultDependencyResolutionRequest request =
Review Comment:
Well, now we are forcing full transitive graph collection + snapshot resolution during cache-key calculation, which could be avoided when you have cache hits, for cache hits where we were skipping dependency resolution.
On a large project, this will add unexpected potential latency
--
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] Include transitive SNAPSHOT inputs in project checksums [maven-build-cache-extension]
Posted by "AlexanderAshitkin (via GitHub)" <gi...@apache.org>.
AlexanderAshitkin commented on PR #534:
URL: https://github.com/apache/maven-build-cache-extension/pull/534#issuecomment-6136539969
I agree that the reproduced invalidation issue requires a fix. However, I have several concerns with the proposed implementation that I'd like resolved before merging.
### 1. Maven already defines and controls acceptable SNAPSHOT staleness
Maven does not guarantee that a build uses the latest available SNAPSHOT. It deliberately trades freshness for resolution cost and makes that trade-off configurable through settings `<snapshots><updatePolicy>`:
- `always` — check for updates on every resolution
- `daily` — check once per day (the default)
- `interval:X` — check at a configured interval
- `never` — do not check for updates
Additionally, `-U` forces update checks, while `-o` disables remote resolution. Maven persists update-check state in `resolver-status.properties`, avoiding unnecessary repository checks within the configured window.
Consequently, building against a day-old `leaf-1.0-SNAPSHOT` is valid Maven behavior, not a defect.
**The cache should preserve Maven's resolution semantics, not introduce a stricter freshness guarantees.** The correct invariant is not "outputs must match the latest artifact available in the repository," which Maven itself does not promise, but rather "outputs must match the inputs Maven actually resolved for this invocation."
For example:
- With `updatePolicy=never`, Maven continues using the previously resolved SNAPSHOT. The effective inputs remain unchanged, so a cache hit is correct.
- With `-U`, Maven checks for updated SNAPSHOTs. If a dependency changes, its checksum must reflect that change.
The cache therefore does not need a separate freshness policy. The acceptable staleness window is already configured per repository by the build's owner.
This also addresses the resolution-cost concern: `resolveArtifact` respects Maven's update policy and does not repeatedly download artifacts within the configured window. Forcing offline collection offers no meaningful advantage here.
### 2. Suggested implementation: track resolved mutable dependencies
Rather than introducing a separate fingerprint of the mediated dependency graph, I suggest expanding the existing mutable-dependency input set with the SNAPSHOT artifacts produced by Maven's dependency resolution, and store it within the effective pom. If the cached buildinfo contains the leaf snapshot dependency, the App will be properly invalidated.
Specifically:
- **Reactor dependencies:** Continue using their project checksums.
- **External SNAPSHOT dependencies:** Use the existing `resolveArtifact` mechanism and hash the resolved artifact files.
- **SNAPSHOTs:** Include them in the tracked inputs or the effective pom entry. This is the missing input in the current implementation.
This approach naturally accounts for dependency management, exclusions, scope mediation, and `<type>pom</type>` expansion because it tracks the results of Maven's resolution rather than attempting to reconstruct or independently fingerprint dependency graph semantics.
Using the original `session.getRepositorySession()` instead of the forced-offline wrapper also preserves Maven's existing update behavior, including `-U`, without additional policy logic.
It also eliminates the need for the proposed "incomplete graph" state and its random-UUID checksum marker. If dependency collection fails, the build will fail shortly afterward during `ensureDependenciesAreResolved` anyway. Introducing a separate fail-closed mechanism into cache key generation provides no useful recovery path.
### 3. Blocking concern: phase-dependent checksums
I cannot support the following change:
```java
String key = BuilderCommon.getKey(project) + "|testDependencies=" + includeTestDependencies;
```
**Identical resolved inputs must produce identical checksums, independently of the requested lifecycle phase.**
Beyond violating that invariant, this change breaks partial cache restoration.
`CacheControllerImpl` maintains one entry per checksum and progressively extends it using `getHighestCompletedGoal()` and `isLaterPhaseThanBuild`. Consequently, an entry created by `mvn compile` must remain discoverable when a subsequent invocation runs `mvn test-compile`.
Including `includeTestDependencies` in the key makes those entries incompatible. Partial restoration across these invocations becomes impossible because the checksum changes before the existing cache entry can be found.
There are additional consequences:
- `shouldIncludeTestDependencies` can change the key based on whether a `MojoDescriptor` was successfully resolved.
- CI builds running `verify` can no longer share remote cache entries with developers running `compile`, even when the relevant inputs are identical.
- The existing progressive cache restoration model becomes fragmented by lifecycle phase.
This is a regression in a fundamental cache invariant and **must be reverted before merging**.
### 4. Keep the PR focused on SNAPSHOT input tracking
I suggest limiting this PR to correcting SNAPSHOT dependency tracking.
The proposed graph-shape fingerprint, fail-closed behavior, and test-scope selectivity introduce separate design decisions with broader implications for cache semantics. These should be tracked as independent issues and evaluated on their own merits rather than bundled into this fix.
I would also remove two test cases:
**`releaseDependency`**
This scenario relies on a release artifact depending on a SNAPSHOT. Such dependencies are incompatible with standard release practices: `maven-release-plugin` rejects them during release preparation, and `enforcer:requireReleaseDeps` can explicitly prohibit them.
The cache intentionally treats release artifacts as immutable. Changing that assumption to accommodate this scenario is outside the scope of SNAPSHOT invalidation and should not be presented as a defect addressed by this PR.
**`testScopedTransitiveSnapshotOnlyInvalidatesTestLifecycle`**
This test passes on `master` for an unrelated reason. The cached entry never progresses beyond `compile`, so `testCompile` executes again and picks up the updated transitive dependency.
As a result, the test does not actually establish that test-scoped transitive SNAPSHOT changes selectively invalidate the test lifecycle. Its passing result is incidental rather than evidence of the behavior its name claims to verify.
### Summary
I agree that the reproduced invalidation issue requires a fix. My concern is with the additional cache semantics introduced by this implementation, not with the underlying bug.
The preferred solution is to track the actual mutable inputs produced by Maven's dependency resolution, including transitive SNAPSHOT dependencies, while preserving Maven's existing update policies and the cache's phase-independent checksum model.
That would address the demonstrated defect without introducing a parallel dependency fingerprint, a separate freshness policy, or regressions in partial cache restoration.
--
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