gnodet-bot commented on code in PR #13728:
URL: https://github.com/apache/maven/pull/13728#discussion_r4393425123
##########
impl/maven-core/src/test/java/org/apache/maven/lifecycle/internal/LifecycleDependencyResolverTest.java:
##########
@@ -79,4 +79,85 @@ void testCachedReactorProjectDependencies() throws Exception
{
.get("org.apache.maven.its.mng6300:mng6300-lib")
.getFile());
}
+
+ /**
+ * Regression test for the concurrent-builder variant of the reactor
artifact file race
+ * (see <a href="https://github.com/apache/maven/pull/13728";>#13728</a>).
+ *
+ * <p>In the concurrent builder a sibling reactor project (lib) may not
have written its
+ * artifact jar yet when {@code resolveProjectDependencies} is called for
a downstream
+ * project (war). The old code snapshotted {@code
reactorProject.getArtifact().getFile()}
+ * up-front into a {@code Map<Artifact, File>}. When that snapshot was
{@code null} (jar not
+ * yet produced), the subsequent null-guard {@code if (reactorProjectFile
!= null)} silently
+ * skipped the {@code setFile()} call. Even after lib finished packaging
and its artifact
+ * file was updated, the snapshot remained {@code null} — a later
re-resolution of war would
+ * still leave lib's artifact with a {@code null} file, causing an NPE in
any plugin that
+ * called {@code artifact.getFile().getAbsolutePath()} (e.g.
maven-dependency-plugin:properties
+ * when used to expose the Mockito agent path to Surefire).
+ *
+ * <p>The fix replaces {@code Map<Artifact, File>} with {@code
Map<Artifact, MavenProject>}
+ * and defers {@code getFile()} to lookup time, so the most up-to-date
value is always used.
+ * This test verifies:
+ * <ol>
+ * <li>When lib's file is {@code null} at first resolution time, war's
resolved lib artifact
+ * file is also {@code null} (no premature NPE).</li>
+ * <li>After lib's artifact file is set (simulating concurrent jar
packaging completing),
+ * a second resolution of war picks up the new file value.</li>
+ * </ol>
+ */
+ @Test
+ void testConcurrentBuilderReactorArtifactFileResolvedLate() throws
Exception {
Review Comment:
⚠️ **This test does not validate the fix.**
Confirmed dynamically: running
`testConcurrentBuilderReactorArtifactFileResolvedLate` against the pre-fix
source code (`Map<Artifact, File>` snapshot) produces a BUILD SUCCESS — all
assertions pass with both old and new code.
**Why:** `resolveProjectDependencies()` rebuilds the `reactorProjects` map
on **every** call. On the second invocation (after
`lib.getArtifact().setFile(new File("lib.jar"))`), the old code constructs a
fresh `Map<Artifact, File>` that snapshots the *updated* `lib.jar` value — so
`artifact.setFile(lib.jar)` is still called, and both assertions succeed.
The actual race occurs **within a single call**: another thread sets the
file *after* the snapshot is taken but *before* the iteration loop uses it —
something a sequential test cannot trigger.
The Javadoc claim "old code would still return null because the snapshot was
taken before lib set the file" is therefore incorrect: the snapshot is
per-call, not per-build.
Options:
1. Use a spy/mock that captures the map state mid-call to detect stale
snapshots, or
2. Revise the Javadoc to be honest: this test verifies the *null-file
graceful-handling* behavior and documents the expected post-fix behavior, but
cannot reproduce the concurrent race itself. Either way, the Javadoc must not
claim the test catches the regression it cannot.
##########
.github/dependabot.yml:
##########
@@ -15,89 +15,4 @@
# limitations under the License.
#
version: 2
-updates:
-
- - package-ecosystem: "maven"
- directory: "/"
- schedule:
- interval: "daily"
-
- - package-ecosystem: "maven"
- directory: "/"
- schedule:
- interval: "daily"
- target-branch: "maven-4.0.x"
- labels:
- - "mvn40"
- - "dependencies"
-
- - package-ecosystem: "maven"
- directory: "/"
- schedule:
- interval: "daily"
- target-branch: "maven-3.9.x"
- labels:
- - "mvn3"
- - "dependencies"
- ignore:
- - dependency-name: "ch.qos.logback:*"
- versions: [ "[1.3.0,)" ]
- - dependency-name: "org.codehaus.plexus:plexus-cipher"
- versions: [ ">=2.1.0" ]
- # Mockito 5 requires Java 11; the build still runs on JDK 8
- - dependency-name: "org.mockito:*"
- versions: [">= 5.0.0"]
-
- - package-ecosystem: "maven"
- directory: "/"
- schedule:
- interval: "daily"
- target-branch: "maven-3.10.x"
- labels:
- - "mvn3"
- - "dependencies"
- ignore:
- - dependency-name: "org.jline:*"
- versions: [ "[4.0.0,)" ]
- - dependency-name: "ch.qos.logback:*"
- versions: [ "[1.3.0,)" ]
- - dependency-name: "org.codehaus.plexus:plexus-cipher"
- versions: [ ">=2.1.0" ]
- # Mockito 5 requires Java 11; the build still runs on JDK 8
- - dependency-name: "org.mockito:*"
- versions: [">= 5.0.0"]
-
- - package-ecosystem: "github-actions"
- directory: "/"
- schedule:
- interval: "daily"
-
- - package-ecosystem: "github-actions"
- directory: "/"
- schedule:
- interval: "daily"
- target-branch: "maven-4.0.x"
- labels:
- - "mvn40"
- - "dependencies"
- - "github_actions"
-
- - package-ecosystem: "github-actions"
- directory: "/"
- schedule:
- interval: "daily"
- target-branch: "maven-3.9.x"
- labels:
- - "mvn3"
- - "dependencies"
- - "github_actions"
-
- - package-ecosystem: "github-actions"
- directory: "/"
- schedule:
- interval: "daily"
- target-branch: "maven-3.10.x"
- labels:
- - "mvn3"
- - "dependencies"
- - "github_actions"
\ No newline at end of file
+updates: []
Review Comment:
⚠️ **Accidental fork housekeeping commit included in this PR** — commit
`50878ac103` (`chore: disable Dependabot on fork`) replaces all 8 Dependabot
update configurations (Maven + GitHub Actions for master/4.0.x/3.9.x/3.10.x)
with `updates: []`, which would **disable all Dependabot automation** on the
upstream apache/maven repo if merged. This is fork housekeeping that has no
place in a bug-fix PR targeting upstream. Please rebase and drop this commit
before merging.
--
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]
