Enable usage of multiple versions of omnisharp instead of just the released version #1972

akshita31 · 2018-01-22T22:02:36Z

This is the first step in a series of changes to solve the issue : #1909

Changes :

Added option alternateVersion in which a string value can be specified, and if the particular version is present in .omnisharp/experimental folder, the server will be started using that.
Added option useLatestExperimentalBuild which if set will start the server from the latest version present in the .omnisharp/experiment folder.

Next steps:

For the latest option , download a version.txt kind of file and get the latest version from that. If the version is not present then download and install the appropriate packages and then start the server.
For the release option , if the version is not present download and install it and then start the server.

rchande · 2018-01-22T23:29:26Z

package.json

+        "omnisharp.experimentOmnisharp":{
+          "type":"string",
+          "default": null,
+          "description": "Specifies if the user wants to use the experiment build. Possible values - latest or some version number" 


I think there's a way of specifying possible values so they show up in completions. Don't block on this, but something to consider.

There's an example of how to do that in this very file: https://github.com/OmniSharp/omnisharp-vscode/blob/master/package.json#L376-L388

rchande · 2018-01-22T23:30:46Z

src/omnisharp/OmnisharpDownloader.ts

+
+export class OmnisharpDownloader {
+
+    public GetLatestExperimentVersion(): string {


Since this tells you the most recently installed version, maybe GetLatestInstalledExperimentalVersion?

rchande · 2018-01-22T23:32:35Z

src/omnisharp/OmnisharpDownloader.ts

+        let latestVersion: string;
+        let items = fs.readdirSync(basePath);
+        if (items) {
+            items.sort(compareVersions);


This could just be a for loop (or whatever JS/TS equivalent there is to Enumerable.Min).

probably better to do a loop and use package like semver to address also the non-semantic version string (this will return a null object) and also handling pre-release versions

rchande · 2018-01-22T23:33:04Z

src/omnisharp/OmnisharpDownloader.ts

+        let items = fs.readdirSync(basePath);
+        if (items) {
+            items.sort(compareVersions);
+            latestVersion = items[items.length - 1];


Also, what if there are folders whose names aren't parseable versions?

rchande · 2018-01-22T23:34:36Z

src/omnisharp/launcher.ts

+            basePath = path.resolve(util.getExtensionPath(), `.omnisharp/experiment/${experimentVersion}`);
+        }
+        else {
+            // If the user has not provided a path, we'll use the locally-installed OmniSharp


This comment needs updated.

rchande · 2018-01-22T23:35:56Z

src/omnisharp/options.ts

        public showTestsCodeLens?: boolean,
-        public disableCodeActions?: boolean) { }
+        public disableCodeActions?: boolean,
+        public experimentOmnisharp?: string) { }


Can we replace all instances of experiment with experimental? :)

rchande · 2018-01-22T23:40:28Z

src/omnisharp/OmnisharpDownloader.ts

+            //get the latest version after sorting
+        }
+
+        return latestVersion;


This looks like something we could unit test :).

…riment version

rchande

Looks great! Just the one question for you.

rchande · 2018-01-24T23:55:13Z

src/omnisharp/launcher.ts

+        }
+        else {
+            // If the user has not provided a path and some experimental version, we'll use the locally-installed OmniSharp
+            basePath = path.resolve(util.getExtensionPath(), '.omnisharp');


Why does this computation happen in server.ts and this file?

rchande · 2018-01-24T23:55:44Z

src/omnisharp/OmnisharpDownloader.ts

+            if (installedItems && installedItems.length > 0) {
+                let validVersions = installedItems.filter(value => semver.valid(value));
+                if (validVersions && validVersions.length > 0) {
+                    latestVersion = validVersions.reduce((latestTillNow, element) => {


added colomn to test functions

remove unused import to comply with tslint

DustinCampbell

It's hard to tell since this PR doesn't represent the entire feature, but I'm a little concerned that it seems like we might be planning to write another downloader here. I would recommend against that and leverage the code we already have to leverage packages.

DustinCampbell · 2018-01-26T16:35:14Z

package.json

+          "type": "string",
+          "default": null,
+          "description": "Specifies if the user wants to use the experiment build. Possible values - latest or some version number"
+        },


Please provide "enum" values to help with JSON completion and schema validation. See "omnisharp.loggingLevel" below for an example.

Also, please clean up the grammar in the description. This is a user-facing string.

Also, omnisharp.experimentalOmniSharp is a bit redundant. It's also not clear to me why this is "experimental". Maybe omnisharp.alternateVersion or something like that? @rchande?

DustinCampbell · 2018-01-26T16:36:37Z

src/omnisharp/OmnisharpDownloader.ts

+export class OmnisharpDownloader {
+
+    public GetLatestInstalledExperimentalVersion(basePath: string) {
+        const semver = require('semver');


Why not just import * as semver from 'semver' at the top of the module?

DustinCampbell · 2018-01-26T16:41:51Z

src/omnisharp/OmnisharpDownloader.ts

+
+import * as fs from 'fs';
+
+export class OmnisharpDownloader {


I'm confused by this name since this class doesn't actually download anything at all.

DustinCampbell · 2018-01-26T16:42:58Z

src/omnisharp/server.ts

+        let extensionPath = utils.getExtensionPath();
+
+        if(experimentalOption == "latest"){
+            let downloader = new OmnisharpDownloader();


Is this expected to actually download something from the Internet somehow? If so, should this be plugged into the package downloader that already exists in omnisharp-vscode?

colombod · 2018-01-26T19:52:58Z

src/omnisharp/latestOptionHelper.ts

@@ -0,0 +1,28 @@
+/*---------------------------------------------------------------------------------------------


would not be better to call this file VersionSelectors.ts ?

TheRealPiotrP · 2018-01-26T22:24:53Z

@DustinCampbell I share your concern about duplication. We took a good hard look at the existing downloader and in fact started by modifying it. However, that code is very lightly tested and the current structure doesn't lend itself to testability. We intend to:

merge this feature
temporarily fork the downloader, refactoring it to be testable and tested
once we're happy with the downloader, extend it to provide for the O# scenario as well

Let's chat about this next week?

DustinCampbell · 2018-01-29T15:42:25Z

@TheRealPiotrP : Sounds gool! Although, this PR doesn't actually do any downloading, so there's not a lot of duplication yet. 😄 I just want to be sure that we don't regress any of the bugs that were fixed in the original downloader.

akshita31 · 2018-02-16T20:30:20Z

The approach to the feature changed a bit #2028 and #2039, hence the functionality added here is no more needed. Closing this for now

akshita31 requested review from DustinCampbell, TheRealPiotrP, filipw and rchande January 22, 2018 22:02

Enable usage of multiple versions of omnisharp

e9457f2

akshita31 force-pushed the experiment_omnisharp_1 branch from 653165c to e9457f2 Compare January 22, 2018 22:13

Added file header

d24726d

colombod approved these changes Jan 22, 2018

View reviewed changes

rchande reviewed Jan 22, 2018

View reviewed changes

akshita31 changed the base branch from master to experiment_download January 23, 2018 22:29

akshita31 added 5 commits January 23, 2018 15:37

Renamed method and changed all occurences to experimental

fc48342

Modified comment and using reduce instead of sort

b657b84

Using semver to validate packages

a303298

Changes for the downloader to take a path for getting the latest expe…

bac18df

…riment version

Added unit test for testing latest experiment version

7237fd3

akshita31 force-pushed the experiment_omnisharp_1 branch from b621073 to 7237fd3 Compare January 24, 2018 21:52

akshita31 added 2 commits January 24, 2018 14:04

Remove unnecessary import

384b190

Added checks for the pre-release versions

49f9d37

rchande approved these changes Jan 24, 2018

View reviewed changes

akshita31 and others added 4 commits January 24, 2018 16:43

ExtensionPath Calculation only in server.ts

05a3cdb

remove unused import to comply with tslint

924db95

added colomn to test functions

Merge pull request #1 from colombod/experiment_omnisharp_1

7efb2a9

remove unused import to comply with tslint

Manually deleting the files in tmp directory

031aa4f

DustinCampbell reviewed Jan 26, 2018

View reviewed changes

colombod reviewed Jan 26, 2018

View reviewed changes

akshita31 force-pushed the experiment_omnisharp_1 branch from f65f377 to c32eda4 Compare January 26, 2018 21:08

Separated the feature in two options and removed the downloader class

498ccab

akshita31 force-pushed the experiment_omnisharp_1 branch from c32eda4 to 498ccab Compare January 26, 2018 21:10

Remove unnecessary import

23819e9

DustinCampbell approved these changes Jan 29, 2018

View reviewed changes

Synchronous creation of directories

cc6f081

akshita31 closed this Feb 16, 2018

akshita31 deleted the experiment_omnisharp_1 branch February 23, 2018 23:13


		export class OmnisharpDownloader {

		public GetLatestExperimentVersion(): string {

		@@ -0,0 +1,28 @@
		/*---------------------------------------------------------------------------------------------

Enable usage of multiple versions of omnisharp instead of just the released version #1972

Enable usage of multiple versions of omnisharp instead of just the released version #1972

Uh oh!

Conversation

akshita31 commented Jan 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rchande left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DustinCampbell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheRealPiotrP commented Jan 26, 2018

Uh oh!

DustinCampbell commented Jan 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akshita31 commented Feb 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

akshita31 commented Jan 22, 2018 •

edited

Loading

DustinCampbell commented Jan 29, 2018 •

edited

Loading