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...
--
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: issues-unsubscr...@maven.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
