#2264: Set environment variables of all installed tools when running a tool - #2266
Open
maybeec wants to merge 1 commit into
Open
#2264: Set environment variables of all installed tools when running a tool#2266maybeec wants to merge 1 commit into
maybeec wants to merge 1 commit into
Conversation
…unning a tool Extract the collection of the tool environment into IdeContext.setEnvironmentOfInstalledTools so that "ide env" and the ProcessContext of a tool run share a single source of truth. The invoked tool and its dependencies still override those variables afterwards. Additionally a failing "ide «cmd»" no longer returns early but always refreshes the shell environment before returning the exit code.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes #2264
Running a tool via
ide «tool» «args»did not provide the environment variables of the other installed tools of the project - only the invoked tool and its declared dependencies got theirsetEnvironmentapplied. The environment was therefore computed by two disagreeing code paths (ideasy enviterated over all installed tools, the tool run did not). Symptom from the issue:ide npm run tauri:devfailed becauseCARGO_HOME/RUSTUP_HOMEwere missing although rust is installed via IDEasy.Implemented changes:
EnvironmentCommandlet.setEnvironmentVariablesInLocalToolsintoIdeContext.setEnvironmentOfInstalledTools(EnvironmentContext)(implemented inAbstractIdeContext) as single source of truth for the tool environment - used byideasy envas well as by tool runs.ToolCommandletnow seeds every newly created toolProcessContextwith the environment variables of all installed tools (new privatenewToolProcess(...), used byrunTool(...)andcompleteRequestProcessContext(...)). The invoked tool and its dependencies apply their ownsetEnvironmentafterwards and therefore still take precedence.isInstalled().isInstalled()spawns processes for some tools (e.g.Npm.computeInstalledVersionrunsnpm --version, npm-based commandlets runnpm list -g «package»), which was tolerable for the rarely usedenvcommandlet but would add seconds of latency to everyide «tool»call. Side effect: a leftoversoftware/«tool»folder without version file now also contributes its variables - consistent with howSystemPathalready treats those folders.cli/src/main/package/functions: a failingide «cmd»no longer returns early. The shell environment is always refreshed viaeval "$(ideasy env --bash)"and the original exit code is returned at the end, so a retry no longer starts from the same incomplete environment.TomcatTest.testTomcatHasEnvironmentOfOtherInstalledTools- the tomcat test project now has rust pre-installed andcatalina.sh/catalina.batechoCARGO_HOME. Written first and verified to fail before the fix (CARGO_HOME=). The pre-existingtestTomcatguards that precedence is unchanged (tomcat still gets the tomcat-specificJAVA_HOMEof java21_35, not the project version8u402b06).Testing instructions
Automated:
mvn -Dtest=TomcatTest testin theclifolder -testTomcatHasEnvironmentOfOtherInstalledToolsasserts thatCARGO_HOMEof the installed rust points intosoftware/rust/.cargoalthough tomcat only depends on java.mvn -Dtest=EnvironmentCommandletTest,EnvironmentCommandletGlobalTest testin theclifolder - the output ofideasy envis unchanged.Manual (the original scenario):
env -u CARGO_HOME -u RUSTUP_HOME ideasy node -e "console.log(process.env.CARGO_HOME, process.env.RUSTUP_HOME)"Both must point into
«IDE_HOME»/software/rust(before this change they wereundefined).ide npm run does-not-existand check that the environment of the shell is still refreshed afterwards (e.g.echo $CARGO_HOMEis set) whileecho $?right after theidecall still reports the non-zero exit code.Checklist for this PR
Make sure everything is checked before merging this PR. For further info please also see
our DoD.
mvn clean testlocally all tests pass and build is successful#«issue-id»: «brief summary»(e.g.#921: fixed setup.bat). If no issue ID exists, title only.In Progressand assigned to you or there is no issue (might happen for very small PRs)with
internalNote: this PR does not add a new tool commandlet, so the commandlet checklist does not apply.