Repository navigation
Fix #12774: skip consumer POM re-attachment on repeated task segments - #12916
Conversation
…gments Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
gnodet
left a comment
There was a problem hiding this comment.
Correct and minimal fix that makes injectTransformedArtifacts() idempotent by guarding against duplicate consumer POM attachment.
One low-severity optimization opportunity noted inline.
Positive observations:
TransformedArtifactalready handles POM-change detection lazily via SHA1 checksumming inmayUpdate(), so skipping re-attachment is safe — the already-attached artifact will still pick up source POM changes on the nextgetFile()call.- The matching criteria (
getClassifier()+getType()) on Maven'sArtifactinterface is correct for this context.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
| deferDeleteFile(consumer); | ||
|
|
||
| project.addAttachedArtifact(createConsumerPomArtifact(project, consumer, session)); | ||
| boolean alreadyAttached = project.getAttachedArtifacts().stream() |
There was a problem hiding this comment.
Low: The idempotency check here is placed after the temp file creation and deferDeleteFile() registration (lines 87-90). On repeated invocations where the consumer POM is already attached, a new temp file is created on disk, registered for deferred deletion, but never used.
Consider hoisting the alreadyAttached check to the top of the if (Features.consumerPom(...)) block, before creating the temp file. This would avoid unnecessary I/O on repeated invocations. The files are cleaned up by @PreDestroy doDeleteFiles() so this is not a leak, just wasteful work.
There was a problem hiding this comment.
Do you want to apply this improvement before merging as we had some performance issue with local repository in rc6 or do you want to apply it in a separate PR/later version?
Avoid unnecessary I/O on repeated invocations by checking whether the consumer POM is already attached before creating a temp file and registering it for deferred deletion. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
gnodet
left a comment
There was a problem hiding this comment.
Delta review — checking new commit 9515783 ("Hoist alreadyAttached check before temp file creation").
✅ Previous finding resolved: The alreadyAttached check is now the first statement inside the if (Features.consumerPom(...)) block, and all temp file creation / deferDeleteFile() / addAttachedArtifact calls are wrapped inside if (!alreadyAttached). No unnecessary I/O on repeated invocations.
A new regression test injectTransformedArtifactsTwiceShouldNotDuplicate verifies the idempotency behavior — clean, well-structured test.
No new issues introduced.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
…#12916) (#12956) * Fix #12774: skip consumer POM re-attachment on repeated task segments * Hoist alreadyAttached check before temp file creation Avoid unnecessary I/O on repeated invocations by checking whether the consumer POM is already attached before creating a temp file and registering it for deferred deletion. --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
rc-7 (apache/maven#12916) skips attaching a consumer POM when one is already attached. The batch runner reuses MavenProject instances across tasks, so the consumer POM attached by an earlier phase kept the raw pom.xml path and install failed to resolve versions from the flattened parent. Detach it before each task so it is rebuilt from the current pom file.
…#37330) ## Current Behavior On Maven 4.0.0-rc-7, batch-mode `install` fails for modules whose dependency versions come from a flattened parent POM (for example, flatten-maven-plugin in `oss` mode): ``` ModelBuilderException - 'dependencies.dependency.version' for ...:gson:jar must be a valid version but is '${gson.version}' - 'dependencies.dependency.version' for ...:spring-boot-starter-webmvc:jar is missing ``` rc-7 no longer re-attaches a consumer POM when one is already attached (apache/maven#12916). The attached consumer POM records the project's pom file when it is attached. The Maven 4 batch adapter reuses `MavenProject` instances across tasks, so the consumer POM attached by an early phase still points at the raw `pom.xml`. `install` then builds the consumer POM from the raw file and resolves the parent to its flattened copy, which has lost its properties and `dependencyManagement`. On rc-5 every task re-attached it from the current pom file, so this worked. Plain `mvn install` and non-batch runs are unaffected. ## Expected Behavior Before each batch task, the Maven 4 adapter detaches any consumer POM attached by an earlier task. Maven then re-attaches one from the current pom file, matching rc-5 behavior. This is a no-op on earlier Maven 4 RCs. The Maven 4 batch e2e now runs on 4.0.0-rc-7. It clears `MAVEN_HOME` so the workspace wrapper's Maven is used instead of an inherited one. A new case installs a module whose gson version comes from a flattened intermediate parent. That case fails without this fix with the errors above, and passes with it. Found while moving this repo to rc-7. The repo itself stays on rc-5 until a release includes this fix, because the repo's batch builds use the published `@nx/maven` adapter. ## Related Issue(s) N/A <!-- polygraph-session-start --> --- <p><a href="https://app.trypolygraph.com/orgs/6a061dcb561c062131116eca/sessions/Maven-batch-runner-prefer-project-wrapper-over-MAVEN_HOME-e316f1de">View Polygraph session ↗</a></p> <!-- polygraph-session-end -->
Summary
ConsumerPomArtifactTransformer.injectTransformedArtifacts()idempotent by checking if a consumer POM artifact (classifier=consumer, type=pom) is already attached before callingaddAttachedArtifact()artifact '...:pom:consumer:...' already attached, replacing previous instancewarning when running multiple task segments (e.g.mvn package compiler:help)Fixes #12774
Test plan
injectTransformedArtifactsTwiceShouldNotDuplicateverifies idempotencymvn package compiler:helpno longer produces the warning🤖 Generated with Claude Code