From 002a5b9f96d960f65e72da53e4e0837096090726 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 9 Dec 2019 13:46:27 -0700 Subject: [PATCH 1/8] Revert "Fixes to FileSystem code to use bitwise operations (#8966)" This reverts commit acb59d3e0333b31003f4b3e47571bc593fceb68e. --- news/2 Fixes/8890.md | 1 - src/client/common/platform/fileSystem.ts | 22 +++++--------- .../locators/services/KnownPathsService.ts | 2 +- src/test/common/platform/filesystem.test.ts | 12 ++++---- .../common/platform/filesystem.unit.test.ts | 29 +++++++++++++++---- src/test/serviceRegistry.ts | 12 ++++---- 6 files changed, 42 insertions(+), 36 deletions(-) delete mode 100644 news/2 Fixes/8890.md diff --git a/news/2 Fixes/8890.md b/news/2 Fixes/8890.md deleted file mode 100644 index 276c04796cff..000000000000 --- a/news/2 Fixes/8890.md +++ /dev/null @@ -1 +0,0 @@ -Fixes to FileSystem code to use bitwise operations. diff --git a/src/client/common/platform/fileSystem.ts b/src/client/common/platform/fileSystem.ts index af2ea15439b8..0d39f30ea4f4 100644 --- a/src/client/common/platform/fileSystem.ts +++ b/src/client/common/platform/fileSystem.ts @@ -38,13 +38,11 @@ function isFileExistsError(err: Error): boolean { function convertFileStat(stat: fsextra.Stats): FileStat { let fileType = FileType.Unknown; if (stat.isFile()) { - fileType = fileType | FileType.File; - } - if (stat.isDirectory()) { - fileType = fileType | FileType.Directory; - } - if (stat.isSymbolicLink()) { - fileType = fileType | FileType.SymbolicLink; + fileType = FileType.File; + } else if (stat.isDirectory()) { + fileType = FileType.Directory; + } else if (stat.isSymbolicLink()) { + fileType = FileType.SymbolicLink; } return { type: fileType, @@ -387,11 +385,7 @@ export class FileSystemUtils implements IFileSystemUtils { if (fileType === undefined) { return true; } - if (fileType === FileType.Unknown) { - // FileType.Unknown == 0, hence do not use bitwise operations. - return stat.type === FileType.Unknown; - } - return (stat.type & fileType) === fileType; + return stat.type === fileType; } public async fileExists(filename: string): Promise { return this.pathExists(filename, FileType.File); @@ -409,12 +403,12 @@ export class FileSystemUtils implements IFileSystemUtils { } public async getSubDirectories(dirname: string): Promise { return (await this.listdir(dirname)) - .filter(([_name, fileType]) => fileType & FileType.Directory) + .filter(([_name, fileType]) => fileType === FileType.Directory) .map(([name, _fileType]) => this.paths.join(dirname, name)); } public async getFiles(dirname: string): Promise { return (await this.listdir(dirname)) - .filter(([_name, fileType]) => fileType & FileType.File) + .filter(([_name, fileType]) => fileType === FileType.File) .map(([name, _fileType]) => this.paths.join(dirname, name)); } diff --git a/src/client/interpreter/locators/services/KnownPathsService.ts b/src/client/interpreter/locators/services/KnownPathsService.ts index b6081fa61bf0..1458d4a52ba9 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 { * Return the interpreters in the given directory. */ private async getInterpretersInDirectory(dir: string): Promise { - if (await this.fs.directoryExists(dir)) { + if (await this.fs.fileExists(dir)) { return lookForInterpretersInDirectory(dir, this.fs); } else { return []; diff --git a/src/test/common/platform/filesystem.test.ts b/src/test/common/platform/filesystem.test.ts index 83f0f54a6cf9..6fe96ccd0f73 100644 --- a/src/test/common/platform/filesystem.test.ts +++ b/src/test/common/platform/filesystem.test.ts @@ -404,17 +404,15 @@ suite('FileSystem Utils', () => { const exists = await utils.pathExists(symlink, FileType.SymbolicLink); - expect(exists).to.equal(true); + expect(exists).to.equal(false); }); - test('unknown', async function () { - // tslint:disable-next-line: no-invalid-this - return this.skip(); - // const sockFile = await fix.createSocket('x/y/z/ipc.sock'); + test('unknown', async () => { + const sockFile = await fix.createSocket('x/y/z/ipc.sock'); - // const exists = await utils.pathExists(sockFile, FileType.Unknown); + const exists = await utils.pathExists(sockFile, FileType.Unknown); - // expect(exists).to.equal(true); + expect(exists).to.equal(false); }); }); diff --git a/src/test/common/platform/filesystem.unit.test.ts b/src/test/common/platform/filesystem.unit.test.ts index 2e84995a1b7b..bedecb2d74c3 100644 --- a/src/test/common/platform/filesystem.unit.test.ts +++ b/src/test/common/platform/filesystem.unit.test.ts @@ -290,12 +290,29 @@ suite('Raw FileSystem', () => { old = createMockLegacyStat(); } - old!.setup(s => s.isFile()) - .returns(() => (stat.type & FileType.File) === FileType.File); - old!.setup(s => s.isDirectory()) - .returns(() => (stat.type & FileType.Directory) === FileType.Directory); - old!.setup(s => s.isSymbolicLink()) - .returns(() => (stat.type & FileType.SymbolicLink) === FileType.SymbolicLink); + if (stat.type === FileType.File) { + old!.setup(s => s.isFile()) + .returns(() => true); + } else if (stat.type === FileType.Directory) { + old!.setup(s => s.isFile()) + .returns(() => false); + old!.setup(s => s.isDirectory()) + .returns(() => true); + } else if (stat.type === FileType.SymbolicLink) { + old!.setup(s => s.isFile()) + .returns(() => false); + old!.setup(s => s.isDirectory()) + .returns(() => false); + old!.setup(s => s.isSymbolicLink()) + .returns(() => true); + } else { + old!.setup(s => s.isFile()) + .returns(() => false); + old!.setup(s => s.isDirectory()) + .returns(() => false); + old!.setup(s => s.isSymbolicLink()) + .returns(() => false); + } old!.setup(s => s.size) .returns(() => stat.size); old!.setup(s => s.ctimeMs) diff --git a/src/test/serviceRegistry.ts b/src/test/serviceRegistry.ts index 6055195bbc95..91ab8a5ef8d8 100644 --- a/src/test/serviceRegistry.ts +++ b/src/test/serviceRegistry.ts @@ -86,13 +86,11 @@ class LegacyRawFileSystem extends RawFileSystem { const stat = await fsextra.stat(filename); let fileType = FileType.Unknown; if (stat.isFile()) { - fileType = fileType | FileType.File; - } - if (stat.isDirectory()) { - fileType = fileType | FileType.Directory; - } - if (stat.isSymbolicLink()) { - fileType = fileType | FileType.SymbolicLink; + fileType = FileType.File; + } else if (stat.isDirectory()) { + fileType = FileType.Directory; + } else if (stat.isSymbolicLink()) { + fileType = FileType.SymbolicLink; } return { type: fileType, From fb315b9ba717429ab220ca5d361425b504b1e863 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 9 Dec 2019 13:46:35 -0700 Subject: [PATCH 2/8] Revert "Make sure IFileSystem is registered in DI during tests. (#8902)" This reverts commit dabd87f29c6fcaab0bad18cfdb7b823d2755d66a. --- .gitignore | 4 ---- src/test/debugger/envVars.test.ts | 1 - 2 files changed, 5 deletions(-) diff --git a/.gitignore b/.gitignore index a51acf522c2f..cefa2212f557 100644 --- a/.gitignore +++ b/.gitignore @@ -39,7 +39,3 @@ tmp/** test-results.xml uitests/out/** !build/ - -# TODO (GH-8925) This is temporary. We will remove it when we adjust -# tests to not leave the file behind. -src/test/get-pip.py diff --git a/src/test/debugger/envVars.test.ts b/src/test/debugger/envVars.test.ts index 2e2af579c88f..95fc1160275a 100644 --- a/src/test/debugger/envVars.test.ts +++ b/src/test/debugger/envVars.test.ts @@ -49,7 +49,6 @@ suite('Resolving Environment Variables when Debugging', () => { function initializeDI() { ioc = new UnitTestIocContainer(); ioc.registerProcessTypes(); - ioc.registerFileSystemTypes(); ioc.registerVariableTypes(); ioc.registerMockProcess(); } From 85165cf0552d16666e46f3f5ea5491b76ae52ac8 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 9 Dec 2019 13:46:37 -0700 Subject: [PATCH 3/8] Revert "Remove IFileSystemUtils from IOC container (#8898)" This reverts commit 8ab93d793c7be3b89e740a1292c2b30f4e62ab6f. --- src/client/common/platform/fileSystem.ts | 1 + src/client/common/platform/serviceRegistry.ts | 5 +++-- src/client/common/platform/types.ts | 1 + 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/client/common/platform/fileSystem.ts b/src/client/common/platform/fileSystem.ts index 0d39f30ea4f4..24f26a1f13d5 100644 --- a/src/client/common/platform/fileSystem.ts +++ b/src/client/common/platform/fileSystem.ts @@ -318,6 +318,7 @@ export class RawFileSystem implements IRawFileSystem { } // High-level filesystem operations used by the extension. +@injectable() export class FileSystemUtils implements IFileSystemUtils { constructor( public readonly raw: IRawFileSystem, diff --git a/src/client/common/platform/serviceRegistry.ts b/src/client/common/platform/serviceRegistry.ts index d15edf5fc388..c44422874a74 100644 --- a/src/client/common/platform/serviceRegistry.ts +++ b/src/client/common/platform/serviceRegistry.ts @@ -3,13 +3,14 @@ 'use strict'; import { IServiceManager } from '../../ioc/types'; -import { FileSystem } from './fileSystem'; +import { FileSystem, FileSystemUtils } from './fileSystem'; import { PlatformService } from './platformService'; import { RegistryImplementation } from './registry'; -import { IFileSystem, IPlatformService, IRegistry } from './types'; +import { IFileSystem, IFileSystemUtils, IPlatformService, IRegistry } from './types'; export function registerTypes(serviceManager: IServiceManager) { serviceManager.addSingleton(IPlatformService, PlatformService); serviceManager.addSingleton(IFileSystem, FileSystem); + serviceManager.addSingletonInstance(IFileSystemUtils, FileSystemUtils.withDefaults()); serviceManager.addSingleton(IRegistry, RegistryImplementation); } diff --git a/src/client/common/platform/types.ts b/src/client/common/platform/types.ts index a2a5ba2c575c..880f87b90175 100644 --- a/src/client/common/platform/types.ts +++ b/src/client/common/platform/types.ts @@ -101,6 +101,7 @@ export interface IRawFileSystem { } // High-level filesystem operations used by the extension. +export const IFileSystemUtils = Symbol('IFileSystemUtils'); export interface IFileSystemUtils { readonly raw: IRawFileSystem; readonly paths: IFileSystemPaths; From 247be66fafb11ff6c9cbabfd7dfdb223f0fc2c23 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 9 Dec 2019 13:46:38 -0700 Subject: [PATCH 4/8] Revert "Skip a few more tests. (#8830)" This reverts commit d19755790677873873c669487ccd8c64f885e5fc. --- .../platform/filesystem.functional.test.ts | 21 +------------------ 1 file changed, 1 insertion(+), 20 deletions(-) diff --git a/src/test/common/platform/filesystem.functional.test.ts b/src/test/common/platform/filesystem.functional.test.ts index 0834c7f4a4f3..0274c064f8a6 100644 --- a/src/test/common/platform/filesystem.functional.test.ts +++ b/src/test/common/platform/filesystem.functional.test.ts @@ -51,14 +51,7 @@ export class FSFixture { if (this.tempDir) { const tempDir = this.tempDir; this.tempDir = undefined; - try { - tempDir.removeCallback(); - } catch { - // The "unsafeCleanup: true" option is supposed - // to support a non-empty directory, but apparently - // that isn't always the case. (see #8804) - await fsextra.remove(tempDir.name); - } + tempDir.removeCallback(); } if (this.sockServer) { const srv = this.sockServer; @@ -382,12 +375,6 @@ suite('Raw FileSystem', () => { }); suite('createWriteStream', () => { - setup(function() { - // Tests disabled due to CI failures: https://github.com/microsoft/vscode-python/issues/8804 - // tslint:disable-next-line:no-invalid-this - return this.skip(); - }); - test('returns the correct WriteStream', async () => { const filename = await fix.resolve('x/y/z/spam.py'); const expected = fsextra.createWriteStream(filename); @@ -551,12 +538,6 @@ suite('FileSystem Utils', () => { }); suite('search', () => { - setup(function() { - // Tests disabled due to CI failures: https://github.com/microsoft/vscode-python/issues/8804 - // tslint:disable-next-line:no-invalid-this - return this.skip(); - }); - test('found matches', async () => { const pattern = await fix.resolve(`x/y/z/spam.*`); const expected: string[] = [ From 6ee676a25656048af438ae41753c51f2e651d9d7 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 9 Dec 2019 13:46:39 -0700 Subject: [PATCH 5/8] Revert "Skip problematic tests. (#8810)" This reverts commit c5f7766129e46c79b44da25a0eb359aee2084114. --- src/test/common/platform/filesystem.functional.test.ts | 6 ------ src/test/common/platform/filesystem.test.ts | 6 ------ .../locators/workspaceVirtualEnvService.test.ts | 4 ---- 3 files changed, 16 deletions(-) diff --git a/src/test/common/platform/filesystem.functional.test.ts b/src/test/common/platform/filesystem.functional.test.ts index 0274c064f8a6..74f3e1f18b7e 100644 --- a/src/test/common/platform/filesystem.functional.test.ts +++ b/src/test/common/platform/filesystem.functional.test.ts @@ -477,12 +477,6 @@ suite('FileSystem Utils', () => { }); suite('getFileHash', () => { - setup(function() { - // Tests disabled due to CI failures: https://github.com/microsoft/vscode-python/issues/8804 - // tslint:disable-next-line:no-invalid-this - return this.skip(); - }); - test('Getting hash for a file should return non-empty string', async () => { const filename = await fix.createFile('x/y/z/spam.py'); diff --git a/src/test/common/platform/filesystem.test.ts b/src/test/common/platform/filesystem.test.ts index 6fe96ccd0f73..937b46ef515e 100644 --- a/src/test/common/platform/filesystem.test.ts +++ b/src/test/common/platform/filesystem.test.ts @@ -464,12 +464,6 @@ suite('FileSystem Utils', () => { }); suite('getFiles', () => { - setup(function() { - // Tests disabled due to CI failures: https://github.com/microsoft/vscode-python/issues/8804 - // tslint:disable-next-line:no-invalid-this - return this.skip(); - }); - test('mixed types', async () => { const symlinkSource = await fix.createFile('x/info.py'); const dirname = await fix.createDirectory('x/y/z/scripts'); diff --git a/src/test/interpreters/locators/workspaceVirtualEnvService.test.ts b/src/test/interpreters/locators/workspaceVirtualEnvService.test.ts index 59ba1ab3ae19..473f20e24ab8 100644 --- a/src/test/interpreters/locators/workspaceVirtualEnvService.test.ts +++ b/src/test/interpreters/locators/workspaceVirtualEnvService.test.ts @@ -163,10 +163,6 @@ suite('Interpreters - Workspace VirtualEnv Service', function() { } suiteSetup(async function() { - // Tests disabled due to CI failures: https://github.com/microsoft/vscode-python/issues/8804 - // tslint:disable-next-line:no-invalid-this - return this.skip(); - // skip for Python < 3, no venv support if (await isPythonVersionInProcess(undefined, '2')) { return this.skip(); From 45e6188efb7ce76b4a376240ce6909467c673370 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 9 Dec 2019 13:47:10 -0700 Subject: [PATCH 6/8] Revert "Use new VS Code filesystem API. (#8307)" This reverts commit 7cbd1c25f544ed9e96c735c61a78a5283fbef141. --- news/3 Code Health/6911.md | 1 - src/client/common/platform/fileSystem.ts | 289 ++++----- src/client/common/platform/types.ts | 8 +- src/client/workspaceSymbols/provider.ts | 8 +- src/test/common/crypto.unit.test.ts | 16 +- .../platform/filesystem.functional.test.ts | 534 +++++++++++++++-- src/test/common/platform/filesystem.test.ts | 560 ------------------ .../common/platform/filesystem.unit.test.ts | 404 ++++++------- .../datascience/dataScienceIocContainer.ts | 1 - src/test/serviceRegistry.ts | 100 +--- src/test/vscode-mock.ts | 1 - 11 files changed, 783 insertions(+), 1139 deletions(-) delete mode 100644 news/3 Code Health/6911.md delete mode 100644 src/test/common/platform/filesystem.test.ts diff --git a/news/3 Code Health/6911.md b/news/3 Code Health/6911.md deleted file mode 100644 index f7a40ecad97d..000000000000 --- a/news/3 Code Health/6911.md +++ /dev/null @@ -1 +0,0 @@ -Use the new VS Code filesystem API as much as possible. diff --git a/src/client/common/platform/fileSystem.ts b/src/client/common/platform/fileSystem.ts index 24f26a1f13d5..25886aabc12d 100644 --- a/src/client/common/platform/fileSystem.ts +++ b/src/client/common/platform/fileSystem.ts @@ -4,6 +4,7 @@ 'use strict'; import { createHash } from 'crypto'; +import * as fs from 'fs'; import * as fsextra from 'fs-extra'; import * as glob from 'glob'; import { injectable } from 'inversify'; @@ -11,6 +12,7 @@ import * as fspath from 'path'; import * as tmpMod from 'tmp'; import * as util from 'util'; import * as vscode from 'vscode'; +import { createDeferred } from '../utils/async'; import { getOSType, OSType } from '../utils/platform'; import { FileStat, FileType, @@ -20,42 +22,25 @@ import { } from './types'; // tslint:disable:max-classes-per-file -// tslint:disable:no-suspicious-comment const ENCODING: string = 'utf8'; -const FILE_NOT_FOUND = vscode.FileSystemError.FileNotFound().name; -const FILE_EXISTS = vscode.FileSystemError.FileExists().name; - -function isFileNotFoundError(err: Error): boolean { - return err.name === FILE_NOT_FOUND; -} - -function isFileExistsError(err: Error): boolean { - return err.name === FILE_EXISTS; -} - -function convertFileStat(stat: fsextra.Stats): FileStat { - let fileType = FileType.Unknown; +// Determine the file type from the given file info. +function getFileType(stat: FileStat): FileType { if (stat.isFile()) { - fileType = FileType.File; + return FileType.File; } else if (stat.isDirectory()) { - fileType = FileType.Directory; + return FileType.Directory; } else if (stat.isSymbolicLink()) { - fileType = FileType.SymbolicLink; - } - return { - type: fileType, - size: stat.size, - ctime: stat.ctimeMs, - mtime: stat.mtimeMs - }; + return FileType.SymbolicLink; + } else { + return FileType.Unknown; + } } // The parts of node's 'path' module used by FileSystemPath. interface INodePath { join(...filenames: string[]): string; - dirname(filename: string): string; normalize(filename: string): string; } @@ -81,10 +66,6 @@ export class FileSystemPaths implements IFileSystemPaths { return this.raw.join(...filenames); } - public dirname(filename: string): string { - return this.raw.dirname(filename); - } - public normCase(filename: string): string { filename = this.raw.normalize(filename); return this.isCaseSensitive ? filename.toUpperCase() : filename; @@ -131,32 +112,36 @@ export class TempFileSystem { } } -// This is the parts of the vscode.workspace.fs API that we use here. -// See: https://code.visualstudio.com/api/references/vscode-api#FileSystem -// Note that we have used all the API functions *except* "rename()". -interface IVSCodeFileSystemAPI { - copy(source: vscode.Uri, target: vscode.Uri, options?: {overwrite: boolean}): Thenable; - createDirectory(uri: vscode.Uri): Thenable; - delete(uri: vscode.Uri, options?: {recursive: boolean; useTrash: boolean}): Thenable; - readDirectory(uri: vscode.Uri): Thenable<[string, FileType][]>; - readFile(uri: vscode.Uri): Thenable; - stat(uri: vscode.Uri): Thenable; - writeFile(uri: vscode.Uri, content: Uint8Array): Thenable; +// This is the parts of node's 'fs' module that we use in RawFileSystem. +interface IRawFS { + // non-async + createWriteStream(filePath: string): fs.WriteStream; } // This is the parts of the 'fs-extra' module that we use in RawFileSystem. interface IRawFSExtra { chmod(filePath: string, mode: string | number): Promise; + readFile(path: string, encoding: string): Promise; + //tslint:disable-next-line:no-any + writeFile(path: string, data: any, options: any): Promise; + unlink(filename: string): Promise; + stat(filename: string): Promise; lstat(filename: string): Promise; + mkdirp(dirname: string): Promise; + rmdir(dirname: string): Promise; + readdir(dirname: string): Promise; + remove(dirname: string): Promise; // non-async statSync(filename: string): fsextra.Stats; readFileSync(path: string, encoding: string): string; - createWriteStream(filePath: string): fsextra.WriteStream; + createReadStream(src: string): fsextra.ReadStream; + createWriteStream(dest: string): fsextra.WriteStream; } +// The parts of IFileSystemPaths used by RawFileSystem. interface IRawPath { - dirname(filename: string): string; + join(...filenames: string[]): string; } // Later we will drop "FileSystem", switching usage to @@ -165,155 +150,105 @@ interface IRawPath { // The low-level filesystem operations used by the extension. export class RawFileSystem implements IRawFileSystem { constructor( - protected readonly paths: IRawPath, - protected readonly vscfs: IVSCodeFileSystemAPI, + protected readonly path: IRawPath, + protected readonly nodefs: IRawFS, protected readonly fsExtra: IRawFSExtra ) { } // Create a new object using common-case default values. - public static withDefaults( - paths?: IRawPath, - vscfs?: IVSCodeFileSystemAPI, - fsExtra?: IRawFSExtra - ): RawFileSystem{ + public static withDefaults(): RawFileSystem{ return new RawFileSystem( - paths || FileSystemPaths.withDefaults(), - vscfs || vscode.workspace.fs, - fsExtra || fsextra + FileSystemPaths.withDefaults(), + fs, + fsextra ); } //**************************** - // VS Code API + // fs-extra public async readText(filename: string): Promise { - const uri = vscode.Uri.file(filename); - const data = Buffer.from( - await this.vscfs.readFile(uri)); - return data.toString(ENCODING); + return this.fsExtra.readFile(filename, ENCODING); } - public async writeText(filename: string, text: string): Promise { - const uri = vscode.Uri.file(filename); - const data = Buffer.from(text); - await this.vscfs.writeFile(uri, data); + public async writeText(filename: string, data: {}): Promise { + const options: fsextra.WriteFileOptions = { + encoding: ENCODING + }; + await this.fsExtra.writeFile(filename, data, options); } - public async rmtree(dirname: string): Promise { - const uri = vscode.Uri.file(dirname); - // TODO (https://github.com/microsoft/vscode/issues/84177) - // The docs say "throws - FileNotFound when uri doesn't exist". - // However, it happily does nothing (at least for remote-over-SSH). - // So we have to manually stat, just to be sure. - await this.vscfs.stat(uri); - return this.vscfs.delete(uri, { - recursive: true, - useTrash: false - }); + public async mkdirp(dirname: string): Promise { + return this.fsExtra.mkdirp(dirname); } - public async rmfile(filename: string): Promise { - const uri = vscode.Uri.file(filename); - return this.vscfs.delete(uri, { - recursive: false, - useTrash: false - }); + public async rmtree(dirname: string): Promise { + return this.fsExtra.stat(dirname) + .then(() => this.fsExtra.remove(dirname)); } - public async stat(filename: string): Promise { - const uri = vscode.Uri.file(filename); - return this.vscfs.stat(uri); + public async rmfile(filename: string): Promise { + return this.fsExtra.unlink(filename); } - public async listdir(dirname: string): Promise<[string, FileType][]> { - const uri = vscode.Uri.file(dirname); - return this.vscfs.readDirectory(uri); + public async chmod(filename: string, mode: string | number): Promise { + return this.fsExtra.chmod(filename, mode); } - public async mkdirp(dirname: string): Promise { - // TODO https://github.com/microsoft/vscode/issues/84175 - // Hopefully VS Code provides this method in their API - // so we don't have to roll our own. - const stack = [dirname]; - while (stack.length > 0) { - const current = stack.pop() || ''; - const uri = vscode.Uri.file(current); - try { - await this.vscfs.createDirectory(uri); - } catch (err) { - if (isFileExistsError(err)) { - // already done! - return; - } - // According to the docs, FileNotFound means the parent - // does not exist. - // See: https://code.visualstudio.com/api/references/vscode-api#FileSystemProvider - if (!isFileNotFoundError(err)) { - // Fail for anything else. - throw err; // re-throw - } - // Try creating the parent first. - const parent = this.paths.dirname(current); - if (parent === '' || parent === current) { - // This shouldn't ever happen. - throw err; - } - stack.push(current); - stack.push(parent); - } - } + public async stat(filename: string): Promise { + return this.fsExtra.stat(filename); } - public async copyFile(src: string, dest: string): Promise { - const srcURI = vscode.Uri.file(src); - const destURI = vscode.Uri.file(dest); - // TODO (https://github.com/microsoft/vscode/issues/84177) - // The docs say "throws - FileNotFound when parent of - // destination doesn't exist". However, it happily creates - // the parent directory (at least for remote-over-SSH). - // So we have to manually stat, just to be sure. - await this.vscfs.stat(vscode.Uri.file(this.paths.dirname(dest))); - await this.vscfs.copy(srcURI, destURI, { - overwrite: true - }); + public async lstat(filename: string): Promise { + return this.fsExtra.lstat(filename); } - //**************************** - // fs-extra - - public async chmod(filename: string, mode: string | number): Promise { - // TODO https://github.com/microsoft/vscode/issues/84175 - // This functionality has been requested for the VS Code API. - return this.fsExtra.chmod(filename, mode); + // Once we move to the VS Code API, this method becomes a trivial wrapper. + public async listdir(dirname: string): Promise<[string, FileType][]> { + const names: string[] = await this.fsExtra.readdir(dirname); + const promises = names + .map(name => { + const filename = this.path.join(dirname, name); + return this.lstat(filename) + .then(stat => [name, getFileType(stat)] as [string, FileType]) + .catch(() => [name, FileType.Unknown] as [string, FileType]); + }); + return Promise.all(promises); } - public async lstat(filename: string): Promise { - // TODO https://github.com/microsoft/vscode/issues/84175 - // This functionality has been requested for the VS Code API. - const stat = await this.fsExtra.lstat(filename); - return convertFileStat(stat); + // Once we move to the VS Code API, this method becomes a trivial wrapper. + public async copyFile(src: string, dest: string): Promise { + const deferred = createDeferred(); + const rs = this.fsExtra.createReadStream(src) + .on('error', (err) => { + deferred.reject(err); + }); + const ws = this.fsExtra.createWriteStream(dest) + .on('error', (err) => { + deferred.reject(err); + }).on('close', () => { + deferred.resolve(); + }); + rs.pipe(ws); + return deferred.promise; } //**************************** // non-async (fs-extra) public statSync(filename: string): FileStat { - // TODO https://github.com/microsoft/vscode/issues/84175 - // This functionality has been requested for the VS Code API. - const stat = this.fsExtra.statSync(filename); - return convertFileStat(stat); + return this.fsExtra.statSync(filename); } public readTextSync(filename: string): string { - // TODO https://github.com/microsoft/vscode/issues/84175 - // This functionality has been requested for the VS Code API. return this.fsExtra.readFileSync(filename, ENCODING); } + //**************************** + // non-async (fs) + public createWriteStream(filename: string): WriteStream { - // TODO https://github.com/microsoft/vscode/issues/84175 - // This functionality has been requested for the VS Code API. - return this.fsExtra.createWriteStream(filename); + return this.nodefs.createWriteStream(filename); } } @@ -322,26 +257,20 @@ export class RawFileSystem implements IRawFileSystem { export class FileSystemUtils implements IFileSystemUtils { constructor( public readonly raw: IRawFileSystem, - public readonly paths: IFileSystemPaths, + public readonly path: IFileSystemPaths, public readonly tmp: ITempFileSystem, protected readonly getHash: (data: string) => string, protected readonly globFile: (pat: string) => Promise ) { } // Create a new object using common-case default values. - public static withDefaults( - raw?: IRawFileSystem, - paths?: IFileSystemPaths, - tmp?: ITempFileSystem, - getHash?: (data: string) => string, - globFile?: (pat: string) => Promise - ): FileSystemUtils { - paths = paths || FileSystemPaths.withDefaults(); + public static withDefaults(): FileSystemUtils { + const paths = FileSystemPaths.withDefaults(); return new FileSystemUtils( - raw || RawFileSystem.withDefaults(paths), + new RawFileSystem(paths, fs, fsextra), paths, - tmp || TempFileSystem.withDefaults(), - getHash || getHashString, - globFile || util.promisify(glob) + TempFileSystem.withDefaults(), + getHashString, + util.promisify(glob) ); } @@ -367,8 +296,8 @@ export class FileSystemUtils implements IFileSystemUtils { if (path1 === path2) { return true; } - path1 = this.paths.normCase(path1); - path2 = this.paths.normCase(path2); + path1 = this.path.normCase(path1); + path2 = this.path.normCase(path2); return path1 === path2; } @@ -378,15 +307,19 @@ export class FileSystemUtils implements IFileSystemUtils { ): Promise { let stat: FileStat; try { - stat = await this._stat(filename); + stat = await this.raw.stat(filename); } catch (err) { return false; } - if (fileType === undefined) { return true; + } else if (fileType === FileType.File) { + return stat.isFile(); + } else if (fileType === FileType.Directory) { + return stat.isDirectory(); + } else { + return false; } - return stat.type === fileType; } public async fileExists(filename: string): Promise { return this.pathExists(filename, FileType.File); @@ -405,12 +338,12 @@ export class FileSystemUtils implements IFileSystemUtils { public async getSubDirectories(dirname: string): Promise { return (await this.listdir(dirname)) .filter(([_name, fileType]) => fileType === FileType.Directory) - .map(([name, _fileType]) => this.paths.join(dirname, name)); + .map(([name, _fileType]) => this.path.join(dirname, name)); } public async getFiles(dirname: string): Promise { return (await this.listdir(dirname)) .filter(([_name, fileType]) => fileType === FileType.File) - .map(([name, _fileType]) => this.paths.join(dirname, name)); + .map(([name, _fileType]) => this.path.join(dirname, name)); } public async isDirReadonly(dirname: string): Promise { @@ -419,7 +352,7 @@ export class FileSystemUtils implements IFileSystemUtils { tmpFile = await this.tmp.createFile('___vscpTest___', dirname); } catch { // Use a stat call to ensure the directory exists. - await this._stat(dirname); + await this.raw.stat(dirname); return true; } tmpFile.dispose(); @@ -428,7 +361,7 @@ export class FileSystemUtils implements IFileSystemUtils { public async getFileHash(filename: string): Promise { const stat = await this.raw.lstat(filename); - const data = `${stat.ctime}-${stat.mtime}`; + const data = `${stat.ctimeMs}-${stat.mtimeMs}`; return this.getHash(data); } @@ -436,10 +369,6 @@ export class FileSystemUtils implements IFileSystemUtils { const files = await this.globFile(globPattern); return Array.isArray(files) ? files : []; } - - public async _stat(filename: string): Promise { - return this.raw.stat(filename); - } } // We *could* use ICryptoUtils, but it's a bit overkill, issue tracked @@ -453,7 +382,7 @@ function getHashString(data: string): string { // more aliases (to cause less churn) @injectable() export class FileSystem implements IFileSystem { - protected utils: FileSystemUtils; + private readonly utils: FileSystemUtils; constructor() { this.utils = FileSystemUtils.withDefaults(); } @@ -513,8 +442,12 @@ export class FileSystem implements IFileSystem { //**************************** // aliases - public async stat(filePath: string): Promise { - return this.utils._stat(filePath); + public async stat(filePath: string): Promise { + // Do not import vscode directly, as this isn't available in the Debugger Context. + // If stat is used in debugger context, it will fail, however theres a separate PR that will resolve this. + // tslint:disable-next-line: no-require-imports + const vsc = require('vscode'); + return vsc.workspace.fs.stat(vscode.Uri.file(filePath)); } public async readFile(filename: string): Promise { diff --git a/src/client/common/platform/types.ts b/src/client/common/platform/types.ts index 880f87b90175..317540932e99 100644 --- a/src/client/common/platform/types.ts +++ b/src/client/common/platform/types.ts @@ -4,6 +4,7 @@ 'use strict'; import * as fs from 'fs'; +import * as fsextra from 'fs-extra'; import { SemVer } from 'semver'; import * as vscode from 'vscode'; import { Architecture, OSType } from '../utils/platform'; @@ -47,12 +48,11 @@ export interface ITempFileSystem { export interface IFileSystemPaths { join(...filenames: string[]): string; - dirname(filename: string): string; normCase(filename: string): string; } export import FileType = vscode.FileType; -export type FileStat = vscode.FileStat; +export type FileStat = fsextra.Stats; export type WriteStream = fs.WriteStream; // Later we will drop "IFileSystem", switching usage to @@ -104,7 +104,7 @@ export interface IRawFileSystem { export const IFileSystemUtils = Symbol('IFileSystemUtils'); export interface IFileSystemUtils { readonly raw: IRawFileSystem; - readonly paths: IFileSystemPaths; + readonly path: IFileSystemPaths; readonly tmp: ITempFileSystem; //*********************** @@ -157,7 +157,7 @@ export interface IFileSystem { search(globPattern: string): Promise; arePathsSame(path1: string, path2: string): boolean; - stat(filePath: string): Promise; + stat(filePath: string): Promise; readFile(filename: string): Promise; writeFile(filename: string, data: {}): Promise; chmod(filename: string, mode: string): Promise; diff --git a/src/client/workspaceSymbols/provider.ts b/src/client/workspaceSymbols/provider.ts index ee2460be6e8c..019bcd25592d 100644 --- a/src/client/workspaceSymbols/provider.ts +++ b/src/client/workspaceSymbols/provider.ts @@ -43,13 +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, - this.fs - ); + const items = await parseTags(generator!.workspaceFolder.fsPath, generator!.tagFilePath, query, token); if (!Array.isArray(items)) { return []; } diff --git a/src/test/common/crypto.unit.test.ts b/src/test/common/crypto.unit.test.ts index e97b5b4adaed..dfa6f1801dcd 100644 --- a/src/test/common/crypto.unit.test.ts +++ b/src/test/common/crypto.unit.test.ts @@ -4,23 +4,19 @@ 'use strict'; import { assert, expect } from 'chai'; -import * as fsextra from 'fs-extra'; import * as path from 'path'; import { CryptoUtils } from '../../client/common/crypto'; +import { FileSystem } from '../../client/common/platform/fileSystem'; import { EXTENSION_ROOT_DIR_FOR_TESTS } from '../constants'; // tslint:disable-next-line: max-func-body-length suite('Crypto Utils', async () => { let crypto: CryptoUtils; + const fs = new FileSystem(); + const file = path.join(EXTENSION_ROOT_DIR_FOR_TESTS, 'src', 'test', 'common', 'randomWords.txt'); setup(() => { crypto = new CryptoUtils(); }); - async function getWordList(): Promise { - const file = path.join(EXTENSION_ROOT_DIR_FOR_TESTS, 'src', 'test', 'common', 'randomWords.txt'); - const words = await fsextra.readFile(file, 'utf8'); - return words.split('\n'); - } - test('If hashFormat equals `number`, method createHash() returns a number', async () => { const hash = crypto.createHash('blabla', 'number'); assert.typeOf(hash, 'number', 'Type should be a number'); @@ -56,7 +52,8 @@ suite('Crypto Utils', async () => { assert.notEqual(hash1, hash2, 'Hashes should be different strings'); }); test('If hashFormat equals `number`, ensure numbers are uniformly distributed on scale from 0 to 100', async () => { - const wordList = await getWordList(); + const words = await fs.readFile(file); + const wordList = words.split('\n'); const buckets: number[] = Array(100).fill(0); const hashes = Array(10).fill(0); for (const w of wordList) { @@ -75,7 +72,8 @@ suite('Crypto Utils', async () => { } }); test('If hashFormat equals `number`, on a scale of 0 to 100, small difference in the input on average produce large differences (about 33) in the output ', async () => { - const wordList = await getWordList(); + const words = await fs.readFile(file); + const wordList = words.split('\n'); const buckets: number[] = Array(100).fill(0); let hashes: number[] = []; let totalDifference = 0; diff --git a/src/test/common/platform/filesystem.functional.test.ts b/src/test/common/platform/filesystem.functional.test.ts index 74f3e1f18b7e..24419c7369d8 100644 --- a/src/test/common/platform/filesystem.functional.test.ts +++ b/src/test/common/platform/filesystem.functional.test.ts @@ -24,26 +24,23 @@ const assertArrays = require('chai-arrays'); use(assertArrays); use(chaiAsPromised); -// Note: all functional tests that trigger the VS Code "fs" API are -// found in filesystem.test.ts. +const WINDOWS = /^win/.test(process.platform); -export const WINDOWS = /^win/.test(process.platform); +const DOES_NOT_EXIST = 'this file does not exist'; -export const DOES_NOT_EXIST = 'this file does not exist'; - -export async function assertDoesNotExist(filename: string) { +async function assertDoesNotExist(filename: string) { await expect( fsextra.stat(filename) ).to.eventually.be.rejected; } -export async function assertExists(filename: string) { +async function assertExists(filename: string) { await expect( fsextra.stat(filename) ).to.not.eventually.be.rejected; } -export class FSFixture { +class FSFixture { public tempDir: tmpMod.SynchrounousResult | undefined; public sockServer: net.Server | undefined; @@ -199,6 +196,138 @@ suite('Raw FileSystem', () => { await fix.cleanUp(); }); + suite('readText', () => { + test('returns contents of a file', async () => { + const expected = ''; + const filename = await fix.createFile('x/y/z/spam.py', expected); + + const content = await filesystem.readText(filename); + + expect(content).to.be.equal(expected); + }); + + test('always UTF-8', async () => { + const expected = '... 😁 ...'; + const filename = await fix.createFile('x/y/z/spam.py', expected); + + const text = await filesystem.readText(filename); + + expect(text).to.equal(expected); + }); + + test('returns garbage if encoding is UCS-2', async () => { + const filename = await fix.resolve('spam.py'); + // There are probably cases where this would fail too. + // However, the extension never has to deal with non-UTF8 + // cases, so it doesn't matter too much. + const original = '... 😁 ...'; + await fsextra.writeFile(filename, original, { encoding: 'ucs2' }); + + const text = await filesystem.readText(filename); + + expect(text).to.equal('.\u0000.\u0000.\u0000 \u0000=�\u0001� \u0000.\u0000.\u0000.\u0000'); + }); + + test('throws an exception if file does not exist', async () => { + const promise = filesystem.readText(DOES_NOT_EXIST); + + await expect(promise).to.eventually.be.rejected; + }); + }); + + suite('writeText', () => { + test('creates the file if missing', async () => { + const filename = await fix.resolve('x/y/z/spam.py'); + await assertDoesNotExist(filename); + const data = 'line1\nline2\n'; + + await filesystem.writeText(filename, data); + + const actual = await fsextra.readFile(filename) + .then(buffer => buffer.toString()); + expect(actual).to.equal(data); + }); + + test('always UTF-8', async () => { + const filename = await fix.resolve('x/y/z/spam.py'); + const data = '... 😁 ...'; + + await filesystem.writeText(filename, data); + + const actual = await fsextra.readFile(filename) + .then(buffer => buffer.toString()); + expect(actual).to.equal(data); + }); + + test('overwrites existing file', async () => { + const filename = await fix.createFile('x/y/z/spam.py', '...'); + const data = 'line1\nline2\n'; + + await filesystem.writeText(filename, data); + + const actual = await fsextra.readFile(filename) + .then(buffer => buffer.toString()); + expect(actual).to.equal(data); + }); + }); + + suite('mkdirp', () => { + test('creates the directory and all missing parents', async () => { + await fix.createDirectory('x'); + // x/y, x/y/z, and x/y/z/spam are all missing. + const dirname = await fix.resolve('x/y/z/spam', false); + await assertDoesNotExist(dirname); + + await filesystem.mkdirp(dirname); + + await assertExists(dirname); + }); + + test('works if the directory already exists', async () => { + const dirname = await fix.createDirectory('spam'); + await assertExists(dirname); + + await filesystem.mkdirp(dirname); + + await assertExists(dirname); + }); + }); + + suite('rmtree', () => { + test('deletes the directory and everything in it', async () => { + const dirname = await fix.createDirectory('x'); + const filename = await fix.createFile('x/y/z/spam.py'); + await assertExists(filename); + + await filesystem.rmtree(dirname); + + await assertDoesNotExist(dirname); + }); + + test('fails if the directory does not exist', async () => { + const promise = filesystem.rmtree(DOES_NOT_EXIST); + + await expect(promise).to.eventually.be.rejected; + }); + }); + + suite('rmfile', () => { + test('deletes the file', async () => { + const filename = await fix.createFile('x/y/z/spam.py', '...'); + await assertExists(filename); + + await filesystem.rmfile(filename); + + await assertDoesNotExist(filename); + }); + + test('fails if the file does not exist', async () => { + const promise = filesystem.rmfile(DOES_NOT_EXIST); + + await expect(promise).to.eventually.be.rejected; + }); + }); + suite('chmod (non-Windows)', () => { suiteSetup(function () { // On Windows, chmod won't have any effect on the file itself. @@ -207,6 +336,7 @@ suite('Raw FileSystem', () => { this.skip(); } }); + async function checkMode(filename: string, expected: number) { const stat = await fsextra.stat(filename); expect(stat.mode & 0o777).to.equal(expected); @@ -255,17 +385,47 @@ suite('Raw FileSystem', () => { }); }); + suite('stat', () => { + test('gets the info for an existing file', async () => { + const filename = await fix.createFile('x/y/z/spam.py', '...'); + const expected = await fsextra.stat(filename); + + const stat = await filesystem.stat(filename); + + expect(stat).to.deep.equal(expected); + }); + + test('gets the info for an existing directory', async () => { + const dirname = await fix.createDirectory('x/y/z/spam'); + const expected = await fsextra.stat(dirname); + + const stat = await filesystem.stat(dirname); + + expect(stat).to.deep.equal(expected); + }); + + test('for symlinks, gets the info for the linked file', async () => { + const filename = await fix.createFile('x/y/z/spam.py', '...'); + const symlink = await fix.createSymlink('x/y/z/eggs.py', filename); + const expected = await fsextra.stat(filename); + + const stat = await filesystem.stat(symlink); + + expect(stat).to.deep.equal(expected); + }); + + test('fails if the file does not exist', async () => { + const promise = filesystem.stat(DOES_NOT_EXIST); + + await expect(promise).to.eventually.be.rejected; + }); + }); + suite('lstat', () => { test('for symlinks, gives the link info', async () => { const filename = await fix.createFile('x/y/z/spam.py', '...'); const symlink = await fix.createSymlink('x/y/z/eggs.py', filename); - const old = await fsextra.lstat(symlink); - const expected = { - type: FileType.SymbolicLink, - size: old.size, - ctime: old.ctimeMs, - mtime: old.mtimeMs - }; + const expected = await fsextra.lstat(symlink); const stat = await filesystem.lstat(symlink); @@ -274,13 +434,7 @@ suite('Raw FileSystem', () => { test('for normal files, gives the file info', async () => { const filename = await fix.createFile('x/y/z/spam.py', '...'); - const old = await fsextra.stat(filename); - const expected = { - type: FileType.File, - size: old.size, - ctime: old.ctimeMs, - mtime: old.mtimeMs - }; + const expected = await fsextra.stat(filename); const stat = await filesystem.lstat(filename); @@ -294,16 +448,120 @@ suite('Raw FileSystem', () => { }); }); + suite('listdir', () => { + test('mixed', async () => { + // Create the target directory and its contents. + const dirname = await fix.createDirectory('x/y/z'); + await fix.createFile('x/y/z/__init__.py', ''); + const script = await fix.createFile('x/y/z/__main__.py', '