From 0797b7e9c837617a05d0fe0d8291f7303accfc1b Mon Sep 17 00:00:00 2001 From: Chuck Ries Date: Mon, 7 Nov 2016 13:12:13 -0800 Subject: [PATCH] Respond to code review feedback --- gulpfile.js | 1 - package.json | 5 ++--- src/common.ts | 6 ++++++ src/coreclr-debug/activate.ts | 14 ++++++------ src/coreclr-debug/install.ts | 1 - src/coreclr-debug/util.ts | 40 +++++------------------------------ src/main.ts | 9 ++++---- 7 files changed, 25 insertions(+), 51 deletions(-) diff --git a/gulpfile.js b/gulpfile.js index 78c3d8a51b..a700a17797 100644 --- a/gulpfile.js +++ b/gulpfile.js @@ -14,7 +14,6 @@ const tslint = require('gulp-tslint'); const vsce = require('vsce'); const debugUtil = require('./out/src/coreclr-debug/util'); const debugInstall = require('./out/src/coreclr-debug/install'); -const fs_extra = require('fs-extra-promise'); const packages = require('./out/src/packages'); const logger = require('./out/src/logger'); const platform = require('./out/src/platform'); diff --git a/package.json b/package.json index 6b425349e6..0df2cdbf98 100644 --- a/package.json +++ b/package.json @@ -25,7 +25,6 @@ "postinstall": "node ./node_modules/vscode/bin/install" }, "dependencies": { - "fs-extra-promise": "^0.3.1", "http-proxy-agent": "^1.0.0", "https-proxy-agent": "^1.0.0", "lodash.debounce": "^4.0.8", @@ -91,7 +90,7 @@ ] }, { - "description": "OmniSharp (.NET Core - OSX / x64)", + "description": "OmniSharp (.NET Core - macOS / x64)", "url": "https://omnisharpdownload.blob.core.windows.net/ext/omnisharp-1.9-beta19-osx-x64-netcoreapp1.0.zip", "installPath": ".omnisharp-coreclr", "runtimeIds": [ @@ -200,7 +199,7 @@ ] }, { - "description": ".NET Core Debugger (OSX / x64)", + "description": ".NET Core Debugger (macOS / x64)", "url": "https://vsdebugger.azureedge.net/coreclr-debug-1-5-0/coreclr-debug-osx.10.11-x64.zip", "installPath": ".debugger", "runtimeIds": [ diff --git a/src/common.ts b/src/common.ts index 034818cf43..0ca265554c 100644 --- a/src/common.ts +++ b/src/common.ts @@ -21,6 +21,10 @@ export function getExtensionPath() { return extensionPath; } +export function getBinPath() { + return path.resolve(getExtensionPath(), "bin"); +} + export function buildPromiseChain(array: T[], builder: (item: T) => Promise): Promise { return array.reduce( (promise, n) => promise.then(() => builder(n)), @@ -75,6 +79,7 @@ export function touchInstallFile(type: InstallFileType): Promise { fs.writeFile(getInstallFilePath(type), '', err => { if (err) { reject(err); + return; } resolve(); @@ -87,6 +92,7 @@ export function deleteInstallFile(type: InstallFileType): Promise { fs.unlink(getInstallFilePath(type), err => { if (err) { reject(err); + return; } resolve(); diff --git a/src/coreclr-debug/activate.ts b/src/coreclr-debug/activate.ts index a01930457e..fc8a828dc5 100644 --- a/src/coreclr-debug/activate.ts +++ b/src/coreclr-debug/activate.ts @@ -12,23 +12,23 @@ import * as debugInstall from './install'; import * as path from 'path'; import { Logger } from './../logger' -let _util: CoreClrDebugUtil = null; +let _debugUtil: CoreClrDebugUtil = null; let _reporter: TelemetryReporter = null; let _logger: Logger = null; export function activate(context: vscode.ExtensionContext, reporter: TelemetryReporter, logger: Logger) { - _util = new CoreClrDebugUtil(context.extensionPath, logger); + _debugUtil = new CoreClrDebugUtil(context.extensionPath, logger); _reporter = reporter; _logger = logger; - if (!CoreClrDebugUtil.existsSync(_util.debugAdapterDir())) { + if (!CoreClrDebugUtil.existsSync(_debugUtil.debugAdapterDir())) { // We have been activated but it looks like our package was not installed. This is bad. logger.appendLine("[ERROR]: C# Extension failed to install the debugger package"); showInstallErrorMessage(); - } else if (!CoreClrDebugUtil.existsSync(_util.installCompleteFilePath())) { - _util.checkDotNetCli() + } else if (!CoreClrDebugUtil.existsSync(_debugUtil.installCompleteFilePath())) { + _debugUtil.checkDotNetCli() .then((dotnetInfo: DotnetInfo) => { - let installer = new debugInstall.DebugInstaller(_util); + let installer = new debugInstall.DebugInstaller(_debugUtil); installer.finishInstall() .then(() => { vscode.window.setStatusBarMessage('Successfully installed .NET Core Debugger.'); @@ -42,7 +42,7 @@ export function activate(context: vscode.ExtensionContext, reporter: TelemetryRe }, (err) => { // Check for dotnet tools failed. pop the UI // err is a DotNetCliError but use defaults in the unexpected case that it's not - showDotnetToolsWarning(err.ErrorMessage || _util.defaultDotNetCliErrorMessage()); + showDotnetToolsWarning(err.ErrorMessage || _debugUtil.defaultDotNetCliErrorMessage()); _logger.appendLine(err.ErrorString || err); // TODO: log telemetry? }); diff --git a/src/coreclr-debug/install.ts b/src/coreclr-debug/install.ts index 95df4ec370..c4116f2d23 100644 --- a/src/coreclr-debug/install.ts +++ b/src/coreclr-debug/install.ts @@ -42,7 +42,6 @@ export class InstallError extends Error { export class DebugInstaller { private _util: CoreClrDebugUtil = null; -// private _isOffline; constructor(util: CoreClrDebugUtil) { this._util = util; diff --git a/src/coreclr-debug/util.ts b/src/coreclr-debug/util.ts index a1e9aa7097..c19b66842f 100644 --- a/src/coreclr-debug/util.ts +++ b/src/coreclr-debug/util.ts @@ -9,6 +9,7 @@ import * as path from 'path'; import * as fs from 'fs'; import * as os from 'os'; import * as semver from 'semver'; +import { execChildProcess } from './../common' import { Logger } from './../logger' const MINIMUM_SUPPORTED_DOTNET_CLI: string = '1.0.0-preview2-003121'; @@ -79,13 +80,14 @@ export class CoreClrDebugUtil // This function checks for the presence of dotnet on the path and ensures the Version // is new enough for us. // Returns: a promise that returns a DotnetInfo class - // Throws: An CotNetCliError() from the return promise if either dotnet does not exist or is too old. + // Throws: An DotNetCliError() from the return promise if either dotnet does not exist or is too old. public checkDotNetCli(): Promise { let dotnetInfo = new DotnetInfo(); - return this.spawnChildProcess('dotnet', ['--info'], process.cwd(), (data: Buffer) => { - let lines: string[] = data.toString().replace(/\r/mg, '').split('\n'); + return execChildProcess('dotnet --info', process.cwd()) + .then((data: string) => { + let lines: string[] = data.replace(/\r/mg, '').split('\n'); lines.forEach(line => { let match: RegExpMatchArray; if (match = /^\ Version:\s*([^\s].*)$/.exec(line)) { @@ -116,38 +118,6 @@ export class CoreClrDebugUtil }); } - public spawnChildProcess(process: string, args: string[], workingDirectory: string, onStdout?: (data: Buffer) => void, onStderr?: (data: Buffer) => void): Promise { - const promise = new Promise((resolve, reject) => { - const child = child_process.spawn(process, args, { cwd: workingDirectory }); - - if (!onStdout) { - onStdout = (data) => { console.log(`${data}`); }; - } - child.stdout.on('data', onStdout); - - if (!onStderr) { - onStderr = (data) => { console.error(`${data}`); }; - } - child.stderr.on('data', onStderr); - - child.on('close', (code: number) => { - if (code != 0) { - console.log(`${process} exited with error code ${code}`);; - reject(new Error(code.toString())); - } - else { - resolve(); - } - }); - - child.on('error', (error: Error) => { - reject(error); - }); - }); - - return promise; - } - public static existsSync(path: string) : boolean { try { fs.accessSync(path, fs.F_OK); diff --git a/src/main.ts b/src/main.ts index f9f4274445..e1b14a277a 100644 --- a/src/main.ts +++ b/src/main.ts @@ -101,10 +101,6 @@ function installRuntimeDependencies(extension: vscode.Extension, logger: Lo installationStage = 'touchLockFile'; return util.touchInstallFile(util.InstallFileType.Lock); }) - .then(() => { - installationStage = 'deleteBeginFile'; - return util.deleteInstallFile(util.InstallFileType.Begin) - }) .catch(error => { errorMessage = error.toString(); logger.appendLine(`Failed at stage: ${installationStage}`); @@ -118,6 +114,11 @@ function installRuntimeDependencies(extension: vscode.Extension, logger: Lo // TODO: Send telemetry event statusItem.dispose(); + }) + .then(() => { + // We do this step at the end so that we clean up the begin file in the case that we hit above catch block + // Attach a an empty catch to this so that errors here do not propogate + return util.deleteInstallFile(util.InstallFileType.Begin).catch((error) => { }); }); }