Merge pull request #2221 from DustinCampbell/fix-omnisharp-launch

Ensure that OmniSharp is launched on global mono even when a version or 'latest' specifed

Author: DustinCampbell

src/observers/OmnisharpLoggerObserver.ts

@@ -54,8 +54,8 @@ export class OmnisharpLoggerObserver extends BaseLoggerObserver {
     }
 
     private handleOmnisharpLaunch(event: OmnisharpLaunch) {
-        if (event.usingMono) {
-            this.logger.appendLine(`OmniSharp server started with Mono`);
+        if (event.monoVersion) {
+            this.logger.appendLine(`OmniSharp server started with Mono ${event.monoVersion}`);
         }
         else {
             this.logger.appendLine(`OmniSharp server started`);

src/omnisharp/OmnisharpManager.ts

@@ -9,60 +9,77 @@ import * as util from '../common';
 import { OmnisharpDownloader } from './OmnisharpDownloader';
 import { PlatformInformation } from '../platform';
 
+export interface LaunchInfo {
+    LaunchPath: string;
+    MonoLaunchPath?: string;
+}
+
 export class OmnisharpManager {
     public constructor(
         private downloader: OmnisharpDownloader,
         private platformInfo: PlatformInformation) {
     }
 
-    public async GetOmnisharpPath(omnisharpPath: string, useMono: boolean, serverUrl: string, latestVersionFileServerPath: string, installPath: string, extensionPath: string): Promise<string> {
+    public async GetOmniSharpLaunchInfo(omnisharpPath: string, serverUrl: string, latestVersionFileServerPath: string, installPath: string, extensionPath: string): Promise<LaunchInfo> {
+        if (!omnisharpPath) {
+            // If omnisharpPath was not specified, return the default path.
+            let basePath = path.resolve(extensionPath, '.omnisharp');
+            return this.GetLaunchInfo(this.platformInfo, basePath);
+        }
+
         // Looks at the options path, installs the dependencies and returns the path to be loaded by the omnisharp server
         if (path.isAbsolute(omnisharpPath)) {
-            if (await util.fileExists(omnisharpPath)) {
-                return omnisharpPath;
-            }
-            else {
+            if (!await util.fileExists(omnisharpPath)) {
                 throw new Error('The system could not find the specified path');
             }
+
+            return {
+                LaunchPath: omnisharpPath
+            };
         }
-        else if (omnisharpPath == "latest") {
-            return await this.InstallLatestAndReturnLaunchPath(useMono, serverUrl, latestVersionFileServerPath, installPath, extensionPath);
+        else if (omnisharpPath === 'latest') {
+            return await this.InstallLatestAndReturnLaunchInfo(serverUrl, latestVersionFileServerPath, installPath, extensionPath);
         }
 
-        //If the path is neither a valid path on disk not the string "latest", treat it as a version 
-        return await this.InstallVersionAndReturnLaunchPath(omnisharpPath, useMono, serverUrl, installPath, extensionPath);
+        // If the path is neither a valid path on disk not the string "latest", treat it as a version 
+        return await this.InstallVersionAndReturnLaunchInfo(omnisharpPath, serverUrl, installPath, extensionPath);
     }
 
-    private async InstallLatestAndReturnLaunchPath(useMono: boolean, serverUrl: string, latestVersionFileServerPath: string, installPath: string, extensionPath: string) {
+    private async InstallLatestAndReturnLaunchInfo(serverUrl: string, latestVersionFileServerPath: string, installPath: string, extensionPath: string): Promise<LaunchInfo> {
         let version = await this.downloader.GetLatestVersion(serverUrl, latestVersionFileServerPath);
-        return await this.InstallVersionAndReturnLaunchPath(version, useMono, serverUrl, installPath, extensionPath);
+        return await this.InstallVersionAndReturnLaunchInfo(version, serverUrl, installPath, extensionPath);
     }
 
-    private async InstallVersionAndReturnLaunchPath(version: string, useMono: boolean, serverUrl: string, installPath: string, extensionPath: string) {
+    private async InstallVersionAndReturnLaunchInfo(version: string, serverUrl: string, installPath: string, extensionPath: string): Promise<LaunchInfo> {
         if (semver.valid(version)) {
             await this.downloader.DownloadAndInstallOmnisharp(version, serverUrl, installPath);
-            return GetLaunchPathForVersion(this.platformInfo, version, installPath, extensionPath, useMono);
+            return this.GetLaunchPathForVersion(this.platformInfo, version, installPath, extensionPath);
         }
         else {
-            throw new Error(`Invalid omnisharp version - ${version}`);
+            throw new Error(`Invalid OmniSharp version - ${version}`);
         }
     }
-}
 
-function GetLaunchPathForVersion(platformInfo: PlatformInformation, version: string, installPath: string, extensionPath: string, useMono: boolean) {
-    if (!version) {
-        throw new Error('Invalid Version');
-    }
+    private GetLaunchPathForVersion(platformInfo: PlatformInformation, version: string, installPath: string, extensionPath: string): LaunchInfo {
+        if (!version) {
+            throw new Error('Invalid Version');
+        }
 
-    let basePath = path.resolve(extensionPath, installPath, version);
+        let basePath = path.resolve(extensionPath, installPath, version);
 
-    if (platformInfo.isWindows()) {
-        return path.join(basePath, 'OmniSharp.exe');
-    }
-    if (useMono) {
-        return path.join(basePath, 'omnisharp', 'OmniSharp.exe');
+        return this.GetLaunchInfo(platformInfo, basePath);
     }
 
-    return path.join(basePath, 'run');
-}
+    private GetLaunchInfo(platformInfo: PlatformInformation, basePath: string): LaunchInfo {
+        if (platformInfo.isWindows()) {
+            return {
+                LaunchPath: path.join(basePath, 'OmniSharp.exe')
+            };
+        }
 
+        return {
+            LaunchPath: path.join(basePath, 'run'),
+            MonoLaunchPath: path.join(basePath, 'omnisharp', 'OmniSharp.exe')
+        };
+    }
+}

src/omnisharp/launcher.ts

@@ -8,8 +8,8 @@ import { satisfies } from 'semver';
 import { PlatformInformation } from '../platform';
 import * as path from 'path';
 import * as vscode from 'vscode';
-import * as util from '../common';
 import { Options } from './options';
+import { LaunchInfo } from './OmnisharpManager';
 
 export enum LaunchTargetKind {
     Solution,
@@ -201,12 +201,12 @@ function isCake(resource: vscode.Uri): boolean {
 export interface LaunchResult {
     process: ChildProcess;
     command: string;
-    usingMono: boolean;
+    monoVersion?: string;
 }
 
-export async function launchOmniSharp(cwd: string, args: string[], launchPath: string): Promise<LaunchResult> {
+export async function launchOmniSharp(cwd: string, args: string[], launchInfo: LaunchInfo): Promise<LaunchResult> {
     return new Promise<LaunchResult>((resolve, reject) => {
-        launch(cwd, args, launchPath)
+        launch(cwd, args, launchInfo)
             .then(result => {
                 // async error - when target not not ENEOT
                 result.process.on('error', err => {
@@ -222,50 +222,42 @@ export async function launchOmniSharp(cwd: string, args: string[], launchPath: s
     });
 }
 
-async function launch(cwd: string, args: string[], launchPath: string): Promise<LaunchResult> {
-    return PlatformInformation.GetCurrent().then(platformInfo => {
-        const options = Options.Read();
-
-        if (options.useEditorFormattingSettings) {
-            let globalConfig = vscode.workspace.getConfiguration();
-            let csharpConfig = vscode.workspace.getConfiguration('[csharp]');
+async function launch(cwd: string, args: string[], launchInfo: LaunchInfo): Promise<LaunchResult> {
+    const platformInfo = await PlatformInformation.GetCurrent();
+    const options = Options.Read();
 
-            args.push(`formattingOptions:useTabs=${!getConfigurationValue(globalConfig, csharpConfig, 'editor.insertSpaces', true)}`);
-            args.push(`formattingOptions:tabSize=${getConfigurationValue(globalConfig, csharpConfig, 'editor.tabSize', 4)}`);
-            args.push(`formattingOptions:indentationSize=${getConfigurationValue(globalConfig, csharpConfig, 'editor.tabSize', 4)}`);
-        }
+    if (options.useEditorFormattingSettings) {
+        let globalConfig = vscode.workspace.getConfiguration();
+        let csharpConfig = vscode.workspace.getConfiguration('[csharp]');
 
-        // If the user has provided an absolute path or the specified version has been installed successfully, we'll use the path.
-        if (launchPath) {
-            if (platformInfo.isWindows()) {
-                return launchWindows(launchPath, cwd, args);
-            }
+        args.push(`formattingOptions:useTabs=${!getConfigurationValue(globalConfig, csharpConfig, 'editor.insertSpaces', true)}`);
+        args.push(`formattingOptions:tabSize=${getConfigurationValue(globalConfig, csharpConfig, 'editor.tabSize', 4)}`);
+        args.push(`formattingOptions:indentationSize=${getConfigurationValue(globalConfig, csharpConfig, 'editor.tabSize', 4)}`);
+    }
 
-            // If we're launching on macOS/Linux, we have two possibilities:
-            //   1. Launch using Mono
-            //   2. Launch process directly (e.g. a 'run' script)
-            return options.useMono
-                ? launchNixMono(launchPath, cwd, args)
-                : launchNix(launchPath, cwd, args);
-        }
+    if (platformInfo.isWindows()) {
+        return launchWindows(launchInfo.LaunchPath, cwd, args);
+    }
 
-        // If the user has not provided a path, we'll use the locally-installed OmniSharp
-        const basePath = path.resolve(util.getExtensionPath(), '.omnisharp');
+    let monoVersion = await getMonoVersion();
+    let isValidMonoAvailable = await satisfies(monoVersion, '>=5.2.0');
 
-        if (platformInfo.isWindows()) {
-            return launchWindows(path.join(basePath, 'OmniSharp.exe'), cwd, args);
+    // If the user specifically said that they wanted to launch on Mono, respect their wishes.
+    if (options.useMono) {
+        if (!isValidMonoAvailable) {
+            throw new Error('Cannot start OmniSharp because Mono version >=5.2.0 is required.');
         }
 
-        // If it's possible to launch on a global Mono, we'll do that. Otherwise, run with our
-        // locally installed Mono runtime.
-        return canLaunchMono()
-            .then(async () => {
-                return launchNixMono(path.join(basePath, 'omnisharp', 'OmniSharp.exe'), cwd, args);
-            })
-            .catch(_ => {
-                return launchNix(path.join(basePath, 'run'), cwd, args);
-            });
-    });
+        return launchNixMono(launchInfo.LaunchPath, monoVersion, cwd, args);
+    }
+
+    // If we can launch on the global Mono, do so; otherwise, launch directly;
+    if (isValidMonoAvailable && launchInfo.MonoLaunchPath) {
+        return launchNixMono(launchInfo.MonoLaunchPath, monoVersion, cwd, args);
+    }
+    else {
+        return launchNix(launchInfo.LaunchPath, cwd, args);
+    }
 }
 
 function getConfigurationValue(globalConfig: vscode.WorkspaceConfiguration, csharpConfig: vscode.WorkspaceConfiguration,
@@ -303,7 +295,6 @@ function launchWindows(launchPath: string, cwd: string, args: string[]): LaunchR
     return {
         process,
         command: launchPath,
-        usingMono: false
     };
 }
 
@@ -315,58 +306,41 @@ function launchNix(launchPath: string, cwd: string, args: string[]): LaunchResul
 
     return {
         process,
-        command: launchPath,
-        usingMono: true
+        command: launchPath
     };
 }
 
-async function launchNixMono(launchPath: string, cwd: string, args: string[]): Promise<LaunchResult> {
-    return canLaunchMono()
-        .then(() => {
-            let argsCopy = args.slice(0); // create copy of details args
-            argsCopy.unshift(launchPath);
-            argsCopy.unshift("--assembly-loader=strict");
-
-            let process = spawn('mono', argsCopy, {
-                detached: false,
-                cwd: cwd
-            });
-
-            return {
-                process,
-                command: launchPath,
-                usingMono: true
-            };
-        });
-}
+function launchNixMono(launchPath: string, monoVersion: string, cwd: string, args: string[]): LaunchResult {
+    let argsCopy = args.slice(0); // create copy of details args
+    argsCopy.unshift(launchPath);
+    argsCopy.unshift("--assembly-loader=strict");
 
-async function canLaunchMono(): Promise<void> {
-    return new Promise<void>((resolve, reject) => {
-        hasMono('>=5.2.0').then(success => {
-            if (success) {
-                resolve();
-            }
-            else {
-                reject(new Error('Cannot start Omnisharp because Mono version >=5.2.0 is required.'));
-            }
-        });
+    let process = spawn('mono', argsCopy, {
+        detached: false,
+        cwd: cwd
     });
+
+    return {
+        process,
+        command: launchPath,
+        monoVersion, 
+    };
 }
 
-export async function hasMono(range?: string): Promise<boolean> {
+async function getMonoVersion(): Promise<string> {
     const versionRegexp = /(\d+\.\d+\.\d+)/;
 
-    return new Promise<boolean>((resolve, reject) => {
+    return new Promise<string>((resolve, reject) => {
         let childprocess: ChildProcess;
         try {
             childprocess = spawn('mono', ['--version']);
         }
         catch (e) {
-            return resolve(false);
+            return resolve(undefined);
         }
 
         childprocess.on('error', function (err: any) {
-            resolve(false);
+            resolve(undefined);
         });
 
         let stdout = '';
@@ -375,20 +349,14 @@ export async function hasMono(range?: string): Promise<boolean> {
         });
 
         childprocess.stdout.on('close', () => {
-            let match = versionRegexp.exec(stdout),
-                ret: boolean;
+            let match = versionRegexp.exec(stdout);
 
-            if (!match) {
-                ret = false;
-            }
-            else if (!range) {
-                ret = true;
+            if (match && match.length > 1) {
+                resolve(match[1]);
             }
             else {
-                ret = satisfies(match[1], range);
+                resolve(undefined);
             }
-
-            resolve(ret);
         });
     });
 }

src/omnisharp/loggingEvents.ts

@@ -27,7 +27,7 @@ export class OmnisharpInitialisation implements BaseEvent {
 }
 
 export class OmnisharpLaunch implements BaseEvent {
-    constructor(public usingMono: boolean, public command: string, public pid: number) { }
+    constructor(public monoVersion: string, public command: string, public pid: number) { }
 }
 
 export class PackageInstallation implements BaseEvent {