[ 
https://issues.apache.org/jira/browse/MNG-7914?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17874529#comment-17874529
 ] 

ASF GitHub Bot commented on MNG-7914:
-------------------------------------

michael-o commented on code in PR #1595:
URL: https://github.com/apache/maven/pull/1595#discussion_r1720797587


##########
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -1204,40 +1211,57 @@ void toolchains(CliRequest cliRequest) throws Exception 
{
                         "The specified user toolchains file does not exist: " 
+ userToolchainsFile);
             }
         } else {
-            userToolchainsFile = DEFAULT_USER_TOOLCHAINS_FILE;
+            String userToolchainsFileStr = 
cliRequest.getUserProperties().getProperty(Constants.MAVEN_USER_TOOLCHAINS);
+            if (userToolchainsFileStr != null) {
+                userToolchainsFile = new File(userToolchainsFileStr);
+            }
         }
 
-        File globalToolchainsFile;
+        File installationToolchainsFile = null;

Review Comment:
   Why not use `Path` here throughout?



##########
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -1624,20 +1643,65 @@ static void populateProperties(
         String mavenBuildVersion = 
CLIReportingUtils.createMavenVersionString(buildProperties);
         systemProperties.setProperty("maven.build.version", mavenBuildVersion);
 
-        BasicInterpolator interpolator =
-                createInterpolator(paths, systemProperties, userProperties, 
userSpecifiedProperties);
-        for (Map.Entry<Object, Object> e : userSpecifiedProperties.entrySet()) 
{
-            String name = (String) e.getKey();
-            String value = interpolator.interpolate((String) e.getValue());
-            userProperties.setProperty(name, value);
-            // 
----------------------------------------------------------------------
-            // I'm leaving the setting of system properties here as not to 
break
-            // the SystemPropertyProfileActivator. This won't harm embedding. 
jvz.
-            // 
----------------------------------------------------------------------
-            if (System.getProperty(name) == null) {
-                System.setProperty(name, value);
-            }
+        // 
----------------------------------------------------------------------
+        // Options that are set on the command line become system properties
+        // and therefore are set in the session properties. System properties
+        // are most dominant.
+        // 
----------------------------------------------------------------------
+
+        Properties userSpecifiedProperties =
+                
commandLine.getOptionProperties(String.valueOf(CLIManager.SET_USER_PROPERTY));
+        userProperties.putAll(userSpecifiedProperties);
+
+        // 
----------------------------------------------------------------------
+        // Load config files
+        // 
----------------------------------------------------------------------
+        Function<String, String> callback =
+                or(paths::getProperty, prefix("cli.", 
commandLine::getOptionValue), systemProperties::getProperty);
+
+        Path mavenConf;
+        if (systemProperties.getProperty(MAVEN_INSTALLATION_CONF) != null) {
+            mavenConf = 
fileSystem.getPath(systemProperties.getProperty(MAVEN_INSTALLATION_CONF));
+        } else if (systemProperties.getProperty("maven.conf") != null) {
+            mavenConf = 
fileSystem.getPath(systemProperties.getProperty("maven.conf"));
+        } else if (systemProperties.getProperty(MAVEN_HOME) != null) {
+            mavenConf = 
fileSystem.getPath(systemProperties.getProperty(MAVEN_HOME), "conf");
+        } else {
+            mavenConf = fileSystem.getPath("");
         }
+        Path propertiesFile = mavenConf.resolve("maven.properties");
+        MavenPropertiesLoader.loadProperties(userProperties, propertiesFile, 
callback, false);
+
+        // 
----------------------------------------------------------------------
+        // I'm leaving the setting of system properties here as not to break
+        // the SystemPropertyProfileActivator. This won't harm embedding. jvz.
+        // 
----------------------------------------------------------------------
+        Set<String> sys = 
SystemProperties.getSystemProperties().stringPropertyNames();
+        userProperties.stringPropertyNames().stream()
+                .filter(k -> !sys.contains(k))
+                .forEach(k -> System.setProperty(k, 
userProperties.getProperty(k)));
+    }

Review Comment:
   We must seriously reconsider this promotion in the future before GA.



##########
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -874,9 +873,17 @@ private static <T> List<T> reverse(List<T> list) {
     }
 
     private List<File> parseExtClasspath(CliRequest cliRequest) {
-        String extClassPath = 
cliRequest.userProperties.getProperty(EXT_CLASS_PATH);
+        String extClassPath = 
cliRequest.userProperties.getProperty(Constants.MAVEN_EXT_CLASS_PATH);
         if (extClassPath == null) {
-            extClassPath = 
cliRequest.systemProperties.getProperty(EXT_CLASS_PATH);
+            extClassPath = 
cliRequest.systemProperties.getProperty(Constants.MAVEN_EXT_CLASS_PATH);
+            if (extClassPath != null) {
+                slf4jLogger.warn(
+                        "The property {} has been set using a JVM system 
property which is deprecated. "
+                                + "The property can be passed as a maven 
argument or in the maven project configuration file,"
+                                + "usually located at {}.",

Review Comment:
   Args should be surrounded by single quotes.



##########
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -874,9 +873,17 @@ private static <T> List<T> reverse(List<T> list) {
     }
 
     private List<File> parseExtClasspath(CliRequest cliRequest) {
-        String extClassPath = 
cliRequest.userProperties.getProperty(EXT_CLASS_PATH);
+        String extClassPath = 
cliRequest.userProperties.getProperty(Constants.MAVEN_EXT_CLASS_PATH);
         if (extClassPath == null) {
-            extClassPath = 
cliRequest.systemProperties.getProperty(EXT_CLASS_PATH);
+            extClassPath = 
cliRequest.systemProperties.getProperty(Constants.MAVEN_EXT_CLASS_PATH);
+            if (extClassPath != null) {
+                slf4jLogger.warn(
+                        "The property {} has been set using a JVM system 
property which is deprecated. "
+                                + "The property can be passed as a maven 
argument or in the maven project configuration file,"
+                                + "usually located at {}.",
+                        Constants.MAVEN_EXT_CLASS_PATH,
+                        "[rootDirectory]/.mvn/maven.properties");

Review Comment:
   This message should use prop syntax: 
`"${rootDirectory}/.mvn/maven.properties"}`, no?



##########
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -1380,14 +1404,14 @@ private static boolean isRunningOnCI(Properties 
systemProperties) {
     }
 
     private String determineLocalRepositoryPath(final MavenExecutionRequest 
request) {
-        String userDefinedLocalRepo = 
request.getUserProperties().getProperty(MavenCli.LOCAL_REPO_PROPERTY);
+        String userDefinedLocalRepo = 
request.getUserProperties().getProperty(Constants.MAVEN_REPO_LOCAL);
         if (userDefinedLocalRepo != null) {
             return userDefinedLocalRepo;
         }
 
         // TODO Investigate why this can also be a Java system property and 
not just a Maven user property like
         // other properties
-        return 
request.getSystemProperties().getProperty(MavenCli.LOCAL_REPO_PROPERTY);
+        return 
request.getSystemProperties().getProperty(Constants.MAVEN_REPO_LOCAL);

Review Comment:
   @gnodet, should this also issue a warning like you did with ext classpath 
being a system property?



##########
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -1204,40 +1211,57 @@ void toolchains(CliRequest cliRequest) throws Exception 
{
                         "The specified user toolchains file does not exist: " 
+ userToolchainsFile);
             }
         } else {
-            userToolchainsFile = DEFAULT_USER_TOOLCHAINS_FILE;
+            String userToolchainsFileStr = 
cliRequest.getUserProperties().getProperty(Constants.MAVEN_USER_TOOLCHAINS);
+            if (userToolchainsFileStr != null) {
+                userToolchainsFile = new File(userToolchainsFileStr);
+            }
         }
 
-        File globalToolchainsFile;
+        File installationToolchainsFile = null;
 
-        if 
(cliRequest.commandLine.hasOption(CLIManager.ALTERNATE_GLOBAL_TOOLCHAINS)) {
-            globalToolchainsFile =
+        if 
(cliRequest.commandLine.hasOption(CLIManager.ALTERNATE_INSTALLATION_TOOLCHAINS))
 {
+            installationToolchainsFile =
+                    new 
File(cliRequest.commandLine.getOptionValue(CLIManager.ALTERNATE_INSTALLATION_TOOLCHAINS));
+            installationToolchainsFile = 
resolveFile(installationToolchainsFile, cliRequest.workingDirectory);
+
+            if (!installationToolchainsFile.isFile()) {
+                throw new FileNotFoundException(
+                        "The specified installation toolchains file does not 
exist: " + installationToolchainsFile);
+            }
+        } else if 
(cliRequest.commandLine.hasOption(CLIManager.ALTERNATE_GLOBAL_TOOLCHAINS)) {
+            installationToolchainsFile =
                     new 
File(cliRequest.commandLine.getOptionValue(CLIManager.ALTERNATE_GLOBAL_TOOLCHAINS));
-            globalToolchainsFile = resolveFile(globalToolchainsFile, 
cliRequest.workingDirectory);
+            installationToolchainsFile = 
resolveFile(installationToolchainsFile, cliRequest.workingDirectory);
 
-            if (!globalToolchainsFile.isFile()) {
+            if (!installationToolchainsFile.isFile()) {
                 throw new FileNotFoundException(
-                        "The specified global toolchains file does not exist: 
" + globalToolchainsFile);
+                        "The specified installation toolchains file does not 
exist: " + installationToolchainsFile);
             }
         } else {
-            globalToolchainsFile = DEFAULT_GLOBAL_TOOLCHAINS_FILE;
+            String installationToolchainsFileStr =
+                    
cliRequest.getUserProperties().getProperty(Constants.MAVEN_INSTALLATION_TOOLCHAINS);
+            if (installationToolchainsFileStr != null) {
+                installationToolchainsFile = new 
File(installationToolchainsFileStr);
+                installationToolchainsFile = 
resolveFile(installationToolchainsFile, cliRequest.workingDirectory);
+            }

Review Comment:
   This entire block makes me think: WE fail here if the toolchains file cannot 
be loaded, but we do not fail if core extensions file is provided, but not 
valid? FNFE and frieds...





> Provide a single entry point for configuration
> ----------------------------------------------
>
>                 Key: MNG-7914
>                 URL: https://issues.apache.org/jira/browse/MNG-7914
>             Project: Maven
>          Issue Type: New Feature
>            Reporter: Guillaume Nodet
>            Priority: Major
>             Fix For: 4.0.x-candidate
>
>
> Looking at MNG-7772, this should not require any code change, but it's all 
> about configuration.
> I propose to load / interpolate the following files:
>  * {{${maven.home}/conf/maven.user.properties}}
>  * {{${maven.home}/conf/maven.system.properties}}
> Those files would be used to load additional user properties and system 
> properties for Maven.  In addition to the simple interpolation mechanism, we 
> should provide two enhancements using special keys {{{}$\{includes{}}}} and 
> {{{}$\{optionals{}}}} which would be used to load additional referenced 
> configuration files such as:
> {{    ${optionals} = ${user.home}/.m2/maven.user.properties, 
> ${session.rootDirectory}/.mvn/maven.user.properties}}
> Being loaded early when Maven is loaded, those files could reference 
> directories to load extensions from:
>   {{{}maven.core.extensions.directories = 
> ${session.rootDirectory}/.mvn/extensions.xml,{}}}{{{}${user.home}/.m2/extensions.xml,${maven.home}/extensions.xml{}}}
>  
> In various places, the maven code could be simplified and offer more 
> configuration points at the same time.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

Reply via email to