gnodet commented on issue #13764: URL: https://github.com/apache/maven/issues/13764#issuecomment-6600670188
PR #13798 changes the picture enough that this issue's framing needs an update before anyone writes the guide. **What #13798 does** Instead of mapping `@Mojo(aggregator=true)` to `after:all`, it makes the concurrent builder auto-schedule aggregator goals as DAG steps *after* the children have completed the declared phase. This is not the same as binding to `after:all`: - It works without any plugin change — existing `aggregator=true` plugins get correct ordering in `-b concurrent` builds automatically - It suppresses inherited copies when an ancestor in the active reactor already provides the execution (no need for `<inherited>false</inherited>`) - But it only applies to the concurrent builder — singlethreaded and `-T N` are unchanged **What this means for the guide** The mapping table row `@Mojo(aggregator=true)` → `after:all` is still the right long-term target, but the guide should not frame it as "migrate or your plugin is broken". The correct framing after #13798: - With `-b concurrent`, `aggregator=true` plugins work correctly without migration - On singlethreaded/`-T N`, the lifecycle-bound aggregator-runs-per-module problem (MNG-6336) remains open regardless of #13798 - `after:all` + `@Resolution` remains the recommended API for new plugins: it's explicit, composable, and works correctly across all builders without special-casing in the planner **Should we still deprecate `aggregator=true`?** Yes, but the rationale shifts. The argument is no longer "it's broken, migrate" — it becomes: 1. Semantic ambiguity: the flag conflates ordering, resolution scope, and inheritance suppression in one boolean 2. Maintenance burden: correct ordering requires special-casing in the build planner; `after:all` is just a phase binding 3. Singlethreaded/`-T N` gap: MNG-6336 (lifecycle-bound aggregators run per-module) remains open and won't be fixed on the legacy builders 4. The `@Execute` replacement path (`@After`) is independent and still fully motivated The deprecation should land once: (a) #13798 is merged, (b) at least one major plugin (javadoc, site) has migrated to demonstrate the new API is practical, and (c) maven-plugin-tools generates `afterLinks` from `@After` annotations so migration doesn't require handcrafted descriptors. **Suggested update to the guide scope** Add a "builder compatibility matrix" section: | Problem | Singlethreaded | `-T N` | `-b concurrent` | |---|---|---|---| | Aggregator runs only at root | ✅ | ✅ | ✅ | | Aggregator runs after children finish phase | ✅ (sequential) | ✅ (`aggregatorLock`) | ✅ after #13798 | | Lifecycle-bound aggregator runs per-module | ⚠️ MNG-6336 | ⚠️ MNG-6336 | ✅ fixed | And step 0 in the incremental migration path: "Your existing plugin already behaves correctly in `-b concurrent` builds after Maven 4.x.y (MNG-XXXX). Migration to `after:all` is recommended for new plugins and for full correctness across all builders." -- 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]
