Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
hohwille opened this issue Aug 26, 2024 · 0 comments · Fixed by #614
Closed

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

hohwille opened this issue Aug 26, 2024 · 0 comments · Fixed by #614
Assignees
Labels
enhancement New feature or request environment EnvironmentCommandlet, env variables, path, etc. process executing external programs (ProcessContext)

Comments

@hohwille
Copy link
Member

hohwille commented Aug 26, 2024

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.

@hohwille hohwille added the enhancement New feature or request label Aug 26, 2024
@github-project-automation github-project-automation bot moved this to 🆕 New in IDEasy board Aug 26, 2024
@hohwille hohwille added process executing external programs (ProcessContext) environment EnvironmentCommandlet, env variables, path, etc. labels Aug 26, 2024
@hohwille hohwille self-assigned this Sep 12, 2024
hohwille added a commit to hohwille/IDEasy that referenced this issue Sep 12, 2024
hohwille added a commit to hohwille/IDEasy that referenced this issue Sep 12, 2024
hohwille added a commit to hohwille/IDEasy that referenced this issue Sep 12, 2024
hohwille added a commit to hohwille/IDEasy that referenced this issue Sep 13, 2024
hohwille added a commit that referenced this issue Sep 19, 2024
@github-project-automation github-project-automation bot moved this from 🆕 New to ✅ Done in IDEasy board Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request environment EnvironmentCommandlet, env variables, path, etc. process executing external programs (ProcessContext)
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

1 participant