[ 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)