Skip to content

Added FSWatching base class and made related changes #14605

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Kartik Raj (karrtikr) merged 10 commits into from
Nov 6, 2020
Merged

Added FSWatching base class and made related changes #14605

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
ad675c1
Add FSWatching locator base class
Nov 4, 2020
06e6f5b
Correct glob pattern to not match python3.2whoa
Nov 4, 2020
c167f57
Add documentation of python binary watcher
Nov 4, 2020
375df6b
Fix lint errors
Nov 4, 2020
3ef70e3
Update ignore list
Nov 4, 2020
5925ab1
Add disposable registry
Nov 5, 2020
31f7c54
Modify FSWatching Locator
Nov 5, 2020
fec8c1b
Code reviews
Nov 5, 2020
21b740a
Use string[]
Nov 5, 2020
2260b17
Remove list disposable getter
Nov 5, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Modify FSWatching Locator
  • Loading branch information
Kartik Raj
Kartik Raj committed Nov 5, 2020
commit 31f7c543282d7ab0fda76c10f6b8bf3451b76ad4
2 changes: 1 addition & 1 deletion src/client/pythonEnvironments/base/locator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ interface IEmitter<E extends BasicPythonEnvsChangedEvent> {
* By default `resolveEnv()` returns undefined. Subclasses may override
* the method to provide an implementation.
*
* Subclasses will call `this.emitter.fire()` * to emit events.
* Subclasses will call `this.emitter.fire()` to emit events.
*
* In most cases this is the class you will want to subclass.
* Only in low-level cases should you consider subclassing `LocatorBase`
Expand Down
70 changes: 69 additions & 1 deletion src/client/pythonEnvironments/base/locators/lowLevel/fsWatchingLocator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

import { FileChangeType } from '../../../../common/platform/fileSystemWatcher';
import { sleep } from '../../../../common/utils/async';
import { watchLocationForPythonBinaries } from '../../../common/pythonBinariesWatcher';
import { PythonEnvKind } from '../../info';
import { Locator } from '../../locator';

/**
Expand All @@ -10,5 +14,69 @@ import { Locator } from '../../locator';
* Subclasses can call `this.emitter.fire()` * to emit events.
*/
export abstract class FSWatchingLocator extends Locator {
public async abstract initialize(): Promise<void>;
private initialized = false;

constructor(
/**
* Location(s) to watch for python binaries.
*/
private readonly getRoots: () => Promise<string[]> | string,

@ericsnowcurrently Eric Snow (ericsnowcurrently) Nov 5, 2020

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private readonly getRoots: () => Promise<string[]> | string,
private readonly getRoots: () => Promise<string[]> | string[],

@karrtikr Kartik Raj (karrtikr) Nov 5, 2020 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it's getRoot(s) as explained in the doc. Not every locator has an array of locations they want to search (getWindowsAppsRoot), some just have a single location, so this is just convenience.

@karrtikr Kartik Raj (karrtikr) Nov 5, 2020

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added the string[] type but not removed string.

@ericsnowcurrently Eric Snow (ericsnowcurrently) Nov 9, 2020

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If they have a single location then they return a single element array. Returning string means that the caller always has to check typeof result === 'string'. So it makes things simpler if we only return Promise<string[]> | string[].

(Also note that if you support return string then the return type should be Promise<string[] | string> | string[] | string. However, as I said, I don't think the distinction between string[] and string adds any value while it increases complexity.

@karrtikr Kartik Raj (karrtikr) Nov 9, 2020

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning string means that the caller always has to check typeof result === 'string'

I don't think so.. not sure why you say that. The caller for the "string" case here just passes in

constructor() {
    super(
        getWindowsStoreAppsRoot,
        async () => this.kind,
        { executableBaseGlob: pythonExeGlob },
    );
}

While in string[] case

constructor() {
    super(
        () => [getWindowsStoreAppsRoot()],
        async () => this.kind,
        { executableBaseGlob: pythonExeGlob },
    );
}

So adding the string type decreases complexity for the caller. Note this is just convenience.

@ericsnowcurrently Eric Snow (ericsnowcurrently) Nov 9, 2020

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see the problem. I was expecting it to return one of the following (and I believe it does):

  • () => Promise<string[]>
  • () => string[]
  • () => string

While you appear to be thinking it is:

  • () => Promise<string[]>
  • string[]
  • string

I'm pretty sure the correct form to also meet your expectations is:

string[] | string | () => Promise<string[]> | string[]

Though parentheses might be needed:

string[] | string | (() => Promise<string[]> | string[])

FWIW, I agree that the string[] | string types are convenient and should be okay.

/**
* Returns the kind of environment specific to locator given the path to exectuable.
*/
private readonly getKind: (executable: string) => Promise<PythonEnvKind>,
private readonly opts: {
/**
* Glob which represents basename of the executable to watch.
*/
executableBaseGlob?: string,
/**
* Time to wait before attempting to identify kind when detected that an environment has been created.
Comment thread
karrtikr marked this conversation as resolved.
Resolved
*/
delayOnCreated?: number, // milliseconds
} = {},
) {
super();
}

public async initialize(): Promise<void> {
if (this.initialized) {
return;
}
this.initialized = true;
await this.startWatchers();
}

public dispose(): void {
super.dispose();
this.initialized = false;
}

private async startWatchers(): Promise<void> {
let roots = await this.getRoots();
if (typeof roots === 'string') {
roots = [roots];
}
roots.forEach((root) => this.startWatcher(root));
}

private startWatcher(root: string): void {
this.disposables.push(
watchLocationForPythonBinaries(
root,
async (type: FileChangeType, executable: string) => {
if (type === FileChangeType.Created) {
if (this.opts.delayOnCreated !== undefined) {
// Note detecting kind of env depends on the file structure around the
// executable, so we need to wait before attempting to detect it.
await sleep(this.opts.delayOnCreated);
}
}
const kind = await this.getKind(executable);
this.emitter.fire({ type, kind });
},
this.opts.executableBaseGlob,
),
);
}
}
35 changes: 9 additions & 26 deletions src/client/pythonEnvironments/discovery/locators/services/globalVirtualEnvronmentLocator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,7 @@
import { uniq } from 'lodash';
import * as path from 'path';
import { traceVerbose } from '../../../../common/logger';
import { FileChangeType } from '../../../../common/platform/fileSystemWatcher';
import { chain, iterable, sleep } from '../../../../common/utils/async';
import { chain, iterable } from '../../../../common/utils/async';
import {
getEnvironmentVariable, getOSType, getUserHomeDir, OSType
} from '../../../../common/utils/platform';
Expand All @@ -15,7 +14,6 @@ import { IPythonEnvsIterator } from '../../../base/locator';
import { FSWatchingLocator } from '../../../base/locators/lowLevel/fsWatchingLocator';
import { findInterpretersInDir } from '../../../common/commonUtils';
import { getFileInfo, pathExists } from '../../../common/externalDependencies';
import { watchLocationForPythonBinaries } from '../../../common/pythonBinariesWatcher';
import { isPipenvEnvironment } from './pipEnvHelper';
import {
isVenvEnvironment,
Expand Down Expand Up @@ -89,12 +87,14 @@ export class GlobalVirtualEnvironmentLocator extends FSWatchingLocator {
PythonEnvKind.Pipenv,
];

public constructor(private readonly searchDepth?: number) {
super();
}

public async initialize(): Promise<void> {
await this.startWatchers();
constructor(private readonly searchDepth?: number) {
super(getGlobalVirtualEnvDirs, getVirtualEnvKind, {
// Note detecting kind of virtual env depends on the file structure around the
// executable, so we need to wait before attempting to detect it. However even
// if the type detected is incorrect, it doesn't do any practical harm as kinds
// in this locator are used in the same way (same activation commands etc.)
delayOnCreated: 1000,
});
}

public iterEnvs(): IPythonEnvsIterator {
Expand Down Expand Up @@ -171,21 +171,4 @@ export class GlobalVirtualEnvironmentLocator extends FSWatchingLocator {
}
return undefined;
}

private async startWatchers(): Promise<void> {
const dirs = await getGlobalVirtualEnvDirs();
dirs.forEach(
(d) => this.disposables.push(
watchLocationForPythonBinaries(d, async (type: FileChangeType, executablePath: string) => {
// Note detecting kind of virtual env depends on the file structure around the
// executable, so we need to wait before attempting to detect it. However even
// if the type detected is incorrect, it doesn't do any practical harm as kinds
// in this locator are used in the same way (same activation commands etc.)
await sleep(1000);
const kind = await getVirtualEnvKind(executablePath);
this.emitter.fire({ type, kind });
}),
),
);
}
}
23 changes: 6 additions & 17 deletions src/client/pythonEnvironments/discovery/locators/services/windowsStoreLocator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,13 @@ import * as fsapi from 'fs-extra';
import * as minimatch from 'minimatch';
import * as path from 'path';
import { traceWarning } from '../../../../common/logger';
import { FileChangeType } from '../../../../common/platform/fileSystemWatcher';
import { Architecture, getEnvironmentVariable } from '../../../../common/utils/platform';
import { PythonEnvInfo, PythonEnvKind } from '../../../base/info';
import { buildEnvInfo } from '../../../base/info/env';
import { getPythonVersionFromPath } from '../../../base/info/pythonVersion';
import { IPythonEnvsIterator } from '../../../base/locator';
import { FSWatchingLocator } from '../../../base/locators/lowLevel/fsWatchingLocator';
import { getFileInfo } from '../../../common/externalDependencies';
import { watchLocationForPythonBinaries } from '../../../common/pythonBinariesWatcher';

/**
* Gets path to the Windows Apps directory.
Expand Down Expand Up @@ -143,8 +141,12 @@ export async function getWindowsStorePythonExes(): Promise<string[]> {
export class WindowsStoreLocator extends FSWatchingLocator {
private readonly kind: PythonEnvKind = PythonEnvKind.WindowsStore;

public async initialize(): Promise<void> {
this.startWatcher();
constructor() {
super(
getWindowsStoreAppsRoot,
async () => this.kind,
{ executableBaseGlob: pythonExeGlob },
);
}

public iterEnvs(): IPythonEnvsIterator {
Expand Down Expand Up @@ -176,17 +178,4 @@ export class WindowsStoreLocator extends FSWatchingLocator {
}
return undefined;
}

private startWatcher(): void {
const windowsAppsRoot = getWindowsStoreAppsRoot();
this.disposables.push(
watchLocationForPythonBinaries(
windowsAppsRoot,
(type: FileChangeType) => {
this.emitter.fire({ type, kind: this.kind });
},
pythonExeGlob,
),
);
}
}