Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable usage of omnisharp.path option for downloading multiple versions of omnisharp #2028

Merged
merged 22 commits into from
Feb 14, 2018

Conversation

akshita31
Copy link
Contributor

@akshita31 akshita31 commented Feb 9, 2018

This is a part of the change to add the feature : #1909

Brief Outline:

  1. The server checks if the omnisharp.path is set and if not proceeds with the normal default launch
  2. If some path has been specified, the GetExperimentalOmnisharpPath is called which returns the appropriate path that will be used by the launcher.

GetExperimentOmnisharpPath : First checks whether the path is an existing path on disk, if so the same path is returned. Else we try to treat this path as a version, then invoke InstallVersionAndReturnLaunchPath which installs the packages and returns the path to be used.

If at any point in the install and download process ,an error is encountered like 'Invalid Version' or 'Download failure', the error is returned upto the server and the launcher is not called.
omnisharpdownload

Also attached is a representation of the feature. Note that the "latest" download is not a part of this PR.

@akshita31 akshita31 changed the base branch from master to experiment_download February 9, 2018 23:06
Copy link

@rchande rchande left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall approach seems reasonable; various questions and suggestions

import { GetDependenciesAndDownloadPackages, GetStatus, GetAndLogPlatformInformation, ReportInstallationError, SendInstallationTelemetry } from '../experimentalOmnisharp.Download.Helper';

export class ExperimentalOmnisharpDownloader {

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra blank line

public constructor(
private channel: vscode.OutputChannel,
private logger: Logger,
private reporter: TelemetryReporter /* optional */,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use optional parameter syntax?

private channel: vscode.OutputChannel,
private logger: Logger,
private reporter: TelemetryReporter /* optional */,
private packageJSON: any) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have a more specific type for this?

throw new Error('Invalid version');
}

this.logger.append('Getting the version packages...');
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what "getting the version packages" means.

this.logger.appendLine();
this.channel.show();

let statusItem = vscode.window.createStatusBarItem(vscode.StatusBarAlignment.Right);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pattern here is: "call raw VSCode APIs to add status bar item, then call a helper to wrap the VSCode status item into something easy to manipulate". Can we push the VSCode API call into the "GetStatus" helper (and call it something like "SetStatus")?

if (await util.fileExists(optionPath)) {
return optionPath;
}
//If the path is not a valid path on disk, treat it as a version
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Newlines

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would make this a little more specific: if what they specified happens to be an absolute path that doesn't exist, print errors about a missing file instead of trying to download it. Otherwise, typos in the file path are going to lead to confusing errors about file downloads.

return await GetLaunchPathForVersion(platformInfo, version, installPath, extensionPath, useMono);
}
else {
throw new Error('Bad Input to Omnisharp Path');
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You know what the problem is, why don't you just print that the Omnisharp.Path isn't a "valid absolute path or semantic version" or something.

}
}

function IsValidSemver(version: string): boolean {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need this method?

file appears at the expected path */
let version = "1.2.3";
let downloader = GetOmnisharpDownloader();
let serverUrl = "https://roslynomnisharp.blob.core.windows.net";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we feel about unit tests that depend on the internet @TheRealPiotrP ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We tried using the mock server so we dont have to depend on the internet, but seems that the code creates an HTTPS request and we could not get the mock server running on the 443 port. Hence we have uploaded some dummy folders on the real server (like https://roslynomnisharp.blob.core.windows.net/releases/1.2.3/omnisharp-win-x64.zip) just for the testing purpose

//Since we need only the runtime dependencies of packageJSON for the downloader create a testPackageJSON
//with just that
export function GetTestPackageJSON() {
let testpackageJSON = {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I bet you could make this read in the package.json that's in the repo...

Copy link
Member

@DustinCampbell DustinCampbell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good.

I'm not really into having "experimental" everywhere -- e.g. ExperimentalOmniSharpDownloader. That leaves me feeling like the code is a work-in-progress. Since this is the OmniSharp download, maybe just call it that?

package.json Outdated
@@ -361,7 +367,7 @@
"null"
],
"default": null,
"description": "Specifies the full path to the OmniSharp server."
"description": "Specifies pre-release OmniSharp to use. Can be one of \"latest\", a specific version number, or the absolute path to a local OmniSharp folder."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, I suspect that "pre-release" is not the right text since this can be set to any OmniSharp -- even those that are released. 😄

import { Logger } from './logger';
import TelemetryReporter from 'vscode-extension-telemetry';

export async function GetDependenciesAndDownloadPackages(packages: Package[],status: Status, platformInfo: PlatformInformation, packageManager: PackageManager,logger: Logger) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: missing space between , and status

private packageJSON: any) {
}

public async DownloadAndInstallExperimentalVersion(version: string, serverUrl: string, installPath: string) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the plan to unify this with the code in https://github.com/OmniSharp/omnisharp-vscode/blob/master/src/CSharpExtDownloader.ts where this is copied and pasted from?

Copy link
Contributor Author

@akshita31 akshita31 Feb 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have extracted most of the common functionality of the two in the downloadHelper file. Hence once this change has been added and tested satisfactorily, we can modify the CSharpExtDownloader in the same way to use the helper methods( in a separate PR)

@@ -207,9 +207,9 @@ export interface LaunchResult {
usingMono: boolean;
}

export function launchOmniSharp(cwd: string, args: string[]): Promise<LaunchResult> {
export function launchOmniSharp(cwd: string, args: string[], experimentalLaunchPath: string): Promise<LaunchResult> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this just be called "launchPath"?

package.json Outdated
@@ -367,7 +367,7 @@
"null"
],
"default": null,
"description": "Specifies pre-release OmniSharp to use. Can be one of \"latest\", a specific version number, or the absolute path to a local OmniSharp folder."
"description": "Specifies the OmniSharp to use. Can be one of \"latest\", a specific version number, or the absolute path to a local OmniSharp folder."
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Specifies how to acquire OmniSharp?" "the OmniSharp" is a weird phrasing

package.json Outdated
@@ -367,7 +367,7 @@
"null"
],
"default": null,
"description": "Specifies the OmniSharp to use. Can be one of \"latest\", a specific version number, or the absolute path to a local OmniSharp folder."
"description": "Specifies how to acquire the OmniSharp to use. Can be one of \"latest\", a specific version number, or the absolute path to a local OmniSharp folder."
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still seems awkward...I think the problem is the phrase "the Omnisharp". Maybe "The copy of Omnisharp"? @DustinCampbell ?

Copy link
Member

@DustinCampbell DustinCampbell Feb 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be easier to be plain and then add a bit more explaining text at the end. Also, I believe that "the absolute path to a local OmniSharp folder" is actually incorrect. IIRC, the path needs to point to the file that will be launched.

Here's a stab at some new text. Additional word-smithing is welcome!

"Specifies the path to OmniSharp. This can be the absolute path to an OmniSharp executable, a specific version number, or "latest". If a version number or "latest" is specified, the appropriate version of OmniSharp will be downloaded on your behalf."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to specify that "latest" implies the latest CI build, because just the word "latest" might also imply the latest released version to the user ?

@akshita31 akshita31 merged commit 01bc4ae into dotnet:experiment_download Feb 14, 2018
@akshita31 akshita31 deleted the useVersion branch March 22, 2018 20:49
TheRealPiotrP pushed a commit that referenced this pull request Apr 27, 2018
* Initial manual validation plan

* Add info about unity

* Enable usage of omnisharp.path option for downloading multiple versions of omnisharp (#2028)

* Enable usage of multiple versions

* Either load the server from a path or download the version packages

* Tests for the package creator

* Added null check and removed semver check in package creator

* Test for the experiment omnisharp downloader

* Added test for package manager

* Code clean up

* Added null or empty check for version

* Changes

* Modified the description

Put the clean up logic in the teardown function

* Remove comment

* Remove unnecessary usage

* CR comments

* Removed experimental

* Modified launcher

* Removed experimental

* Modified tests

* Modified package description to include version information

* Renamed launch path

* Add more tests

* Changed the description in package.json

* "Run All Tests" and "Debug All Tests" (#1961)

* Run All Tests Running But Building repeatedly

* Disposable variable for run all test

* Added code for debug all tests

* Code Cleaning

* Run all Tests running - Better logs required

* Run Tests running, all output shown at the end of all the tests

* Renamed variable to methodsInClass

* Changes for Debug All Tests

* Changes for debug tests request

* Debug All Tests running

* Added common helpers for single test and all test functions

* Improved logs for Run All Tests

* Changes to get only 1 response for a set of methods

* Extracted a common helper to get the test feature

* Resolved review comments

* Changes to not show this change for legacy projects

* Renamed incorrect variable

* Removing foreach for proper order of execution

* Remove unnecessary import

* Do not show the festure for legacy projects

* Make hover test run by adding the file in the project (#2058)

* Added hover.cs file

* Not requiring server restart

* Using Structured Documentation for Signature Help and improving Parameter Documentation (#1958)

* Structured Documentation in Signature Help

* Code clean up

*  Using only summary of the documentation

* Code clean up

* Documentation for parameters showing in signature help

* Removing unnecesaary import

* Using interploated string and fixed spacing

* Parameter Documentation using interpolated text

* Added tests

* Enable download and usage of latest version of omnisharp (#2039)

* Enable usage of multiple versions

* Either load the server from a path or download the version packages

* Tests for the package creator

* Added null check and removed semver check in package creator

* Test for the experiment omnisharp downloader

* Added test for package manager

* Code clean up

* Added null or empty check for version

* Changes

* Modified the description

* Put the clean up logic in the teardown function

* Remove comment

* Remove unnecessary usage

* CR comments

* Removed experimental

* Modified launcher

* Removed experimental

* Modified tests

* Modified package description to include version information

* Renamed launch path

* Add more tests

* Changed the description in package.json

* Getting latest version info

* Refactored code and added tests

* Remove unnecessary using

* CR comments

* Use common function for latest download

* Add new line

* Resolve binaries on linux

* Copy pacakges by value

* Renamed experimentalId and some methods

* Moved status item out of method and added version to message

* Fix typo (#2066)

* Fix year in changelog (#2068)

Fixed wrong year in changelog

* Formatting for better display structure

* Move to const fields in server

* Make methods private

* Move chai-as-promised to devDependencies

* Removed unnecessary function

* Insert new debugger with support for Symbol Server and Source Link (#2070)

* launch.json schema changes and documentation

This has the new launch.json schema to support XPlat symbols

* Update debugger packages

* Update CHANGELOG.md with debugger change

* Modified test description

* Update .editorconfig JSON rules (#2063)

Last week I ran into yet another time when something added a BOM to one of our json files and broke things. This adds some .editorconfig rules for .json to try and prevent such things.

While I was at it, I also wanted to switch the indent size for src/tools/*.json to be '2' so that it is consistent with other json schema files the debugger owns.

* Updated to reflect limited Desktop CLR support (#2046)

* Updated to reflect limited Desktop CLR support

* Fixed grammatical error

* Refactor CSharpExtDownloader to use the extracted helper functions (#2072)

* Refactor downloader
* Renamed method and corrected the position of appendLine
* Modified GetStatus and the Status object to append the dispose method

* Added settings file

* Emphasize test codelens

* Add another bullet point

* Add more details about signature help and tests codelenses

* Fix typo

* Update Changelog and test-plan for multiple download of omnisharp (#2082)

* Update changelog and test-plan for multiple download of omnisharp

* Add mocha+wallaby tests (#2091)

* Add mocha+wallaby tests

eventually the feature tests should be removed and most of our tests should become unit tests that are runnable from the command line or via wallaby.

npm run tdd will enable using mocha's command line tdd capability

* Fix `tdd` command

* Fix test paths

* move to tslint-no-unused-expression-chai

The package was already installed, but not used. This makes our tests that use chai.should() pass tslint checks

* Add Architecture check in AdapterExectuableCommand (#2094)

* Add Architecture check in AdapterExectuableCommand

x86 messages only show during the first installation. But it still
registeres the vsdbg-ui.exe command. This adds a check so it will
error instead of returning the command.

* Moving arch check into completeDebuggerInstall

* Beta1 -> Beta2

* Remove angle brackets in ChangeLog (#2099)

* Add product-wide code coverage + codecov.io integration (#2101)

Add product-wide code coverage + codecov.io integration

Several new scripts were added:

npm run cov:instrument: rebuilds your sources, then instruments them for coverage. Subsequent 

npm run test will generate coverage data into the .nyc_output directory

npm run cov:merge-html: merges all reports from .nyc_output and puts a locally viewable coverage report into coverage directory

* Refactor logger, reporter and channel into a unified EventStream using Rx Observable (#2084)

* Changes to refactor logging in server

* Adding packages

* Changes

* Remove reporter from CSharpExtDownloader

* remove telemtery reporter from server.ts

* remove reporter from definitionProvider

* Remove reporter from dotnetTest.ts

* Debugger Activation + Commands

* reduce message types

* remove reporter from commands.ts

* remove channel from  status.ts

* Remove reporter & logger from extension.ts

* Build issues

* Add missing rx dependency

* Changed to download progress

* Removed using and pass platformInfo

* Moved files in observer folder

* Renamed the files and added omnisharp channel observer

* Remove unnecessary format

* Changes in main.ts

* Remove channel from global declaration

* Preserving the context in onNext invocations

* Pulled platformInfo out of server

* Remove unnecessary variable

* Formatting

* Renamed observers

* Add mocha+wallaby tests

eventually the feature tests should be removed and most of our tests should become unit tests that are runnable from the command line or via wallaby.

npm run tdd will enable using mocha's command line tdd capability

* Code clean up

* Fix `tdd` command

* Fix test paths

* Add initial DotnetChannelObserver test

* Testing the download messages

* Remove logger from requestQueue.ts

* Fix builds

* Use package manager factory

* Remove Lines

* Remove extra appendLine

* Added test for csharp logger and channel

* Extracted base class for observers

* Test having dependency on vscode

* vscode adapter changes

* Changes for adapter

* Refactored Omnisharp Manager

* Moved from interfaces to classes

* Renamed onNext to post

* Created class EventStream

* Removed comment

* Added missing break

* Added test for Omnisharp Logger

* Test for OmnisharpLoggerObserver

* Test for telemetry reporter observer

* Added test for all the observers

* minor nits

* Changes

* Remove unnecessary imports

* remove import

* Modified failing test

* Make tests pass

* Renamed platformInfo

* CR feedback

* 2 -> 3

* Fix codecov integration (#2108)

Fix js->ts coverage map transformation in Travis CI where `node` is not on the path.

* enable codecov on unit tests (#2118)

* enable codecov on unit tests

* rename coverage files for uniform globbing

* Validate Indentation for OmnisharpProcess messages (#2119)

* Clean up signature help test (#2117)

* Added parameter

* modifications

* Cleaned up tests

* Changed test codelens casing (#2077)

* CodeCov flags (#2125)

Lights up integration test coverage and splits reporting data between unit & integration tests

* Update project npm dependencies (#2126)

* low-risk package updates

* high-risk package updates

* disable uninstrumented test pass (#2127)

* Fix size (#2142)

* 1.15.0-beta4 (#2143)

* Verify vsix size before release (#2144)

* Verify vsix size before release

* Remove istanbul from shipping payload

* Use the size appropriately in the release test (#2145)

* Fix offline packaging using event stream (#2138)

* Creating a new eventStream

* invokeNode

* Artifact tests

* Directing the vsix files to a temp folder

* Changes to travis to run release tests on release only

* if statement

* Convert gulpfile to ts

* Add bracket

* Clean up the observer tests for better readability (#2157)

* CsharpLoggerObserver

* dotnetchannelobserver

* TelemetryObserver

* DebugModeObserver

* Correct message in the debug observer

* Refactoring status using rxjs (#2133)

* hack

* Refactored status into 4 separate observers

* Resolved warning and information messages

* Deleted status.ts

* Changes to retain this context

* Created fascade for statusbar and texteditor

* Subscribe to event stream

* Working!

* Nits

* Mocking warning message

* Tried mocking setTimeOut

* warning message changes

* warning message correct definition

* virtual time running

* done called multiple time

* renamed observer and subject

* changes

* some changes

* refactor^2

* merge conflicts

* using rx debounce

* Warning Message Observer tests

* Remove loadsh.debounce and renamed observer

* Move the workspace info invocation to server

* Clean up

* More test to statusBarObserver

* Clean up tests

* Fixed ttd

* Use vscode commands instead of calling into commands.ts

* Added test for information message observer

* Tests for status bar obsever

* project status observer

* Changes to show two observers

* some more changes

* Lot of questions!

* build issues

* Remove usings

* comments

* Remove unnecessary cases

* Changes

* Remove usings

* Dipsose the disposables after the server stop event

* Remove the cake thing

* Project Status Bar

* Clean up the tests

* Changed to dcoument

* Remove unnecessary functions from the adapter

* remove unnecessary change

* Remove some changes

* changes

* Test for server error

* Removed comment and modified the initialisation

* Empty disposable

* Corrected the usage of the disposables

* Added comments for debouncer

* disposable try

* Test the vsix, not the build layout (#2156)

The VSCode C# Extension build process follows the VS Code docs and runs tests directly inside of its repo root. This unfortunately gives a false sense of security because bugs can be introduced during VSIX packaging [particularly due to missing content in node_modules or excluded via .vscodeignore].

This change addresses this problem by moving our CI tests to execute the VSIX instead of the build's intermediate artifacts. Specifically:

build the vsix
unpackage the vsix
instrument the unpackaged vsix
run tests with VS Code Host pointing to the unpackaged vsix
This makes our CI tests ~= to the user's runtime experience and will greatly help us with size reduction efforts.

To support this change, I also moved our build system from package.json to Gulp. This makes the build scripts significantly easier to understand, provides intellisense for build scripts, and build-time type checking for their contents.

I also strengthened the repo's use of .vscodeignore by creating a copy of the file for each scenario [online packages and offline packages]. The new gulp packaging scripts take advantage of these files to produce packages with predictable contents regardless of when packaging occurs. [small caveat, @akshita31 will be adding a test that validates that net-new content does not start sneaking into the vsix package].

* Enable `noImplicitAny` and `alwaysStrict` in `tsconfig.json (#2159)

Along the way, fixed a few bugs that were in place due to implicit anys. Also removed dependency on deprecated vscode API.

* tsconfig.json: noUnusedLocals, noFallThroughCaseInSwitch, tslint.json: promise-function-async (#2162)

Adds noUnusedLocals to tsconfig.json to keep our sources clean
Adds noFallThroughCaseInSwitch in tsconfig.json to prevent unintended switch behavior
Adds promise-function-async to tslint.json to force all async functions to be marked as async. This is a building block towards eliminating promises in favor of async/await.

* Eliminate Thennable<any> (#2163)

Provides strong typing for the repo's Thennables. This is a step towards eliminating promises and will provide compiler support for validating that the refactored code has not changed its final return type.

* promise to await packaging tasks (#2164)

* Remove the status handling from the package manager (#2160)

* Changes to remove status from packages.ts

* Tests failing

* Added tests

* Used tooltip and changed flame color

* fix merge problems

* PR feedback

* Removed implicit any

* Changes

* Update the links to the Source Link spec (#2170)

This updates the links we had to the Source Link spec to point at the more official version.

* Make the server and the downloader rely on the vscode adapter (#2167)

* Moving fakes to testAssets

* Change the import for fakes.ts

* Test Assets in feature tests

* Remove vscode dependency from server.ts

* Make the downloading stuff use vscode adapter

* Remove vscode as a property

* make test package json a field

* Run release test only on release (#2172)

* Show the channel on download start (#2178)

* Add "Launch Unit Test" option (#2180)

* unify rx usage to rxjs (#2168)

The current codebase uses both rx [v4] and rxjs [v5] implementations. This PR consolidates our use of rx onto the v5 libraries.

* Fix disposable (#2189)

* Divide the package manager into separate components (#2188)

* Feature tests running with refactored package manager

* Refactoring packages-1

* Test for the downloader running using the mock server

* Using network settings

* Changing the package path

* Dividing packages.ts into separate components

* use filter async

* Use tmpfile interface

* Check for the event stream

* Using FilePathResolver

* Remove using

* Testing for normal and fallback case working

* Resolve the paths

* Remove failing case

* package installer test-1

* Add package installer test

* Create tmp asset

* Package Downloader test refactored

* Rename files

* resolve compile issues

* Clean up installer

* Clean up the tests

* Rename packages

* Package installer test

* PR feedback

* Package Filter test

* Remove yauzl.d.ts

* Filter test

* Added test for invalid zip file

* method for getting the request options

* remove imports

* please resolve the path

* remove  latest in settings

* Package Manager test executing

* Use free port in package manager test

* Package Manager (using a https server running)

* using http mock server

* Downloader test running using free port

* Update ChangeLog for codelens and status bar (#2205)

* PR feedback

* Introduce OmniSharpLaunchInfo type

* Fix casing of 'OmniSharp' in user-facing error message

* Return both 'LaunchPath' and 'MonoLaunchPath' from OmniSharpManager

* move back to latest for O# release

* Refactor launcher and ensure that we launch on global Mono if possible

* Improve coverage a bit

* Use async/await in OmniSharpServer._start()

* Fix test

* Update debugger links for 1.15.0-beta5

This updates the debugger packages to the latest corelr-debug which is
now built on .NET Core 2.1 Preview 2.

* Update debugger changelog and debugger-launchjson.md for 1.15 (#2230)

- Add debugger items to the changelog
- Update debugger-launchjson.md to reference the new launchSettings.json feature.

* Mark the C# extension as non-preview (#2220)

* Mark the C# extension as non-preview

This changes the branding on the C# extension so that it is no longer labeled a 'preview'.

Two changes:
1. Change `preview` to `false` in package.json
2. Update the license that is used in official builds of the C# extension. This new EULA is no longer a pre-release EULA and it also has the latest text.

* Update README.md as well
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants