You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@maven.apache.org by mi...@apache.org on 2022/12/26 19:46:42 UTC

[maven] branch MNG-7651 created (now 3c0f98272)

This is an automated email from the ASF dual-hosted git repository.

michaelo pushed a change to branch MNG-7651
in repository https://gitbox.apache.org/repos/asf/maven.git


      at 3c0f98272 [MNG-7651] Simplify and document merge of maven.config file and CLI args

This branch includes the following new commits:

     new 3c0f98272 [MNG-7651] Simplify and document merge of maven.config file and CLI args

The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails.  The revisions
listed as "add" were already present in the repository and have only
been added to this reference.



[maven] 01/01: [MNG-7651] Simplify and document merge of maven.config file and CLI args

Posted by mi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.

michaelo pushed a commit to branch MNG-7651
in repository https://gitbox.apache.org/repos/asf/maven.git

commit 3c0f98272c5371f96ba154425a8e2858b608e60d
Author: Michael Osipov <mi...@apache.org>
AuthorDate: Mon Dec 26 18:29:11 2022 +0100

    [MNG-7651] Simplify and document merge of maven.config file and CLI args
    
    This closes #939
---
 .../main/java/org/apache/maven/cli/MavenCli.java   | 44 ++++++++++++----------
 .../java/org/apache/maven/cli/MavenCliTest.java    |  6 +--
 2 files changed, 27 insertions(+), 23 deletions(-)

diff --git a/maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java b/maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java
index 509b02ede..6002e6a18 100644
--- a/maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java
+++ b/maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java
@@ -339,35 +339,33 @@ public class MavenCli {
 
         cliManager = new CLIManager();
 
-        List<String> args = new ArrayList<>();
         CommandLine mavenConfig = null;
         try {
             File configFile = new File(cliRequest.multiModuleProjectDirectory, MVN_MAVEN_CONFIG);
 
             if (configFile.isFile()) {
-                for (String arg : Files.readAllLines(configFile.toPath(), Charset.defaultCharset())) {
-                    if (!arg.isEmpty()) {
-                        args.add(arg);
-                    }
-                }
-
-                mavenConfig = cliManager.parse(args.toArray(new String[0]));
+                String[] args = Files.lines(configFile.toPath(), Charset.defaultCharset())
+                        .filter(arg -> !arg.isEmpty())
+                        .toArray(size -> new String[size]);
+                mavenConfig = cliManager.parse(args);
                 List<?> unrecognized = mavenConfig.getArgList();
                 if (!unrecognized.isEmpty()) {
-                    throw new ParseException("Unrecognized maven.config entries: " + unrecognized);
+                    // This files can only contain options, not args (goals or phases)
+                    throw new ParseException("Unrecognized maven.config file entries: " + unrecognized);
                 }
             }
         } catch (ParseException e) {
-            System.err.println("Unable to parse maven.config: " + e.getMessage());
+            System.err.println("Unable to parse maven.config file options: " + e.getMessage());
             cliManager.displayHelp(System.out);
             throw e;
         }
 
         try {
+            CommandLine mavenCli = cliManager.parse(cliRequest.args);
             if (mavenConfig == null) {
-                cliRequest.commandLine = cliManager.parse(cliRequest.args);
+                cliRequest.commandLine = mavenCli;
             } else {
-                cliRequest.commandLine = cliMerge(cliManager.parse(cliRequest.args), mavenConfig);
+                cliRequest.commandLine = cliMerge(mavenConfig, mavenCli);
             }
         } catch (ParseException e) {
             System.err.println("Unable to parse command line options: " + e.getMessage());
@@ -392,20 +390,26 @@ public class MavenCli {
         }
     }
 
-    private CommandLine cliMerge(CommandLine mavenArgs, CommandLine mavenConfig) {
+    private CommandLine cliMerge(CommandLine mavenConfig, CommandLine mavenCli) {
         CommandLine.Builder commandLineBuilder = new CommandLine.Builder();
 
-        // the args are easy, cli first then config file
-        for (String arg : mavenArgs.getArgs()) {
-            commandLineBuilder.addArg(arg);
-        }
-        for (String arg : mavenConfig.getArgs()) {
+        // the args are easy, CLI only since maven.config file can only contain options
+        for (String arg : mavenCli.getArgs()) {
             commandLineBuilder.addArg(arg);
         }
 
-        // now add all options, except for -D with cli first then config file
+        /* Although this looks wrong in terms of order Commons CLI stores the value of options in
+         * an array and when a value is potentionally overriden it is added to the array. The single
+         * arg option value is retrieved and instead of returning values[values.length-1] it returns
+         * values[0] which means that the original value instead of the overridden one is returned
+         * (first wins). With properties values are truely overriden since at the end a map is used
+         * to merge which means last wins.
+         *
+         * TODO Report this behavioral bug with Commons CLI
+         */
+        // now add all options, except for user properties with CLI first then maven.config file
         List<Option> setPropertyOptions = new ArrayList<>();
-        for (Option opt : mavenArgs.getOptions()) {
+        for (Option opt : mavenCli.getOptions()) {
             if (String.valueOf(CLIManager.SET_USER_PROPERTY).equals(opt.getOpt())) {
                 setPropertyOptions.add(opt);
             } else {
diff --git a/maven-embedder/src/test/java/org/apache/maven/cli/MavenCliTest.java b/maven-embedder/src/test/java/org/apache/maven/cli/MavenCliTest.java
index 1f1da2e7c..54e937b31 100644
--- a/maven-embedder/src/test/java/org/apache/maven/cli/MavenCliTest.java
+++ b/maven-embedder/src/test/java/org/apache/maven/cli/MavenCliTest.java
@@ -244,7 +244,7 @@ public class MavenCliTest {
         cli.cli(request);
         cli.properties(request);
 
-        String revision = System.getProperty("revision");
+        String revision = request.getUserProperties().getProperty("revision");
         assertEquals("8.1.0", revision);
     }
 
@@ -273,7 +273,7 @@ public class MavenCliTest {
         cli.cli(request);
         cli.properties(request);
 
-        String revision = System.getProperty("revision");
+        String revision = request.getUserProperties().getProperty("revision");
         assertEquals("8.2.0", revision);
     }
 
@@ -308,7 +308,7 @@ public class MavenCliTest {
 
         assertEquals("3", request.commandLine.getOptionValue(CLIManager.THREADS));
 
-        String revision = System.getProperty("revision");
+        String revision = request.getUserProperties().getProperty("revision");
         assertEquals("8.2.0", revision);
 
         assertEquals("bar ", request.getUserProperties().getProperty("foo"));