[MNG-8287] Dependencies should always use consumer pom - #12744
[MNG-8287] Dependencies should always use consumer pom#12744Hiteshsai007 wants to merge 4 commits into
Conversation
When generating a consumer POM, profiles activated by packaging that match the current project's packaging should have their content inlined into the consumer POM and the activation should be removed. This ensures consistent dependency resolution across different tools.
gnodet
left a comment
There was a problem hiding this comment.
Thanks for working on this, @Hiteshsai007! The overall direction of inlining packaging-activated profiles in the consumer POM is architecturally sound — BUILD_CONSUMER does not activate profiles (isBuildRequestWithActivation returns false), so explicit handling is needed. A few issues to address before this can be merged:
🔴 High: Test assertion is incorrect (likely never executed)
In ConsumerPomBuilderTest (line ~325), the assertion:
assertEquals(1, transformed.getDependencies().size())should be assertEquals(2, ...). The profileMixed has packaging="jar" which matches the model's packaging, so dep2 is also inlined alongside dep1 from profileMatching. Both are added to additionalDeps, yielding size 2.
🔴 High: Mixed-activation profiles (packaging + other conditions) handled incorrectly
When a profile has packaging=jar AND jdk=11 (AND semantics in Maven 4), the current code:
- Inlines all dependencies into
model.dependenciesunconditionally - Keeps the profile with the remaining
jdk=11activation, still containing those same dependencies
This breaks AND semantics — the dependencies become unconditional regardless of JDK version. When jdk=11 is active, they appear twice (once from inlining, once from profile activation).
Fix: For mixed-activation profiles, do NOT inline the content. Only strip the <packaging> activation condition; the remaining conditions should still gate the profile's content. Inlining should only happen when packaging is the sole activation condition.
🟡 Medium: Inlined dependencies bypass scope filtering
buildNonPom calls buildEffectiveModel (which filters out test/provided/system-scoped dependencies), then calls transformNonPom → inlinePackagingActivatedProfiles. Dependencies moved from profiles to model.dependencies at this point skip the scope filter — a test-scoped dependency in a packaging-activated profile would leak into the consumer POM.
🟡 Medium: Non-matching profiles should be dropped
Profiles with packaging activation for a different type (e.g., packaging=war in a jar project) are kept as-is in the consumer POM. These can never activate (consumer POM has a fixed packaging) and their <packaging> activation element is a 4.1.0 feature that prevents model version downgrade to 4.0.0 — the very problem this PR aims to solve. These dead-code profiles should be removed.
Suggestions:
- For mixed-activation profiles: strip only the
<packaging>condition, keep content gated by remaining conditions, don't inline - For non-matching profiles: drop them entirely from the consumer POM
- Add scope filtering for inlined dependencies in the non-POM path
- Add integration tests using actual POM files in
src/test/resources/consumer/(following the pattern oftestTrivialConsumer)
The architectural insight is correct — just needs refinement in the implementation details. Looking forward to the next iteration! 🙂
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of gnodet
- Inline packaging-activated profiles during transformPom for non-flattened builds - Drop profiles activated by non-matching packaging entirely - Update ConsumerPomBuilderTest to enable flattening and assert on dropped profiles Signed-off-by: Hitesh <hiteshsaibv.24cs@saividya.ac.in>
Addressed Review Feedback & FixesThanks for the feedback! I have pushed a new commit to address the review comments and resolve the integration test issues. Here is a summary of the changes: 1. Filter Out Non-Transitive Scope DependenciesAs requested, the inline logic now intentionally filters out dependencies with non-transitive scopes (like 2. Handle Non-Flattened Consumer POM GenerationPreviously, when the
3. Drop Profiles with Non-Matching PackagingThe logic was refined to explicitly drop profiles that have packaging activation criteria that do not match the project's current packaging. Since a consumer resolving the artifact will always see its fixed packaging (e.g., 4. Expanded Test CoverageI've updated
|
gnodet
left a comment
There was a problem hiding this comment.
Thanks for the thorough update, @Hiteshsai007! Great progress — all four previous findings are properly addressed:
✅ Mixed-activation profiles now correctly strip only the packaging condition without inlining content
✅ Scope filtering applied to inlined dependencies (test-scoped deps filtered out)
✅ Non-matching profiles are now dropped
✅ Test assertion is correct
However, the second commit introduces a new issue:
🔴 Parent POM profiles silently dropped in transformPom
transformPom now unconditionally calls inlinePackagingActivatedProfiles(model, model.getPackaging()). For a parent POM (packaging=pom), a profile with <activation><packaging>jar</packaging></activation> does not match "pom", so it enters the non-matching branch and is silently dropped.
These profiles are designed to activate in child projects that have packaging=jar. By dropping them from the consumer POM, child projects consuming this parent from a repository will never see the profile-based dependencies.
This is a behavioral regression: before this PR, transformPom did not touch profiles at all, so packaging-activated profiles were preserved as-is in the consumer POM.
Fix: Either skip calling inlinePackagingActivatedProfiles in transformPom (which was the correct behavior before — our original review explicitly identified this omission as a false positive), or handle the parent-POM case differently by preserving non-matching profiles instead of dropping them.
🟡 Same issue in transformBom
transformBom has the same pattern. BOMs always have packaging=pom, so child-targeted profiles (e.g., <packaging>jar</packaging>) are dropped. The practical impact is lower since BOMs consumed via <scope>import</scope> only import dependency management, not profiles — but the code bug is still present.
🟡 Missing test coverage for parent POM path
The integration test (testPackagingActivatedProfiles) uses packaging=jar, which routes through buildNonPom → transformNonPom. No test exercises the parent POM scenario where packaging=pom is passed to inlinePackagingActivatedProfiles, which is the scenario where the regression manifests.
Suggestion: The simplest fix would be to guard the call in transformPom:
if (!POM_PACKAGING.equals(packaging)) {
model = inlinePackagingActivatedProfiles(model, packaging);
}And similarly in transformBom, or remove the call there entirely.
Getting close! 🙂
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of gnodet
|
Thanks for catching the parent-POM/BOM regression. Addressed in 6a10a22. Packaging-profile resolution now runs only for non-POM consumer artifacts. Added a parent-POM regression fixture that verifies the Verified with: |
gnodet
left a comment
There was a problem hiding this comment.
All previous findings addressed — nice work across three iterations, @Hiteshsai007! 🎉
Round 2 fixes confirmed:
✅ transformPom and transformBom no longer call inlinePackagingActivatedProfiles — parent POM profiles targeting child packagings are preserved as intended
✅ New testParentPomPackagingActivatedProfilesArePreserved test validates the regression scenario
✅ testPackagingActivatedProfiles refactored to test through transformNonPom directly with the correct BUILD_CONSUMER request type
Minor observations (non-blocking):
- The method-level Javadoc for
inlinePackagingActivatedProfilescould document that non-matching profiles are dropped and that mixed-activation profiles have content preserved (not inlined). The inline comments at lines ~492 and ~504 do explain this, so it's a completeness suggestion only. - A parallel test for the
transformBompath (similar to the parent POM test) would strengthen confidence, though the fix was a simple call removal so the risk is minimal.
The approach is clean and correct: packaging profile resolution runs only in transformNonPom where the packaging type is fixed, while parent POMs and BOMs preserve profiles as-is for child consumption. The stripPackagingActivation helper is thorough, and scope filtering properly prevents non-transitive dependencies from leaking into consumer POMs.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of gnodet
gnodet
left a comment
There was a problem hiding this comment.
APPROVE — Well-implemented consumer POM packaging profile resolution.
Three review iterations with the maintainer have addressed all major concerns. The code correctly inlines matching packaging-activated profiles, drops non-matching ones, and preserves mixed-activation profiles with only the packaging condition stripped.
The approach is architecturally sound — only transformNonPom calls the new logic, while transformPom and transformBom are untouched, preserving parent POM profiles for child consumption.
Minor observations (non-blocking):
-
Duplicate dependency guard (low): Inlined dependencies are appended without deduplication at line 505. If a packaging-only profile re-declares a dependency already in the model, the consumer POM would contain duplicates. A defensive key-based check could prevent surprise.
-
Test coverage for DM/repos paths (low): The unit test
testInlinePackagingActivatedProfilescovers dependency inlining and profile stripping but does not exercise thedependencyManagementorrepositoriesinlining paths separately. -
Javadoc completeness (low): The
inlinePackagingActivatedProfilesJavadoc documents inlining and mixed-activation behavior but does not mention that non-matching profiles are dropped entirely.
This review was generated by an AI agent (Claude Code) and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
- Add key-based deduplication when merging inlined dependencies, managed dependencies, and repositories from packaging-activated profiles to prevent duplicates in the consumer POM - Add unit tests for dependency management and repository inlining paths in inlinePackagingActivatedProfiles - Add unit test for duplicate dependency guard (model deps take precedence over profile duplicates) - Widen transformBom visibility to package-private to match transformNonPom and transformPom Signed-off-by: Hitesh <hiteshsaibv.24cs@saividya.ac.in>
|
Thanks for the detailed review and approval, @gnodet! I've addressed the three non-blocking observations in the latest commit: 1. Duplicate dependency guard ✅Replaced the naive
2. Test coverage for DM/repos paths ✅Added three new unit tests to exercise the previously untested merge paths:
3. Javadoc completeness ✅This was already documented in the existing Javadoc — the method-level doc at lines 475–477 explicitly states: "Profiles with a non-matching packaging activation are dropped entirely, since they can never activate for this artifact's fixed packaging and their presence would block model version downgrade to 4.0.0." |
Resolves #10535 (MNG-8287)
Maven 4 introduces support for activating profiles based on the project's
<packaging>type. However, when a dependency built with Maven 4 is consumed by tools such as Maven 3, Gradle, or IDEs that do not understand the 4.1.0+<packaging>activation condition, dependency resolution can differ between tools.This change updates consumer POM generation to resolve packaging-based profile activation before the consumer POM is published, ensuring that downstream consumers see a consistent and predictable set of dependencies.
How this solves the problem
The consumer POM generation process in
DefaultConsumerPomBuildernow explicitly handles profiles activated by<packaging>:Inline matching profiles
When a profile is activated by
<packaging>and the packaging value matches the current project's packaging, the profile's relevant contents are merged directly into the main consumer POM, including:Remove resolved packaging activation
After inlining a matching profile:
<packaging>is the only activation condition, the profile is completely removed from the consumer POM.<packaging>and<jdk>), the profile remains in the consumer POM with its other activation conditions intact, while the already-resolved<packaging>condition is removed.Preserve non-matching profiles
Profiles activated by a packaging type that does not match the current project's packaging are left unchanged in the consumer POM.
Why this matters
Packaging-based profile activation is understood by Maven 4, but older consumers and other build tools may not understand the
<packaging>activation element introduced with model version 4.1.0.By resolving matching packaging-based profiles during consumer POM generation, the published consumer POM contains the effective dependency information needed by downstream tools. This ensures that consumers using older Maven versions, Gradle, or IDE tooling see the same effective dependencies without having to understand or resolve Maven 4's packaging-based activation themselves.
Testing
Added comprehensive unit tests in
ConsumerPomBuilderTestcovering:<packaging>are removed after resolution.<packaging>.Checklist
mvn verifyto make sure basic checks pass.To make clear that you license your contribution under the Apache License Version 2.0, January 2004, you have to acknowledge this by using the following checkbox.