Respond to code review feedback

Author: chuckries

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');

package.json

@@ -91,7 +91,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 +200,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": [

src/common.ts

@@ -21,6 +21,10 @@ export function getExtensionPath() {
     return extensionPath;
 }
 
+export function getBinPath() {
+    return path.resolve(getExtensionPath(), "bin");
+}
+
 export function buildPromiseChain<T, TResult>(array: T[], builder: (item: T) => Promise<TResult>): Promise<TResult> {
     return array.reduce(
         (promise, n) => promise.then(() => builder(n)),
@@ -75,6 +79,7 @@ export function touchInstallFile(type: InstallFileType): Promise<void> {
         fs.writeFile(getInstallFilePath(type), '', err => {
             if (err) {
                 reject(err);
+                return;
             }
 
             resolve();
@@ -87,6 +92,7 @@ export function deleteInstallFile(type: InstallFileType): Promise<void> {
         fs.unlink(getInstallFilePath(type), err => {
             if (err) {
                 reject(err);
+                return;
             }
 
             resolve();

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?
             });

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;

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<DotnetInfo>
     {
         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<void> {
-        const promise = new Promise<void>((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);

src/main.ts

@@ -101,10 +101,6 @@ function installRuntimeDependencies(extension: vscode.Extension<any>, 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<any>, 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) => { });
         });
 }