From 2b473ff6c90ed4aed5c59d89caa00c2cba38bed1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Hohwiller?= Date: Mon, 11 Nov 2024 19:14:47 +0100 Subject: [PATCH] #745: fix loading and evaluation of variables (#746) --- CHANGELOG.adoc | 1 + .../tools/ide/context/AbstractIdeContext.java | 14 ++++---- .../AbstractEnvironmentVariables.java | 20 +++++++++++ .../ide/environment/EnvironmentVariables.java | 12 ++----- .../EnvironmentVariablesPropertiesFile.java | 34 ++++++++++++++++--- .../tools/ide/environment/VariableLine.java | 4 +-- .../tools/ide/variable/IdeVariables.java | 11 ++++++ .../ide/context/AbstractIdeTestContext.java | 9 +++-- .../tools/ide/context/IdeSlf4jContext.java | 6 ++-- .../tools/ide/context/IdeTestContext.java | 24 ++++++++----- ...nvironmentVariablesPropertiesFileTest.java | 27 ++++++++++----- .../ide/environment/VariableLineTest.java | 2 +- .../{env => environment}/var/devon.properties | 7 +++- 13 files changed, 122 insertions(+), 49 deletions(-) rename cli/src/test/resources/com/devonfw/tools/ide/{env => environment}/var/devon.properties (66%) diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index e019a6f48..23d414538 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -11,6 +11,7 @@ Release with new features and bugfixes: * https://github.com/devonfw/IDEasy/issues/708[#708]: Open vscode in workspace path * https://github.com/devonfw/IDEasy/issues/608[#608]: Enhanced error messages. Now logs missing command output and error messages * https://github.com/devonfw/IDEasy/issues/715[#715]: Show "cygwin is not supported" message for cygwin users +* https://github.com/devonfw/IDEasy/issues/745[#745]: Maven install fails with NPE The full list of changes for this release can be found in https://github.com/devonfw/IDEasy/milestone/15?closed=1[milestone 2024.11.001]. diff --git a/cli/src/main/java/com/devonfw/tools/ide/context/AbstractIdeContext.java b/cli/src/main/java/com/devonfw/tools/ide/context/AbstractIdeContext.java index 9fde2bbf9..056bd3945 100644 --- a/cli/src/main/java/com/devonfw/tools/ide/context/AbstractIdeContext.java +++ b/cli/src/main/java/com/devonfw/tools/ide/context/AbstractIdeContext.java @@ -128,9 +128,9 @@ public abstract class AbstractIdeContext implements IdeContext { * The constructor. * * @param startContext the {@link IdeLogger}. - * @param userDir the optional {@link Path} to current working directory. + * @param workingDirectory the optional {@link Path} to current working directory. */ - public AbstractIdeContext(IdeStartContextImpl startContext, Path userDir) { + public AbstractIdeContext(IdeStartContextImpl startContext, Path workingDirectory) { super(); this.startContext = startContext; @@ -138,13 +138,13 @@ public AbstractIdeContext(IdeStartContextImpl startContext, Path userDir) { this.commandletManager = new CommandletManagerImpl(this); this.fileAccess = new FileAccessImpl(this); String workspace = WORKSPACE_MAIN; - if (userDir == null) { - userDir = Path.of(System.getProperty("user.dir")); + if (workingDirectory == null) { + workingDirectory = Path.of(System.getProperty("user.dir")); } else { - userDir = userDir.toAbsolutePath(); + workingDirectory = workingDirectory.toAbsolutePath(); } // detect IDE_HOME and WORKSPACE - Path currentDir = userDir; + Path currentDir = workingDirectory; String name1 = ""; String name2 = ""; while (currentDir != null) { @@ -166,7 +166,7 @@ public AbstractIdeContext(IdeStartContextImpl startContext, Path userDir) { // detection completed, initializing variables this.ideRoot = findIdeRoot(currentDir); - setCwd(userDir, workspace, currentDir); + setCwd(workingDirectory, workspace, currentDir); if (this.ideRoot == null) { this.toolRepositoryPath = null; diff --git a/cli/src/main/java/com/devonfw/tools/ide/environment/AbstractEnvironmentVariables.java b/cli/src/main/java/com/devonfw/tools/ide/environment/AbstractEnvironmentVariables.java index 3ba749123..8983a0166 100644 --- a/cli/src/main/java/com/devonfw/tools/ide/environment/AbstractEnvironmentVariables.java +++ b/cli/src/main/java/com/devonfw/tools/ide/environment/AbstractEnvironmentVariables.java @@ -12,6 +12,7 @@ import com.devonfw.tools.ide.variable.IdeVariables; import com.devonfw.tools.ide.variable.VariableDefinition; import com.devonfw.tools.ide.variable.VariableSyntax; +import com.devonfw.tools.ide.version.VersionIdentifier; /** * Abstract base implementation of {@link EnvironmentVariables}. @@ -320,6 +321,25 @@ public String inverseResolve(String string, Object src, VariableSyntax syntax) { return result; } + @Override + public VersionIdentifier getToolVersion(String tool) { + + String variable = EnvironmentVariables.getToolVersionVariable(tool); + String value = get(variable); + if (value == null) { + return VersionIdentifier.LATEST; + } else if (value.isEmpty()) { + this.context.warning("Variable {} is configured with empty value, please fix your configuration.", variable); + return VersionIdentifier.LATEST; + } + VersionIdentifier version = VersionIdentifier.of(value); + if (version == null) { + // can actually never happen, but for robustness + version = VersionIdentifier.LATEST; + } + return version; + } + @Override public String toString() { diff --git a/cli/src/main/java/com/devonfw/tools/ide/environment/EnvironmentVariables.java b/cli/src/main/java/com/devonfw/tools/ide/environment/EnvironmentVariables.java index 2c6dd1b7f..40339213c 100644 --- a/cli/src/main/java/com/devonfw/tools/ide/environment/EnvironmentVariables.java +++ b/cli/src/main/java/com/devonfw/tools/ide/environment/EnvironmentVariables.java @@ -87,15 +87,7 @@ default String getToolEdition(String tool) { * @return the {@link VersionIdentifier} with the version of the tool to use. May also be a {@link VersionIdentifier#isPattern() version pattern}. Will be * {@link VersionIdentifier#LATEST} if undefined. */ - default VersionIdentifier getToolVersion(String tool) { - - String variable = getToolVersionVariable(tool); - String value = get(variable); - if (value == null) { - return VersionIdentifier.LATEST; - } - return VersionIdentifier.of(value); - } + VersionIdentifier getToolVersion(String tool); /** * @return the {@link EnvironmentVariablesType type} of this {@link EnvironmentVariables}. @@ -204,7 +196,7 @@ default EnvironmentVariables findVariable(String name) { * their {@link #get(String) value}. * @param source the source where the {@link String} to resolve originates from. Should have a reasonable {@link Object#toString() string representation} * that will be used in error or log messages if a variable could not be resolved. - * @param legacySupport + * @param legacySupport - {@code true} for legacy support with {@link VariableSyntax#CURLY} as fallback, {@code false} otherwise. * @return the given {@link String} with the variables resolved. * @see com.devonfw.tools.ide.tool.ide.IdeToolCommandlet */ diff --git a/cli/src/main/java/com/devonfw/tools/ide/environment/EnvironmentVariablesPropertiesFile.java b/cli/src/main/java/com/devonfw/tools/ide/environment/EnvironmentVariablesPropertiesFile.java index f97f075da..64e1d9cd6 100644 --- a/cli/src/main/java/com/devonfw/tools/ide/environment/EnvironmentVariablesPropertiesFile.java +++ b/cli/src/main/java/com/devonfw/tools/ide/environment/EnvironmentVariablesPropertiesFile.java @@ -67,6 +67,7 @@ private void load() { return; } this.context.trace("Loading properties from {}", this.propertiesFilePath); + boolean legacyProperties = this.propertiesFilePath.getFileName().toString().equals(LEGACY_PROPERTIES); try (BufferedReader reader = Files.newBufferedReader(this.propertiesFilePath)) { String line; do { @@ -75,12 +76,35 @@ private void load() { VariableLine variableLine = VariableLine.of(line, this.context, getSource()); String name = variableLine.getName(); if (name != null) { - variableLine = migrateLine(variableLine, false); - name = variableLine.getName(); - String value = variableLine.getValue(); - this.variables.put(name, value); + VariableLine migratedVariableLine = migrateLine(variableLine, false); + if (migratedVariableLine == null) { + this.context.warning("Illegal variable definition: {}", variableLine); + continue; + } + String migratedName = migratedVariableLine.getName(); + String migratedValue = migratedVariableLine.getValue(); + boolean legacyVariable = IdeVariables.isLegacyVariable(name); + if (legacyVariable && !legacyProperties) { + this.context.warning("Legacy variable name is used to define variable {} in {} - please cleanup your configuration.", variableLine, + this.propertiesFilePath); + } + String oldValue = this.variables.get(migratedName); + if (oldValue != null) { + VariableDefinition variableDefinition = IdeVariables.get(name); + if (legacyVariable) { + // if the legacy name was configured we do not want to override the official variable! + this.context.warning("Both legacy variable {} and official variable {} are configured in {} - ignoring legacy variable declaration!", + variableDefinition.getLegacyName(), variableDefinition.getName(), this.propertiesFilePath); + } else { + this.context.warning("Duplicate variable definition {} with old value '{}' and new value '{}' in {}", name, oldValue, migratedValue, + this.propertiesFilePath); + this.variables.put(migratedName, migratedValue); + } + } else { + this.variables.put(migratedName, migratedValue); + } if (variableLine.isExport()) { - this.exportedVariables.add(name); + this.exportedVariables.add(migratedName); } } } diff --git a/cli/src/main/java/com/devonfw/tools/ide/environment/VariableLine.java b/cli/src/main/java/com/devonfw/tools/ide/environment/VariableLine.java index 6ae0b19dc..7d89de937 100644 --- a/cli/src/main/java/com/devonfw/tools/ide/environment/VariableLine.java +++ b/cli/src/main/java/com/devonfw/tools/ide/environment/VariableLine.java @@ -280,9 +280,7 @@ public static VariableLine of(String line, IdeLogger logger, VariableSource sour name = line.substring(start, end).trim(); } String value = line.substring(end + 1).trim(); - if (value.isEmpty()) { - value = null; - } else if (value.startsWith("\"") && value.endsWith("\"")) { + if (value.startsWith("\"") && value.endsWith("\"")) { value = value.substring(1, value.length() - 1); } return new Variable(export, name, value, line, source); diff --git a/cli/src/main/java/com/devonfw/tools/ide/variable/IdeVariables.java b/cli/src/main/java/com/devonfw/tools/ide/variable/IdeVariables.java index b981e67cf..4ba2ea612 100644 --- a/cli/src/main/java/com/devonfw/tools/ide/variable/IdeVariables.java +++ b/cli/src/main/java/com/devonfw/tools/ide/variable/IdeVariables.java @@ -102,4 +102,15 @@ static VariableDefinition get(String name) { return IdeVariablesList.get(name); } + /** + * @param name the name of the variable. + * @return {@code true} if a {@link VariableDefinition#getLegacyName() legacy variable}, {@code false} otherwise. + */ + static boolean isLegacyVariable(String name) { + VariableDefinition variableDefinition = IdeVariablesList.get(name); + if (variableDefinition != null) { + return name.equals(variableDefinition.getLegacyName()); + } + return false; + } } diff --git a/cli/src/test/java/com/devonfw/tools/ide/context/AbstractIdeTestContext.java b/cli/src/test/java/com/devonfw/tools/ide/context/AbstractIdeTestContext.java index 7fff3da92..d9d730c04 100644 --- a/cli/src/test/java/com/devonfw/tools/ide/context/AbstractIdeTestContext.java +++ b/cli/src/test/java/com/devonfw/tools/ide/context/AbstractIdeTestContext.java @@ -24,6 +24,9 @@ */ public class AbstractIdeTestContext extends AbstractIdeContext { + /** {@link Path} to use as workingDirectory for mocking. */ + protected static final Path PATH_MOCK = Path.of("/"); + private String[] answers; private int answerIndex; @@ -38,12 +41,12 @@ public class AbstractIdeTestContext extends AbstractIdeContext { * The constructor. * * @param logger the {@link IdeLogger}. - * @param userDir the optional {@link Path} to current working directory. + * @param workingDirectory the optional {@link Path} to current working directory. * @param answers the automatic answers simulating a user in test. */ - public AbstractIdeTestContext(IdeStartContextImpl logger, Path userDir, String... answers) { + public AbstractIdeTestContext(IdeStartContextImpl logger, Path workingDirectory, String... answers) { - super(logger, userDir); + super(logger, workingDirectory); this.answers = new String[0]; this.progressBarMap = new HashMap<>(); this.systemInfo = super.getSystemInfo(); diff --git a/cli/src/test/java/com/devonfw/tools/ide/context/IdeSlf4jContext.java b/cli/src/test/java/com/devonfw/tools/ide/context/IdeSlf4jContext.java index e9282653e..b7413879c 100644 --- a/cli/src/test/java/com/devonfw/tools/ide/context/IdeSlf4jContext.java +++ b/cli/src/test/java/com/devonfw/tools/ide/context/IdeSlf4jContext.java @@ -22,11 +22,11 @@ public IdeSlf4jContext() { /** * The constructor. * - * @param userDir the optional {@link Path} to current working directory. + * @param workingDirectory the optional {@link Path} to current working directory. */ - public IdeSlf4jContext(Path userDir) { + public IdeSlf4jContext(Path workingDirectory) { - super(new IdeStartContextImpl(IdeLogLevel.TRACE, level -> new IdeSubLoggerSlf4j(level)), userDir); + super(new IdeStartContextImpl(IdeLogLevel.TRACE, IdeSubLoggerSlf4j::new), workingDirectory); } } diff --git a/cli/src/test/java/com/devonfw/tools/ide/context/IdeTestContext.java b/cli/src/test/java/com/devonfw/tools/ide/context/IdeTestContext.java index 2826569ee..2e5040a1c 100644 --- a/cli/src/test/java/com/devonfw/tools/ide/context/IdeTestContext.java +++ b/cli/src/test/java/com/devonfw/tools/ide/context/IdeTestContext.java @@ -15,30 +15,38 @@ public class IdeTestContext extends AbstractIdeTestContext { private GitContext gitContext; + /** + * The constructor. + */ + public IdeTestContext() { + + this(PATH_MOCK); + } + /** * The constructor. * - * @param userDir the optional {@link Path} to current working directory. + * @param workingDirectory the optional {@link Path} to current working directory. */ - public IdeTestContext(Path userDir) { + public IdeTestContext(Path workingDirectory) { - this(userDir, IdeLogLevel.TRACE); + this(workingDirectory, IdeLogLevel.TRACE); } /** * The constructor. * - * @param userDir the optional {@link Path} to current working directory. + * @param workingDirectory the optional {@link Path} to current working directory. * @param logLevel the {@link IdeLogLevel} used as threshold for logging. */ - public IdeTestContext(Path userDir, IdeLogLevel logLevel) { + public IdeTestContext(Path workingDirectory, IdeLogLevel logLevel) { - this(new IdeTestLogger(logLevel), userDir); + this(new IdeTestLogger(logLevel), workingDirectory); } - private IdeTestContext(IdeTestLogger logger, Path userDir) { + private IdeTestContext(IdeTestLogger logger, Path workingDirectory) { - super(logger, userDir); + super(logger, workingDirectory); this.logger = logger; this.gitContext = new GitContextMock(); } diff --git a/cli/src/test/java/com/devonfw/tools/ide/environment/EnvironmentVariablesPropertiesFileTest.java b/cli/src/test/java/com/devonfw/tools/ide/environment/EnvironmentVariablesPropertiesFileTest.java index 3d830750e..6a6898e02 100644 --- a/cli/src/test/java/com/devonfw/tools/ide/environment/EnvironmentVariablesPropertiesFileTest.java +++ b/cli/src/test/java/com/devonfw/tools/ide/environment/EnvironmentVariablesPropertiesFileTest.java @@ -6,40 +6,51 @@ import java.util.ArrayList; import java.util.List; -import org.assertj.core.api.Assertions; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; +import com.devonfw.tools.ide.context.AbstractIdeContextTest; +import com.devonfw.tools.ide.context.IdeTestContext; import com.devonfw.tools.ide.context.IdeTestContextMock; +import com.devonfw.tools.ide.log.IdeLogLevel; +import com.devonfw.tools.ide.version.VersionIdentifier; /** * Test of {@link EnvironmentVariablesPropertiesFile}. */ @SuppressWarnings("javadoc") -class EnvironmentVariablesPropertiesFileTest extends Assertions { +class EnvironmentVariablesPropertiesFileTest extends AbstractIdeContextTest { + + private static final Path ENV_VAR_PATH = Path.of("src/test/resources/com/devonfw/tools/ide/environment/var/"); + private static final EnvironmentVariablesType TYPE = EnvironmentVariablesType.SETTINGS; /** * Test of {@link EnvironmentVariablesPropertiesFile} including legacy support. */ - - private static final Path ENV_VAR_PATH = Path.of("src/test/resources/com/devonfw/tools/ide/env/var/"); - private static final EnvironmentVariablesType TYPE = EnvironmentVariablesType.SETTINGS; - @Test public void testLoad() { // arrange Path propertiesFilePath = ENV_VAR_PATH.resolve("devon.properties"); + IdeTestContext context = new IdeTestContext(); // act EnvironmentVariablesPropertiesFile variables = new EnvironmentVariablesPropertiesFile(null, TYPE, - propertiesFilePath, IdeTestContextMock.get()); + propertiesFilePath, context); // assert assertThat(variables.getType()).isSameAs(TYPE); assertThat(variables.get("MVN_VERSION")).isEqualTo("3.9.0"); assertThat(variables.get("IDE_TOOLS")).isEqualTo("mvn, npm"); assertThat(variables.get("CREATE_START_SCRIPTS")).isEqualTo("eclipse"); assertThat(variables.get("KEY")).isEqualTo("value"); - assertThat(variables.getVariables()).hasSize(4); + assertThat(variables.get("IDE_MIN_VERSION")).isEqualTo("2024.11.001"); + assertThat(variables.getToolVersion("java")).isEqualTo(VersionIdentifier.LATEST); + assertThat(variables.getVariables()).hasSize(6); + assertThat(context).log(IdeLogLevel.WARNING) + .hasEntries("Duplicate variable definition MVN_VERSION with old value 'undefined' and new value '3.9.0' in " + propertiesFilePath, + "Both legacy variable MAVEN_VERSION and official variable MVN_VERSION are configured in " + propertiesFilePath + + " - ignoring legacy variable declaration!", + "Duplicate variable definition IDE_MIN_VERSION with old value '2023.07.003' and new value '2024.11.001' in " + propertiesFilePath, + "Variable JAVA_VERSION is configured with empty value, please fix your configuration."); } @Test diff --git a/cli/src/test/java/com/devonfw/tools/ide/environment/VariableLineTest.java b/cli/src/test/java/com/devonfw/tools/ide/environment/VariableLineTest.java index 0c587a06a..463d470aa 100644 --- a/cli/src/test/java/com/devonfw/tools/ide/environment/VariableLineTest.java +++ b/cli/src/test/java/com/devonfw/tools/ide/environment/VariableLineTest.java @@ -74,7 +74,7 @@ public void testVariable() { checkVariable("TOOL_VERSION=\"47.11\"", false, "TOOL_VERSION", "47.11"); checkVariable("TOOL_VERSION = \"47.11\" ", false, "TOOL_VERSION", "47.11"); checkVariable("export MAVEN_OPTS=-Xmx2g -Pdev", true, "MAVEN_OPTS", "-Xmx2g -Pdev"); - checkVariable("vaR_namE=", false, "vaR_namE", null); + checkVariable("vaR_namE=", false, "vaR_namE", ""); // edge-cases checkVariable(" export MAVEN_OPTS = -Xmx2g -Pdev", true, "MAVEN_OPTS", "-Xmx2g -Pdev"); checkVariable("export=value", false, "export", "value"); diff --git a/cli/src/test/resources/com/devonfw/tools/ide/env/var/devon.properties b/cli/src/test/resources/com/devonfw/tools/ide/environment/var/devon.properties similarity index 66% rename from cli/src/test/resources/com/devonfw/tools/ide/env/var/devon.properties rename to cli/src/test/resources/com/devonfw/tools/ide/environment/var/devon.properties index 374a10e5e..f6c89ff06 100644 --- a/cli/src/test/resources/com/devonfw/tools/ide/env/var/devon.properties +++ b/cli/src/test/resources/com/devonfw/tools/ide/environment/var/devon.properties @@ -3,5 +3,10 @@ #************************************************************************************************ DEVON_IDE_TOOLS=(mvn npm) DEVON_CREATE_START_SCRIPTS=(eclipse) -MAVEN_VERSION=3.9.0 +MVN_VERSION=undefined +MVN_VERSION=3.9.0 +MAVEN_VERSION=${MVN_VERSION} +DEVON_IDE_MIN_VERSION=2023.07.003 +IDE_MIN_VERSION=2024.11.001 +JAVA_VERSION= KEY=value