Add option to run with a custom mono installation #2425

shana · 2018-07-19T12:48:26Z

👋

When useGlobalMono is set to always or auto, the launcher uses whatever mono it finds in the path, which may or may not be a usable mono version for omnisharp. The user may have a suitable mono installation for omnisharp somewhere else, but not set as the default system one, or they may want to have omnisharp launched with a specific mono for omnisharp for debugging purposes. In my case, I have:

drwxr-xr-x  4.2.2.30
drwxr-xr-x  4.6.2.16
drwxr-xr-x  4.8.1.0
drwxr-xr-x  5.12.0.226_1
drwxr-xr-x  5.14.0.169_1
drwxr-xr-x  5.4.1.6
drwxr-xr-x  5.8.1.0_1

and I switch between them often, depending on what I need to do. Having vscode tied to whatever happens to be currently set on my system is really brittle, and if I need to have vscode use a particular mono version because of some mono bug, it's hard.

This PR adds a monoPath configuration option, and changes the launcher so that the environment variables PATH and MONO_GAC_PREFIX are changed to include the monoPath value, if set, when launching mono. If the monoPath option isn't set, it behaves as normal (also if useGlobalMono is set to never). I figured maybe this is useful for other people out there too 😃

When `useGlobalMono` is set to `always` or `auto`, the launcher uses whatever mono it finds in the path, which may or may not be a usable mono version for omnisharp. The user may have a suitable mono installation for omnisharp somewhere else, but not set as the default system one, or they may want to have omnisharp launched with a specific mono for omnisharp for debugging purposes. This adds a `monoPath` configuration option, and changes the launcher so that the environment variables PATH and MONO_GAC_PREFIX are changed to include the `monoPath` value, if set, when launching mono.

dnfclas · 2018-07-19T12:48:37Z

All CLA requirements met.

akshita31 · 2018-07-19T19:06:11Z

@shana Thank you for taking the time to contribute to the repo.
I notice that the integration tests are failing somehow. The timeout suggests that there is a problem with starting the "OmniSharp" process. Did you try running the integration tests on your machine?
You could run them by using npm run test:integration.

rchande · 2018-07-19T21:01:38Z

@shana Thanks for your contribution! Looks good to me, once tests pass.

… made other comparisons fail :/

codecov · 2018-07-20T10:32:29Z

Codecov Report

Merging #2425 into master will decrease coverage by 0.04%.
The diff coverage is 72.22%.

@@            Coverage Diff             @@
##           master    #2425      +/-   ##
==========================================
- Coverage   63.73%   63.68%   -0.05%     
==========================================
  Files          88       88              
  Lines        3998     4007       +9     
  Branches      564      567       +3     
==========================================
+ Hits         2548     2552       +4     
- Misses       1290     1292       +2     
- Partials      160      163       +3

Flag	Coverage Δ
#integration	`52.92% <55.55%> (-0.07%)`	⬇️
#unit	`84.71% <100%> (+0.05%)`	⬆️

Impacted Files	Coverage Δ
src/omnisharp/server.ts	`72.43% <100%> (+0.7%)`	⬆️
src/omnisharp/loggingEvents.ts	`100% <100%> (ø)`	⬆️
src/omnisharp/options.ts	`100% <100%> (ø)`	⬆️
src/observers/OmnisharpLoggerObserver.ts	`100% <100%> (ø)`	⬆️
src/omnisharp/launcher.ts	`53.22% <37.5%> (-0.95%)`	⬇️
src/omnisharp/delayTracker.ts	`68.42% <0%> (-5.27%)`	⬇️
src/omnisharp/requestQueue.ts	`72.72% <0%> (-3.04%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 16c5546...69ba549. Read the comment docs.

shana · 2018-07-20T10:40:12Z

Thanks for the review! Looks like the return value from the settings file was null instead of undefined, which meant the block overriding the env vars was always running, and I imagine screwing up the environment for spawning omnisharp in the process. I ran the integration tests locally with an up to date default system mono with no monoPath override, and with an older system mono and the monoPath override, and everything passes locally.

Can't really add much coverage for the integration tests though, not without bundling mono just for that purpose :/

akshita31 · 2018-07-20T19:11:32Z

src/omnisharp/launcher.ts

@@ -306,14 +312,15 @@ function launchNix(launchPath: string, cwd: string, args: string[]): LaunchResul
    };
 }

-function launchNixMono(launchPath: string, monoVersion: string, cwd: string, args: string[]): LaunchResult {
+function launchNixMono(launchPath: string, monoVersion: string, cwd: string, args: string[], environment: NodeJS.ProcessEnv): LaunchResult {
    let argsCopy = args.slice(0); // create copy of details args
    argsCopy.unshift(launchPath);
    argsCopy.unshift("--assembly-loader=strict");



Should we add some logging here, like printing the mono launch path, just like we print the version?

We create the log event here from the launch result: https://github.com/OmniSharp/omnisharp-vscode/blob/master/src/omnisharp/server.ts#L324. We might also return a monoLaunchPath in the launch result and display it here: https://github.com/OmniSharp/omnisharp-vscode/blob/master/src/observers/OmnisharpLoggerObserver.ts#L58. We will have to add the appropriate property to OmnisharpLaunch event here: https://github.com/OmniSharp/omnisharp-vscode/blob/master/src/omnisharp/loggingEvents.ts#L30.
Basically to do logging in omnisharp-vscode, we post the events into an event stream to which some observers subscribe. Whenever we post an event to the event stream, all the observers are invoked and take action appropriately. Does that make sense ?

Yeah, I hacked up some manual logging directly to a separate window just so I could see what was going on while I was putting this together, but adding some more information to the event stream probably makes sense, I can give that a try.

@akshita31 I added some logging and in the process tweaked the test method a bit to avoid growing the if block, let me know if that's what you had in mind.

akshita31 · 2018-07-23T17:59:34Z

src/omnisharp/server.ts

@@ -321,7 +321,7 @@ export class OmniSharpServer {

        try {
            let launchResult = await launchOmniSharp(cwd, args, launchInfo, this.platformInfo, options);
-            this.eventStream.post(new ObservableEvents.OmnisharpLaunch(launchResult.monoVersion, launchResult.command, launchResult.process.pid));
+            this.eventStream.post(new ObservableEvents.OmnisharpLaunch(launchResult.monoVersion, launchResult.command, launchResult.process.pid, options.monoPath));


I think it would be better if the LaunchResult returned the monoLaunchPath(if any) that it used, instead of passing the options.monoPath here. That is because the launcher is the one that must decide whatever mono launch path should be used and here we are always assuming that if a monoPath is set that will be the one used by the launcher.

Oh yes, that makes way more sense, not sure how I missed that. ☕️ ☕️ ☕️

Also no reason why the logging object should have optional arguments, fix that.

shana · 2018-07-24T10:33:16Z

@akshita31 Logging fixed! 😄

akshita31 · 2018-07-24T19:24:10Z

@shana Thank you so much for patiently going through the feedback.
If you like, you can do a changelog update in a separate PR, otherwise I will do that.
Thanks again for taking time to make the repo better!

shana · 2018-07-26T14:09:54Z

@akshita31 Sorry, I didn't see your comment. Yay merge! I was not aware of the need to update the changelog manually, how does that work? It's just adding a new entry for the change?

akshita31 · 2018-07-26T17:01:40Z

@shana Yes that is right.

This reverts commit b77d89f, reversing changes made to 16c5546.

rchande approved these changes Jul 19, 2018

View reviewed changes

VSCode returns null instead of undefined when there's no value, which…

c181c74

… made other comparisons fail :/

Merge branch 'master' into master

a35c4ab

akshita31 reviewed Jul 20, 2018

View reviewed changes

Log the mono path when it's set, and add tests for logging output

a7226d6

akshita31 suggested changes Jul 23, 2018

View reviewed changes

Record the mono path in the launch result for the log

69ba549

Also no reason why the logging object should have optional arguments, fix that.

akshita31 approved these changes Jul 24, 2018

View reviewed changes

akshita31 merged commit b77d89f into dotnet:master Jul 24, 2018

rchande pushed a commit to rchande/omnisharp-vscode that referenced this pull request Jul 31, 2018

Revert "Merge pull request dotnet#2425 from shana/master"

80d8848

This reverts commit b77d89f, reversing changes made to 16c5546.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to run with a custom mono installation #2425

Add option to run with a custom mono installation #2425

shana commented Jul 19, 2018

dnfclas commented Jul 19, 2018 •

edited

Loading

akshita31 commented Jul 19, 2018 •

edited

Loading

rchande commented Jul 19, 2018

codecov bot commented Jul 20, 2018 •

edited

Loading

shana commented Jul 20, 2018 •

edited

Loading

akshita31 Jul 20, 2018

shana Jul 23, 2018

shana Jul 23, 2018

akshita31 Jul 23, 2018 •

edited

Loading

shana Jul 24, 2018

shana commented Jul 24, 2018

akshita31 commented Jul 24, 2018

shana commented Jul 26, 2018

akshita31 commented Jul 26, 2018

Add option to run with a custom mono installation #2425

Add option to run with a custom mono installation #2425

Conversation

shana commented Jul 19, 2018

dnfclas commented Jul 19, 2018 • edited Loading

akshita31 commented Jul 19, 2018 • edited Loading

rchande commented Jul 19, 2018

codecov bot commented Jul 20, 2018 • edited Loading

Codecov Report

shana commented Jul 20, 2018 • edited Loading

akshita31 Jul 20, 2018

Choose a reason for hiding this comment

shana Jul 23, 2018

Choose a reason for hiding this comment

shana Jul 23, 2018

Choose a reason for hiding this comment

akshita31 Jul 23, 2018 • edited Loading

Choose a reason for hiding this comment

shana Jul 24, 2018

Choose a reason for hiding this comment

shana commented Jul 24, 2018

akshita31 commented Jul 24, 2018

shana commented Jul 26, 2018

akshita31 commented Jul 26, 2018

dnfclas commented Jul 19, 2018 •

edited

Loading

akshita31 commented Jul 19, 2018 •

edited

Loading

codecov bot commented Jul 20, 2018 •

edited

Loading

shana commented Jul 20, 2018 •

edited

Loading

akshita31 Jul 23, 2018 •

edited

Loading