Add a windows "known-paths" locator. by ericsnowcurrently · Pull Request #14675 · microsoft/vscode-python

ericsnowcurrently · 2020-11-09T21:58:59Z

Note that all but ~100 lines of this PR is tests.

Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR).
Title summarizes what is changing.
~~[ ] Has a news entry file (remember to thank yourself!).~~
Appropriate comments and documentation strings in the code.
Has sufficient logging.
~~[ ] Has telemetry for enhancements.~~
Unit tests & system/integration tests are added/updated.
~~[ ] Test plan is updated as appropriate.~~
~~[ ] package-lock.json has been regenerated by running npm install (if dependencies have changed).~~
~~[ ] The wiki is updated with any design decisions/details.~~

codecov-io · 2020-11-10T00:42:44Z

Codecov Report

Merging #14675 (6faa56b) into main (3698950) will decrease coverage by 0%.
The diff coverage is 63%.

@@           Coverage Diff           @@
##            main   #14675    +/-   ##
=======================================
- Coverage     65%      65%    -1%     
=======================================
  Files        552      552            
  Lines      25926    26026   +100     
  Branches    3678     3693    +15     
=======================================
+ Hits       16918    16926     +8     
- Misses      8315     8413    +98     
+ Partials     693      687     -6

Impacted Files	Coverage Δ
src/client/activation/jedi/languageServerProxy.ts	`25% <0%> (-2%)`	⬇️
src/client/activation/node/analysisOptions.ts	`55% <0%> (-16%)`	⬇️
src/client/common/utils/exec.ts	`20% <0%> (-2%)`	⬇️
src/client/pythonEnvironments/base/envsCache.ts	`68% <0%> (-2%)`	⬇️
...s/discovery/locators/services/KnownPathsService.ts	`48% <0%> (ø)`
src/client/activation/node/languageServerProxy.ts	`25% <15%> (-2%)`	⬇️
src/client/common/utils/text.ts	`50% <28%> (-19%)`	⬇️
...t/activation/languageServer/languageServerProxy.ts	`90% <75%> (-1%)`	⬇️
.../locators/services/virtualEnvironmentIdentifier.ts	`92% <75%> (+<1%)`	⬆️
.../pythonEnvironments/common/externalDependencies.ts	`63% <87%> (-10%)`	⬇️
... and 12 more

sonarqubecloud · 2020-11-10T18:18:26Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
0.0% Duplication

karthiknadig · 2020-11-11T09:30:35Z

You need to rebase. The API in index has changed a bit.

karrtikr

It seems we have a disagreement in the way testing is supposed to be done, I am concerned this will make testing the component as a whole harder.

karrtikr · 2020-12-08T00:38:55Z

+        async function* generator() {
+            try {
+                for await (const executable of findInterpretersInDir(entry)) {
+                    const okay = await isValidAndExecutable(executable);


We should only be returning python.exe or python to avoid returning duplicates (as done in other locators using isStandardPythonBinary).

That's what the reducer is supposed to do. Each low-level locator is responsible only for tracking any valid environment within its knowledge domain. They should be focused on the one job and leave any other responsibilities to composite locators.

The reducer doesn't resolve symlinks. cc @karthiknadig
We decided to delegate the responsibility to the locators as they know best regarding that. (for eg. windows store locators symlinks for the same env maybe located in different folders, so there's no way for the reducer to resolve that, hence the locator itself makes sure to not return symlinks).

@karrtikr is right. Depending on the context, which the individual locator have, we may or may not want to resolve symlinks.

~~This has been addressed.~~

So I should be checking for symlinks? How does that work on Windows?

Hmm, I guess I'm still not clear on why ReducingLocator wouldn't take care of this. Is it just that we want to favor python.exe when it is in the same directory as python3.8.exe? Why wouldn't we do that in the reducer (via a call to some helper)?

We can address the question of a better approach separately. I'll use isStandardPythonBinary() for now.

Is it just that we want to favor python.exe when it is in the same directory as python3.8.exe? Why wouldn't we do that in the reducer (via a call to some helper)?

We don't want to do it for every locator, for eg. in Windows store we prioritize python3.8.exe over python.exe, hence logic is specific to the locator and not the reducer.

karrtikr · 2020-12-10T00:41:58Z

+
+    constructor() {
+        const dirLocators: DirFilesLocator[] = getSearchPathEntries()
+            .map((dirname) => new DirFilesLocator(dirname, PythonEnvKind.Unknown));


This should probably be marked as PythonEnvKind.Global instead.

It could be System, OtherGlobal, Custom, Venv, or really any of them. We simply do not know. Hence Unknown.

If it's Venv it should be resolved in the reducer as it'll also be discovered by Venv locators. If we're not able to distinguish between System, OtherGlobal, Custom for now, we should just go with System IMO. Having an Unknown type could mean anything.

Note we also had an OKR to move away from Unknown type.

We currently also detect in as Unknown, so nevermind.

This locator should not be responsible for guessing/determining the environment's kind. That is the responsibility of kind-specific locators. So ultimately every env that this locator identifies should also be identified by at least one other locator that can determine the kind. If we end up keeping this locator in the long term then it will only be as a fallback when we are missing the appropriate locator. Regardless, rather than a dedicated $PATH-based locator we should have a $PATH helper that the kind-specific locators use. If we kept a generic $PATH-based locator then it would just be a light wrapper around that helper.

(FWIW, all the same things apply to any generic Windows registry locator, and probably a couple of others.)

All that said, we have diverged a bit from the kind-specific focus of locators and this PR is not the place to fix that. 😄

karthiknadig

Look good to me. Approving assuming Kartik's comments will be addressed.

karrtikr

LGTM once the filter comment is addressed.

Co-authored-by: Eric Snow <eric.snow@microsoft.com>

…#14675) (microsoft#14951)" This reverts commit 651a6b9.

…14951)" (#14968) This reverts commit 651a6b9.

…14675) (#14951)" (#14968)" This reverts commit 5f93cd1.

…#14675) (microsoft#14951)" (microsoft#14968) This reverts commit 651a6b9.

ericsnowcurrently added the no-changelog No news entry required label Nov 9, 2020

ericsnowcurrently requested review from karrtikr and karthiknadig November 9, 2020 21:58

ericsnowcurrently force-pushed the pyenvs-component-windows-known-paths-locator branch from 292d176 to c79f3cd Compare November 10, 2020 18:16

karrtikr reviewed Nov 13, 2020

View reviewed changes

ericsnowcurrently marked this pull request as draft November 17, 2020 18:08

karthiknadig reviewed Nov 17, 2020

View reviewed changes

Comment thread src/test/utils/text.ts Outdated

Comment thread src/test/utils/fs.ts

Comment thread src/test/testing/helper.ts Outdated

brettcannon assigned ericsnowcurrently Nov 26, 2020

ericsnowcurrently force-pushed the pyenvs-component-windows-known-paths-locator branch from c79f3cd to ce255b1 Compare December 1, 2020 22:44

ericsnowcurrently marked this pull request as ready for review December 1, 2020 22:46

github-actions Bot requested review from karrtikr and karthiknadig December 1, 2020 22:47

ericsnowcurrently changed the title ~~Add a windows known-paths locator.~~ Add a windows "known-paths" locator. Dec 1, 2020

karthiknadig requested changes Dec 3, 2020

View reviewed changes

Comment thread src/client/common/utils/exec.ts

Comment thread src/client/pythonEnvironments/base/locators/lowLevel/windowsKnownPathsLocator.ts Outdated

karrtikr reviewed Dec 8, 2020

View reviewed changes

ericsnowcurrently force-pushed the pyenvs-component-windows-known-paths-locator branch from c82acb6 to 6faa56b Compare December 8, 2020 15:44

ericsnowcurrently requested review from karrtikr and karthiknadig December 10, 2020 00:16

karrtikr reviewed Dec 10, 2020

View reviewed changes

karthiknadig approved these changes Dec 10, 2020

View reviewed changes

karrtikr approved these changes Dec 10, 2020

View reviewed changes

ericsnowcurrently added 6 commits December 10, 2020 16:40

Add FS-related testing helpers.

5b5205e

Add WindowsKnownPathsLocator.

bbd0f72

Drop WindowsKnownPathsLocator.triggerRefresh().

1e46276

Make sure the files are actually executable.

9d1d4dd

Deal with missing directories on $PATH.

be48b07

Register the locator.

6fa898f

ericsnowcurrently added 18 commits December 10, 2020 16:40

Add a better doc comment for the parseTree() helper.

8a33257

Add better doc comments for parseFSTree().

dced4ef

Move the text-oriented helpers to the appropriate module.

39332be

Fix the tests.

06e7d2e

Drop errant line.

8b4a4eb

Fix the tests.

db71c64

lint

8b1180c

Do not check if files are executable.

917cbc0

Drop an outdated comment.

0021f3a

Do not fail if directory not found.

b6c350a

Add DirFilesLocator.

ed31414

Fix WindowsKnownPathsLocator.

45689da

WindowsKnownPathsLocator -> WindowsPathEnvVarLocator.

12a3688

Drop an outdated comment.

13b997d

Drop the "opts" param from findInterpretersInDir().

a5ac756

Factor out getDirFilesLocator().

e19a429

Wrap DirFilesLocator.

f5d1284

Filter out "non-standard" executables.

68151a3

ericsnowcurrently force-pushed the pyenvs-component-windows-known-paths-locator branch from b87cc62 to 68151a3 Compare December 10, 2020 23:40

ericsnowcurrently merged commit df2dff3 into microsoft:main Dec 10, 2020

ericsnowcurrently deleted the pyenvs-component-windows-known-paths-locator branch December 10, 2020 23:55

karthiknadig pushed a commit to karthiknadig/vscode-python that referenced this pull request Dec 11, 2020

Add a locator for executables on $PATH on Windows. (microsoft#14675)

96a01e9

karthiknadig added a commit that referenced this pull request Dec 11, 2020

Add a locator for executables on $PATH on Windows. (#14675) (#14951)

651a6b9

Co-authored-by: Eric Snow <eric.snow@microsoft.com>

karthiknadig added a commit to karthiknadig/vscode-python that referenced this pull request Dec 14, 2020

Revert "Add a locator for executables on $PATH on Windows. (microsoft…

9868a09

…#14675) (microsoft#14951)" This reverts commit 651a6b9.

karthiknadig added a commit that referenced this pull request Dec 14, 2020

Revert "Add a locator for executables on $PATH on Windows. (#14675) (#…

5f93cd1

…14951)" (#14968) This reverts commit 651a6b9.

karrtikr pushed a commit that referenced this pull request Dec 14, 2020

Revert "Revert "Add a locator for executables on $PATH on Windows. (#…

69cdd1d

…14675) (#14951)" (#14968)" This reverts commit 5f93cd1.

karrtikr pushed a commit to karrtikr/vscode-python that referenced this pull request Dec 14, 2020

Add a locator for executables on $PATH on Windows. (microsoft#14675)

411b573

karrtikr pushed a commit that referenced this pull request Dec 14, 2020

Add a locator for executables on $PATH on Windows. (#14675)

d775d8f

karrtikr pushed a commit to karrtikr/vscode-python that referenced this pull request Dec 15, 2020

Revert "Add a locator for executables on $PATH on Windows. (microsoft…

133d019

…#14675) (microsoft#14951)" (microsoft#14968) This reverts commit 651a6b9.

ericsnowcurrently mentioned this pull request Dec 15, 2020

WindowsPathEnvVarLocator is causing extension activation to hang. #14983

Closed

Conversation

ericsnowcurrently commented Nov 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Nov 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sonarqubecloud Bot commented Nov 10, 2020

Uh oh!

karthiknadig commented Nov 11, 2020

Uh oh!

karrtikr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karrtikr Dec 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericsnowcurrently Dec 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericsnowcurrently Dec 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karrtikr Dec 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karrtikr Dec 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

karthiknadig left a comment

Choose a reason for hiding this comment

Uh oh!

karrtikr left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

ericsnowcurrently commented Nov 9, 2020 •

edited

Loading

codecov-io commented Nov 10, 2020 •

edited

Loading

karrtikr Dec 8, 2020 •

edited

Loading

ericsnowcurrently Dec 10, 2020 •

edited

Loading

ericsnowcurrently Dec 10, 2020 •

edited

Loading

karrtikr Dec 10, 2020 •

edited

Loading

karrtikr Dec 10, 2020 •

edited

Loading