From 5b6955abf091fc0e9e182e751083278db3e8d6a5 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 9 Dec 2019 16:44:09 -0700 Subject: [PATCH 01/26] Use IFileSystem.fileExists() instead of fsextra.pathExists(). --- .../common/application/webPanels/webPanel.ts | 8 +++++--- .../application/webPanels/webPanelProvider.ts | 9 ++++++--- src/client/common/envFileParser.ts | 16 ++++++++++++++-- src/client/common/variables/environment.ts | 14 +++++++++----- .../locators/services/windowsRegistryService.ts | 10 ++++++---- src/client/providers/jediProxy.ts | 5 +++-- .../variables/envVarsProvider.multiroot.test.ts | 4 +++- .../common/variables/envVarsService.unit.test.ts | 7 ++++++- 8 files changed, 52 insertions(+), 21 deletions(-) diff --git a/src/client/common/application/webPanels/webPanel.ts b/src/client/common/application/webPanels/webPanel.ts index 9c8286960f20..140de7a986cd 100644 --- a/src/client/common/application/webPanels/webPanel.ts +++ b/src/client/common/application/webPanels/webPanel.ts @@ -3,13 +3,13 @@ 'use strict'; import '../../extensions'; -import * as fs from 'fs-extra'; import * as uuid from 'uuid/v4'; import { Uri, Webview, WebviewPanel, window } from 'vscode'; import { Identifiers } from '../../../datascience/constants'; import { InteractiveWindowMessages } from '../../../datascience/interactive-common/interactiveWindowTypes'; import { SharedMessages } from '../../../datascience/messages'; +import { IFileSystem } from '../../platform/types'; import { IDisposableRegistry } from '../../types'; import * as localize from '../../utils/localize'; import { IWebPanel, IWebPanelOptions, WebPanelMessage } from '../types'; @@ -26,10 +26,12 @@ export class WebPanel implements IWebPanel { private id = uuid(); constructor( + private fs: IFileSystem, private disposableRegistry: IDisposableRegistry, private port: number | undefined, private token: string | undefined, - private options: IWebPanelOptions) { + private options: IWebPanelOptions + ) { this.panel = window.createWebviewPanel( options.title.toLowerCase().replace(' ', ''), options.title, @@ -86,7 +88,7 @@ export class WebPanel implements IWebPanel { // tslint:disable-next-line:no-any private async load() { if (this.panel) { - const localFilesExist = await Promise.all(this.options.scripts.map(s => fs.pathExists(s))); + const localFilesExist = await Promise.all(this.options.scripts.map(s => this.fs.fileExists(s))); if (localFilesExist.every(exists => exists === true)) { // Call our special function that sticks this script inside of an html page diff --git a/src/client/common/application/webPanels/webPanelProvider.ts b/src/client/common/application/webPanels/webPanelProvider.ts index c6a8b416e111..3d6b04fc8a07 100644 --- a/src/client/common/application/webPanels/webPanelProvider.ts +++ b/src/client/common/application/webPanels/webPanelProvider.ts @@ -5,6 +5,7 @@ import { inject, injectable } from 'inversify'; import * as portfinder from 'portfinder'; import * as uuid from 'uuid/v4'; +import { IFileSystem } from '../../platform/types'; import { IDisposableRegistry } from '../../types'; import { IWebPanel, IWebPanelOptions, IWebPanelProvider } from '../types'; import { WebPanel } from './webPanel'; @@ -15,13 +16,15 @@ export class WebPanelProvider implements IWebPanelProvider { private port: number | undefined; private token: string | undefined; - constructor(@inject(IDisposableRegistry) private disposableRegistry: IDisposableRegistry) { - } + constructor( + @inject(IDisposableRegistry) private disposableRegistry: IDisposableRegistry, + @inject(IFileSystem) private fs: IFileSystem + ) { } // tslint:disable-next-line:no-any public async create(options: IWebPanelOptions): Promise { const serverData = options.startHttpServer ? await this.ensureServerIsRunning() : { port: undefined, token: undefined }; - return new WebPanel(this.disposableRegistry, serverData.port, serverData.token, options); + return new WebPanel(this.fs, this.disposableRegistry, serverData.port, serverData.token, options); } private async ensureServerIsRunning(): Promise<{ port: number; token: string }> { diff --git a/src/client/common/envFileParser.ts b/src/client/common/envFileParser.ts index f1cfda52b430..ee180eeae87f 100644 --- a/src/client/common/envFileParser.ts +++ b/src/client/common/envFileParser.ts @@ -1,6 +1,8 @@ import * as fs from 'fs-extra'; import { IS_WINDOWS } from './platform/constants'; +import { FileSystem } from './platform/fileSystem'; import { PathUtils } from './platform/pathUtils'; +import { PlatformService } from './platform/platformService'; import { EnvironmentVariablesService } from './variables/environment'; import { EnvironmentVariables } from './variables/types'; function parseEnvironmentVariables(contents: string): EnvironmentVariables | undefined { @@ -37,7 +39,12 @@ export function parseEnvFile(envFile: string, mergeWithProcessEnvVars: boolean = * @returns {EnvironmentVariables} */ export function mergeEnvVariables(targetEnvVars: EnvironmentVariables, sourceEnvVars: EnvironmentVariables = process.env): EnvironmentVariables { - const service = new EnvironmentVariablesService(new PathUtils(IS_WINDOWS)); + const service = new EnvironmentVariablesService( + new PathUtils(IS_WINDOWS), + new FileSystem( + new PlatformService() + ) + ); service.mergeVariables(sourceEnvVars, targetEnvVars); if (sourceEnvVars.PYTHONPATH) { service.appendPythonPath(targetEnvVars, sourceEnvVars.PYTHONPATH); @@ -57,7 +64,12 @@ export function mergePythonPath(env: EnvironmentVariables, currentPythonPath: st if (typeof currentPythonPath !== 'string' || currentPythonPath.length === 0) { return env; } - const service = new EnvironmentVariablesService(new PathUtils(IS_WINDOWS)); + const service = new EnvironmentVariablesService( + new PathUtils(IS_WINDOWS), + new FileSystem( + new PlatformService() + ) + ); service.appendPythonPath(env, currentPythonPath); return env; } diff --git a/src/client/common/variables/environment.ts b/src/client/common/variables/environment.ts index 92921345a6f6..5455c4e7250f 100644 --- a/src/client/common/variables/environment.ts +++ b/src/client/common/variables/environment.ts @@ -1,28 +1,32 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. -import * as fs from 'fs-extra'; +import * as fsextra from 'fs-extra'; import { inject, injectable } from 'inversify'; import * as path from 'path'; import { sendTelemetryEvent } from '../../telemetry'; import { EventName } from '../../telemetry/constants'; +import { IFileSystem } from '../platform/types'; import { IPathUtils } from '../types'; import { EnvironmentVariables, IEnvironmentVariablesService } from './types'; @injectable() export class EnvironmentVariablesService implements IEnvironmentVariablesService { private readonly pathVariable: 'PATH' | 'Path'; - constructor(@inject(IPathUtils) pathUtils: IPathUtils) { + constructor( + @inject(IPathUtils) pathUtils: IPathUtils, + @inject(IFileSystem) private readonly fs: IFileSystem + ) { this.pathVariable = pathUtils.getPathVariableName(); } public async parseFile(filePath?: string, baseVars?: EnvironmentVariables): Promise { - if (!filePath || !await fs.pathExists(filePath)) { + if (!filePath || !await this.fs.fileExists(filePath)) { return; } - if (!fs.lstatSync(filePath).isFile()) { + if (!fsextra.lstatSync(filePath)) { return; } - return parseEnvFile(await fs.readFile(filePath), baseVars); + return parseEnvFile(await fsextra.readFile(filePath), baseVars); } public mergeVariables(source: EnvironmentVariables, target: EnvironmentVariables) { if (!target) { diff --git a/src/client/interpreter/locators/services/windowsRegistryService.ts b/src/client/interpreter/locators/services/windowsRegistryService.ts index e99f29a1be66..ae300f4dfa18 100644 --- a/src/client/interpreter/locators/services/windowsRegistryService.ts +++ b/src/client/interpreter/locators/services/windowsRegistryService.ts @@ -1,10 +1,10 @@ // tslint:disable:no-require-imports no-var-requires underscore-consistent-invocation -import * as fs from 'fs-extra'; + import { inject, injectable } from 'inversify'; import * as path from 'path'; import { Uri } from 'vscode'; import { traceError } from '../../../common/logger'; -import { IPlatformService, IRegistry, RegistryHive } from '../../../common/platform/types'; +import { IFileSystem, IPlatformService, IRegistry, RegistryHive } from '../../../common/platform/types'; import { IPathUtils } from '../../../common/types'; import { Architecture } from '../../../common/utils/platform'; import { parsePythonVersion } from '../../../common/utils/version'; @@ -34,6 +34,7 @@ type CompanyInterpreter = { @injectable() export class WindowsRegistryService extends CacheableLocatorService { private readonly pathUtils: IPathUtils; + private readonly fs: IFileSystem; constructor( @inject(IRegistry) private registry: IRegistry, @inject(IPlatformService) private readonly platform: IPlatformService, @@ -42,6 +43,7 @@ export class WindowsRegistryService extends CacheableLocatorService { ) { super('WindowsRegistryService', serviceContainer); this.pathUtils = serviceContainer.get(IPathUtils); + this.fs = serviceContainer.get(IFileSystem); } // tslint:disable-next-line:no-empty public dispose() {} @@ -158,8 +160,8 @@ export class WindowsRegistryService extends CacheableLocatorService { }) .then(interpreter => interpreter - ? fs - .pathExists(interpreter.path) + ? this.fs + .fileExists(interpreter.path) .catch(() => false) .then(exists => (exists ? interpreter : null)) : null diff --git a/src/client/providers/jediProxy.ts b/src/client/providers/jediProxy.ts index 19a55328f85a..e7c81326de71 100644 --- a/src/client/providers/jediProxy.ts +++ b/src/client/providers/jediProxy.ts @@ -3,7 +3,6 @@ // tslint:disable:no-var-requires no-require-imports no-any import { ChildProcess } from 'child_process'; -import * as fs from 'fs-extra'; import * as path from 'path'; // @ts-ignore import * as pidusage from 'pidusage'; @@ -11,6 +10,7 @@ import { CancellationToken, CancellationTokenSource, CompletionItemKind, Disposa import { isTestExecution } from '../common/constants'; import '../common/extensions'; import { IS_WINDOWS } from '../common/platform/constants'; +import { IFileSystem } from '../common/platform/types'; import { IPythonExecutionFactory } from '../common/process/types'; import { BANNER_NAME_PROPOSE_LS, IConfigurationService, ILogger, IPythonExtensionBanner, IPythonSettings } from '../common/types'; import { createDeferred, Deferred } from '../common/utils/async'; @@ -640,7 +640,8 @@ export class JediProxy implements Disposable { if (lines.length === 0) { return ''; } - const exists = await fs.pathExists(lines[0]); + const fs = this.serviceContainer.get(IFileSystem); + const exists = await fs.fileExists(lines[0]); return exists ? lines[0] : ''; } catch { return ''; diff --git a/src/test/common/variables/envVarsProvider.multiroot.test.ts b/src/test/common/variables/envVarsProvider.multiroot.test.ts index 2fce35335a72..2c36fbfda6ca 100644 --- a/src/test/common/variables/envVarsProvider.multiroot.test.ts +++ b/src/test/common/variables/envVarsProvider.multiroot.test.ts @@ -10,6 +10,7 @@ import { WorkspaceService } from '../../../client/common/application/workspace'; import { ConfigurationService } from '../../../client/common/configuration/service'; import { IS_WINDOWS, NON_WINDOWS_PATH_VARIABLE_NAME, WINDOWS_PATH_VARIABLE_NAME } from '../../../client/common/platform/constants'; import { PlatformService } from '../../../client/common/platform/platformService'; +import { IFileSystem } from '../../../client/common/platform/types'; import { IDisposableRegistry, IPathUtils } from '../../../client/common/types'; import { clearCache } from '../../../client/common/utils/cacheUtils'; import { EnvironmentVariablesService } from '../../../client/common/variables/environment'; @@ -66,8 +67,9 @@ suite('Multiroot Environment Variables Provider', () => { function getVariablesProvider(mockVariables: EnvironmentVariables = { ...process.env }) { const pathUtils = ioc.serviceContainer.get(IPathUtils); + const fs = ioc.serviceContainer.get(IFileSystem); const mockProcess = new MockProcess(mockVariables); - const variablesService = new EnvironmentVariablesService(pathUtils); + const variablesService = new EnvironmentVariablesService(pathUtils, fs); const disposables = ioc.serviceContainer.get(IDisposableRegistry); ioc.serviceManager.addSingletonInstance(IInterpreterAutoSelectionService, new MockAutoSelectionService()); const cfgService = new ConfigurationService(ioc.serviceContainer); diff --git a/src/test/common/variables/envVarsService.unit.test.ts b/src/test/common/variables/envVarsService.unit.test.ts index 54e0c61f9ac6..1c876b8cbe45 100644 --- a/src/test/common/variables/envVarsService.unit.test.ts +++ b/src/test/common/variables/envVarsService.unit.test.ts @@ -6,7 +6,9 @@ import { expect, use } from 'chai'; import * as chaiAsPromised from 'chai-as-promised'; import * as path from 'path'; +import { FileSystem } from '../../../client/common/platform/fileSystem'; import { PathUtils } from '../../../client/common/platform/pathUtils'; +import { PlatformService } from '../../../client/common/platform/platformService'; import { IPathUtils } from '../../../client/common/types'; import { OSType } from '../../../client/common/utils/platform'; import { EnvironmentVariablesService, parseEnvFile } from '../../../client/common/variables/environment'; @@ -23,7 +25,10 @@ suite('Environment Variables Service', () => { let variablesService: IEnvironmentVariablesService; setup(() => { pathUtils = new PathUtils(getOSType() === OSType.Windows); - variablesService = new EnvironmentVariablesService(pathUtils); + const fs = new FileSystem( + new PlatformService() + ); + variablesService = new EnvironmentVariablesService(pathUtils, fs); }); test('Custom variables should be undefined with no argument', async () => { From 740e0d70529073935a7cff1973bbf11a3738a6cf Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 9 Dec 2019 16:52:31 -0700 Subject: [PATCH 02/26] Drop the lstatSync() call in EnvironmentVariablesService. --- src/client/common/variables/environment.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/client/common/variables/environment.ts b/src/client/common/variables/environment.ts index 5455c4e7250f..c0e3f163b5ab 100644 --- a/src/client/common/variables/environment.ts +++ b/src/client/common/variables/environment.ts @@ -23,9 +23,6 @@ export class EnvironmentVariablesService implements IEnvironmentVariablesService if (!filePath || !await this.fs.fileExists(filePath)) { return; } - if (!fsextra.lstatSync(filePath)) { - return; - } return parseEnvFile(await fsextra.readFile(filePath), baseVars); } public mergeVariables(source: EnvironmentVariables, target: EnvironmentVariables) { From b464494a1822459e8f9ea8f54807e67965a0327b Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 9 Dec 2019 17:02:33 -0700 Subject: [PATCH 03/26] Use IFileSystem instead of fs-extra in various places. --- src/client/common/editor.ts | 42 +++++++++++----------- src/client/common/variables/environment.ts | 3 +- src/client/formatters/baseFormatter.ts | 5 +-- 3 files changed, 26 insertions(+), 24 deletions(-) diff --git a/src/client/common/editor.ts b/src/client/common/editor.ts index f777de96e246..cc56b37717a7 100644 --- a/src/client/common/editor.ts +++ b/src/client/common/editor.ts @@ -1,10 +1,11 @@ import { Diff, diff_match_patch } from 'diff-match-patch'; -import * as fs from 'fs-extra'; import { injectable } from 'inversify'; import * as md5 from 'md5'; import { EOL } from 'os'; import * as path from 'path'; import { Position, Range, TextDocument, TextEdit, Uri, WorkspaceEdit } from 'vscode'; +import { FileSystem } from './platform/fileSystem'; +import { PlatformService } from './platform/platformService'; import { IEditorUtils } from './types'; // Code borrowed from goFormat.ts (Go Extension for VS Code) @@ -107,7 +108,7 @@ export function getWorkspaceEditsFromPatch(filePatches: string[], workspaceRoot? let fileName = fileNameLines[0].substring(fileNameLines[0].indexOf(' a') + 3).trim(); fileName = workspaceRoot && !path.isAbsolute(fileName) ? path.resolve(workspaceRoot, fileName) : fileName; - if (!fs.existsSync(fileName)) { + if (!fs.fileExistsSync(fileName)) { return; } @@ -123,7 +124,7 @@ export function getWorkspaceEditsFromPatch(filePatches: string[], workspaceRoot? throw new Error('Unable to parse Patch string'); } - const fileSource = fs.readFileSync(fileName).toString('utf8'); + const fileSource = fsextra.readFileSync(fileName).toString('utf8'); const fileUri = Uri.file(fileName); // Add line feeds and build the text edits @@ -226,24 +227,25 @@ function getTextEditsInternal(before: string, diffs: [number, string][], startLi return edits; } -export function getTempFileWithDocumentContents(document: TextDocument): Promise { - return new Promise((resolve, reject) => { - const ext = path.extname(document.uri.fsPath); - // Don't create file in temp folder since external utilities - // look into configuration files in the workspace and are not able - // to find custom rules if file is saved in a random disk location. - // This means temp file has to be created in the same folder - // as the original one and then removed. +export async function getTempFileWithDocumentContents(document: TextDocument): Promise { + const ext = path.extname(document.uri.fsPath); + // Don't create file in temp folder since external utilities + // look into configuration files in the workspace and are not able + // to find custom rules if file is saved in a random disk location. + // This means temp file has to be created in the same folder + // as the original one and then removed. - // tslint:disable-next-line:no-require-imports - const fileName = `${document.uri.fsPath}.${md5(document.uri.fsPath)}${ext}`; - fs.writeFile(fileName, document.getText(), ex => { - if (ex) { - reject(`Failed to create a temporary file, ${ex.message}`); - } - resolve(fileName); - }); - }); + // tslint:disable-next-line:no-require-imports + const fileName = `${document.uri.fsPath}.${md5(document.uri.fsPath)}${ext}`; + const fs = new FileSystem( + new PlatformService() + ); + try { + await fs.writeFile(fileName, document.getText()); + } catch (ex) { + throw Error(`Failed to create a temporary file, ${ex.message}`); + } + return fileName; } /** diff --git a/src/client/common/variables/environment.ts b/src/client/common/variables/environment.ts index c0e3f163b5ab..ce78210bee36 100644 --- a/src/client/common/variables/environment.ts +++ b/src/client/common/variables/environment.ts @@ -1,7 +1,6 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. -import * as fsextra from 'fs-extra'; import { inject, injectable } from 'inversify'; import * as path from 'path'; import { sendTelemetryEvent } from '../../telemetry'; @@ -23,7 +22,7 @@ export class EnvironmentVariablesService implements IEnvironmentVariablesService if (!filePath || !await this.fs.fileExists(filePath)) { return; } - return parseEnvFile(await fsextra.readFile(filePath), baseVars); + return parseEnvFile(await this.fs.readFile(filePath), baseVars); } public mergeVariables(source: EnvironmentVariables, target: EnvironmentVariables) { if (!target) { diff --git a/src/client/formatters/baseFormatter.ts b/src/client/formatters/baseFormatter.ts index bd84428f8481..505598365ecd 100644 --- a/src/client/formatters/baseFormatter.ts +++ b/src/client/formatters/baseFormatter.ts @@ -1,4 +1,3 @@ -import * as fs from 'fs-extra'; import * as path from 'path'; import * as vscode from 'vscode'; import { IApplicationShell, IWorkspaceService } from '../common/application/types'; @@ -6,6 +5,7 @@ import { STANDARD_OUTPUT_CHANNEL } from '../common/constants'; import '../common/extensions'; import { isNotInstalledError } from '../common/helpers'; import { traceError } from '../common/logger'; +import { IFileSystem } from '../common/platform/types'; import { IPythonToolExecutionService } from '../common/process/types'; import { IDisposableRegistry, IInstaller, IOutputChannel, Product } from '../common/types'; import { IServiceContainer } from '../ioc/types'; @@ -109,7 +109,8 @@ export abstract class BaseFormatter { private deleteTempFile(originalFile: string, tempFile: string): Promise { if (originalFile !== tempFile) { - return fs.unlink(tempFile); + const fs = this.serviceContainer.get(IFileSystem); + return fs.deleteFile(tempFile); } return Promise.resolve(); } From 2d59328372b59e60f54789483da37905cbaa7684 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 9 Dec 2019 17:08:23 -0700 Subject: [PATCH 04/26] Add IFileSystem.readFileSync() and use it instead of fsextra.readFileSync(). --- src/client/common/editor.ts | 7 +++++-- src/client/common/envFileParser.ts | 6 ++++-- src/client/common/platform/fileSystem.ts | 4 ++++ src/client/common/platform/types.ts | 1 + 4 files changed, 14 insertions(+), 4 deletions(-) diff --git a/src/client/common/editor.ts b/src/client/common/editor.ts index cc56b37717a7..40cd080b9b01 100644 --- a/src/client/common/editor.ts +++ b/src/client/common/editor.ts @@ -106,6 +106,9 @@ export function getWorkspaceEditsFromPatch(filePatches: string[], workspaceRoot? return; } + const fs = new FileSystem( + new PlatformService() + ); let fileName = fileNameLines[0].substring(fileNameLines[0].indexOf(' a') + 3).trim(); fileName = workspaceRoot && !path.isAbsolute(fileName) ? path.resolve(workspaceRoot, fileName) : fileName; if (!fs.fileExistsSync(fileName)) { @@ -124,7 +127,7 @@ export function getWorkspaceEditsFromPatch(filePatches: string[], workspaceRoot? throw new Error('Unable to parse Patch string'); } - const fileSource = fsextra.readFileSync(fileName).toString('utf8'); + const fileSource = fs.readFileSync(fileName); const fileUri = Uri.file(fileName); // Add line feeds and build the text edits @@ -230,7 +233,7 @@ function getTextEditsInternal(before: string, diffs: [number, string][], startLi export async function getTempFileWithDocumentContents(document: TextDocument): Promise { const ext = path.extname(document.uri.fsPath); // Don't create file in temp folder since external utilities - // look into configuration files in the workspace and are not able + // look into configuration files in the workspace and are not // to find custom rules if file is saved in a random disk location. // This means temp file has to be created in the same folder // as the original one and then removed. diff --git a/src/client/common/envFileParser.ts b/src/client/common/envFileParser.ts index ee180eeae87f..d3ce08827a99 100644 --- a/src/client/common/envFileParser.ts +++ b/src/client/common/envFileParser.ts @@ -1,4 +1,3 @@ -import * as fs from 'fs-extra'; import { IS_WINDOWS } from './platform/constants'; import { FileSystem } from './platform/fileSystem'; import { PathUtils } from './platform/pathUtils'; @@ -25,7 +24,10 @@ function parseEnvironmentVariables(contents: string): EnvironmentVariables | und } export function parseEnvFile(envFile: string, mergeWithProcessEnvVars: boolean = true): EnvironmentVariables { - const buffer = fs.readFileSync(envFile, 'utf8'); + const fs = new FileSystem( + new PlatformService() + ); + const buffer = fs.readFileSync(envFile); const env = parseEnvironmentVariables(buffer)!; return mergeWithProcessEnvVars ? mergeEnvVariables(env, process.env) : mergePythonPath(env, process.env.PYTHONPATH as string); } diff --git a/src/client/common/platform/fileSystem.ts b/src/client/common/platform/fileSystem.ts index b1413e5b422d..6c0d5b6227b0 100644 --- a/src/client/common/platform/fileSystem.ts +++ b/src/client/common/platform/fileSystem.ts @@ -204,4 +204,8 @@ export class FileSystem implements IFileSystem { }); }); } + + public readFileSync(filePath: string): string { + return fs.readFileSync(filePath, 'utf8'); + } } diff --git a/src/client/common/platform/types.ts b/src/client/common/platform/types.ts index 586b5557a070..9043e5c89e86 100644 --- a/src/client/common/platform/types.ts +++ b/src/client/common/platform/types.ts @@ -50,6 +50,7 @@ export interface IFileSystem { arePathsSame(path1: string, path2: string): boolean; readFile(filePath: string): Promise; writeFile(filePath: string, data: {}, options?: string | fsextra.WriteFileOptions): Promise; + readFileSync(filename: string): string; appendFileSync(filename: string, data: {}, encoding: string): void; appendFileSync(filename: string, data: {}, options?: { encoding?: string; mode?: number; flag?: string }): void; // tslint:disable-next-line:unified-signatures From a1f1684c201dfea093c676f4eb159330d19249b9 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 9 Dec 2019 17:16:26 -0700 Subject: [PATCH 05/26] Add IFileSystem.createReadStream() and use it. --- src/client/common/application/webPanels/webPanelServer.ts | 6 +++++- src/client/common/platform/fileSystem.ts | 4 ++++ src/client/common/platform/types.ts | 1 + 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/client/common/application/webPanels/webPanelServer.ts b/src/client/common/application/webPanels/webPanelServer.ts index 5dd386a8b4f5..d5fecd1f371b 100644 --- a/src/client/common/application/webPanels/webPanelServer.ts +++ b/src/client/common/application/webPanels/webPanelServer.ts @@ -1,7 +1,6 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. import * as Cors from '@koa/cors'; -import * as fs from 'fs-extra'; import * as http from 'http'; import * as Koa from 'koa'; import * as compress from 'koa-compress'; @@ -11,6 +10,8 @@ import * as path from 'path'; import { EXTENSION_ROOT_DIR } from '../../../constants'; import { Identifiers } from '../../../datascience/constants'; import { SharedMessages } from '../../../datascience/messages'; +import { FileSystem } from '../../platform/fileSystem'; +import { PlatformService } from '../../platform/platformService'; interface IState { cwd: string; @@ -110,6 +111,9 @@ export class WebPanelServer { break; } + const fs = new FileSystem( + new PlatformService() + ); ctx.body = fs.createReadStream(filePath); } diff --git a/src/client/common/platform/fileSystem.ts b/src/client/common/platform/fileSystem.ts index 6c0d5b6227b0..ba4405e40421 100644 --- a/src/client/common/platform/fileSystem.ts +++ b/src/client/common/platform/fileSystem.ts @@ -190,6 +190,10 @@ export class FileSystem implements IFileSystem { }); } + public createReadStream(filePath: string): fileSystem.ReadStream { + return fileSystem.createReadStream(filePath); + } + public createWriteStream(filePath: string): fileSystem.WriteStream { return fileSystem.createWriteStream(filePath); } diff --git a/src/client/common/platform/types.ts b/src/client/common/platform/types.ts index 9043e5c89e86..3ae13f1ab32f 100644 --- a/src/client/common/platform/types.ts +++ b/src/client/common/platform/types.ts @@ -61,6 +61,7 @@ export interface IFileSystem { getFileHash(filePath: string): Promise; search(globPattern: string): Promise; createTemporaryFile(extension: string): Promise; + createReadStream(path: string): fs.ReadStream; createWriteStream(path: string): fs.WriteStream; chmod(path: string, mode: string): Promise; } From e4d668afdc9122919abbeb8207bec2bc42f26765 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 9 Dec 2019 17:36:14 -0700 Subject: [PATCH 06/26] Use IFileSystem in various datascience places instead of fs-extra. --- src/client/datascience/codeCssGenerator.ts | 15 +++++----- .../interactive-common/interactiveBase.ts | 7 ++--- .../datascience/jupyter/jupyterNotebook.ts | 15 +++++++--- src/client/datascience/themeFinder.ts | 29 ++++++++++++++----- src/test/datascience/color.test.ts | 12 ++++++-- 5 files changed, 54 insertions(+), 24 deletions(-) diff --git a/src/client/datascience/codeCssGenerator.ts b/src/client/datascience/codeCssGenerator.ts index f19e9dbc3178..af28a246561b 100644 --- a/src/client/datascience/codeCssGenerator.ts +++ b/src/client/datascience/codeCssGenerator.ts @@ -2,13 +2,13 @@ // Licensed under the MIT License. 'use strict'; import { JSONArray, JSONObject } from '@phosphor/coreutils'; -import * as fs from 'fs-extra'; import { inject, injectable } from 'inversify'; import { parse } from 'jsonc-parser'; import * as monacoEditor from 'monaco-editor/esm/vs/editor/editor.api'; import * as path from 'path'; import { IWorkspaceService } from '../common/application/types'; +import { IFileSystem } from '../common/platform/types'; import { IConfigurationService, ILogger } from '../common/types'; import { DefaultTheme } from './constants'; import { ICodeCssGenerator, IThemeFinder } from './types'; @@ -98,8 +98,9 @@ export class CodeCssGenerator implements ICodeCssGenerator { @inject(IWorkspaceService) private workspaceService: IWorkspaceService, @inject(IThemeFinder) private themeFinder: IThemeFinder, @inject(IConfigurationService) private configService: IConfigurationService, - @inject(ILogger) private logger: ILogger) { - } + @inject(ILogger) private logger: ILogger, + @inject(IFileSystem) private fs: IFileSystem + ) { } public generateThemeCss(isDark: boolean, theme: string): Promise { return this.applyThemeData(isDark, theme, '', this.generateCss.bind(this)); @@ -330,7 +331,7 @@ export class CodeCssGenerator implements ICodeCssGenerator { } private readTokenColors = async (themeFile: string): Promise => { - const tokenContent = await fs.readFile(themeFile, 'utf8'); + const tokenContent = await this.fs.readFile(themeFile); const theme = parse(tokenContent); const tokenColors = theme.tokenColors as JSONArray; if (tokenColors && tokenColors.length > 0) { @@ -356,7 +357,7 @@ export class CodeCssGenerator implements ICodeCssGenerator { } private readBaseColors = async (themeFile: string): Promise => { - const tokenContent = await fs.readFile(themeFile, 'utf8'); + const tokenContent = await this.fs.readFile(themeFile); const theme = parse(tokenContent); const colors = theme.colors as JSONObject; @@ -383,7 +384,7 @@ export class CodeCssGenerator implements ICodeCssGenerator { this.logger.logInformation(`Loading colors from ${themeRoot} ...`); // This should be the path to the file. Load it as a json object - const contents = await fs.readFile(themeRoot, 'utf8'); + const contents = await this.fs.readFile(themeRoot); const json = parse(contents); // There should be a theme colors section @@ -436,7 +437,7 @@ export class CodeCssGenerator implements ICodeCssGenerator { this.logger.logInformation(`Loading base colors from ${themeRoot} ...`); // This should be the path to the file. Load it as a json object - const contents = await fs.readFile(themeRoot, 'utf8'); + const contents = await this.fs.readFile(themeRoot); const json = parse(contents); // There should be a theme colors section diff --git a/src/client/datascience/interactive-common/interactiveBase.ts b/src/client/datascience/interactive-common/interactiveBase.ts index 4be1da0458e6..41e5b78ce258 100644 --- a/src/client/datascience/interactive-common/interactiveBase.ts +++ b/src/client/datascience/interactive-common/interactiveBase.ts @@ -3,7 +3,6 @@ 'use strict'; import '../../common/extensions'; -import * as fs from 'fs-extra'; import { injectable, unmanaged } from 'inversify'; import * as os from 'os'; import * as path from 'path'; @@ -955,7 +954,7 @@ export abstract class InteractiveBase extends WebViewHost { if (this.launchInfo && this.launchInfo.connectionInfo.localLaunch && !this._workingDirectory) { + const fs = new FileSystem( + new PlatformService() + ); // See what our working dir is supposed to be const suggested = this.launchInfo.workingDir; - if (suggested && (await fs.pathExists(suggested))) { + if (suggested && (await fs.fileExists(suggested))) { // We should use the launch info directory. It trumps the possible dir this._workingDirectory = suggested; return this.changeDirectoryIfPossible(this._workingDirectory); - } else if (launchingFile && (await fs.pathExists(launchingFile))) { + } else if (launchingFile && (await fs.fileExists(launchingFile))) { // Combine the working directory with this file if possible. this._workingDirectory = expandWorkingDir(this.launchInfo.workingDir, launchingFile, this.workspace); if (this._workingDirectory) { @@ -705,7 +709,10 @@ export class JupyterNotebookBase implements INotebook { } private changeDirectoryIfPossible = async (directory: string): Promise => { - if (this.launchInfo && this.launchInfo.connectionInfo.localLaunch && (await fs.pathExists(directory))) { + const fs = new FileSystem( + new PlatformService() + ); + if (this.launchInfo && this.launchInfo.connectionInfo.localLaunch && (await fs.directoryExists(directory))) { await this.executeSilently(`%cd "${directory}"`); } } diff --git a/src/client/datascience/themeFinder.ts b/src/client/datascience/themeFinder.ts index 470f8d604400..3c9a9e96ca70 100644 --- a/src/client/datascience/themeFinder.ts +++ b/src/client/datascience/themeFinder.ts @@ -1,12 +1,13 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. 'use strict'; -import * as fs from 'fs-extra'; import * as glob from 'glob'; import { inject, injectable } from 'inversify'; import * as path from 'path'; import { EXTENSION_ROOT_DIR, PYTHON_LANGUAGE } from '../common/constants'; +import { FileSystem } from '../common/platform/fileSystem'; +import { PlatformService } from '../common/platform/platformService'; import { ICurrentProcess, IExtensions, ILogger } from '../common/types'; import { IThemeFinder } from './types'; @@ -78,7 +79,10 @@ export class ThemeFinder implements IThemeFinder { // Should be somewhere under currentPath/resources/app/extensions inside of a json file let extensionsPath = path.join(currentPath, 'resources', 'app', 'extensions'); - if (!(await fs.pathExists(extensionsPath))) { + const fs = new FileSystem( + new PlatformService() + ); + if (!(await fs.directoryExists(extensionsPath))) { // Might be on mac or linux. try a different path currentPath = path.resolve(currentPath, '../../../..'); extensionsPath = path.join(currentPath, 'resources', 'app', 'extensions'); @@ -89,7 +93,7 @@ export class ThemeFinder implements IThemeFinder { // If that didn't work, see if it's our MagicPython predefined tmLanguage if (!results && language === PYTHON_LANGUAGE) { - results = await fs.readFile(path.join(EXTENSION_ROOT_DIR, 'resources', 'MagicPython.tmLanguage.json'), 'utf-8'); + results = await fs.readFile(path.join(EXTENSION_ROOT_DIR, 'resources', 'MagicPython.tmLanguage.json')); } return results; @@ -146,7 +150,10 @@ export class ThemeFinder implements IThemeFinder { // Should be somewhere under currentPath/resources/app/extensions inside of a json file let extensionsPath = path.join(currentPath, 'resources', 'app', 'extensions'); - if (!(await fs.pathExists(extensionsPath))) { + const fs = new FileSystem( + new PlatformService() + ); + if (!(await fs.directoryExists(extensionsPath))) { // Might be on mac or linux. try a different path currentPath = path.resolve(currentPath, '../../../..'); extensionsPath = path.join(currentPath, 'resources', 'app', 'extensions'); @@ -181,8 +188,12 @@ export class ThemeFinder implements IThemeFinder { } private async findMatchingLanguageFromJson(packageJson: string, language: string) : Promise { + const fs = new FileSystem( + new PlatformService() + ); // Read the contents of the json file - const json = await fs.readJSON(packageJson, { encoding: 'utf-8'}); + const text = await fs.readFile(packageJson); + const json = JSON.parse(text); // Should have a name entry and a contributes entry if (json.hasOwnProperty('name') && json.hasOwnProperty('contributes')) { @@ -195,7 +206,7 @@ export class ThemeFinder implements IThemeFinder { if (t.hasOwnProperty('language') && t.language === language) { // Path is relative to the package.json file. const rootFile = t.hasOwnProperty('path') ? path.join(path.dirname(packageJson), t.path.toString()) : ''; - return fs.readFile(rootFile, 'utf-8'); + return fs.readFile(rootFile); } } } @@ -203,8 +214,12 @@ export class ThemeFinder implements IThemeFinder { } private async findMatchingThemeFromJson(packageJson: string, themeName: string) : Promise { + const fs = new FileSystem( + new PlatformService() + ); // Read the contents of the json file - const json = await fs.readJSON(packageJson, { encoding: 'utf-8'}); + const text = await fs.readFile(packageJson); + const json = JSON.parse(text); // Should have a name entry and a contributes entry if (json.hasOwnProperty('name') && json.hasOwnProperty('contributes')) { diff --git a/src/test/datascience/color.test.ts b/src/test/datascience/color.test.ts index d8f242e60ef2..2a49f3b89b89 100644 --- a/src/test/datascience/color.test.ts +++ b/src/test/datascience/color.test.ts @@ -9,6 +9,8 @@ import { Extensions } from '../../client/common/application/extensions'; import { IWorkspaceService } from '../../client/common/application/types'; import { PythonSettings } from '../../client/common/configSettings'; import { Logger } from '../../client/common/logger'; +import { FileSystem } from '../../client/common/platform/fileSystem'; +import { PlatformService } from '../../client/common/platform/platformService'; import { CurrentProcess } from '../../client/common/process/currentProcess'; import { IConfigurationService } from '../../client/common/types'; import { CodeCssGenerator } from '../../client/datascience/codeCssGenerator'; @@ -79,7 +81,10 @@ suite('Theme colors', () => { workspaceService.setup(c => c.getConfiguration(TypeMoq.It.isAny())).returns(() => workspaceConfig.object); workspaceService.setup(c => c.getConfiguration(TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => workspaceConfig.object); - cssGenerator = new CodeCssGenerator(workspaceService.object, themeFinder, configService.object, logger); + const fs = new FileSystem( + new PlatformService() + ); + cssGenerator = new CodeCssGenerator(workspaceService.object, themeFinder, configService.object, logger, fs); }); function runTest(themeName: string, isDark: boolean, shouldExist: boolean) { @@ -144,7 +149,10 @@ suite('Theme colors', () => { mockThemeFinder.setup(m => m.isThemeDark(TypeMoq.It.isAnyString())).returns(() => Promise.resolve(false)); mockThemeFinder.setup(m => m.findThemeRootJson(TypeMoq.It.isAnyString())).returns(() => Promise.resolve(undefined)); - cssGenerator = new CodeCssGenerator(workspaceService.object, mockThemeFinder.object, configService.object, logger); + const fs = new FileSystem( + new PlatformService() + ); + cssGenerator = new CodeCssGenerator(workspaceService.object, mockThemeFinder.object, configService.object, logger, fs); const colors = await cssGenerator.generateThemeCss(false, 'Kimbie Dark'); assert.ok(colors, 'Cannot find theme colors for Kimbie Dark'); From b65c5bfc91e53fd7d587ea1cf4bec56571650dfc Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 9 Dec 2019 17:43:41 -0700 Subject: [PATCH 07/26] Add IFileSystem.appendFile() and use it. --- src/client/common/platform/fileSystem.ts | 3 +++ src/client/common/platform/types.ts | 1 + src/client/datascience/jupyter/jupyterImporter.ts | 3 +-- 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/client/common/platform/fileSystem.ts b/src/client/common/platform/fileSystem.ts index ba4405e40421..057113cf3e94 100644 --- a/src/client/common/platform/fileSystem.ts +++ b/src/client/common/platform/fileSystem.ts @@ -116,6 +116,9 @@ export class FileSystem implements IFileSystem { } } + public appendFile(filename: string, data: {}): Promise { + return fs.appendFile(filename, data); + } public appendFileSync(filename: string, data: {}, encoding: string): void; public appendFileSync(filename: string, data: {}, options?: { encoding?: string; mode?: number; flag?: string }): void; // tslint:disable-next-line:unified-signatures diff --git a/src/client/common/platform/types.ts b/src/client/common/platform/types.ts index 3ae13f1ab32f..5157a3503821 100644 --- a/src/client/common/platform/types.ts +++ b/src/client/common/platform/types.ts @@ -51,6 +51,7 @@ export interface IFileSystem { readFile(filePath: string): Promise; writeFile(filePath: string, data: {}, options?: string | fsextra.WriteFileOptions): Promise; readFileSync(filename: string): string; + appendFile(filename: string, data: {}): Promise; appendFileSync(filename: string, data: {}, encoding: string): void; appendFileSync(filename: string, data: {}, options?: { encoding?: string; mode?: number; flag?: string }): void; // tslint:disable-next-line:unified-signatures diff --git a/src/client/datascience/jupyter/jupyterImporter.ts b/src/client/datascience/jupyter/jupyterImporter.ts index 2c3b5921fa0b..f9b388036f22 100644 --- a/src/client/datascience/jupyter/jupyterImporter.ts +++ b/src/client/datascience/jupyter/jupyterImporter.ts @@ -4,7 +4,6 @@ import '../../common/extensions'; import { nbformat } from '@jupyterlab/coreutils'; -import * as fs from 'fs-extra'; import { inject, injectable } from 'inversify'; import * as os from 'os'; import * as path from 'path'; @@ -185,7 +184,7 @@ export class JupyterImporter implements INotebookImporter { try { // Save this file into our disposables so the temp file goes away this.disposableRegistry.push(file); - await fs.appendFile(file.filePath, this.nbconvertTemplateFormat.format(this.defaultCellMarker)); + await this.fileSystem.appendFile(file.filePath, this.nbconvertTemplateFormat.format(this.defaultCellMarker)); // Now we should have a template that will convert return file.filePath; From a21558b1d237d4de501be6e3f282e226d7239bb3 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 10 Dec 2019 11:40:04 -0700 Subject: [PATCH 08/26] Export WriteStream from platform/types. --- src/client/common/net/fileDownloader.ts | 3 +-- src/client/common/platform/types.ts | 2 ++ src/client/debugger/extension/adapter/logging.ts | 5 ++--- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/client/common/net/fileDownloader.ts b/src/client/common/net/fileDownloader.ts index 53162c965304..281538a47a5d 100644 --- a/src/client/common/net/fileDownloader.ts +++ b/src/client/common/net/fileDownloader.ts @@ -3,12 +3,11 @@ 'use strict'; -import { WriteStream } from 'fs'; import { inject, injectable } from 'inversify'; import * as requestTypes from 'request'; import { Progress, ProgressLocation } from 'vscode'; import { IApplicationShell } from '../application/types'; -import { IFileSystem } from '../platform/types'; +import { IFileSystem, WriteStream } from '../platform/types'; import { DownloadOptions, IFileDownloader, IHttpClient } from '../types'; import { Http } from '../utils/localize'; import { noop } from '../utils/misc'; diff --git a/src/client/common/platform/types.ts b/src/client/common/platform/types.ts index 5157a3503821..afa198489cc5 100644 --- a/src/client/common/platform/types.ts +++ b/src/client/common/platform/types.ts @@ -35,6 +35,8 @@ export interface IPlatformService { export type TemporaryFile = { filePath: string } & Disposable; export type TemporaryDirectory = { path: string } & Disposable; +export type WriteStream = fs.WriteStream; + export const IFileSystem = Symbol('IFileSystem'); export interface IFileSystem { directorySeparatorChar: string; diff --git a/src/client/debugger/extension/adapter/logging.ts b/src/client/debugger/extension/adapter/logging.ts index 9c21831f71cf..95df39440c73 100644 --- a/src/client/debugger/extension/adapter/logging.ts +++ b/src/client/debugger/extension/adapter/logging.ts @@ -2,19 +2,18 @@ // Licensed under the MIT License. 'use strict'; -import * as fs from 'fs'; import { inject, injectable } from 'inversify'; import * as path from 'path'; import { DebugAdapterTracker, DebugAdapterTrackerFactory, DebugConfiguration, DebugSession, ProviderResult } from 'vscode'; import { DebugProtocol } from 'vscode-debugprotocol'; -import { IFileSystem } from '../../../common/platform/types'; +import { IFileSystem, WriteStream } from '../../../common/platform/types'; import { StopWatch } from '../../../common/utils/stopWatch'; import { EXTENSION_ROOT_DIR } from '../../../constants'; class DebugSessionLoggingTracker implements DebugAdapterTracker { private readonly enabled: boolean = false; - private stream: fs.WriteStream | undefined; + private stream: WriteStream | undefined; private timer = new StopWatch(); constructor(private readonly session: DebugSession, fileSystem: IFileSystem) { From 28e7a0b37bd3afa15b3cc071e02c0c132470c45f Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 10 Dec 2019 11:50:12 -0700 Subject: [PATCH 09/26] Use FileSystem in various places instead of node's fs. --- src/client/common/utils/localize.ts | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/client/common/utils/localize.ts b/src/client/common/utils/localize.ts index 3a3c1e66e22c..0065d4b8697c 100644 --- a/src/client/common/utils/localize.ts +++ b/src/client/common/utils/localize.ts @@ -3,9 +3,10 @@ 'use strict'; -import * as fs from 'fs'; import * as path from 'path'; import { EXTENSION_ROOT_DIR } from '../../constants'; +import { FileSystem } from '../platform/fileSystem'; +import { PlatformService } from '../platform/platformService'; // External callers of localize use these tables to retrieve localized values. export namespace Diagnostics { @@ -505,13 +506,17 @@ function getString(key: string, defValue?: string) { } function load() { + const fs = new FileSystem( + new PlatformService() + ); + // Figure out our current locale. loadedLocale = parseLocale(); // Find the nls file that matches (if there is one) const nlsFile = path.join(EXTENSION_ROOT_DIR, `package.nls.${loadedLocale}.json`); - if (fs.existsSync(nlsFile)) { - const contents = fs.readFileSync(nlsFile, 'utf8'); + if (fs.fileExistsSync(nlsFile)) { + const contents = fs.readFileSync(nlsFile); loadedCollection = JSON.parse(contents); } else { // If there isn't one, at least remember that we looked so we don't try to load a second time @@ -521,8 +526,8 @@ function load() { // Get the default collection if necessary. Strings may be in the default or the locale json if (!defaultCollection) { const defaultNlsFile = path.join(EXTENSION_ROOT_DIR, 'package.nls.json'); - if (fs.existsSync(defaultNlsFile)) { - const contents = fs.readFileSync(defaultNlsFile, 'utf8'); + if (fs.fileExistsSync(defaultNlsFile)) { + const contents = fs.readFileSync(defaultNlsFile); defaultCollection = JSON.parse(contents); } else { defaultCollection = {}; From 454b25e7a642b51733f80202ae9499525f022c5e Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 10 Dec 2019 11:58:54 -0700 Subject: [PATCH 10/26] Add IFileSystem.isDirReadonly() and use it. --- .../common/installer/moduleInstaller.ts | 24 +++++-------------- src/client/common/platform/fileSystem.ts | 15 ++++++++++++ src/client/common/platform/types.ts | 1 + 3 files changed, 22 insertions(+), 18 deletions(-) diff --git a/src/client/common/installer/moduleInstaller.ts b/src/client/common/installer/moduleInstaller.ts index e2093898948a..85d5438ff443 100644 --- a/src/client/common/installer/moduleInstaller.ts +++ b/src/client/common/installer/moduleInstaller.ts @@ -1,7 +1,6 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. -import * as fs from 'fs'; import { injectable } from 'inversify'; import * as path from 'path'; import { CancellationToken, OutputChannel, ProgressLocation, ProgressOptions, window } from 'vscode'; @@ -12,10 +11,11 @@ import { EventName } from '../../telemetry/constants'; import { IApplicationShell } from '../application/types'; import { wrapCancellationTokens } from '../cancellation'; import { STANDARD_OUTPUT_CHANNEL } from '../constants'; +import { IFileSystem } from '../platform/types'; import { ITerminalServiceFactory } from '../terminal/types'; import { ExecutionInfo, IConfigurationService, IOutputChannel } from '../types'; import { Products } from '../utils/localize'; -import { isResource, noop } from '../utils/misc'; +import { isResource } from '../utils/misc'; import { IModuleInstaller, InterpreterUri } from './types'; @injectable() @@ -42,10 +42,11 @@ export abstract class ModuleInstaller implements IModuleInstaller { if (!interpreter || interpreter.type !== InterpreterType.Unknown) { await terminalService.sendCommand(pythonPath, args, token); } else if (settings.globalModuleInstallation) { - if (await this.isPathWritableAsync(path.dirname(pythonPath))) { - await terminalService.sendCommand(pythonPath, args, token); - } else { + const fs = this.serviceContainer.get(IFileSystem); + if (await fs.isDirReadonly(path.dirname(pythonPath))) { this.elevatedInstall(pythonPath, args); + } else { + await terminalService.sendCommand(pythonPath, args, token); } } else { await terminalService.sendCommand(pythonPath, args.concat(['--user']), token); @@ -88,19 +89,6 @@ export abstract class ModuleInstaller implements IModuleInstaller { } return args; } - private async isPathWritableAsync(directoryPath: string): Promise { - const filePath = `${directoryPath}${path.sep}___vscpTest___`; - return new Promise(resolve => { - fs.open(filePath, fs.constants.O_CREAT | fs.constants.O_RDWR, (error, fd) => { - if (!error) { - fs.close(fd, () => { - fs.unlink(filePath, noop); - }); - } - return resolve(!error); - }); - }); - } private elevatedInstall(execPath: string, args: string[]) { const options = { diff --git a/src/client/common/platform/fileSystem.ts b/src/client/common/platform/fileSystem.ts index 057113cf3e94..b0c0b57140ef 100644 --- a/src/client/common/platform/fileSystem.ts +++ b/src/client/common/platform/fileSystem.ts @@ -11,6 +11,7 @@ import * as path from 'path'; import * as tmp from 'tmp'; import { FileStat } from 'vscode'; import { createDeferred } from '../utils/async'; +import { noop } from '../utils/misc'; import { IFileSystem, IPlatformService, TemporaryFile } from './types'; @injectable() @@ -215,4 +216,18 @@ export class FileSystem implements IFileSystem { public readFileSync(filePath: string): string { return fs.readFileSync(filePath, 'utf8'); } + + public async isDirReadonly(dirname: string): Promise { + const filePath = `${dirname}${path.sep}___vscpTest___`; + return new Promise(resolve => { + fs.open(filePath, fs.constants.O_CREAT | fs.constants.O_RDWR, (error, fd) => { + if (!error) { + fs.close(fd, () => { + fs.unlink(filePath, noop); + }); + } + return resolve(!error); + }); + }); + } } diff --git a/src/client/common/platform/types.ts b/src/client/common/platform/types.ts index afa198489cc5..10951f7d6610 100644 --- a/src/client/common/platform/types.ts +++ b/src/client/common/platform/types.ts @@ -67,4 +67,5 @@ export interface IFileSystem { createReadStream(path: string): fs.ReadStream; createWriteStream(path: string): fs.WriteStream; chmod(path: string, mode: string): Promise; + isDirReadonly(dirname: string): Promise; } From a929fccc9d0c5af5ad594d3c9626a14bc382ba9b Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 10 Dec 2019 12:13:58 -0700 Subject: [PATCH 11/26] Add IFileSystem.move() and use it. --- src/client/common/platform/fileSystem.ts | 4 ++++ src/client/common/platform/types.ts | 1 + src/client/sourceMapSupport.ts | 13 +++++++------ 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/src/client/common/platform/fileSystem.ts b/src/client/common/platform/fileSystem.ts index b0c0b57140ef..c5c1f5490819 100644 --- a/src/client/common/platform/fileSystem.ts +++ b/src/client/common/platform/fileSystem.ts @@ -217,6 +217,10 @@ export class FileSystem implements IFileSystem { return fs.readFileSync(filePath, 'utf8'); } + public async move(src: string, tgt: string) { + await fs.rename(src, tgt); + } + public async isDirReadonly(dirname: string): Promise { const filePath = `${dirname}${path.sep}___vscpTest___`; return new Promise(resolve => { diff --git a/src/client/common/platform/types.ts b/src/client/common/platform/types.ts index 10951f7d6610..530c4519ec44 100644 --- a/src/client/common/platform/types.ts +++ b/src/client/common/platform/types.ts @@ -67,5 +67,6 @@ export interface IFileSystem { createReadStream(path: string): fs.ReadStream; createWriteStream(path: string): fs.WriteStream; chmod(path: string, mode: string): Promise; + move(src: string, tgt: string): Promise; isDirReadonly(dirname: string): Promise; } diff --git a/src/client/sourceMapSupport.ts b/src/client/sourceMapSupport.ts index 02907eaaf5ae..b64d8a5a2319 100644 --- a/src/client/sourceMapSupport.ts +++ b/src/client/sourceMapSupport.ts @@ -3,12 +3,12 @@ 'use strict'; -import * as fs from 'fs'; import * as path from 'path'; -import { promisify } from 'util'; import { WorkspaceConfiguration } from 'vscode'; import './common/extensions'; import { traceError } from './common/logger'; +import { FileSystem } from './common/platform/fileSystem'; +import { PlatformService } from './common/platform/platformService'; import { EXTENSION_ROOT_DIR } from './constants'; type VSCode = typeof import('vscode'); @@ -59,12 +59,13 @@ export class SourceMapSupport { } } protected async rename(sourceFile: string, targetFile: string) { - const fsExists = promisify(fs.exists); - const fsRename = promisify(fs.rename); - if (await fsExists(targetFile)) { + const fs = new FileSystem( + new PlatformService() + ); + if (await fs.fileExists(targetFile)) { return; } - await fsRename(sourceFile, targetFile); + await fs.move(sourceFile, targetFile); } } export function initialize(vscode: VSCode = require('vscode')) { From acf6c2cb77c15d0e6e01b4b5d0d6db6e9b72c6f4 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 10 Dec 2019 12:26:56 -0700 Subject: [PATCH 12/26] Drop utils/fs.fsExistsAsync(). --- src/client/common/utils/fs.ts | 7 ------- .../interpreter/locators/services/KnownPathsService.ts | 6 +++--- src/client/workspaceSymbols/parser.ts | 8 ++++++-- 3 files changed, 9 insertions(+), 12 deletions(-) diff --git a/src/client/common/utils/fs.ts b/src/client/common/utils/fs.ts index 4ab1e19686c8..9440c8d7075a 100644 --- a/src/client/common/utils/fs.ts +++ b/src/client/common/utils/fs.ts @@ -7,13 +7,6 @@ import * as fs from 'fs'; import * as path from 'path'; import * as tmp from 'tmp'; -export function fsExistsAsync(filePath: string): Promise { - return new Promise(resolve => { - fs.exists(filePath, exists => { - return resolve(exists); - }); - }); -} export function fsReaddirAsync(root: string): Promise { return new Promise(resolve => { // Now look for Interpreters in this directory diff --git a/src/client/interpreter/locators/services/KnownPathsService.ts b/src/client/interpreter/locators/services/KnownPathsService.ts index ee033e322bc7..7f04b90c787b 100644 --- a/src/client/interpreter/locators/services/KnownPathsService.ts +++ b/src/client/interpreter/locators/services/KnownPathsService.ts @@ -2,9 +2,8 @@ import { inject, injectable } from 'inversify'; import * as path from 'path'; import { Uri } from 'vscode'; -import { IPlatformService } from '../../../common/platform/types'; +import { IFileSystem, IPlatformService } from '../../../common/platform/types'; import { ICurrentProcess, IPathUtils } from '../../../common/types'; -import { fsExistsAsync } from '../../../common/utils/fs'; import { IServiceContainer } from '../../../ioc/types'; import { IInterpreterHelper, IKnownSearchPathsForInterpreters, InterpreterType, PythonInterpreter } from '../../contracts'; import { lookForInterpretersInDirectory } from '../helpers'; @@ -73,7 +72,8 @@ export class KnownPathsService extends CacheableLocatorService { * Return the interpreters in the given directory. */ private getInterpretersInDirectory(dir: string) { - return fsExistsAsync(dir) + const fs = this.serviceContainer.get(IFileSystem); + return fs.directoryExists(dir) .then(exists => exists ? lookForInterpretersInDirectory(dir) : Promise.resolve([])); } } diff --git a/src/client/workspaceSymbols/parser.ts b/src/client/workspaceSymbols/parser.ts index 54bd50571360..f9e729e9983e 100644 --- a/src/client/workspaceSymbols/parser.ts +++ b/src/client/workspaceSymbols/parser.ts @@ -1,6 +1,7 @@ import * as path from 'path'; import * as vscode from 'vscode'; -import { fsExistsAsync } from '../common/utils/fs'; +import { FileSystem } from '../common/platform/fileSystem'; +import { PlatformService } from '../common/platform/platformService'; import { ITag } from './contracts'; // tslint:disable:no-require-imports no-var-requires no-suspicious-comment @@ -109,7 +110,10 @@ export function parseTags( query: string, token: vscode.CancellationToken ): Promise { - return fsExistsAsync(tagFile).then(exists => { + const fs = new FileSystem( + new PlatformService() + ); + return fs.fileExists(tagFile).then(exists => { if (!exists) { return Promise.resolve([]); } From 76d85ba491c9cb73a512c51b5bb569f5699ee9a0 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 10 Dec 2019 12:35:11 -0700 Subject: [PATCH 13/26] Drop utils/fs.fsReaddirAsync(). --- src/client/common/utils/fs.ts | 12 ------------ src/client/interpreter/locators/helpers.ts | 7 +++---- .../locators/services/KnownPathsService.ts | 2 +- .../locators/services/baseVirtualEnvService.ts | 2 +- 4 files changed, 5 insertions(+), 18 deletions(-) diff --git a/src/client/common/utils/fs.ts b/src/client/common/utils/fs.ts index 9440c8d7075a..e35978422c84 100644 --- a/src/client/common/utils/fs.ts +++ b/src/client/common/utils/fs.ts @@ -7,18 +7,6 @@ import * as fs from 'fs'; import * as path from 'path'; import * as tmp from 'tmp'; -export function fsReaddirAsync(root: string): Promise { - return new Promise(resolve => { - // Now look for Interpreters in this directory - fs.readdir(root, (err, subDirs) => { - if (err) { - return resolve([]); - } - resolve(subDirs.map(subDir => path.join(root, subDir))); - }); - }); -} - export function getSubDirectories(rootDir: string): Promise { return new Promise(resolve => { fs.readdir(rootDir, (error, files) => { diff --git a/src/client/interpreter/locators/helpers.ts b/src/client/interpreter/locators/helpers.ts index bf1a0a8424dd..d6381f7e63de 100644 --- a/src/client/interpreter/locators/helpers.ts +++ b/src/client/interpreter/locators/helpers.ts @@ -3,17 +3,16 @@ import * as path from 'path'; import { traceError } from '../../common/logger'; import { IS_WINDOWS } from '../../common/platform/constants'; import { IFileSystem } from '../../common/platform/types'; -import { fsReaddirAsync } from '../../common/utils/fs'; import { IInterpreterLocatorHelper, InterpreterType, PythonInterpreter } from '../contracts'; import { IPipEnvServiceHelper } from './types'; const CheckPythonInterpreterRegEx = IS_WINDOWS ? /^python(\d+(.\d+)?)?\.exe$/ : /^python(\d+(.\d+)?)?$/; -export function lookForInterpretersInDirectory(pathToCheck: string): Promise { - return fsReaddirAsync(pathToCheck) +export function lookForInterpretersInDirectory(pathToCheck: string, fs: IFileSystem): Promise { + return fs.getFiles(pathToCheck) .then(subDirs => subDirs.filter(fileName => CheckPythonInterpreterRegEx.test(path.basename(fileName)))) .catch(err => { - traceError('Python Extension (lookForInterpretersInDirectory.fsReaddirAsync):', err); + traceError('Python Extension (lookForInterpretersInDirectory.fs.getFiles):', err); return [] as string[]; }); } diff --git a/src/client/interpreter/locators/services/KnownPathsService.ts b/src/client/interpreter/locators/services/KnownPathsService.ts index 7f04b90c787b..6f7c44478eaa 100644 --- a/src/client/interpreter/locators/services/KnownPathsService.ts +++ b/src/client/interpreter/locators/services/KnownPathsService.ts @@ -74,7 +74,7 @@ export class KnownPathsService extends CacheableLocatorService { private getInterpretersInDirectory(dir: string) { const fs = this.serviceContainer.get(IFileSystem); return fs.directoryExists(dir) - .then(exists => exists ? lookForInterpretersInDirectory(dir) : Promise.resolve([])); + .then(exists => exists ? lookForInterpretersInDirectory(dir, fs) : Promise.resolve([])); } } diff --git a/src/client/interpreter/locators/services/baseVirtualEnvService.ts b/src/client/interpreter/locators/services/baseVirtualEnvService.ts index 4b46de4a2261..1dec370789fd 100644 --- a/src/client/interpreter/locators/services/baseVirtualEnvService.ts +++ b/src/client/interpreter/locators/services/baseVirtualEnvService.ts @@ -40,7 +40,7 @@ export class BaseVirtualEnvService extends CacheableLocatorService { return this.fileSystem.getSubDirectories(pathToCheck) .then(subDirs => Promise.all(this.getProspectiveDirectoriesForLookup(subDirs))) .then(dirs => dirs.filter(dir => dir.length > 0)) - .then(dirs => Promise.all(dirs.map(lookForInterpretersInDirectory))) + .then(dirs => Promise.all(dirs.map(d => lookForInterpretersInDirectory(d, this.fileSystem)))) .then(pathsWithInterpreters => flatten(pathsWithInterpreters)) .then(interpreters => Promise.all(interpreters.map(interpreter => this.getVirtualEnvDetails(interpreter, resource)))) .then(interpreters => interpreters.filter(interpreter => !!interpreter).map(interpreter => interpreter!)) From a02b459c3cb3f93e65eaddf8eaf58b521c21ba3a Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 10 Dec 2019 12:38:22 -0700 Subject: [PATCH 14/26] Drop utils/fs.getSubDirectories(). --- src/client/common/utils/fs.ts | 24 ------------------- .../managers/testConfigurationManager.ts | 5 ++-- 2 files changed, 3 insertions(+), 26 deletions(-) diff --git a/src/client/common/utils/fs.ts b/src/client/common/utils/fs.ts index e35978422c84..a5abe18bc545 100644 --- a/src/client/common/utils/fs.ts +++ b/src/client/common/utils/fs.ts @@ -3,32 +3,8 @@ 'use strict'; -import * as fs from 'fs'; -import * as path from 'path'; import * as tmp from 'tmp'; -export function getSubDirectories(rootDir: string): Promise { - return new Promise(resolve => { - fs.readdir(rootDir, (error, files) => { - if (error) { - return resolve([]); - } - const subDirs: string[] = []; - files.forEach(name => { - const fullPath = path.join(rootDir, name); - try { - if (fs.statSync(fullPath).isDirectory()) { - subDirs.push(fullPath); - } - } - // tslint:disable-next-line:no-empty one-line - catch (ex) { } - }); - resolve(subDirs); - }); - }); -} - export function createTemporaryFile(extension: string, temporaryDirectory?: string): Promise<{ filePath: string; cleanupCallback: Function }> { // tslint:disable-next-line:no-any const options: any = { postfix: extension }; diff --git a/src/client/testing/common/managers/testConfigurationManager.ts b/src/client/testing/common/managers/testConfigurationManager.ts index ca4d7ce43ebb..229775c8d1a4 100644 --- a/src/client/testing/common/managers/testConfigurationManager.ts +++ b/src/client/testing/common/managers/testConfigurationManager.ts @@ -1,9 +1,9 @@ import * as path from 'path'; import { OutputChannel, QuickPickItem, Uri } from 'vscode'; import { IApplicationShell } from '../../../common/application/types'; +import { IFileSystem } from '../../../common/platform/types'; import { IInstaller, ILogger, IOutputChannel } from '../../../common/types'; import { createDeferred } from '../../../common/utils/async'; -import { getSubDirectories } from '../../../common/utils/fs'; import { IServiceContainer } from '../../../ioc/types'; import { ITestConfigSettingsService, ITestConfigurationManager } from '../../types'; import { TEST_OUTPUT_CHANNEL, UNIT_TEST_PRODUCTS } from '../constants'; @@ -101,7 +101,8 @@ export abstract class TestConfigurationManager implements ITestConfigurationMana return def.promise; } protected getTestDirs(rootDir: string): Promise { - return getSubDirectories(rootDir).then(subDirs => { + const fs = this.serviceContainer.get(IFileSystem); + return fs.getSubDirectories(rootDir).then(subDirs => { subDirs.sort(); // Find out if there are any dirs with the name test and place them on the top. From fa2737c9bf8a9b3bb2d22d15baa641be31eced1b Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 10 Dec 2019 12:57:28 -0700 Subject: [PATCH 15/26] Drop utils/fs.createTemporaryFile(). --- src/test/common/process/pythonDaemon.functional.test.ts | 2 +- src/test/common/process/pythonDaemonPool.functional.test.ts | 2 +- src/test/datascience/nativeEditor.functional.test.tsx | 2 +- src/{client/common => test}/utils/fs.ts | 0 4 files changed, 3 insertions(+), 3 deletions(-) rename src/{client/common => test}/utils/fs.ts (100%) diff --git a/src/test/common/process/pythonDaemon.functional.test.ts b/src/test/common/process/pythonDaemon.functional.test.ts index 7f6a865d9c82..be381fd96376 100644 --- a/src/test/common/process/pythonDaemon.functional.test.ts +++ b/src/test/common/process/pythonDaemon.functional.test.ts @@ -16,11 +16,11 @@ import { PythonDaemonExecutionService } from '../../../client/common/process/pyt import { PythonExecutionService } from '../../../client/common/process/pythonProcess'; import { IPythonExecutionService, PythonVersionInfo } from '../../../client/common/process/types'; import { IDisposable } from '../../../client/common/types'; -import { createTemporaryFile } from '../../../client/common/utils/fs'; import { Architecture } from '../../../client/common/utils/platform'; import { parsePythonVersion } from '../../../client/common/utils/version'; import { EXTENSION_ROOT_DIR } from '../../../client/constants'; import { isPythonVersion, PYTHON_PATH } from '../../common'; +import { createTemporaryFile } from '../../utils/fs'; use(chaiPromised); // tslint:disable-next-line: max-func-body-length diff --git a/src/test/common/process/pythonDaemonPool.functional.test.ts b/src/test/common/process/pythonDaemonPool.functional.test.ts index 190f42d7b4ed..32cb2b06597d 100644 --- a/src/test/common/process/pythonDaemonPool.functional.test.ts +++ b/src/test/common/process/pythonDaemonPool.functional.test.ts @@ -21,13 +21,13 @@ import { PythonExecutionService } from '../../../client/common/process/pythonPro import { IProcessLogger, IPythonDaemonExecutionService, IPythonExecutionService, ObservableExecutionResult, Output, PythonVersionInfo } from '../../../client/common/process/types'; import { IDisposable } from '../../../client/common/types'; import { sleep } from '../../../client/common/utils/async'; -import { createTemporaryFile } from '../../../client/common/utils/fs'; import { noop } from '../../../client/common/utils/misc'; import { Architecture } from '../../../client/common/utils/platform'; import { parsePythonVersion } from '../../../client/common/utils/version'; import { EXTENSION_ROOT_DIR } from '../../../client/constants'; import { PythonDaemonModule } from '../../../client/datascience/constants'; import { isPythonVersion, PYTHON_PATH, waitForCondition } from '../../common'; +import { createTemporaryFile } from '../../utils/fs'; use(chaiPromised); // tslint:disable: max-func-body-length diff --git a/src/test/datascience/nativeEditor.functional.test.tsx b/src/test/datascience/nativeEditor.functional.test.tsx index cf2d96068d18..893f5230aa9f 100644 --- a/src/test/datascience/nativeEditor.functional.test.tsx +++ b/src/test/datascience/nativeEditor.functional.test.tsx @@ -17,7 +17,6 @@ import { Disposable, TextDocument, TextEditor, Uri, WindowState } from 'vscode'; import { IApplicationShell, IDocumentManager } from '../../client/common/application/types'; import { IFileSystem } from '../../client/common/platform/types'; import { createDeferred, sleep, waitForPromise } from '../../client/common/utils/async'; -import { createTemporaryFile } from '../../client/common/utils/fs'; import { noop } from '../../client/common/utils/misc'; import { Identifiers } from '../../client/datascience/constants'; import { InteractiveWindowMessages } from '../../client/datascience/interactive-common/interactiveWindowTypes'; @@ -33,6 +32,7 @@ import { IKeyboardEvent } from '../../datascience-ui/react-common/event'; import { ImageButton } from '../../datascience-ui/react-common/imageButton'; import { IMonacoEditorState, MonacoEditor } from '../../datascience-ui/react-common/monacoEditor'; import { waitForCondition } from '../common'; +import { createTemporaryFile } from '../utils/fs'; import { DataScienceIocContainer } from './dataScienceIocContainer'; import { MockDocumentManager } from './mockDocumentManager'; import { addCell, closeNotebook, createNewEditor, getNativeCellResults, mountNativeWebView, openEditor, runMountedTest, setupWebview } from './nativeEditorTestHelpers'; diff --git a/src/client/common/utils/fs.ts b/src/test/utils/fs.ts similarity index 100% rename from src/client/common/utils/fs.ts rename to src/test/utils/fs.ts From 24e58e4ec20995512f2c9c7fe29ff47c98e4e083 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 10 Dec 2019 12:58:07 -0700 Subject: [PATCH 16/26] Use IFileSystem.search() instead of glob(). --- src/client/common/platform/fileSystem.ts | 23 +++++++++++++--------- src/client/common/platform/types.ts | 2 +- src/client/datascience/themeFinder.ts | 25 ++++++++---------------- 3 files changed, 23 insertions(+), 27 deletions(-) diff --git a/src/client/common/platform/fileSystem.ts b/src/client/common/platform/fileSystem.ts index c5c1f5490819..f557869d259f 100644 --- a/src/client/common/platform/fileSystem.ts +++ b/src/client/common/platform/fileSystem.ts @@ -9,11 +9,14 @@ import * as glob from 'glob'; import { inject, injectable } from 'inversify'; import * as path from 'path'; import * as tmp from 'tmp'; +import { promisify } from 'util'; import { FileStat } from 'vscode'; import { createDeferred } from '../utils/async'; import { noop } from '../utils/misc'; import { IFileSystem, IPlatformService, TemporaryFile } from './types'; +const globAsync = promisify(glob); + @injectable() export class FileSystem implements IFileSystem { constructor(@inject(IPlatformService) private platformService: IPlatformService) {} @@ -173,15 +176,17 @@ export class FileSystem implements IFileSystem { }); }); } - public search(globPattern: string): Promise { - return new Promise((resolve, reject) => { - glob(globPattern, (ex, files) => { - if (ex) { - return reject(ex); - } - resolve(Array.isArray(files) ? files : []); - }); - }); + public async search(globPattern: string, cwd?: string): Promise { + let found: string[]; + if (cwd) { + const options = { + cwd: cwd + }; + found = await globAsync(globPattern, options); + } else { + found = await globAsync(globPattern); + } + return Array.isArray(found) ? found : []; } public createTemporaryFile(extension: string): Promise { return new Promise((resolve, reject) => { diff --git a/src/client/common/platform/types.ts b/src/client/common/platform/types.ts index 530c4519ec44..bc13027ddfdf 100644 --- a/src/client/common/platform/types.ts +++ b/src/client/common/platform/types.ts @@ -62,7 +62,7 @@ export interface IFileSystem { copyFile(src: string, dest: string): Promise; deleteFile(filename: string): Promise; getFileHash(filePath: string): Promise; - search(globPattern: string): Promise; + search(globPattern: string, cwd?: string): Promise; createTemporaryFile(extension: string): Promise; createReadStream(path: string): fs.ReadStream; createWriteStream(path: string): fs.WriteStream; diff --git a/src/client/datascience/themeFinder.ts b/src/client/datascience/themeFinder.ts index 3c9a9e96ca70..c0429e322a89 100644 --- a/src/client/datascience/themeFinder.ts +++ b/src/client/datascience/themeFinder.ts @@ -1,7 +1,6 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. 'use strict'; -import * as glob from 'glob'; import { inject, injectable } from 'inversify'; import * as path from 'path'; @@ -107,14 +106,10 @@ export class ThemeFinder implements IThemeFinder { // Search through all package.json files in the directory and below, looking // for the themeName in them. - const foundPackages = await new Promise((resolve, reject) => { - glob('**/package.json', { cwd: rootPath }, (err, matches) => { - if (err) { - reject(err); - } - resolve(matches); - }); - }); + const fs = new FileSystem( + new PlatformService() + ); + const foundPackages = await fs.search('**/package.json', rootPath); if (foundPackages.length > 0) { // For each one, open it up and look for the theme name. for (const f of foundPackages) { @@ -167,14 +162,10 @@ export class ThemeFinder implements IThemeFinder { private async findMatchingThemes(rootPath: string, themeName: string) : Promise { // Search through all package.json files in the directory and below, looking // for the themeName in them. - const foundPackages = await new Promise((resolve, reject) => { - glob('**/package.json', { cwd: rootPath }, (err, matches) => { - if (err) { - reject(err); - } - resolve(matches); - }); - }); + const fs = new FileSystem( + new PlatformService() + ); + const foundPackages = await fs.search('**/package.json', rootPath); if (foundPackages.length > 0) { // For each one, open it up and look for the theme name. for (const f of foundPackages) { From d7df4325b0dc738200e72e1c9be97dff050ab6e6 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 10 Dec 2019 14:11:07 -0700 Subject: [PATCH 17/26] Fix the Windows registry tests. --- .../interpreters/windowsRegistryService.unit.test.ts | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/test/interpreters/windowsRegistryService.unit.test.ts b/src/test/interpreters/windowsRegistryService.unit.test.ts index e0cf7bffd056..0df7f96752e1 100644 --- a/src/test/interpreters/windowsRegistryService.unit.test.ts +++ b/src/test/interpreters/windowsRegistryService.unit.test.ts @@ -1,7 +1,8 @@ import * as assert from 'assert'; +import * as fsextra from 'fs-extra'; import * as path from 'path'; import * as TypeMoq from 'typemoq'; -import { IPlatformService, RegistryHive } from '../../client/common/platform/types'; +import { IFileSystem, IPlatformService, RegistryHive } from '../../client/common/platform/types'; import { IPathUtils, IPersistentStateFactory } from '../../client/common/types'; import { Architecture } from '../../client/common/utils/platform'; import { IInterpreterHelper, InterpreterType } from '../../client/interpreter/contracts'; @@ -18,6 +19,7 @@ suite('Interpreters from Windows Registry (unit)', () => { let serviceContainer: TypeMoq.IMock; let interpreterHelper: TypeMoq.IMock; let platformService: TypeMoq.IMock; + let fs: TypeMoq.IMock; let windowsStoreInterpreter: TypeMoq.IMock; setup(() => { serviceContainer = TypeMoq.Mock.ofType(); @@ -25,13 +27,20 @@ suite('Interpreters from Windows Registry (unit)', () => { interpreterHelper = TypeMoq.Mock.ofType(); const pathUtils = TypeMoq.Mock.ofType(); platformService = TypeMoq.Mock.ofType(); + fs = TypeMoq.Mock.ofType(); windowsStoreInterpreter = TypeMoq.Mock.ofType(); windowsStoreInterpreter.setup(w => w.isHiddenInterpreter(TypeMoq.It.isAny())).returns(() => false); windowsStoreInterpreter.setup(w => w.isWindowsStoreInterpreter(TypeMoq.It.isAny())).returns(() => false); serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IPersistentStateFactory))).returns(() => stateFactory.object); serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IInterpreterHelper))).returns(() => interpreterHelper.object); serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IPathUtils))).returns(() => pathUtils.object); + serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IFileSystem))).returns(() => fs.object); pathUtils.setup(p => p.basename(TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns((p: string) => p.split(/[\\,\/]/).reverse()[0]); + // So effectively these are functional tests... + fs.setup(f => f.fileExists(TypeMoq.It.isAny())) + .returns(filename => { + return fsextra.pathExists(filename); + }); const state = new MockState(undefined); // tslint:disable-next-line:no-empty no-any interpreterHelper.setup(h => h.getInterpreterInformation(TypeMoq.It.isAny())).returns(() => Promise.resolve({} as any)); From 913a27840e3351f246982193c621764cbc05ceb9 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Thu, 12 Dec 2019 12:55:49 -0700 Subject: [PATCH 18/26] Add IFileSystem.readData() and use it. --- src/client/common/platform/fileSystem.ts | 3 +++ src/client/common/platform/types.ts | 1 + src/client/datascience/interactive-common/interactiveBase.ts | 4 ++-- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/client/common/platform/fileSystem.ts b/src/client/common/platform/fileSystem.ts index f557869d259f..a75df6af898c 100644 --- a/src/client/common/platform/fileSystem.ts +++ b/src/client/common/platform/fileSystem.ts @@ -58,6 +58,9 @@ export class FileSystem implements IFileSystem { public readFile(filePath: string): Promise { return fs.readFile(filePath).then(buffer => buffer.toString()); } + public readData(filePath: string): Promise { + return fs.readFile(filePath); + } public async writeFile(filePath: string, data: {}, options: string | fs.WriteFileOptions = { encoding: 'utf8' }): Promise { await fs.writeFile(filePath, data, options); diff --git a/src/client/common/platform/types.ts b/src/client/common/platform/types.ts index bc13027ddfdf..2277685e76e7 100644 --- a/src/client/common/platform/types.ts +++ b/src/client/common/platform/types.ts @@ -50,6 +50,7 @@ export interface IFileSystem { getSubDirectories(rootDir: string): Promise; getFiles(rootDir: string): Promise; arePathsSame(path1: string, path2: string): boolean; + readData(filePath: string): Promise; readFile(filePath: string): Promise; writeFile(filePath: string, data: {}, options?: string | fsextra.WriteFileOptions): Promise; readFileSync(filename: string): string; diff --git a/src/client/datascience/interactive-common/interactiveBase.ts b/src/client/datascience/interactive-common/interactiveBase.ts index 41e5b78ce258..2245c30ebbdc 100644 --- a/src/client/datascience/interactive-common/interactiveBase.ts +++ b/src/client/datascience/interactive-common/interactiveBase.ts @@ -1264,14 +1264,14 @@ export abstract class InteractiveBase extends WebViewHost Date: Thu, 12 Dec 2019 13:10:43 -0700 Subject: [PATCH 19/26] Drop an unused file. --- src/client/common/envFileParser.ts | 77 ------------------------------ 1 file changed, 77 deletions(-) delete mode 100644 src/client/common/envFileParser.ts diff --git a/src/client/common/envFileParser.ts b/src/client/common/envFileParser.ts deleted file mode 100644 index d3ce08827a99..000000000000 --- a/src/client/common/envFileParser.ts +++ /dev/null @@ -1,77 +0,0 @@ -import { IS_WINDOWS } from './platform/constants'; -import { FileSystem } from './platform/fileSystem'; -import { PathUtils } from './platform/pathUtils'; -import { PlatformService } from './platform/platformService'; -import { EnvironmentVariablesService } from './variables/environment'; -import { EnvironmentVariables } from './variables/types'; -function parseEnvironmentVariables(contents: string): EnvironmentVariables | undefined { - if (typeof contents !== 'string' || contents.length === 0) { - return undefined; - } - - const env: EnvironmentVariables = {}; - contents.split('\n').forEach(line => { - const match = line.match(/^\s*([\w\.\-]+)\s*=\s*(.*)?\s*$/); - if (match !== null) { - let value = typeof match[2] === 'string' ? match[2] : ''; - if (value.length > 0 && value.charAt(0) === '"' && value.charAt(value.length - 1) === '"') { - value = value.replace(/\\n/gm, '\n'); - } - env[match[1]] = value.replace(/(^['"]|['"]$)/g, ''); - } - }); - return env; -} - -export function parseEnvFile(envFile: string, mergeWithProcessEnvVars: boolean = true): EnvironmentVariables { - const fs = new FileSystem( - new PlatformService() - ); - const buffer = fs.readFileSync(envFile); - const env = parseEnvironmentVariables(buffer)!; - return mergeWithProcessEnvVars ? mergeEnvVariables(env, process.env) : mergePythonPath(env, process.env.PYTHONPATH as string); -} - -/** - * Merge the target environment variables into the source. - * Note: The source variables are modified and returned (i.e. it modifies value passed in). - * @export - * @param {EnvironmentVariables} targetEnvVars target environment variables. - * @param {EnvironmentVariables} [sourceEnvVars=process.env] source environment variables (defaults to current process variables). - * @returns {EnvironmentVariables} - */ -export function mergeEnvVariables(targetEnvVars: EnvironmentVariables, sourceEnvVars: EnvironmentVariables = process.env): EnvironmentVariables { - const service = new EnvironmentVariablesService( - new PathUtils(IS_WINDOWS), - new FileSystem( - new PlatformService() - ) - ); - service.mergeVariables(sourceEnvVars, targetEnvVars); - if (sourceEnvVars.PYTHONPATH) { - service.appendPythonPath(targetEnvVars, sourceEnvVars.PYTHONPATH); - } - return targetEnvVars; -} - -/** - * Merge the target PYTHONPATH value into the env variables passed. - * Note: The env variables passed in are modified and returned (i.e. it modifies value passed in). - * @export - * @param {EnvironmentVariables} env target environment variables. - * @param {string | undefined} [currentPythonPath] PYTHONPATH value. - * @returns {EnvironmentVariables} - */ -export function mergePythonPath(env: EnvironmentVariables, currentPythonPath: string | undefined): EnvironmentVariables { - if (typeof currentPythonPath !== 'string' || currentPythonPath.length === 0) { - return env; - } - const service = new EnvironmentVariablesService( - new PathUtils(IS_WINDOWS), - new FileSystem( - new PlatformService() - ) - ); - service.appendPythonPath(env, currentPythonPath); - return env; -} From fb7bcb42611a91cf803619f8d20fdc843122a03e Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Thu, 12 Dec 2019 13:27:42 -0700 Subject: [PATCH 20/26] Pass IFileSystem in where needed. --- src/client/common/editor.ts | 13 +++---------- src/client/formatters/baseFormatter.ts | 3 ++- src/client/providers/renameProvider.ts | 4 +++- src/client/workspaceSymbols/parser.ts | 9 +++------ src/client/workspaceSymbols/provider.ts | 2 +- 5 files changed, 12 insertions(+), 19 deletions(-) diff --git a/src/client/common/editor.ts b/src/client/common/editor.ts index 40cd080b9b01..3dff683568f5 100644 --- a/src/client/common/editor.ts +++ b/src/client/common/editor.ts @@ -4,8 +4,7 @@ import * as md5 from 'md5'; import { EOL } from 'os'; import * as path from 'path'; import { Position, Range, TextDocument, TextEdit, Uri, WorkspaceEdit } from 'vscode'; -import { FileSystem } from './platform/fileSystem'; -import { PlatformService } from './platform/platformService'; +import { IFileSystem } from '../common/platform/types'; import { IEditorUtils } from './types'; // Code borrowed from goFormat.ts (Go Extension for VS Code) @@ -81,7 +80,7 @@ export function getTextEditsFromPatch(before: string, patch: string): TextEdit[] return textEdits; } -export function getWorkspaceEditsFromPatch(filePatches: string[], workspaceRoot?: string): WorkspaceEdit { +export function getWorkspaceEditsFromPatch(filePatches: string[], workspaceRoot: string | undefined, fs: IFileSystem): WorkspaceEdit { const workspaceEdit = new WorkspaceEdit(); filePatches.forEach(patch => { const indexOfAtAt = patch.indexOf('@@'); @@ -106,9 +105,6 @@ export function getWorkspaceEditsFromPatch(filePatches: string[], workspaceRoot? return; } - const fs = new FileSystem( - new PlatformService() - ); let fileName = fileNameLines[0].substring(fileNameLines[0].indexOf(' a') + 3).trim(); fileName = workspaceRoot && !path.isAbsolute(fileName) ? path.resolve(workspaceRoot, fileName) : fileName; if (!fs.fileExistsSync(fileName)) { @@ -230,7 +226,7 @@ function getTextEditsInternal(before: string, diffs: [number, string][], startLi return edits; } -export async function getTempFileWithDocumentContents(document: TextDocument): Promise { +export async function getTempFileWithDocumentContents(document: TextDocument, fs: IFileSystem): Promise { const ext = path.extname(document.uri.fsPath); // Don't create file in temp folder since external utilities // look into configuration files in the workspace and are not @@ -240,9 +236,6 @@ export async function getTempFileWithDocumentContents(document: TextDocument): P // tslint:disable-next-line:no-require-imports const fileName = `${document.uri.fsPath}.${md5(document.uri.fsPath)}${ext}`; - const fs = new FileSystem( - new PlatformService() - ); try { await fs.writeFile(fileName, document.getText()); } catch (ex) { diff --git a/src/client/formatters/baseFormatter.ts b/src/client/formatters/baseFormatter.ts index 505598365ecd..1b5cfa0367c1 100644 --- a/src/client/formatters/baseFormatter.ts +++ b/src/client/formatters/baseFormatter.ts @@ -102,8 +102,9 @@ export abstract class BaseFormatter { } private async createTempFile(document: vscode.TextDocument): Promise { + const fs = this.serviceContainer.get(IFileSystem); return document.isDirty - ? getTempFileWithDocumentContents(document) + ? getTempFileWithDocumentContents(document, fs) : document.fileName; } diff --git a/src/client/providers/renameProvider.ts b/src/client/providers/renameProvider.ts index a29de3281be6..e0d73d96cf73 100644 --- a/src/client/providers/renameProvider.ts +++ b/src/client/providers/renameProvider.ts @@ -6,6 +6,7 @@ import { import { EXTENSION_ROOT_DIR, STANDARD_OUTPUT_CHANNEL } from '../common/constants'; import { getWorkspaceEditsFromPatch } from '../common/editor'; import { traceError } from '../common/logger'; +import { IFileSystem } from '../common/platform/types'; import { IConfigurationService, IInstaller, IOutputChannel, Product } from '../common/types'; import { IServiceContainer } from '../ioc/types'; import { RefactorProxy } from '../refactor/proxy'; @@ -57,7 +58,8 @@ export class PythonRenameProvider implements RenameProvider { const proxy = new RefactorProxy(EXTENSION_ROOT_DIR, pythonSettings, workspaceRoot, this.serviceContainer); return proxy.rename(document, newName, document.uri.fsPath, range).then(response => { const fileDiffs = response.results.map(fileChanges => fileChanges.diff); - return getWorkspaceEditsFromPatch(fileDiffs, workspaceRoot); + const fs = this.serviceContainer.get(IFileSystem); + return getWorkspaceEditsFromPatch(fileDiffs, workspaceRoot, fs); }).catch(reason => { if (reason === 'Not installed') { const installer = this.serviceContainer.get(IInstaller); diff --git a/src/client/workspaceSymbols/parser.ts b/src/client/workspaceSymbols/parser.ts index f9e729e9983e..6a0797f9b6d9 100644 --- a/src/client/workspaceSymbols/parser.ts +++ b/src/client/workspaceSymbols/parser.ts @@ -1,7 +1,6 @@ import * as path from 'path'; import * as vscode from 'vscode'; -import { FileSystem } from '../common/platform/fileSystem'; -import { PlatformService } from '../common/platform/platformService'; +import { IFileSystem } from '../common/platform/types'; import { ITag } from './contracts'; // tslint:disable:no-require-imports no-var-requires no-suspicious-comment @@ -108,11 +107,9 @@ export function parseTags( workspaceFolder: string, tagFile: string, query: string, - token: vscode.CancellationToken + token: vscode.CancellationToken, + fs: IFileSystem ): Promise { - const fs = new FileSystem( - new PlatformService() - ); return fs.fileExists(tagFile).then(exists => { if (!exists) { return Promise.resolve([]); diff --git a/src/client/workspaceSymbols/provider.ts b/src/client/workspaceSymbols/provider.ts index 019bcd25592d..2ced11d70175 100644 --- a/src/client/workspaceSymbols/provider.ts +++ b/src/client/workspaceSymbols/provider.ts @@ -43,7 +43,7 @@ export class WorkspaceSymbolProvider implements IWorspaceSymbolProvider { .filter(generator => generator !== undefined && generator.enabled) .map(async generator => { // load tags - const items = await parseTags(generator!.workspaceFolder.fsPath, generator!.tagFilePath, query, token); + const items = await parseTags(generator!.workspaceFolder.fsPath, generator!.tagFilePath, query, token, this.fs); if (!Array.isArray(items)) { return []; } From 33377cbf3190188ec395d4e94513c97a9f080a72 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Thu, 12 Dec 2019 13:54:17 -0700 Subject: [PATCH 21/26] Pass IFileSystem in where needed (in the datascience code). --- .../application/webPanels/webPanelProvider.ts | 2 +- .../application/webPanels/webPanelServer.ts | 10 ++--- .../datascience/jupyter/jupyterNotebook.ts | 18 +++----- .../jupyter/jupyterServerFactory.ts | 7 +++- .../jupyter/liveshare/hostJupyterNotebook.ts | 6 ++- .../jupyter/liveshare/hostJupyterServer.ts | 8 +++- src/client/datascience/themeFinder.ts | 42 ++++++------------- .../common/webPanel/webPanel.unit.test.ts | 8 +++- src/test/datascience/color.test.ts | 8 ++-- 9 files changed, 49 insertions(+), 60 deletions(-) diff --git a/src/client/common/application/webPanels/webPanelProvider.ts b/src/client/common/application/webPanels/webPanelProvider.ts index 3d6b04fc8a07..780fe2a86f6d 100644 --- a/src/client/common/application/webPanels/webPanelProvider.ts +++ b/src/client/common/application/webPanels/webPanelProvider.ts @@ -39,7 +39,7 @@ export class WebPanelProvider implements IWebPanelProvider { const webPanelServerModule = require('./webPanelServer') as typeof import('./webPanelServer'); // Start the server listening. - const webPanelServer = new webPanelServerModule.WebPanelServer(this.port, this.token); + const webPanelServer = new webPanelServerModule.WebPanelServer(this.port, this.token, this.fs); webPanelServer.start(); this.disposableRegistry.push(webPanelServer); } diff --git a/src/client/common/application/webPanels/webPanelServer.ts b/src/client/common/application/webPanels/webPanelServer.ts index d5fecd1f371b..f19eff31fac7 100644 --- a/src/client/common/application/webPanels/webPanelServer.ts +++ b/src/client/common/application/webPanels/webPanelServer.ts @@ -10,8 +10,7 @@ import * as path from 'path'; import { EXTENSION_ROOT_DIR } from '../../../constants'; import { Identifiers } from '../../../datascience/constants'; import { SharedMessages } from '../../../datascience/messages'; -import { FileSystem } from '../../platform/fileSystem'; -import { PlatformService } from '../../platform/platformService'; +import { IFileSystem } from '../../platform/types'; interface IState { cwd: string; @@ -24,7 +23,7 @@ export class WebPanelServer { private server: http.Server | undefined; private state = new Map(); - constructor(private port: number, private token: string) { + constructor(private port: number, private token: string, private fs: IFileSystem) { this.app.use(Cors()); this.app.use(compress()); this.app.use(logger()); @@ -111,10 +110,7 @@ export class WebPanelServer { break; } - const fs = new FileSystem( - new PlatformService() - ); - ctx.body = fs.createReadStream(filePath); + ctx.body = this.fs.createReadStream(filePath); } // Debugging tips: diff --git a/src/client/datascience/jupyter/jupyterNotebook.ts b/src/client/datascience/jupyter/jupyterNotebook.ts index 5aa09dd1b7d3..189927d556f3 100644 --- a/src/client/datascience/jupyter/jupyterNotebook.ts +++ b/src/client/datascience/jupyter/jupyterNotebook.ts @@ -12,8 +12,7 @@ import { IApplicationShell, ILiveShareApi, IWorkspaceService } from '../../commo import { Cancellation, CancellationError } from '../../common/cancellation'; import '../../common/extensions'; import { traceError, traceInfo, traceWarning } from '../../common/logger'; -import { FileSystem } from '../../common/platform/fileSystem'; -import { PlatformService } from '../../common/platform/platformService'; +import { IFileSystem } from '../../common/platform/types'; import { IConfigurationService, IDisposableRegistry } from '../../common/types'; import { createDeferred, Deferred, waitForPromise } from '../../common/utils/async'; import * as localize from '../../common/utils/localize'; @@ -161,7 +160,8 @@ export class JupyterNotebookBase implements INotebook { resource: Uri, private getDisposedError: () => Error, private workspace: IWorkspaceService, - private applicationService: IApplicationShell + private applicationService: IApplicationShell, + private fs: IFileSystem ) { this.sessionStartTime = Date.now(); @@ -689,16 +689,13 @@ export class JupyterNotebookBase implements INotebook { private async updateWorkingDirectory(launchingFile?: string): Promise { if (this.launchInfo && this.launchInfo.connectionInfo.localLaunch && !this._workingDirectory) { - const fs = new FileSystem( - new PlatformService() - ); // See what our working dir is supposed to be const suggested = this.launchInfo.workingDir; - if (suggested && (await fs.fileExists(suggested))) { + if (suggested && (await this.fs.fileExists(suggested))) { // We should use the launch info directory. It trumps the possible dir this._workingDirectory = suggested; return this.changeDirectoryIfPossible(this._workingDirectory); - } else if (launchingFile && (await fs.fileExists(launchingFile))) { + } else if (launchingFile && (await this.fs.fileExists(launchingFile))) { // Combine the working directory with this file if possible. this._workingDirectory = expandWorkingDir(this.launchInfo.workingDir, launchingFile, this.workspace); if (this._workingDirectory) { @@ -709,10 +706,7 @@ export class JupyterNotebookBase implements INotebook { } private changeDirectoryIfPossible = async (directory: string): Promise => { - const fs = new FileSystem( - new PlatformService() - ); - if (this.launchInfo && this.launchInfo.connectionInfo.localLaunch && (await fs.directoryExists(directory))) { + if (this.launchInfo && this.launchInfo.connectionInfo.localLaunch && (await this.fs.directoryExists(directory))) { await this.executeSilently(`%cd "${directory}"`); } } diff --git a/src/client/datascience/jupyter/jupyterServerFactory.ts b/src/client/datascience/jupyter/jupyterServerFactory.ts index 7f68c8ccec2e..20ee4b3f7e4f 100644 --- a/src/client/datascience/jupyter/jupyterServerFactory.ts +++ b/src/client/datascience/jupyter/jupyterServerFactory.ts @@ -8,6 +8,7 @@ import { CancellationToken } from 'vscode-jsonrpc'; import * as vsls from 'vsls/vscode'; import { IApplicationShell, ILiveShareApi, IWorkspaceService } from '../../common/application/types'; import '../../common/extensions'; +import { IFileSystem } from '../../common/platform/types'; import { IAsyncDisposableRegistry, IConfigurationService, IDisposableRegistry } from '../../common/types'; import { IInterpreterService } from '../../interpreter/contracts'; import { IConnection, IDataScience, IJupyterSessionManagerFactory, INotebook, INotebookExecutionLogger, INotebookServer, INotebookServerLaunchInfo } from '../types'; @@ -30,6 +31,7 @@ type JupyterServerClassType = { workspaceService: IWorkspaceService, loggers: INotebookExecutionLogger[], appShell: IApplicationShell, + fs: IFileSystem, interpreterService: IInterpreterService ): IJupyterServerInterface; }; @@ -52,7 +54,9 @@ export class JupyterServerFactory implements INotebookServer, ILiveShareHasRole @inject(IWorkspaceService) workspaceService: IWorkspaceService, @multiInject(INotebookExecutionLogger) @optional() loggers: INotebookExecutionLogger[] | undefined, @inject(IApplicationShell) appShell: IApplicationShell, - @inject(IInterpreterService) interpreterService: IInterpreterService) { + @inject(IFileSystem) fs: IFileSystem, + @inject(IInterpreterService) interpreterService: IInterpreterService + ) { this.serverFactory = new RoleBasedFactory( liveShare, HostJupyterServer, @@ -66,6 +70,7 @@ export class JupyterServerFactory implements INotebookServer, ILiveShareHasRole workspaceService, loggers ? loggers : [], appShell, + fs, interpreterService ); } diff --git a/src/client/datascience/jupyter/liveshare/hostJupyterNotebook.ts b/src/client/datascience/jupyter/liveshare/hostJupyterNotebook.ts index fe877b90257f..7933137fc2f0 100644 --- a/src/client/datascience/jupyter/liveshare/hostJupyterNotebook.ts +++ b/src/client/datascience/jupyter/liveshare/hostJupyterNotebook.ts @@ -8,6 +8,7 @@ import * as vsls from 'vsls/vscode'; import { IApplicationShell, ILiveShareApi, IWorkspaceService } from '../../../common/application/types'; import '../../../common/extensions'; import { traceError } from '../../../common/logger'; +import { IFileSystem } from '../../../common/platform/types'; import { IConfigurationService, IDisposableRegistry } from '../../../common/types'; import { createDeferred } from '../../../common/utils/async'; import { Identifiers, LiveShare, LiveShareCommands } from '../../constants'; @@ -43,9 +44,10 @@ export class HostJupyterNotebook resource: vscode.Uri, getDisposedError: () => Error, workspace: IWorkspaceService, - appService: IApplicationShell + appService: IApplicationShell, + fs: IFileSystem ) { - super(liveShare, session, configService, disposableRegistry, owner, launchInfo, loggers, resource, getDisposedError, workspace, appService); + super(liveShare, session, configService, disposableRegistry, owner, launchInfo, loggers, resource, getDisposedError, workspace, appService, fs); } public dispose = async (): Promise => { diff --git a/src/client/datascience/jupyter/liveshare/hostJupyterServer.ts b/src/client/datascience/jupyter/liveshare/hostJupyterServer.ts index b8141b7b110f..c9048a669654 100644 --- a/src/client/datascience/jupyter/liveshare/hostJupyterServer.ts +++ b/src/client/datascience/jupyter/liveshare/hostJupyterServer.ts @@ -10,6 +10,7 @@ import * as vsls from 'vsls/vscode'; import { IApplicationShell, ILiveShareApi, IWorkspaceService } from '../../../common/application/types'; import { traceInfo } from '../../../common/logger'; +import { IFileSystem } from '../../../common/platform/types'; import { IAsyncDisposableRegistry, IConfigurationService, IDisposableRegistry } from '../../../common/types'; import * as localize from '../../../common/utils/localize'; import { Identifiers, LiveShare, LiveShareCommands, RegExpValues } from '../../constants'; @@ -46,7 +47,8 @@ export class HostJupyterServer sessionManager: IJupyterSessionManagerFactory, private workspaceService: IWorkspaceService, loggers: INotebookExecutionLogger[], - private appService: IApplicationShell + private appService: IApplicationShell, + private fs: IFileSystem ) { super(liveShare, asyncRegistry, disposableRegistry, configService, sessionManager, loggers); } @@ -176,7 +178,9 @@ export class HostJupyterServer resource, this.getDisposedError.bind(this), this.workspaceService, - this.appService); + this.appService, + this.fs + ); // Wait for it to be ready traceInfo(`Waiting for idle (session) ${this.id}`); diff --git a/src/client/datascience/themeFinder.ts b/src/client/datascience/themeFinder.ts index c0429e322a89..f417c1b9b522 100644 --- a/src/client/datascience/themeFinder.ts +++ b/src/client/datascience/themeFinder.ts @@ -5,8 +5,7 @@ import { inject, injectable } from 'inversify'; import * as path from 'path'; import { EXTENSION_ROOT_DIR, PYTHON_LANGUAGE } from '../common/constants'; -import { FileSystem } from '../common/platform/fileSystem'; -import { PlatformService } from '../common/platform/platformService'; +import { IFileSystem } from '../common/platform/types'; import { ICurrentProcess, IExtensions, ILogger } from '../common/types'; import { IThemeFinder } from './types'; @@ -25,8 +24,9 @@ export class ThemeFinder implements IThemeFinder { constructor( @inject(IExtensions) private extensions: IExtensions, @inject(ICurrentProcess) private currentProcess: ICurrentProcess, - @inject(ILogger) private logger: ILogger) { - } + @inject(ILogger) private logger: ILogger, + @inject(IFileSystem) private fs: IFileSystem + ) { } public async findThemeRootJson(themeName: string) : Promise { // find our data @@ -78,10 +78,7 @@ export class ThemeFinder implements IThemeFinder { // Should be somewhere under currentPath/resources/app/extensions inside of a json file let extensionsPath = path.join(currentPath, 'resources', 'app', 'extensions'); - const fs = new FileSystem( - new PlatformService() - ); - if (!(await fs.directoryExists(extensionsPath))) { + if (!(await this.fs.directoryExists(extensionsPath))) { // Might be on mac or linux. try a different path currentPath = path.resolve(currentPath, '../../../..'); extensionsPath = path.join(currentPath, 'resources', 'app', 'extensions'); @@ -92,7 +89,7 @@ export class ThemeFinder implements IThemeFinder { // If that didn't work, see if it's our MagicPython predefined tmLanguage if (!results && language === PYTHON_LANGUAGE) { - results = await fs.readFile(path.join(EXTENSION_ROOT_DIR, 'resources', 'MagicPython.tmLanguage.json')); + results = await this.fs.readFile(path.join(EXTENSION_ROOT_DIR, 'resources', 'MagicPython.tmLanguage.json')); } return results; @@ -106,10 +103,7 @@ export class ThemeFinder implements IThemeFinder { // Search through all package.json files in the directory and below, looking // for the themeName in them. - const fs = new FileSystem( - new PlatformService() - ); - const foundPackages = await fs.search('**/package.json', rootPath); + const foundPackages = await this.fs.search('**/package.json', rootPath); if (foundPackages.length > 0) { // For each one, open it up and look for the theme name. for (const f of foundPackages) { @@ -145,10 +139,7 @@ export class ThemeFinder implements IThemeFinder { // Should be somewhere under currentPath/resources/app/extensions inside of a json file let extensionsPath = path.join(currentPath, 'resources', 'app', 'extensions'); - const fs = new FileSystem( - new PlatformService() - ); - if (!(await fs.directoryExists(extensionsPath))) { + if (!(await this.fs.directoryExists(extensionsPath))) { // Might be on mac or linux. try a different path currentPath = path.resolve(currentPath, '../../../..'); extensionsPath = path.join(currentPath, 'resources', 'app', 'extensions'); @@ -162,10 +153,7 @@ export class ThemeFinder implements IThemeFinder { private async findMatchingThemes(rootPath: string, themeName: string) : Promise { // Search through all package.json files in the directory and below, looking // for the themeName in them. - const fs = new FileSystem( - new PlatformService() - ); - const foundPackages = await fs.search('**/package.json', rootPath); + const foundPackages = await this.fs.search('**/package.json', rootPath); if (foundPackages.length > 0) { // For each one, open it up and look for the theme name. for (const f of foundPackages) { @@ -179,11 +167,8 @@ export class ThemeFinder implements IThemeFinder { } private async findMatchingLanguageFromJson(packageJson: string, language: string) : Promise { - const fs = new FileSystem( - new PlatformService() - ); // Read the contents of the json file - const text = await fs.readFile(packageJson); + const text = await this.fs.readFile(packageJson); const json = JSON.parse(text); // Should have a name entry and a contributes entry @@ -197,7 +182,7 @@ export class ThemeFinder implements IThemeFinder { if (t.hasOwnProperty('language') && t.language === language) { // Path is relative to the package.json file. const rootFile = t.hasOwnProperty('path') ? path.join(path.dirname(packageJson), t.path.toString()) : ''; - return fs.readFile(rootFile); + return this.fs.readFile(rootFile); } } } @@ -205,11 +190,8 @@ export class ThemeFinder implements IThemeFinder { } private async findMatchingThemeFromJson(packageJson: string, themeName: string) : Promise { - const fs = new FileSystem( - new PlatformService() - ); // Read the contents of the json file - const text = await fs.readFile(packageJson); + const text = await this.fs.readFile(packageJson); const json = JSON.parse(text); // Should have a name entry and a contributes entry diff --git a/src/test/common/webPanel/webPanel.unit.test.ts b/src/test/common/webPanel/webPanel.unit.test.ts index 40596f64aa26..540dd2ea13ea 100644 --- a/src/test/common/webPanel/webPanel.unit.test.ts +++ b/src/test/common/webPanel/webPanel.unit.test.ts @@ -11,6 +11,8 @@ import chaiHttp = require('chai-http'); chai.use(chaiHttp); import { WebPanelServer } from '../../../client/common/application/webPanels/webPanelServer'; +import { FileSystem } from '../../../client/common/platform/fileSystem'; +import { PlatformService } from '../../../client/common/platform/platformService'; import { EXTENSION_ROOT_DIR } from '../../../client/constants'; // tslint:disable:no-any @@ -21,7 +23,11 @@ suite('WebPanelServer', () => { const token = uuid(); const historyBundle = path.join(EXTENSION_ROOT_DIR, 'out', 'datascience-ui', 'history-react', 'index_bundle.js'); setup(async () => { - host = new WebPanelServer(await portfinder.getPortPromise(), token); + // So these are effectively functional tests rather than unit tests... + const fs = new FileSystem( + new PlatformService() + ); + host = new WebPanelServer(await portfinder.getPortPromise(), token, fs); server = host.start(); }); diff --git a/src/test/datascience/color.test.ts b/src/test/datascience/color.test.ts index 2a49f3b89b89..a2b29f91b73a 100644 --- a/src/test/datascience/color.test.ts +++ b/src/test/datascience/color.test.ts @@ -34,7 +34,10 @@ suite('Theme colors', () => { extensions = new Extensions(); currentProcess = new CurrentProcess(); logger = new Logger(); - themeFinder = new ThemeFinder(extensions, currentProcess, logger); + const fs = new FileSystem( + new PlatformService() + ); + themeFinder = new ThemeFinder(extensions, currentProcess, logger, fs); workspaceConfig = TypeMoq.Mock.ofType(); workspaceConfig.setup(ws => ws.has(TypeMoq.It.isAnyString())) @@ -81,9 +84,6 @@ suite('Theme colors', () => { workspaceService.setup(c => c.getConfiguration(TypeMoq.It.isAny())).returns(() => workspaceConfig.object); workspaceService.setup(c => c.getConfiguration(TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => workspaceConfig.object); - const fs = new FileSystem( - new PlatformService() - ); cssGenerator = new CodeCssGenerator(workspaceService.object, themeFinder, configService.object, logger, fs); }); From f2079447dd6edfdce721218c096838560916edab Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Thu, 12 Dec 2019 15:25:29 -0700 Subject: [PATCH 22/26] Use directoryExists() instead of fileExists(). --- src/client/datascience/jupyter/jupyterNotebook.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/client/datascience/jupyter/jupyterNotebook.ts b/src/client/datascience/jupyter/jupyterNotebook.ts index 189927d556f3..e910d286cd87 100644 --- a/src/client/datascience/jupyter/jupyterNotebook.ts +++ b/src/client/datascience/jupyter/jupyterNotebook.ts @@ -691,7 +691,7 @@ export class JupyterNotebookBase implements INotebook { if (this.launchInfo && this.launchInfo.connectionInfo.localLaunch && !this._workingDirectory) { // See what our working dir is supposed to be const suggested = this.launchInfo.workingDir; - if (suggested && (await this.fs.fileExists(suggested))) { + if (suggested && (await this.fs.directoryExists(suggested))) { // We should use the launch info directory. It trumps the possible dir this._workingDirectory = suggested; return this.changeDirectoryIfPossible(this._workingDirectory); From 9e86f1107f58cfd3b358848ee5b32ace9bccf39c Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Thu, 12 Dec 2019 17:01:03 -0700 Subject: [PATCH 23/26] Be a little more efficient in readFile(). --- src/client/common/platform/fileSystem.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/client/common/platform/fileSystem.ts b/src/client/common/platform/fileSystem.ts index a75df6af898c..1622f7c1a859 100644 --- a/src/client/common/platform/fileSystem.ts +++ b/src/client/common/platform/fileSystem.ts @@ -56,7 +56,7 @@ export class FileSystem implements IFileSystem { * @memberof FileSystem */ public readFile(filePath: string): Promise { - return fs.readFile(filePath).then(buffer => buffer.toString()); + return fs.readFile(filePath, 'utf8'); } public readData(filePath: string): Promise { return fs.readFile(filePath); From a3ef4922555dad5447b83c0efc0a9269890801c5 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 17 Dec 2019 12:55:48 -0700 Subject: [PATCH 24/26] Decrease a timeout if not CI. --- .../interpreters/locators/workspaceVirtualEnvService.test.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/test/interpreters/locators/workspaceVirtualEnvService.test.ts b/src/test/interpreters/locators/workspaceVirtualEnvService.test.ts index f5f0f9acd36b..af9f4184f607 100644 --- a/src/test/interpreters/locators/workspaceVirtualEnvService.test.ts +++ b/src/test/interpreters/locators/workspaceVirtualEnvService.test.ts @@ -18,12 +18,13 @@ import { } from '../../../client/interpreter/contracts'; import { WorkspaceVirtualEnvWatcherService } from '../../../client/interpreter/locators/services/workspaceVirtualEnvWatcherService'; import { IServiceContainer } from '../../../client/ioc/types'; +import { IS_CI_SERVER } from '../../ciConstants'; import { deleteFiles, getOSType, isPythonVersionInProcess, OSType, PYTHON_PATH, rootWorkspaceUri, waitForCondition } from '../../common'; import { IS_MULTI_ROOT_TEST } from '../../constants'; import { sleep } from '../../core'; import { initialize, multirootPath } from '../../initialize'; -const timeoutMs = 60_000; +const timeoutMs = IS_CI_SERVER ? 60_000 : 15_000; suite('Interpreters - Workspace VirtualEnv Service', function() { this.timeout(timeoutMs); this.retries(0); From 260c114941f44e570bd63e8066019c86cbcc3321 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 17 Dec 2019 13:59:22 -0700 Subject: [PATCH 25/26] Refactor (for clarity) the venv creation testing helper. --- .../workspaceVirtualEnvService.test.ts | 88 +++++++++++++------ 1 file changed, 63 insertions(+), 25 deletions(-) diff --git a/src/test/interpreters/locators/workspaceVirtualEnvService.test.ts b/src/test/interpreters/locators/workspaceVirtualEnvService.test.ts index af9f4184f607..059e69195eee 100644 --- a/src/test/interpreters/locators/workspaceVirtualEnvService.test.ts +++ b/src/test/interpreters/locators/workspaceVirtualEnvService.test.ts @@ -7,6 +7,7 @@ import { expect } from 'chai'; import { exec } from 'child_process'; import * as path from 'path'; +import { promisify } from 'util'; import { Uri } from 'vscode'; import '../../../client/common/extensions'; import { createDeferredFromPromise, Deferred } from '../../../client/common/utils/async'; @@ -24,16 +25,66 @@ import { IS_MULTI_ROOT_TEST } from '../../constants'; import { sleep } from '../../core'; import { initialize, multirootPath } from '../../initialize'; +const execAsync = promisify(exec); +async function run(argv: string[], cwd: string) { + const cmdline = argv.join(' '); + const { stderr } = await execAsync(cmdline, { + cwd: cwd + }); + if (stderr && stderr.length > 0) { + throw Error(stderr); + } +} + +class Venvs { + constructor( + private readonly cwd: string, + private readonly prefix = '.venv-' + ) { } + + public async create(name: string): Promise { + const venvRoot = this.resolve(name); + const argv = [ + PYTHON_PATH.fileToCommandArgument(), + '-m', 'venv', + venvRoot + ]; + try { + await run(argv, this.cwd); + } catch (err) { + throw new Error(`Failed to create Env ${path.basename(venvRoot)}, ${PYTHON_PATH}, Error: ${err}`); + } + return venvRoot; + } + + public async cleanUp() { + const globPattern = path.join(this.cwd, `${this.prefix}*`); + await deleteFiles(globPattern); + } + + private getID(name: string): string { + // Ensure env is random to avoid conflicts in tests (currupting test data). + const now = new Date().getTime().toString(); + return `${this.prefix}${name}${now}`; + } + + private resolve(name: string): string { + const id = this.getID(name); + return path.join(this.cwd, id); + } +} + const timeoutMs = IS_CI_SERVER ? 60_000 : 15_000; suite('Interpreters - Workspace VirtualEnv Service', function() { this.timeout(timeoutMs); this.retries(0); - let locator: IInterpreterLocatorService; const workspaceUri = IS_MULTI_ROOT_TEST ? Uri.file(path.join(multirootPath, 'workspace3')) : rootWorkspaceUri!; const workspace4 = Uri.file(path.join(multirootPath, 'workspace4')); - const venvPrefix = '.venv'; + const venvs = new Venvs(workspaceUri.fsPath); + let serviceContainer: IServiceContainer; + let locator: IInterpreterLocatorService; async function manuallyTriggerFSWatcher(deferred: Deferred) { // Monitoring files on virtualized environments can be finicky... @@ -51,7 +102,8 @@ suite('Interpreters - Workspace VirtualEnv Service', function() { await sleep(1000); } } - async function waitForInterpreterToBeDetected(envNameToLookFor: string) { + async function waitForInterpreterToBeDetected(venvRoot: string) { + const envNameToLookFor = path.basename(venvRoot); const predicate = async () => { const items = await locator.getInterpreters(workspaceUri); return items.some(item => item.envName === envNameToLookFor); @@ -66,24 +118,7 @@ suite('Interpreters - Workspace VirtualEnv Service', function() { await deferred.promise; } async function createVirtualEnvironment(envSuffix: string) { - // Ensure env is random to avoid conflicts in tests (currupting test data). - const envName = `${venvPrefix}${envSuffix}${new Date().getTime().toString()}`; - return new Promise((resolve, reject) => { - exec( - `${PYTHON_PATH.fileToCommandArgument()} -m venv ${envName}`, - { cwd: workspaceUri.fsPath }, - (ex, _, stderr) => { - if (ex) { - return reject(ex); - } - if (stderr && stderr.length > 0) { - reject(new Error(`Failed to create Env ${envName}, ${PYTHON_PATH}, Error: ${stderr}`)); - } else { - resolve(envName); - } - } - ); - }); + return venvs.create(envSuffix); } suiteSetup(async function() { @@ -99,12 +134,12 @@ suite('Interpreters - Workspace VirtualEnv Service', function() { ); // This test is required, we need to wait for interpreter listing completes, // before proceeding with other tests. - await deleteFiles(path.join(workspaceUri.fsPath, `${venvPrefix}*`)); + await venvs.cleanUp(); await locator.getInterpreters(workspaceUri); }); - suiteTeardown(async () => deleteFiles(path.join(workspaceUri.fsPath, `${venvPrefix}*`))); - teardown(async () => deleteFiles(path.join(workspaceUri.fsPath, `${venvPrefix}*`))); + suiteTeardown(async () => venvs.cleanUp()); + teardown(async () => venvs.cleanUp()); test('Detect Virtual Environment', async () => { const envName = await createVirtualEnvironment('one'); @@ -132,7 +167,10 @@ suite('Interpreters - Workspace VirtualEnv Service', function() { createVirtualEnvironment('first3'), createVirtualEnvironment('second3') ]); - await Promise.all([waitForInterpreterToBeDetected(env1), waitForInterpreterToBeDetected(env2)]); + await Promise.all([ + waitForInterpreterToBeDetected(env1), + waitForInterpreterToBeDetected(env2) + ]); // Workspace4 should still not have any interpreters. items4 = await locator.getInterpreters(workspace4); From 5080b06429ae8eafba75a21a6bda266dc302e152 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 17 Dec 2019 14:02:16 -0700 Subject: [PATCH 26/26] Fix a test by using the old listdir behavior. --- src/client/common/platform/fileSystem.ts | 12 ++++++++++++ src/client/common/platform/types.ts | 1 + src/client/interpreter/locators/helpers.ts | 6 ++++-- .../locators/workspaceVirtualEnvService.test.ts | 1 + 4 files changed, 18 insertions(+), 2 deletions(-) diff --git a/src/client/common/platform/fileSystem.ts b/src/client/common/platform/fileSystem.ts index 1622f7c1a859..3ed20df830f4 100644 --- a/src/client/common/platform/fileSystem.ts +++ b/src/client/common/platform/fileSystem.ts @@ -80,6 +80,18 @@ export class FileSystem implements IFileSystem { return deferred.promise; } + public async listdir(root: string): Promise { + return new Promise(resolve => { + // Now look for Interpreters in this directory + fs.readdir(root, (err, names) => { + if (err) { + return resolve([]); + } + resolve(names.map(name => path.join(root, name))); + }); + }); + } + public getSubDirectories(rootDir: string): Promise { return new Promise(resolve => { fs.readdir(rootDir, async (error, files) => { diff --git a/src/client/common/platform/types.ts b/src/client/common/platform/types.ts index 2277685e76e7..cadd7b0ae1a1 100644 --- a/src/client/common/platform/types.ts +++ b/src/client/common/platform/types.ts @@ -47,6 +47,7 @@ export interface IFileSystem { directoryExists(path: string): Promise; createDirectory(path: string): Promise; deleteDirectory(path: string): Promise; + listdir(dirname: string): Promise; getSubDirectories(rootDir: string): Promise; getFiles(rootDir: string): Promise; arePathsSame(path1: string, path2: string): boolean; diff --git a/src/client/interpreter/locators/helpers.ts b/src/client/interpreter/locators/helpers.ts index d6381f7e63de..096437ac624f 100644 --- a/src/client/interpreter/locators/helpers.ts +++ b/src/client/interpreter/locators/helpers.ts @@ -9,10 +9,12 @@ import { IPipEnvServiceHelper } from './types'; const CheckPythonInterpreterRegEx = IS_WINDOWS ? /^python(\d+(.\d+)?)?\.exe$/ : /^python(\d+(.\d+)?)?$/; export function lookForInterpretersInDirectory(pathToCheck: string, fs: IFileSystem): Promise { - return fs.getFiles(pathToCheck) + // Technically, we should be able to use fs.getFiles(). However, + // that breaks some tests. So we stick with the broader behavior. + return fs.listdir(pathToCheck) .then(subDirs => subDirs.filter(fileName => CheckPythonInterpreterRegEx.test(path.basename(fileName)))) .catch(err => { - traceError('Python Extension (lookForInterpretersInDirectory.fs.getFiles):', err); + traceError('Python Extension (lookForInterpretersInDirectory.fs.listdir):', err); return [] as string[]; }); } diff --git a/src/test/interpreters/locators/workspaceVirtualEnvService.test.ts b/src/test/interpreters/locators/workspaceVirtualEnvService.test.ts index 059e69195eee..d6e05712bb50 100644 --- a/src/test/interpreters/locators/workspaceVirtualEnvService.test.ts +++ b/src/test/interpreters/locators/workspaceVirtualEnvService.test.ts @@ -80,6 +80,7 @@ suite('Interpreters - Workspace VirtualEnv Service', function() { this.retries(0); const workspaceUri = IS_MULTI_ROOT_TEST ? Uri.file(path.join(multirootPath, 'workspace3')) : rootWorkspaceUri!; + // "workspace4 does not exist. const workspace4 = Uri.file(path.join(multirootPath, 'workspace4')); const venvs = new Venvs(workspaceUri.fsPath);