Skip to content

Commit

Permalink
Cherry pic fixes into release for tests. (#14673)
Browse files Browse the repository at this point in the history
* Added FSWatching base class and made related changes (#14605)

* Add FSWatching locator base class

* Correct glob pattern to not match python3.2whoa

* Add documentation of python binary watcher

* Fix lint errors

* Update ignore list

* Add disposable registry

* Modify FSWatching Locator

* Code reviews

* Use string[]

* Remove list disposable getter

* Fix failing global virtual env watcher tests (#14633)

Co-authored-by: Kartik Raj <karraj@microsoft.com>
  • Loading branch information
karthiknadig and Kartik Raj authored Nov 9, 2020
1 parent 2c2b7e1 commit 5eab81d
Show file tree
Hide file tree
Showing 9 changed files with 204 additions and 64 deletions.
46 changes: 31 additions & 15 deletions src/client/common/platform/fileSystemWatcher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import * as chokidar from 'chokidar';
import * as path from 'path';
import { RelativePattern, workspace } from 'vscode';
import { traceError, traceVerbose, traceWarning } from '../logger';
import { DisposableRegistry } from '../syncDisposableRegistry';
import { IDisposable } from '../types';
import { normCasePath } from './fs-paths';

/**
Expand All @@ -23,35 +25,36 @@ export function watchLocationForPattern(
baseDir: string,
pattern: string,
callback: (type: FileChangeType, absPath: string) => void
): void {
): IDisposable {
// Use VSCode API iff base directory to exists within the current workspace folders
const found = workspace.workspaceFolders?.find((e) => normCasePath(baseDir).startsWith(normCasePath(e.uri.fsPath)));
if (found) {
watchLocationUsingVSCodeAPI(baseDir, pattern, callback);
} else {
// Fallback to chokidar as base directory to lookup doesn't exist within the current workspace folders
watchLocationUsingChokidar(baseDir, pattern, callback);
return watchLocationUsingVSCodeAPI(baseDir, pattern, callback);
}
// Fallback to chokidar as base directory to lookup doesn't exist within the current workspace folders
return watchLocationUsingChokidar(baseDir, pattern, callback);
}

function watchLocationUsingVSCodeAPI(
baseDir: string,
pattern: string,
callback: (type: FileChangeType, absPath: string) => void
) {
): IDisposable {
const globPattern = new RelativePattern(baseDir, pattern);
const disposables = new DisposableRegistry();
traceVerbose(`Start watching: ${baseDir} with pattern ${pattern} using VSCode API`);
const watcher = workspace.createFileSystemWatcher(globPattern);
watcher.onDidCreate((e) => callback(FileChangeType.Created, e.fsPath));
watcher.onDidChange((e) => callback(FileChangeType.Changed, e.fsPath));
watcher.onDidDelete((e) => callback(FileChangeType.Deleted, e.fsPath));
disposables.push(watcher.onDidCreate((e) => callback(FileChangeType.Created, e.fsPath)));
disposables.push(watcher.onDidChange((e) => callback(FileChangeType.Changed, e.fsPath)));
disposables.push(watcher.onDidDelete((e) => callback(FileChangeType.Deleted, e.fsPath)));
return disposables;
}

function watchLocationUsingChokidar(
baseDir: string,
pattern: string,
callback: (type: FileChangeType, absPath: string) => void
) {
): IDisposable {
const watcherOpts: chokidar.WatchOptions = {
cwd: baseDir,
ignoreInitial: true,
Expand All @@ -69,7 +72,9 @@ function watchLocationUsingChokidar(
'**/.hg/store/**',
'/dev/**',
'/proc/**',
'/sys/**'
'/sys/**',
'**/lib/**',
'**/includes/**'
], // https://github.com/microsoft/vscode/issues/23954
followSymlinks: false
};
Expand All @@ -96,6 +101,18 @@ function watchLocationUsingChokidar(
callback(eventType, absPath);
});

const stopWatcher = async () => {
if (watcher) {
const obj = watcher;
watcher = null;
try {
await obj.close();
} catch (err) {
traceError(`Failed to close FS watcher (${err})`);
}
}
};

watcher.on('error', async (error: NodeJS.ErrnoException) => {
if (error) {
// Specially handle ENOSPC errors that can happen when
Expand All @@ -105,13 +122,12 @@ function watchLocationUsingChokidar(
// See https://github.com/Microsoft/vscode/issues/7950
if (error.code === 'ENOSPC') {
traceError(`Inotify limit reached (ENOSPC) for ${baseDir} with pattern ${pattern}`);
if (watcher) {
await watcher.close();
watcher = null;
}
await stopWatcher();
} else {
traceWarning(error.toString());
}
}
});

return { dispose: () => stopWatcher().ignoreErrors() };
}
31 changes: 31 additions & 0 deletions src/client/common/syncDisposableRegistry.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

'use strict';

import { traceWarning } from './logger';
import { IDisposable } from './types';

/**
* Responsible for disposing a list of disposables synchronusly.
*/
export class DisposableRegistry implements IDisposable {
private _list: IDisposable[] = [];

public dispose(): void {
this._list.forEach((l, index) => {
try {
l.dispose();
} catch (err) {
traceWarning(`dispose() #${index} failed (${err})`);
}
});
this._list = [];
}

public push(disposable?: IDisposable): void {
if (disposable) {
this._list.push(disposable);
}
}
}
18 changes: 15 additions & 3 deletions src/client/pythonEnvironments/base/locator.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

// eslint-disable-next-line max-classes-per-file
import { Event, Uri } from 'vscode';
import { DisposableRegistry } from '../../common/syncDisposableRegistry';
import { IDisposable } from '../../common/types';
import { iterEmpty } from '../../common/utils/async';
import { PythonEnvInfo, PythonEnvKind } from './info';
import {
Expand Down Expand Up @@ -178,19 +181,28 @@ interface IEmitter<E extends BasicPythonEnvsChangedEvent> {
* should be used. Only in low-level cases should you consider using
* `BasicPythonEnvsChangedEvent`.
*/
export abstract class LocatorBase<E extends BasicPythonEnvsChangedEvent = PythonEnvsChangedEvent> implements ILocator<E> {
abstract class LocatorBase<E extends BasicPythonEnvsChangedEvent = PythonEnvsChangedEvent>
implements IDisposable, ILocator<E> {
public readonly onChanged: Event<E>;
protected readonly emitter: IEmitter<E>;

protected readonly emitter: IPythonEnvsWatcher<E> & IEmitter<E>;

protected readonly disposables = new DisposableRegistry();

constructor(watcher: IPythonEnvsWatcher<E> & IEmitter<E>) {
this.emitter = watcher;
this.onChanged = watcher.onChanged;
this.onChanged = this.emitter.onChanged;
}

public abstract iterEnvs(query?: QueryForEvent<E>): IPythonEnvsIterator;

public async resolveEnv(_env: string | PythonEnvInfo): Promise<PythonEnvInfo | undefined> {
return undefined;
}

public dispose(): void {
this.disposables.dispose();
}
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
// 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';

/**
* The base for Python envs locators who watch the file system.
* Most low-level locators should be using this.
*
* Subclasses can call `this.emitter.fire()` * to emit events.
*/
export abstract class FSWatchingLocator extends Locator {
private initialized = false;

constructor(
/**
* Location(s) to watch for python binaries.
*/
private readonly getRoots: () => Promise<string[]> | string | string[],
/**
* 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 handling an environment-created event.
*/
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,
),
);
}
}
24 changes: 18 additions & 6 deletions src/client/pythonEnvironments/common/pythonBinariesWatcher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,25 +6,37 @@
import * as minimatch from 'minimatch';
import * as path from 'path';
import { FileChangeType, watchLocationForPattern } from '../../common/platform/fileSystemWatcher';
import { DisposableRegistry } from '../../common/syncDisposableRegistry';
import { IDisposable } from '../../common/types';
import { getOSType, OSType } from '../../common/utils/platform';

const [executable, binName] = getOSType() === OSType.Windows ? ['python.exe', 'Scripts'] : ['python', 'bin'];

/**
* @param baseDir The base directory from which watch paths are to be derived.
* @param callback The listener function will be called when the event happens.
* @param executableBaseGlob Glob which represents basename of the executable to watch.
*/
export function watchLocationForPythonBinaries(
baseDir: string,
callback: (type: FileChangeType, absPath: string) => void,
executableGlob: string = executable,
): void {
const patterns = [executableGlob, `*/${executableGlob}`, `*/${binName}/${executableGlob}`];
executableBaseGlob: string = executable,
): IDisposable {
if (executableBaseGlob.includes(path.sep)) {
throw new Error('Glob basename contains invalid characters');
}
const patterns = [executableBaseGlob, `*/${executableBaseGlob}`, `*/${binName}/${executableBaseGlob}`];
const disposables = new DisposableRegistry();
for (const pattern of patterns) {
watchLocationForPattern(baseDir, pattern, (type: FileChangeType, e: string) => {
const isMatch = minimatch(e, path.join('**', executableGlob), { nocase: getOSType() === OSType.Windows });
disposables.push(watchLocationForPattern(baseDir, pattern, (type: FileChangeType, e: string) => {
const isMatch = minimatch(path.basename(e), executableBaseGlob, { nocase: getOSType() === OSType.Windows });
if (!isMatch) {
// When deleting the file for some reason path to all directories leading up to python are reported
// Skip those events
return;
}
callback(type, e);
});
}));
}
return disposables;
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,16 @@
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';
import { PythonEnvInfo, PythonEnvKind, UNKNOWN_PYTHON_VERSION } from '../../../base/info';
import { buildEnvInfo } from '../../../base/info/env';
import { IPythonEnvsIterator, Locator } from '../../../base/locator';
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 @@ -80,17 +79,22 @@ async function getVirtualEnvKind(interpreterPath: string): Promise<PythonEnvKind
/**
* Finds and resolves virtual environments created in known global locations.
*/
export class GlobalVirtualEnvironmentLocator extends Locator {
export class GlobalVirtualEnvironmentLocator extends FSWatchingLocator {
private virtualEnvKinds = [
PythonEnvKind.Venv,
PythonEnvKind.VirtualEnv,
PythonEnvKind.VirtualEnvWrapper,
PythonEnvKind.Pipenv,
];

public constructor(private readonly searchDepth?: number) {
super();
this.registerWatchers().ignoreErrors();
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 @@ -167,16 +171,4 @@ export class GlobalVirtualEnvironmentLocator extends Locator {
}
return undefined;
}

private async registerWatchers(): Promise<void> {
const dirs = await getGlobalVirtualEnvDirs();
dirs.forEach((d) => 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 });
}));
}
}
Loading

0 comments on commit 5eab81d

Please sign in to comment.