gnodet-bot commented on code in PR #13249: URL: https://github.com/apache/maven/pull/13249#discussion_r4084065532
########## 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: ⚠️ **[3rd occurrence — still unaddressed]** The description says *"The first non-flag argument on the command line is matched against alias names"* but `MavenParser.expandAlias()` expands **every** argument that matches — not just the first one. The `multipleAliasesAllExpanded` test confirms this: `[ci, rel]` expands both. This text ends up in the generated XSD and Javadoc — it will mislead users who define multiple aliases and expect them all to expand in a single invocation. ```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. Expansion is 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: ⚠️ **[3rd occurrence — still unaddressed]** Same incorrect description on the `Alias` class: *"When the first non-flag argument matches the alias name, it is replaced by the alias expansion."* ```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: ⚠️ **[3rd occurrence — still unaddressed]** `e.getCause()` on a `ParseException` returns `null` — `ParseException` has no wrapped cause. The `IllegalArgumentException` is therefore constructed with a `null` cause, silently dropping the original exception from the stack trace. The same pattern appears at lines 160, 171, and 206 (pre-existing), but this is new code added by this PR. If the intent is to match the pre-existing style, add a comment explaining why `e.getCause()` is preferred over `e`. If it's unintentional: ```suggestion "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. + * + * <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); Review Comment: ⚠️ **[2nd occurrence — still unaddressed]** The Javadoc example calls `LifecycleProcessor.withInjectedPhase(...)`, a static helper that does not exist on this interface (or anywhere in the API). Anyone copying this example gets a compile error. ```suggestion * return new PhaseEnrichedLifecycle(lifecycle, * List.of(new PhaseEnrichedLifecycle.InjectedPhase("docker-push", null, "deploy", null))); ``` ########## 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: ⚠️ **[2nd occurrence — still unaddressed]** *"Processors are called once per lifecycle per session"* is factually incorrect. `DefaultLifecycles.getPhaseToLifecycleMap()` contains an explicit comment: *"Lifecycles cannot be cached as extensions might add custom lifecycles later in the execution."* The processor chain is invoked on every call to `getPhaseToLifecycleMap()`, which happens per mojo execution in `DefaultLifecycleExecutionPlanCalculator` — not once per session. ```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. ``` -- 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]
