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