gnodet-bot commented on code in PR #13249: URL: https://github.com/apache/maven/pull/13249#discussion_r4083969012
########## api/maven-api-core/src/main/mdo/reactor.mdo: ########## @@ -0,0 +1,249 @@ +<?xml version="1.0" encoding="UTF-8"?> + +<!-- + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, + software distributed under the License is distributed on an + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + KIND, either express or implied. See the License for the + specific language governing permissions and limitations + under the License. + +--> + +<model xmlns="http://codehaus-plexus.github.io/MODELLO/2.0.0"; xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; + xsi:schemaLocation="http://codehaus-plexus.github.io/MODELLO/2.0.0 https://codehaus-plexus.github.io/modello/xsd/modello-2.0.0.xsd"; + xml.namespace="http://maven.apache.org/REACTOR/${version}"; + xml.schemaLocation="https://maven.apache.org/xsd/reactor-${version}.xsd";> + + <id>reactor</id> + <name>ReactorConfig</name> + <description><![CDATA[ + <p>This is a reference for the Reactor Configuration descriptor.</p> + <p>The default location for the Reactor Configuration descriptor file is <code>${maven.projectBasedir}/.mvn/reactor.xml</code></p> + <p>It consolidates reactor-scoped build configuration previously split across <code>.mvn/maven.config</code> + (CLI flag defaults) and <code>.mvn/extensions.xml</code> (core extensions), and adds support for + named build aliases and reactor-level lifecycle phase injection.</p> + ]]></description> + + <defaults> + <default> + <key>package</key> + <value>org.apache.maven.api.reactor</value> + </default> + </defaults> + + <classes> + <class rootElement="true" xml.tagName="reactor" xsd.compositor="sequence"> + <name>ReactorConfig</name> + <description>Reactor-scoped build configuration for a Maven project.</description> + <version>1.0.0+</version> + <fields> + <field> + <name>options</name> + <description>CLI options to prepend to every invocation. Replaces .mvn/maven.config when present. + Expressed as whitespace-separated, quote-aware inline text (e.g. {@code -T4 --no-transfer-progress}). + For options with spaces in their values, use the structured form via {@link #getOptionArgs()}.</description> + <version>1.0.0+</version> + <required>false</required> + <type>String</type> + </field> + <field> + <name>optionArgs</name> + <description>Structured CLI options — each <arg> element is one option token. + Use instead of {@link #getOptions()} when option values contain spaces. + Mutually exclusive with options.</description> + <version>1.0.0+</version> + <required>false</required> + <association xml.itemsStyle="flat" xml.tagName="arg"> + <type>String</type> + <multiplicity>*</multiplicity> + </association> + </field> + <field> + <name>extensions</name> + <description>Core extensions to load. Replaces .mvn/extensions.xml when present.</description> + <version>1.0.0+</version> + <required>false</required> + <association xml.itemsStyle="flat" xml.tagName="extension"> + <type>ReactorExtension</type> + <multiplicity>*</multiplicity> + </association> + </field> + <field> + <name>aliases</name> + <description>Named build aliases. The first non-flag argument on the command line is matched against + alias names; if found it is replaced by the alias expansion.</description> Review Comment: ⚠️ **[Previous finding — NOT addressed]** Still says "first non-flag argument" but `MavenParser.expandAlias()` expands **every** argument that matches, not just the first one. This description ends up in the generated XSD and Javadoc and will confuse users who define multiple aliases (e.g. `mvn ci rel` where both are aliases — both expand). ```suggestion <description>Named build aliases. Every argument on the command line is checked against alias names in order; each match is replaced in-place by the alias expansion tokens. Not recursive.</description> ``` ########## api/maven-api-core/src/main/mdo/reactor.mdo: ########## @@ -0,0 +1,249 @@ +<?xml version="1.0" encoding="UTF-8"?> + +<!-- + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, + software distributed under the License is distributed on an + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + KIND, either express or implied. See the License for the + specific language governing permissions and limitations + under the License. + +--> + +<model xmlns="http://codehaus-plexus.github.io/MODELLO/2.0.0"; xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; + xsi:schemaLocation="http://codehaus-plexus.github.io/MODELLO/2.0.0 https://codehaus-plexus.github.io/modello/xsd/modello-2.0.0.xsd"; + xml.namespace="http://maven.apache.org/REACTOR/${version}"; + xml.schemaLocation="https://maven.apache.org/xsd/reactor-${version}.xsd";> + + <id>reactor</id> + <name>ReactorConfig</name> + <description><![CDATA[ + <p>This is a reference for the Reactor Configuration descriptor.</p> + <p>The default location for the Reactor Configuration descriptor file is <code>${maven.projectBasedir}/.mvn/reactor.xml</code></p> + <p>It consolidates reactor-scoped build configuration previously split across <code>.mvn/maven.config</code> + (CLI flag defaults) and <code>.mvn/extensions.xml</code> (core extensions), and adds support for + named build aliases and reactor-level lifecycle phase injection.</p> + ]]></description> + + <defaults> + <default> + <key>package</key> + <value>org.apache.maven.api.reactor</value> + </default> + </defaults> + + <classes> + <class rootElement="true" xml.tagName="reactor" xsd.compositor="sequence"> + <name>ReactorConfig</name> + <description>Reactor-scoped build configuration for a Maven project.</description> + <version>1.0.0+</version> + <fields> + <field> + <name>options</name> + <description>CLI options to prepend to every invocation. Replaces .mvn/maven.config when present. + Expressed as whitespace-separated, quote-aware inline text (e.g. {@code -T4 --no-transfer-progress}). + For options with spaces in their values, use the structured form via {@link #getOptionArgs()}.</description> + <version>1.0.0+</version> + <required>false</required> + <type>String</type> + </field> + <field> + <name>optionArgs</name> + <description>Structured CLI options — each <arg> element is one option token. + Use instead of {@link #getOptions()} when option values contain spaces. + Mutually exclusive with options.</description> + <version>1.0.0+</version> + <required>false</required> + <association xml.itemsStyle="flat" xml.tagName="arg"> + <type>String</type> + <multiplicity>*</multiplicity> + </association> + </field> + <field> + <name>extensions</name> + <description>Core extensions to load. Replaces .mvn/extensions.xml when present.</description> + <version>1.0.0+</version> + <required>false</required> + <association xml.itemsStyle="flat" xml.tagName="extension"> + <type>ReactorExtension</type> + <multiplicity>*</multiplicity> + </association> + </field> + <field> + <name>aliases</name> + <description>Named build aliases. The first non-flag argument on the command line is matched against + alias names; if found it is replaced by the alias expansion.</description> + <version>1.0.0+</version> + <required>false</required> + <association xml.itemsStyle="flat" xml.tagName="alias"> + <type>Alias</type> + <multiplicity>*</multiplicity> + </association> + </field> + <field> + <name>phases</name> + <description>Custom lifecycle phase injections. Phases are registered in the lifecycle DAG + with the declared after/before edges.</description> + <version>1.0.0+</version> + <required>false</required> + <association xml.itemsStyle="flat" xml.tagName="phase"> + <type>PhaseInjection</type> + <multiplicity>*</multiplicity> + </association> + </field> + </fields> + </class> + + <class xml.tagName="extension"> + <name>ReactorExtension</name> + <description>Describes a core extension to load.</description> + <version>1.0.0+</version> + <fields> + <field> + <name>groupId</name> + <description>The group ID of the extension's artifact.</description> + <version>1.0.0+</version> + <required>true</required> + <type>String</type> + </field> + <field> + <name>artifactId</name> + <description>The artifact ID of the extension.</description> + <version>1.0.0+</version> + <required>true</required> + <type>String</type> + </field> + <field> + <name>version</name> + <description>The version of the extension.</description> + <version>1.0.0+</version> + <required>true</required> + <type>String</type> + </field> + <field> + <name>classLoadingStrategy</name> + <description>The class loading strategy: 'self-first' (the default), 'parent-first' (loads classes from the + parent, then from the extension) or 'plugin' (follows the rules from extensions defined as plugins).</description> + <version>1.0.0+</version> + <defaultValue>self-first</defaultValue> + <required>false</required> + <type>String</type> + </field> + <field> + <name>configuration</name> + <description>Extension-specific configuration passed to the extension during initialization. + The structure is extension-defined.</description> + <version>1.0.0+</version> + <required>false</required> + <type>DOM</type> + </field> + </fields> + <codeSegments> + <codeSegment> + <version>1.0.0+</version> + <code> + <![CDATA[ + /** + * Gets the identifier of the extension. + * + * @return The extension id in the form {@code <groupId>:<artifactId>:<version>}, never {@code null}. + */ + public String getId() { + return (getGroupId() == null ? "[unknown-group-id]" : getGroupId()) + + ":" + (getArtifactId() == null ? "[unknown-artifact-id]" : getArtifactId()) + + ":" + (getVersion() == null ? "[unknown-version]" : getVersion()); + } + ]]> + </code> + </codeSegment> + </codeSegments> + </class> + + <class xml.tagName="alias"> + <name>Alias</name> + <description>A named build alias. When the first non-flag argument matches the alias name, + it is replaced by the alias expansion. Supports both inline text (shorthand, whitespace-separated) Review Comment: ⚠️ **[Previous finding — NOT addressed]** Same "first non-flag argument" description on the `Alias` class itself. The `multipleAliasesAllExpanded` test demonstrates the real behaviour: every matching argument is expanded. ```suggestion <description>A named build alias. Every argument on the command line is checked against alias names; when a match is found it is replaced in-place by the alias expansion tokens. Supports both inline text (shorthand, whitespace-separated) and structured <arg> elements for values that contain spaces.</description> ``` ########## impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvn/MavenParser.java: ########## @@ -76,6 +174,21 @@ protected MavenOptions parseMavenAtFileOptions(Path atFile) { } } + protected MavenOptions parseMavenReactorOptions(List<String> args) { + try { + MavenOptions options = parseArgs("reactor.xml", args); + if (options.goals().isPresent()) { + // <options> can only contain options, not goals/phases + throw new IllegalArgumentException("Unrecognized entries in reactor.xml <options>: " + + options.goals().get()); + } + return options; + } catch (ParseException e) { + throw new IllegalArgumentException( + "Failed to parse arguments from reactor.xml <options>: " + e.getMessage(), e.getCause()); Review Comment: ⚠️ **[Previous finding — NOT addressed]** `e.getCause()` returns `null` for `ParseException` (it has no cause), so the `IllegalArgumentException` is constructed with a `null` cause. The original exception is silently dropped — only its message survives in the stack trace. If intentional (matching the pre-existing pattern in `parseMavenCliOptions`/`parseMavenAtFileOptions`), add a comment explaining why `e.getCause()` is preferred over `e`. If not intentional: ```suggestion throw new IllegalArgumentException( "Failed to parse arguments from reactor.xml <options>: " + e.getMessage(), e); ``` ########## api/maven-api-spi/src/main/java/org/apache/maven/api/spi/LifecycleProcessor.java: ########## @@ -0,0 +1,66 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.maven.api.spi; + +import org.apache.maven.api.Lifecycle; +import org.apache.maven.api.annotations.Consumer; +import org.apache.maven.api.annotations.Experimental; +import org.apache.maven.api.annotations.Nonnull; +import org.apache.maven.api.di.Named; + +/** + * SPI for transforming Maven lifecycles at session initialisation time. + * + * <p>A {@code LifecycleProcessor} receives each built-in (or extension-contributed) lifecycle + * and may return a modified version of it — for example to inject additional phases into the + * lifecycle DAG. Multiple processors are applied in order; each one receives the output of the + * previous one. + * + * <p>Processors are called once per lifecycle per session, after the base lifecycles have been + * assembled by the {@link LifecycleProvider}s, so they have access to full session context. + * Review Comment: ⚠️ **Inaccurate Javadoc — processors are NOT called once per session.** `DefaultLifecycles.getPhaseToLifecycleMap()` explicitly states: *"Lifecycles cannot be cached as extensions might add custom lifecycles later in the execution."* Every call to `defaultLifecycles.get(phase)` (which happens per mojo execution in `DefaultLifecycleExecutionPlanCalculator`) rebuilds the map and re-runs the full processor chain. Processors are called once **per `getPhaseToLifecycleMap()` invocation**, not once per session. This is important because `ReactorXmlLifecycleProcessor.process()` calls `legacySupport.getSession()` on every invocation — understanding the actual call frequency matters for performance reasoning. ```suggestion * <p>Processors are called each time the lifecycle phase map is rebuilt (potentially multiple * times per build). Implementations must be stateless and idempotent — the same lifecycle * input must always produce the same output. They receive the output of the previous processor * in the chain. ``` ########## api/maven-api-spi/src/main/java/org/apache/maven/api/spi/LifecycleProcessor.java: ########## @@ -0,0 +1,66 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.maven.api.spi; + +import org.apache.maven.api.Lifecycle; +import org.apache.maven.api.annotations.Consumer; +import org.apache.maven.api.annotations.Experimental; +import org.apache.maven.api.annotations.Nonnull; +import org.apache.maven.api.di.Named; + +/** + * SPI for transforming Maven lifecycles at session initialisation time. + * + * <p>A {@code LifecycleProcessor} receives each built-in (or extension-contributed) lifecycle + * and may return a modified version of it — for example to inject additional phases into the + * lifecycle DAG. Multiple processors are applied in order; each one receives the output of the + * previous one. + * + * <p>Processors are called once per lifecycle per session, after the base lifecycles have been + * assembled by the {@link LifecycleProvider}s, so they have access to full session context. + * + * <p>Example — injecting a {@code docker-push} phase after {@code deploy}: + * <pre>{@code + * @Named + * public class DockerLifecycleProcessor implements LifecycleProcessor { + * public Lifecycle process(Lifecycle lifecycle) { + * if (!Lifecycle.DEFAULT.equals(lifecycle.id())) return lifecycle; + * return LifecycleProcessor.withInjectedPhase(lifecycle, + * "docker-push", "deploy", null); + * } + * } + * }</pre> Review Comment: ⚠️ **Javadoc example references `LifecycleProcessor.withInjectedPhase()` which does not exist.** This static helper method is not declared on the `LifecycleProcessor` interface (or anywhere in the API). Anyone copying this example will get a compile error. Replace with an accurate example using the actual `PhaseEnrichedLifecycle` constructor, or remove the helper call if it was planned but not yet added. ```suggestion * <p>Example — injecting a {@code docker-push} phase after {@code deploy}: * <pre>{@code * @Named * public class DockerLifecycleProcessor implements LifecycleProcessor { * public Lifecycle process(Lifecycle lifecycle) { * if (!Lifecycle.DEFAULT.equals(lifecycle.id())) return lifecycle; * return new PhaseEnrichedLifecycle(lifecycle, * List.of(new PhaseEnrichedLifecycle.InjectedPhase("docker-push", null, "deploy", null))); * } * } * }</pre> ``` -- 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]
