gnodet-bot commented on code in PR #13249:
URL: https://github.com/apache/maven/pull/13249#discussion_r4291479979
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/BaseParser.java:
##########
@@ -499,11 +538,31 @@ protected List<CoreExtensions>
readCoreExtensionsDescriptor(LocalContext context
Map<String, String> eff = new HashMap<>(context.systemProperties);
eff.putAll(context.userProperties);
- // project
- file =
context.cwd.resolve(eff.get(Constants.MAVEN_PROJECT_EXTENSIONS));
- loaded = readCoreExtensionsDescriptorFromFile(file, false);
- if (!loaded.isEmpty()) {
- result.add(new CoreExtensions(file, loaded));
+ // project extensions: use reactor.xml <extensions> when present,
otherwise extensions.xml
+ if (context.reactorConfig != null
+ && context.reactorConfig.getExtensions() != null
+ && !context.reactorConfig.getExtensions().isEmpty()) {
+ // reactor.xml is authoritative for project extensions
+ Path dir = context.rootDirectory != null ? context.rootDirectory :
context.topDirectory;
+ Path reactorXml = dir != null ? dir.resolve(".mvn/reactor.xml") :
null;
+ loaded = context.reactorConfig.getExtensions().stream()
+ .map(re ->
org.apache.maven.api.cli.extensions.CoreExtension.newBuilder()
+ .groupId(re.getGroupId())
+ .artifactId(re.getArtifactId())
+ .version(re.getVersion())
+ .classLoadingStrategy(re.getClassLoadingStrategy())
+ .build())
Review Comment:
⚠️ **`ReactorExtension.configuration` is silently dropped**
The `ReactorConfig` model defines a `<configuration>` field on
`ReactorExtension` (type `DOM`, for extension-specific initialization
configuration). The conversion to `CoreExtension` here omits it — extensions
declared in `reactor.xml` with a `<configuration>` block will have their
configuration silently discarded. The legacy `extensions.xml` path correctly
maps this via `CoreExtensionsStaxReader`.
```suggestion
.map(re ->
org.apache.maven.api.cli.extensions.CoreExtension.newBuilder()
.groupId(re.getGroupId())
.artifactId(re.getArtifactId())
.version(re.getVersion())
.classLoadingStrategy(re.getClassLoadingStrategy())
.configuration(re.getConfiguration())
.build())
```
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvn/MavenParser.java:
##########
@@ -47,14 +56,103 @@ protected Options parseCliOptions(LocalContext context) {
throw new IllegalArgumentException("Specified file does not
exists (" + file + ")");
}
}
- // maven.config; if exists
- Path mavenConfig = context.rootDirectory != null ?
context.rootDirectory.resolve(".mvn/maven.config") : null;
- if (mavenConfig != null && Files.isRegularFile(mavenConfig)) {
- result.add(parseMavenConfigOptions(mavenConfig));
+
+ // reactor.xml <options> — used instead of maven.config when
reactor.xml is present
+ if (context.reactorConfig != null) {
+ List<String> optionArgs =
resolveReactorOptions(context.reactorConfig);
+ if (!optionArgs.isEmpty()) {
+ result.add(parseMavenReactorOptions(optionArgs));
+ }
+ } else {
+ // legacy maven.config; if exists
+ Path mavenConfig =
+ context.rootDirectory != null ?
context.rootDirectory.resolve(".mvn/maven.config") : null;
+ if (mavenConfig != null && Files.isRegularFile(mavenConfig)) {
+ result.add(parseMavenConfigOptions(mavenConfig));
+ }
}
+
return LayeredMavenOptions.layerMavenOptions(result);
}
+ /**
+ * Resolves the {@code <options>} or {@code <optionArgs>} block of a
{@link ReactorConfig}
+ * to a flat list of argument strings.
+ *
+ * <p>When the config has structured {@code <arg>} children ({@code
optionArgs}), those are used as-is.
+ * When it has inline text content ({@code options}), it is tokenized with
{@link ArgumentTokenizer}.
+ */
+ private static List<String> resolveReactorOptions(ReactorConfig rc) {
+ List<String> optionArgs = rc.getOptionArgs();
+ if (optionArgs != null && !optionArgs.isEmpty()) {
+ return optionArgs;
+ }
+ String options = rc.getOptions();
+ if (options != null && !options.isBlank()) {
+ return ArgumentTokenizer.tokenize(options);
+ }
+ return List.of();
+ }
+
+ /**
+ * Expands alias arguments in {@code args} using aliases defined in {@code
reactorConfig}.
+ *
+ * <p>Alias expansion is a pure string manipulation — it happens before
any Maven DI or session setup.
+ * The argument list is rebuilt on the fly: each argument is checked
against the alias map in order;
+ * if it matches an alias name, it is replaced by the alias expansion
tokens; otherwise it is kept as-is.
+ * Expansion is not recursive — tokens produced by an alias are passed
through as-is.
+ *
+ * @param args original argument list
+ * @param reactorConfig parsed reactor config, or {@code null} if
reactor.xml is absent
+ * @return the (possibly modified) argument list
+ */
+ static List<String> expandAlias(List<String> args, ReactorConfig
reactorConfig) {
+ if (reactorConfig == null
+ || reactorConfig.getAliases() == null
+ || reactorConfig.getAliases().isEmpty()) {
+ return args;
+ }
+
+ // Build a name → expansion map for O(1) lookup per arg
+ Map<String, List<String>> aliasMap = new LinkedHashMap<>();
+ for (Alias alias : reactorConfig.getAliases()) {
+ String name = alias.getName();
+ if (name == null || name.isBlank()) {
+ throw new IllegalArgumentException("Alias name must not be
blank in reactor.xml");
+ }
+ aliasMap.put(name, resolveAlias(alias));
Review Comment:
💡 **Duplicate alias names are silently overwritten**
`aliasMap.put(name, resolveAlias(alias))` overwrites any existing entry
without warning. If a user accidentally declares two `<alias name="ci">` blocks
in `reactor.xml`, the second wins silently — no log, no error. Given that blank
names already throw (`line 121`), duplicate names should at minimum warn. A
throw is more consistent:
```suggestion
if (aliasMap.containsKey(name)) {
throw new IllegalArgumentException(
"Duplicate alias name '" + name + "' in
reactor.xml");
}
aliasMap.put(name, resolveAlias(alias));
```
--
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]
