Added Windows store file system watcher #14577

karrtikr · 2020-10-29T23:06:25Z

No description provided.

codecov-io · 2020-10-29T23:32:54Z

Codecov Report

Merging #14577 into main will decrease coverage by 0.02%.
The diff coverage is 59.09%.

@@            Coverage Diff             @@
##             main   #14577      +/-   ##
==========================================
- Coverage   64.91%   64.88%   -0.03%     
==========================================
  Files         541      541              
  Lines       25131    25156      +25     
  Branches     3544     3545       +1     
==========================================
+ Hits        16313    16323      +10     
- Misses       8155     8171      +16     
+ Partials      663      662       -1

Impacted Files	Coverage Δ
...discovery/locators/services/windowsStoreLocator.ts	`86.79% <58.82%> (-13.21%)`	⬇️
...pythonEnvironments/common/pythonBinariesWatcher.ts	`61.53% <60.00%> (+1.53%)`	⬆️
src/client/common/utils/platform.ts	`68.00% <0.00%> (-4.00%)`	⬇️
...rc/client/common/process/internal/scripts/index.ts	`65.85% <0.00%> (-3.89%)`	⬇️
src/client/common/experiments/groups.ts	`100.00% <0.00%> (ø)`
...c/client/application/misc/joinMailingListPrompt.ts	`97.56% <0.00%> (ø)`
...ocators/services/globalVirtualEnvronmentLocator.ts	`96.42% <0.00%> (+0.04%)`	⬆️
src/client/terminals/codeExecution/helper.ts	`14.58% <0.00%> (+1.53%)`	⬆️
.../pythonEnvironments/common/externalDependencies.ts	`67.56% <0.00%> (+2.70%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ed0dad5...a1dcd0a. Read the comment docs.

src/client/pythonEnvironments/common/pythonBinariesWatcher.ts

karrtikr · 2020-10-30T16:47:34Z

src/client/pythonEnvironments/discovery/locators/services/windowsStoreLocator.ts

+ * python2.7.exe
+ * python3.exe
+ * python38.exe
+ * Note the below is constructed in a way that it can be used as a glob pattern, or as a regex.


src/client/pythonEnvironments/common/pythonBinariesWatcher.ts

ericsnowcurrently

The major comments:

why are we using globs instead of regexes?
do not start the watcher in the constructor

src/client/pythonEnvironments/common/pythonBinariesWatcher.ts

src/client/pythonEnvironments/discovery/locators/services/windowsStoreLocator.ts

src/client/pythonEnvironments/common/pythonBinariesWatcher.ts

src/client/pythonEnvironments/discovery/locators/services/windowsStoreLocator.ts

karrtikr · 2020-11-03T01:56:53Z

@ericsnowcurrently The glob module that we use is built on top of minimatch, so that one is a dependency freebie. We also use that in the extension. Hence directly used that in favor of picomatch to avoid the disclaimer comments, converting regex to glob and then vice versa without any additional cost. Will add the disposable and other base class changes in a separate PR.

ericsnowcurrently

Overall this is looking good. Thanks for the fixes. I've left a few minor comments in the non-test files. Otherwise all the rest of the comments are in the test files.

src/client/pythonEnvironments/common/pythonBinariesWatcher.ts

src/client/pythonEnvironments/discovery/locators/services/windowsStoreLocator.ts

ericsnowcurrently · 2020-11-03T15:34:06Z

src/client/pythonEnvironments/common/pythonBinariesWatcher.ts

 ): void {
+    const patterns = [executableGlob, `*/${executableGlob}`, `*/${binName}/${executableGlob}`];


It would be helpful to callers to throw an error here if executableGlob has path.separator in it. I still think a similar check for wildcards (*) would be likewise helpful.

I went with your initial suggestion to expand the glob pattern to support more than base patterns. #14577 (comment)

So we don't have to put a restriction on the wildcards or path separators. I can rename it to executableSuffixGlob if that would help.

I went with your initial suggestion to expand the glob pattern to support more than base patterns.

I don't see that. What I see is:

const pythonExeGlob = 'python3\.[0-9]*\.exe';

A more-than-base pattern would be:

const pythonExeGlob = '**/python3\.[0-9]*\.exe';

or:

const pythonExeGlob = path.join('**', 'python3\.[0-9]*\.exe');

I'm talking about the API, not this particular usecase. The API supports more than base patterns, i.e user can pass in a path.join('**', 'python3\.[0-9]*\.exe') and things should work. Just needed to do this here.

path.join('**', executablesGlob) is exactly what I would expect this function to do for the caller.

I think you're assuming executablesGlob to be the base executable glob, it's not. It refers to the suffix of the full executable path we intend to watch. For eg. it can be path.join('**', 'python3\.[0-9]*\.exe') itself, so the function need not do anything for the caller.

Hence why I suggested to rename it to executablesSuffixGlob.

The patterns on this line suggest otherwise:

executableGlob - look for the file in the current directory

*/${executableGlob} - look for the file one level down

*/${binName}/${executableGlob} - look for the file in the "bin" directory one level down

What you seen to be saying is that a caller may want to search at some arbitrary depth relative to a given root:

**/python3.exe

*/**/python3.exe

*/${binName}/**/python3.exe

However, those don't seem right to me. Perhaps you meant a user might want to do something like the following?

**/python3.exe

**/*/python3.exe

**/*/${binName}/python3.exe

The problem is that kind of defeats the structure-oriented design of this function. So I don't see that to be right either.

All this is why I would expect executableGlob to always be a base name.

What you seen to be saying is that a caller may want to search at some arbitrary depth relative to a given root:

**/python3.exe

*/**/python3.exe

*/${binName}/**/python3.exe
However, those don't seem right to me.

I realize it doesn't seem right. But if you look closely, the first pattern is enough to do what the user asks (watch for suffixes ending with python3.exe). And the second and third pattern are just subsets of the first. So yes, they do what the user intended to.

That is only true if the glob is for the basename. If they put **/... then it won't be what they expect. If it shouldn't matter then this function should not be auto-watching */${executableGlob} and */${binName}/${executableGlob}. The two things just don't make sense together.

src/client/pythonEnvironments/discovery/locators/services/windowsStoreLocator.ts

src/test/pythonEnvironments/discovery/locators/windowsStoreLocator.test.ts

ericsnowcurrently

I only have a few more minor comments. So...

LGTM once all the open comments have been addressed.

ericsnowcurrently · 2020-11-03T22:01:09Z

src/client/pythonEnvironments/discovery/locators/services/windowsStoreLocator.ts

+export function isWindowsStorePythonExe(interpreterPath: string): boolean {
+    return minimatch(path.basename(interpreterPath), pythonExeGlob, { nocase: true });
 }


Why aren't we just using isWindowsPythonExe()?

Because of the reason mentioned in the sprint demo today: We're matching only python3.*.exes so that we do collect only unique executables. isWindowsPythonExe() also matches with python.exe which is not what we want.

The doc comment explains this.

Then I'd expect the following to take care of it:

Suggested change

export function isWindowsStorePythonExe(interpreterPath: string): boolean {

return minimatch(path.basename(interpreterPath), pythonExeGlob, { nocase: true });

}

export function isWindowsStorePythonExe(interpreterPath: string): boolean {

if (!isWindowsPythonExe(interpreterPath)) {

return false;

}

return path.basename(interpreterPath) !== 'python.exe';

}

That way we don't accept executables we shouldn't, like "python3-whoa!!!.exe".

Right, there's also python3.exe, and python2.exe that we would need to ignore. I can either do this or correct the glob pattern to not accept the whoaaa string. I'll have to correct the glob either way so we don't watch for such executables.

Ah, so we're okay with "extended glob". What you have looks fine, but I'd change the * to ?.

src/client/pythonEnvironments/discovery/locators/services/windowsStoreLocator.ts

ericsnowcurrently · 2020-11-03T22:04:36Z

src/client/pythonEnvironments/discovery/locators/services/windowsStoreLocator.ts

+        const windowsAppsRoot = getWindowsStoreAppsRoot();
+        watchLocationForPythonBinaries(windowsAppsRoot, (type: FileChangeType) => {
+            this.emitter.fire({ type, kind: this.kind });
+        }, pythonExeGlob);


Note that this is really the only place that we need pythonExeGlob. So it may make sense to move it inline here.

We need it in isWindowsStorePythonExe as well.

If we use isWindowsPythonExe() there then only need it here.

But then we would have two places where we capture what a windows store executable is? The glob pattern itself and isWindowsStorePythonExe.

If we only use the glob, that way we have the logic in one central place.

sonarqubecloud · 2020-11-03T22:16:00Z

Kudos, SonarCloud Quality Gate passed!

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

No Coverage information
0.0% Duplication

karrtikr · 2020-11-03T22:17:53Z

@ericsnowcurrently I believe I have addressed all the comments. I generally like to keep comments open for the reviewers to resolve them themselves. Hope I addressed most of your concerns.

Added Windows store file system watcher

ec02410

karrtikr added the no-changelog No news entry required label Oct 29, 2020

karrtikr requested review from ericsnowcurrently and int19h October 29, 2020 23:06

int19h reviewed Oct 30, 2020

View reviewed changes

src/client/pythonEnvironments/common/pythonBinariesWatcher.ts Outdated Show resolved Hide resolved

karrtikr commented Oct 30, 2020

View reviewed changes

Use glob pattern matcher instead of regex

1461297

karrtikr force-pushed the windowsappswatcher branch from bf48246 to 1461297 Compare October 30, 2020 17:57

int19h reviewed Oct 30, 2020

View reviewed changes

src/client/pythonEnvironments/common/pythonBinariesWatcher.ts Outdated Show resolved Hide resolved

int19h approved these changes Oct 30, 2020

View reviewed changes

Code reviews

73fc23e

ericsnowcurrently suggested changes Nov 2, 2020

View reviewed changes

Code reviews

d4818ea

karrtikr force-pushed the windowsappswatcher branch from 3994c45 to d4818ea Compare November 2, 2020 22:58

Remove picomatch and use already existing minimatch

feb7f18

karrtikr requested a review from ericsnowcurrently November 3, 2020 01:57

ericsnowcurrently suggested changes Nov 3, 2020

View reviewed changes

Kartik Raj added 4 commits November 3, 2020 12:27

Code reviews

595cc32

Correct python exe glob for python3.10

a7b1753

Add a comment stating why we only capture python3.*.exes

ad31943

Refactor locator setup out of tests

79b25d1

karrtikr requested a review from ericsnowcurrently November 3, 2020 21:22

Kartik Raj added 2 commits November 3, 2020 13:38

Correct glob

f3262ac

Oops

4cde6df

ericsnowcurrently approved these changes Nov 3, 2020

View reviewed changes

Cleanly indent

a1dcd0a

karrtikr merged commit f752706 into microsoft:main Nov 3, 2020

karrtikr deleted the windowsappswatcher branch November 3, 2020 23:17

karrtikr mentioned this pull request Nov 4, 2020

Added FSWatching base class and made related changes #14605

Merged

		): void {
		const patterns = [executableGlob, `/${executableGlob}`, `/${binName}/${executableGlob}`];

Added Windows store file system watcher #14577

Added Windows store file system watcher #14577

Uh oh!

Conversation

karrtikr commented Oct 29, 2020

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ericsnowcurrently 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!

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

Uh oh!

Uh oh!

ericsnowcurrently left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karrtikr Nov 3, 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 Nov 3, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ericsnowcurrently left a comment

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!

Choose a reason for hiding this comment

Uh oh!

karrtikr Nov 4, 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!

Uh oh!

codecov-io commented Oct 29, 2020 •

edited

Loading

karrtikr commented Nov 3, 2020 •

edited

Loading

karrtikr Nov 3, 2020 •

edited

Loading

karrtikr Nov 3, 2020 •

edited

Loading

karrtikr Nov 4, 2020 •

edited

Loading

karrtikr Nov 4, 2020 •

edited

Loading