devonfw#745: fix loading and evaluation of variables (devonfw#746)

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].
 

cli/src/main/java/com/devonfw/tools/ide/context/AbstractIdeContext.java

@@ -128,23 +128,23 @@ 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;
     this.systemInfo = SystemInfoImpl.INSTANCE;
     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;

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() {
 

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
    */

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);
             }
           }
         }

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

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;
+  }
 }

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();

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);
   }
 
 }

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();
   }

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

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");

cli/src/test/resources/com/devonfw/tools/ide/env/var/devon.properties → 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