fix: rework environment fetching [IDE-1314] #969
Merged
Conversation
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
Snapshot the environment before scans (fetching of the project env files) and restore it after. This is so environment variables from one project don't mix into another. Split reading user shell env and reading project env files to different places. Close env ready channel instead of writing a message to it, so it can be checked in multiple routines. Fix incorrect handling of JAVA_HOME introduced in the previous commit.
Instead rely on the config reference already in the test.
A previous commit made it check the incorrect config.
It was breaking test parallelaism and had a race condition as it was implemented in the wrong places.
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ license/snyk check is complete. No issues have been found. (View Details) ✅ code/snyk check is complete. No issues have been found. (View Details) |
rrama
commented
Aug 19, 2025
rrama
commented
Aug 19, 2025
rrama
commented
Aug 19, 2025
rrama
commented
Aug 19, 2025
rrama
commented
Aug 19, 2025
This was referenced Aug 19, 2025
Was searching the default binary search paths before, which is too slow. Removed environment snapshotting and restoring code.
Make the settings path wait for the default env if the user has a custom PATH set. Fix the race condition where it was caching the PATH prior to addDefaults finishing. Revert the split of loading the shell and configuration files separately. Set GAF to fix/IDE-1314_use-shell-paths
bastiandoetsch
approved these changes
Aug 22, 2025
Refactor the function to be clearer about setting PATH Remove unnecessary SetCurrentConfig calls in 2 config tests. Increase the timeout in `Test_handleUntrustedFolders_shouldTriggerTrustRequestAndScanAfterConfirmation`.
The GAF change would have made it be behind the user's SHELL's PATH. Also allowed optimising `updatePathFromSettings` to not need to wait for the default env during initialisation.
Realised we can rely on the scan setting the very first value for us. Update comments to use English (simplified).
Use latest GAF from fix/IDE-1314_use-shell-paths
Revert SDK logic back to version on main.
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.
Description
Move initial environment setup to one place that runs in a Goroutine to not block, then wait on the env before scanning to prevent a race condition.
Fix a race condition where the LS settings
Pathwas causing a cache of thePATHbefore the environment was fully set up, then later restoring to this old value.Note: Splitting the reading of user shell env and reading project env files to different places was scrapped, so we're staying with
LoadConfiguredEnvironment, but still adding calls toWaitForDefaultEnvbeforehand.Note: Environment snapshotting for each workspace before reading the env files was scrapped last minute, as I realised it would prevent parallel project scanning. Plus, the ordering of whether the IDE SDK paths are prepended before or after the env files are read is a bit random. This being said, it is no worse than it was before the changes (probably), I had just hoped to make it a lot better all at once.
Added an option to
config.New()to allow tests to create a config without having to search all the directories for binaries.Also, removed
Config.Path()as it was only used by tests andos.Getenv("PATH")is more reliable.Checklist
make generate)make lint-fix)