gnodet commented on PR #12430: URL: https://github.com/apache/maven/pull/12430#issuecomment-4912241223
## In-Depth PR Review I've reviewed all assertions in this PR about Maven 4 behavior, checked upstream sources, and verified edge cases. Here's a detailed analysis. --- ### 🔴 Critical: gmavenplus-plugin claim is factually incorrect The `KNOWN_INCOMPATIBLE_PLUGINS` entry states: > "Even the latest version (4.1.1) fails with UnsupportedOperationException on goals like removeStubs." **This is wrong.** The latest version is **5.1.0** (released 2026-07-02), and the Maven 4 fix was merged in **4.2.0** — via [groovy/GMavenPlus#328](https://github.com/groovy/GMavenPlus/pull/328), authored by @gnodet. The 4.2.1 release notes explicitly say "Support Maven 4 (#328) Thanks to @gnodet for this PR!" Additionally, the Maven-side issue [MNG-8657](https://issues.apache.org/jira/browse/MNG-8657) was also fixed (resolved 2025-05-27) via [MNG-8662](https://issues.apache.org/jira/browse/MNG-8662) which added the missing `removeTestCompileSourceRoot` method. A follow-up issue [#341](https://github.com/groovy/GMavenPlus/issues/341) about `iterator.remove()` deprecation warnings was also fixed and closed (2026-06-24). **Recommendation:** Convert `gmavenplus-plugin` from a "permanently incompatible" warning to a `PluginUpgrade` with minimum version `4.2.0` (or `5.0.0` if you want to skip the deprecation warning era). The current code misleads users into thinking there's no fix when one exists — and was contributed by the PR author. --- ### 🟡 Questionable: "3.31.x ceiling" claim is unsubstantiated The Javadoc on `emitVersionGapWarning` states: > "The plugin can be bumped ahead of the platform up to 3.31.x; versions >= 3.32.0 are binary-incompatible with older platform/runtime libraries." And the warning message tells users: > "the plugin cannot be upgraded beyond 3.31.x without a corresponding platform upgrade" I could not verify this claim: 1. **No `SerializedApplication` changes between 3.31 and 3.32** — `gh api repos/quarkusio/quarkus/compare/3.31...3.32` shows no changes to `SerializedApplication.java`. 2. **No upstream issue or discussion** documents this boundary — I searched GitHub issues, migration guides (3.32, 3.33), and mailing lists. 3. **PR #52224** (aot-jar packaging, milestoned 3.32) does refactor `SerializedApplication` but explicitly states "This doesn't change the format, it just reorganizes the code." 4. **The Quarkus platform does enforce version alignment** between plugin and BOM, but that's a general policy, not a binary incompatibility at a specific version boundary. **Recommendation:** Either provide evidence for the 3.31.x ceiling or soften the warning to something like: "Consider upgrading the platform to match — mismatched plugin and platform versions may cause unexpected behavior." A false hard boundary can lead users to avoid valid upgrades. --- ### 🟡 Wrong upstream reference in PR description The PR body references `quarkusio/quarkus#46889` — that's a [WebAuthn TLS issue](https://github.com/quarkusio/quarkus/issues/46889), not the Maven 4 fix. The correct reference is: - **Issue:** [quarkusio/quarkus#37627](https://github.com/quarkusio/quarkus/issues/37627) — "Quarkus fails to build with Maven 4.0.0-beta-5" - **Fix PR:** [quarkusio/quarkus#48248](https://github.com/quarkusio/quarkus/pull/48248) - **Milestone:** 3.26.0.CR1 The minimum version 3.26.0 is correct. --- ### 🟢 Verified: Quarkus 3.26.0 minimum version Confirmed via upstream: [quarkusio/quarkus#37627](https://github.com/quarkusio/quarkus/issues/37627) was fixed by [PR #48248](https://github.com/quarkusio/quarkus/pull/48248), milestoned 3.26.0.CR1. The fix addresses Aether API changes in Maven 4 where `quarkus-maven-plugin` called methods on the old resolver API. --- ### 🟢 Verified: CLI retry loop (CommonsCliUpgradeOptions) The retry-loop approach for handling unrecognized options from `.mvn/maven.config` is sound. The trailing argument removal (fixed in 7444c4d) correctly handles options like `-T 4`. **Minor edge case to be aware of:** Boolean flags (e.g., `-B`) followed by positional arguments (goal names) would cause the next token to be incorrectly removed if it doesn't start with `-`. Example: `["-B", "check"]` → removes `-B`, then also removes `check` because it doesn't start with `-`. In practice this is unlikely since `.mvn/maven.config` options don't interleave with goal names, but it's worth noting. A `Set<String>` of known Maven boolean flags could make the heuristic more robust if this ever surfaces. --- ### 🟢 Verified: BOM property decoupling logic The Quarkus BOM decoupling logic is correct: - `isPropertyUsedByQuarkusBom()` correctly identifies shared properties by checking dependencyManagement for `io.quarkus`/`io.quarkus.platform` BOMs with type=pom, scope=import - `decoupleQuarkusPluginVersion()` introduces `quarkus-plugin.version` without touching the BOM property - The inherited property guard (returning false when `currentVersion == null`, fixed in 7444c4d) prevents downgrading inherited versions --- ### 🟢 Verified: Warning checks - **Property-interpolated module paths:** Correctly identifies `${...}` expressions in `<module>` and `<subproject>` elements, including profile-level modules. This is a real Maven 4 issue (module paths are validated before interpolation). - **CI-friendly missing dependency versions:** Correctly targets the pattern where a parent POM uses `${revision}` and child dependencies rely on inherited `dependencyManagement`. Note: only checks root-level `<dependencies>`, not dependencies within `<profiles>` — a minor gap that's unlikely to matter in practice. --- ### Summary | Check | Status | |---|---| | Quarkus 3.26.0 minimum version | ✅ Correct | | CLI retry loop robustness | ✅ Sound (minor edge case noted) | | BOM property decoupling | ✅ Correct | | Property-interpolated module path warning | ✅ Correct | | CI-friendly version warning | ✅ Correct (minor gap in profiles) | | gmavenplus-plugin "incompatible" claim | ❌ **Wrong** — fixed since 4.2.0 | | 3.31.x ceiling claim | ⚠️ **Unsubstantiated** — no evidence found | | Quarkus PR reference in description | ⚠️ **Wrong issue number** (#46889 → #37627) | -- 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]
