Skip to content

ProcessContext should compute PATH on run and not in constructor #556

@hohwille

Description

@hohwille

As a IDEasy developer, I want to use ProcessContext seamlessly so that I do not cause bugs if I do not know its implementation details.

The problem started in PR #515 with this code:

      if (existsEnvironmentContext) {
        pc = this.context.newProcess().errorHandling(ProcessErrorHandling.THROW).executable(binaryPath).addArgs(args);
        install(pc, true);
      } else {
        install(true);
        pc = this.context.newProcess().errorHandling(ProcessErrorHandling.THROW).executable(binaryPath).addArgs(args);
      }

I left a review comment that we should move the duplicated code to create the ProcessContext before the if statement to avoid redundancy.
However, what we then figured out is the following:

this.context.newProcess() is creating a new ProcessContextImpl and its constructor is computing all environment variables and setting them in the ProcessBuilder. Doing this at construction time in general makes sense since we might use withEnvVar method to override some environment variable.

However, the when the environment variables are evaluated and set, this also includes the PATH variable:

VariableDefinitionSystemPath PATH = new VariableDefinitionSystemPath("PATH", null, c -> c.getPath(), true, true);

Therefore the recent path is evaluated from the SystemPath:
public String toString(WindowsPathSyntax pathSyntax) {

However, that value (can) change[s] during the installation of tools because if a tool gets newly installed a new folder like software/«tool»/bin may be created and therefore added to SystemPath and therefore end up in the value of PATH.
So if install is called after the ProcessContext is created, we can cause a bug.

Even worse with the tool-dependencies approach we now have to create the ProcessContext before in order to pass it to the install method and "tweak" it in there (e.g. for tomcat the variable CATALINA_HOME may be set and because tomcat depends on java it will delegate installation of a compatible version of java and therefore set JAVA_HOME to that installation).

As a result we IMHO need to handle the PATH variable in a special way and when we call ProcessContext.run() we should compute the latest PATH and set it as environment variable in the ProcessBuilder. IMHO it is simpler to just update the PATH in run() and therefore "compute" it two times: at construction time and then again in run(). This does not harm since we manage the PATH efficiently via SystemPath and actively update it from install so we do not have to do directory traversals to compute the PATH every time (only happens when IDEasy is launched and context is created).

Theoretically some case could happen, where someone creates a ProcessContext and wants to set a custom PATH via withEnvVar and that would then stop working because we overwrite this later in the run method. If somebody wants to solve this story with perfection, he could track this case and then prevent the auto-update, but it is also fine if we ignore this until this problem may ever happen and then create another bug issue for it.

Metadata

Metadata

Assignees

Labels

enhancementNew feature or requestenvironmentEnvironmentCommandlet, env variables, path, etc.processexecuting external programs (ProcessContext)

Projects

Status

✅ Done

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions