Ensure pyenv virtual envs are not skipped when in discovery experiment #15451
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
kimadeline
approved these changes
Feb 18, 2021
src/client/pythonEnvironments/discovery/locators/services/pyenvLocator.ts
Outdated
Show resolved
Hide resolved
karrtikr
approved these changes
Feb 18, 2021
There was a problem hiding this comment.
LGTM but few concerns,
- Why has this problem only started appearing since the last release? Is this only when in experiment? I can't seem to confirm that from the comments.
- Is this only a special case with Pyenv, or should we fix this when listing subdirectories in other locators as well?
- I don't recall the old locators having a problem with listing such interpreters mentioned in the issue. We used the VSCode FS API in the old locators,
vs.workspace.fs.readDirectory. Maybe that doesn't have this problem.
| if (stat.isDirectory()) { | ||
| yield item; | ||
| yield fullPath; | ||
| } else if (resolveSymlinks && stat.isSymbolicLink()) { |
There was a problem hiding this comment.
What about symlinks which are not directories? It seems we'll be returning those as well :/
Member
Author
There was a problem hiding this comment.
Why has this problem only started appearing since the last release? Is this only when in experiment? I can't seem to confirm that from the comments.
This is only in the experiment. I don't think it is affecting the old code. There is a different problem with the old code.
Is this only a special case with Pyenv, or should we fix this when listing subdirectories in other locators as well?
As far as I can tell this is a special case with Pyenvs. pyenv seems to optimize things this way.
I don't recall the old locators having a problem with listing such interpreters mentioned in the issue. We used the VSCode FS API in the old locators, vs.workspace.fs.readDirectory. Maybe that doesn't have this problem.
That is right vs.workspace.fs.readDirectory reads symlinks and resolves them for directories.
What about symlinks which are not directories?
Good catch. I will update the conde to handle that case.
There was a problem hiding this comment.
Gotcha.
As far as I can tell this is a special case with Pyenvs. pyenv seems to optimize things this way.
We know vs.workspace.fs.readDirectory which reads symlinks and resolves them for directories works well for old virtual env locators that we had.
So I think we should also pass resolveSymlinks as true for other virtual env locators as well to be safe.
There was a problem hiding this comment.
I have created a work item corresponding to the precautionary comment I made above.
karrtikr
changed the title
Codecov Report
@@ Coverage Diff @@
## main #15451 +/- ##
======================================
- Coverage 65% 65% -1%
======================================
Files 560 561 +1
Lines 26926 26959 +33
Branches 3900 3913 +13
======================================
- Hits 17535 17531 -4
- Misses 8666 8700 +34
- Partials 725 728 +3
|
karthiknadig
added a commit
to karthiknadig/vscode-python
that referenced
this pull request
Feb 19, 2021
microsoft#15451) * Ensure pyenv virtual envs are not skipped * Add news * Clean up * Address comments
karthiknadig
added a commit
that referenced
this pull request
Mar 8, 2021
* Cherry pick fix for native notebook support into release branch (#15369) * Fix problem with notebook apis not being used. (#15366) * Update changelog * Remove news file * Add extraPaths support to JediLSP (#15365) * Add extraPaths support * Remove duplicate opt option * Eslint cleanup * Fix tests * Ignore directories with the same name as python binaries (#15367) * Ignore directories with the same name as pyhton binaries * Use withFileTypes instead of lstat * Correct pipenv activation provider when in discovery experiment (#15319) * Correct pipenv activation provider when in discovery experiment * Fix ESLint errors * In PythonEnvsReducer.resolveEnv(), always fall back to the wrapped locator. (#15350) fixes #15118 * Fix issue with missing interpreter info for some cases (#15376) * Use child process apis directly. * Use raw API in process service * Handle process cleanup * Address sonar * Refactor process service by pulling the raw process APIs out of the class * Address comments * Add reference to CVE-2020-16977 fixed in Oct 2020. (#15381) * Fix CI failing with the number of constructor arguments error (#15347) * Fix Command 'workbench.action.closeAllEditors' timed out failure on CI (#15322) * Ensure environment variables provider can be created using standard classes (#15377) * Ensure environment variables provider can be created using standard classes * Fix unit tests * Fix ESLint errors for environment variable provider (#15383) * Fix problem with notebook apis not being used. (#15366) * Show Python: Launch TensorBoard command in command palette (#15382) (#15386) * Always register Launch TensorBoard command * Put usage tracking behind experiment * Cherry picks from main to release (#15421) * Do not call activate the discovery component before registering all the classes (#15379) * Do not attempt to activate discovery component before registering all the classes * Add clarification comment * Code reviews * Skip windows store and shims paths when using known path locators (#15388) * Skip windows store and shims paths when using known path locators * Clean up and comments * Tests * Handle cases where envs variables might not be set * Typo Co-authored-by: Kim-Adeline Miguel <51720070+kimadeline@users.noreply.github.com> Co-authored-by: Kim-Adeline Miguel <51720070+kimadeline@users.noreply.github.com> * Change "Pylance not installed" prompt to allow reverting to Jedi (#15420) * Allow on suggestion refresh by default (#15430) Co-authored-by: Kartik Raj <karraj@microsoft.com> Co-authored-by: Kim-Adeline Miguel <51720070+kimadeline@users.noreply.github.com> Co-authored-by: Jake Bailey <5341706+jakebailey@users.noreply.github.com> * Release final (#15433) * Version update * Change log updates * TPN update * Ensure pyenv virtual envs are not skipped when in discovery experiment (#15451) * Ensure pyenv virtual envs are not skipped * Add news * Clean up * Address comments * Register Jedi regardless of what language server is configured (#15452) * Register Jedi regardless of what language server is configured * News entry * Only call component adapter behind the discovery experiment (#15459) * Update version and change log for point release (#15463) * Version update * Update change log * Update start-up telemetry for Jedi LSP (#15419) (#15571) * Version and change log update for point release (#15572) Co-authored-by: Rich Chiodo <rchiodo@users.noreply.github.com> Co-authored-by: Kartik Raj <karraj@microsoft.com> Co-authored-by: Eric Snow <eric.snow@microsoft.com> Co-authored-by: Jim Griesmer <jimgries@microsoft.com> Co-authored-by: Joyce Er <joyceerhuiling@gmail.com> Co-authored-by: Kim-Adeline Miguel <51720070+kimadeline@users.noreply.github.com> Co-authored-by: Jake Bailey <5341706+jakebailey@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
For #15439
This is a special case for
pyenvwhere the env dirs are listed a symbolic links. To add tests for this scenario we need to bring up the virutal FS test framework so we can use declarative test file layout for things like symbolic links and other OS and FS specific things.