Skip to content

Fix effective-pom_properties IT for Maven 4 compatibility#371

Closed
ascheman wants to merge 1 commit into
apache:masterfrom
aschemaven:fix-effective-pom-properties-it-maven4
Closed

Fix effective-pom_properties IT for Maven 4 compatibility#371
ascheman wants to merge 1 commit into
apache:masterfrom
aschemaven:fix-effective-pom-properties-it-maven4

Conversation

@ascheman

@ascheman ascheman commented Jun 7, 2026

Copy link
Copy Markdown

Summary

The effective-pom_properties integration test is the single IT failing under Maven 4.0.0-rc-5 on master, which leaves PR #363 ("enable build with Maven 4") with a red CI matrix. This PR fixes the IT fixture so the rc-5 matrix turns green.

Root cause

verify.groovy was updated in #344 (merged 2025-10-31) to branch on mavenVersion.startsWith('4.') and assert that maven.compiler.source/target = 8 appear in the rendered effective POM — testing that user properties override pom properties on Maven 4.

The change updated only verify.groovy. The companion invoker.properties was never extended to actually pass -Dmaven.compiler.source=8 -Dmaven.compiler.target=8 (or the invoker.systemProperties.* form) into the invoked Maven build. So under Maven 4 the invocation didn't define those user properties at all, the rendered effective POM legitimately showed the pom-property values (1.6/1.6), and the new Maven-4 assertion fired unconditionally.

The bug is entirely in the IT fixture; the production MavenHelpPlugin is unaffected.

Fix

Three lines added to src/it/projects/effective-pom_properties/invoker.properties:

invoker.systemProperties.maven.compiler.source = 8
invoker.systemProperties.maven.compiler.target = 8

Verification

  • mvn -P run-its -Dinvoker.test='effective-pom_properties' verify against Maven 4.0.0-rc-5 (sdkman, JDK 17.0.18 Temurin / macOS aarch64): Passed: 1, Failed: 0, Errors: 0, Skipped: 0.
  • Without the fix the same invocation reproduces the failure that PR enable build with Maven 4 #363's CI shows.

Context

The effective-pom_properties IT's verify.groovy was updated in PR apache#344 (merged 2025-10-31) to assert that user properties override pom properties on Maven 4, expecting maven.compiler.source/target=8 in the rendered effective POM.

However the change updated only verify.groovy. The corresponding invoker.properties was never extended to actually pass the user properties into the invoked Maven build, so the assertion fired unconditionally on Maven 4 (the rendered effective POM legitimately showed the pom-property values 1.6/1.6).

Add invoker.systemProperties.maven.compiler.source/target=8 so the IT exercises the assertion path it was rewritten for. No production code change; only the IT fixture.

This unblocks PR apache#363 (enable Maven 4 CI matrix).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@@ -16,3 +16,9 @@
# under the License.

invoker.goals = ${project.groupId}:${project.artifactId}:${project.version}:effective-pom

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^M ... looks like problem with file encoding

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, no encoding, but Windows newline!

@hboutemy hboutemy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newlines on Windows: I don't know what config could be improved to avoid such stupid lost time in the future

@@ -16,3 +16,9 @@
# under the License.

invoker.goals = ${project.groupId}:${project.artifactId}:${project.version}:effective-pom

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, no encoding, but Windows newline!

@ascheman

Copy link
Copy Markdown
Author

Well, there is some inconsistency on this repository wrt to line endings of *.properties:

  • 30 are UNIX-Style (LF)
  • 2 are Windows-Style (CRLF) including the one I changed (consistently with CRLF)
    How to cope with this, @slawekjaranowski / @hboutemy:
  • Leave it consistently
  • Change everything to LF (or CRLF) in a second commit (just for the sake of uniqueness)?

@ascheman

Copy link
Copy Markdown
Author

...

How to cope with this, @slawekjaranowski / @hboutemy:

  • Leave it consistently
  • Change everything to LF (or CRLF) in a second commit (just for the sake of uniqueness)?

BTW: If you opt for the second option (normalization) we could also add a proper .gitattributes file to prevent such inconsistencies for the future. IMHO this would perhaps call for a second line of defense in the future by a spotless rule. But perhaps we currently we have more urgent stuff to do ...

@hboutemy

hboutemy commented Jun 14, 2026

Copy link
Copy Markdown
Member

notice: I want to drop this PR fully
what we need to revert is 2be3211#diff-7b81cfece99ab40b32e3d95307f758917927d80a77fc3ab06f6fd057ac7d5318 that should never have been merged in the first place

we'll see the newlines normalization in a separate PR

@ascheman

ascheman commented Jun 17, 2026

Copy link
Copy Markdown
Author

Superseded by 2c155b2 ("revert broken IT updates from #344") on master — thanks @hboutemy.

That commit takes exactly the approach this analysis pointed to: a plain revert of the verify.groovy change does not pass on Maven 4 because the rendered <properties> block is reordered (maven.compiler.target before maven.compiler.source); keeping the Maven-4 branch with the unchanged 1.6 values (the =8 expectation was the actual mistake, since the matching user properties were never injected) is what makes it green.

Verified locally green on 4.0.0-rc-5 and 3.9.16. Closing as obsolete.
#363 should go green once rebased on master.

@ascheman ascheman closed this Jun 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants