Add support for configuring default startup *.sln in settings.json #2053

janaka · 2018-02-18T20:25:46Z

This PR is a proposed solution for allowing the default startup *.sln file to be configured in settings.json (issue #2052).

Thanks in advance for considering this PR.

Current Behaviour

At present, if a workspace has multiple *.sln files, the first one in alphabetical order is selected at startup. If this solution file isn't for the primary project then it get's very confusing when intelli-sense doesn't work. There's no reminder or otherwise intuitive path paved for the user prompting them to switch solutions by clicking on the right of the status bar (or OmniSharp: Select Project from the command palette). Further switch option only seems to be available when on a file belonging to .NET code which may be inside a /src or similar.

Proposed Solution

The proposed solution adds the config option omnisharp.defaultLaunchSolution allowing users to optionally set the desired default solution file. It can even be checked in as a workspace setting in .vscode/settings.json allows it to be fixed for everybody.

Note that today when a solution is selected using theOmniSharp: Select Project command is persisted in local storage within the user profile on their machine. If omnisharp.defaultLaunchSolution has a value it will be used instead on startup.

Testing

Manual testing (MacOS)
Manual testing (Windows)
Unit tests added
Integration tests added

… there are multiple

- To store default LaunchTarget (true/false) as property. - Apply the configured default when Omnisharp starts.

- tweak bin and obj exlusion - refactor lauch config

dnfclas · 2018-02-18T20:25:57Z

All CLA requirements met.

TheRealPiotrP · 2018-02-27T02:51:12Z

Thanks @janaka! @rchande will you have cycles to review?

@DustinCampbell can you look at test/integrationTests/testAssets/slnWithCsproj/.vscode/settings.json? what are your thoughts on the settings file addition?

DustinCampbell · 2018-02-27T13:54:01Z

package.json

+        },
+        "linux": {
+          "program": "./.debugger/vsdbg-ui"
+        }


I'm pretty sure that you don't want these changes in your PR. Is that correct @gregg-miskelly?

Not sure how/why those got in there tbh.

It can happen when debugging the extension.

DustinCampbell · 2018-02-27T13:54:19Z

package.json

+          "program": "./.debugger/vsdbg-ui"
+        },
+        "linux": {
+          "program": "./.debugger/vsdbg-ui"


I'm pretty sure that you don't want these changes in your PR. Is that correct @gregg-miskelly?

@janaka: You need to revert this bit.

DustinCampbell · 2018-02-27T13:55:19Z

package.json

+        "omnisharp.defaultLaunchSolution": {
+          "type": "string",
+          "default": null,
+          "description": "The name of the default solution used at start up if the repo has multiple. e.g.'MyAwesomeSolution.sln'. Default value is `null` which will cause the first in alphabetical order to be chosen"


Nit: This should have a period at the end.

DustinCampbell · 2018-02-27T13:58:32Z

src/omnisharp/server.ts

+            const defaultLaunchSolution = launchTargets.filter((a) => (a.isDefault === true));
+            // If there's more than one target, launch the server with the launch target that
+            // matches the default solution (if it's configured).
+            // The user can still manually switch to another solution from  the 'Omnisharp: Select Project command'


Will it still remember the last selected project on the next start up (i.e. the preferredPath). Or, does setting this option effectively override it? I would expect defaultLaunchSolution to be used in the case where there isn't a preferredPath.

Ifomnisharp.defaultLaunchSolution is set (to a valid existing solution), it will be used at start up. That's the intended behaviour. If it's not set then the last selected should be remembered and used at start up per the original logic.

rchande · 2018-02-27T20:58:40Z

.vscode/launch.json

            ],
            "stopOnEntry": false,
            "sourceMaps": true,
            "outFiles": [
-                "${workspaceRoot}/out/test/**/*.js"
+                "${workspaceFolder}/out/test/**/*.js"


{workspaceFolder} [](start = 18, length = 17)

Can you explain this? It looks like VSCode deprecated workspaceroot for workspacefolder?

That's right. workspaceRoot was deprecated when multi-root workspaces were added to VS Code.

rchande · 2018-02-27T20:59:21Z

.vscode/tasks.json

-			"args": ["Run tests in VS Code by launching the debugger with the 'Launch Tests' configuration."],
+			"args": [
+				"Run tests in VS Code by launching the debugger with the 'Launch Tests' configuration."
+			],


While we're making whitespace-only changes, can we tab->space ?

rchande · 2018-02-27T20:59:56Z

package.json

+        "omnisharp.defaultLaunchSolution": {
+          "type": "string",
+          "default": null,
+          "description": "The name of the default solution used at start up if the repo has multiple. e.g.'MyAwesomeSolution.sln'. Default value is `null` which will cause the first in alphabetical order to be chosen"


"multiple solutions"

rchande · 2018-02-27T21:02:43Z

src/omnisharp/launcher.ts

-    return targets.sort((a, b) => a.directory.localeCompare(b.directory));
+    let rtnTargets: LaunchTarget[];
+
+    const options = Options.Read();


onst options = Options.Read( [](start = 5, length = 28)

Why not read the options at the beginning of this method and check each solution file's name as we add them to targets?

This is definitely checking targets that aren't solutions...

In reply to: 171064995 [](ancestors = 171064995)

Ah, originally I generalised the config key for any target then realised it didn't make sense in most/all other context so switched it to be solution specific.

Your suggestion makes sense. Should be able to remove the isDefault property as per your suggestion below. Good call.

rchande · 2018-02-27T21:06:41Z

src/omnisharp/server.ts

-            // matches the preferred path. Otherwise, we fire the "MultipleLaunchTargets" event,
-            // which is handled in status.ts to display the launch target selector.
-            if (launchTargets.length > 1 && preferredPath) {
+            const defaultLaunchSolution = launchTargets.filter((a) => (a.isDefault === true));


const defaultLaunchSolution = launchTargets.filter((a) => (a.isDefault === true)); [](start = 11, length = 83)

I wonder if we can avoid spreading the default launch target selection through 2 different components.

If we're going to scan this list for the "Default" one anyway, why not just read the option here too?

rchande · 2018-02-27T21:07:29Z

test/integrationTests/testAssets/slnWithCsproj/aslnWithCsproj.sln

+		{717BE881-D74C-45FC-B55D-2085499E1BF8}.Release|x86.ActiveCfg = Release|x86
+		{717BE881-D74C-45FC-B55D-2085499E1BF8}.Release|x86.Build.0 = Release|x86
+		{4679428B-0CA0-4228-B8C0-B676B34A1B30}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
+		{4679428B-0CA0-4228-B8C0-B676B34A1B30}.Debug|Any CPU.Build.0 = Debug|Any CPU


Did you mean to add this file?

Oh, I see. Can we give this a more informative name?

In reply to: 171066267 [](ancestors = 171066267)

rchande · 2018-02-27T21:09:06Z

test/integrationTests/testAssets/slnWithCsproj/.vscode/launch.json

+            "name": ".NET Core Attach",
+            "type": "coreclr",
+            "request": "attach",
+            "processId": "${command:pickProcess}"


I don't think we want to check these in--we have a test that tests generation of these files.

rchande

Thanks for the PR! I added some feedback for you.

janaka · 2018-02-28T15:25:32Z

@rchande much appreciated.

gregg-miskelly · 2018-02-28T16:53:03Z

package.json

@@ -1314,13 +1319,13 @@
              "type": "coreclr",
              "request": "launch",
              "preLaunchTask": "build",
-              "program": "^\"\\${workspaceRoot}/bin/Debug/${1:<target-framework>}/${2:<project-name.dll>}\"",


All of this part of launch.json is generated from src/tools/OptionsSchema.json. Can you fix it there too? Otherwise this will be lost the next time we run the generator.

Looks like this is fixed now in the latest master.

janaka · 2018-03-26T09:23:59Z

I've been away for the last 4 weeks. Will start working on these this week.

rchande · 2018-06-14T16:09:42Z

@janaka I sent you a small PR for this branch 😄 janaka#1, mostly to show you our new options API.

janaka · 2018-06-14T20:30:28Z

Cool, taking a look. Thanks

Cleanup/simplify

janaka · 2018-06-14T22:13:14Z

Nice. Merged in ^.

I've tried running integration and feature tests on master and they fail same/similarly.

It looks like tests that involve executing VS Code complex commands are the ones having issues so like
<vscode.SignatureHelp>await vscode.commands.executeCommand("vscode.executeSignatureHelpProvider", fileUri, position);
or
let c = await vscode.commands.executeCommand("vscode.executeCodeActionProvider", fileUri, new vscode.Range(0, 7, 0, 7)) as {command: string, title: string, arguments: string[]}[];

with errors like this (just a snippet)


12) SignatureHelp: single csproj at root of workspace
       Signature Help identifies active parameter based on comma for multiple commas:
     TypeError: Cannot read property 'signatures' of undefined
      at Context.<anonymous> (test/integrationTests/signatureHelp.integration.test.ts:81:17)
      at Generator.next (<anonymous>)
      at fulfilled (out/test/integrationTests/signatureHelp.integration.test.js:8:58)
      at <anonymous>






Tests exited with code: 1
[23:08:36] 'test:integration:singleCsproj' errored after 1 min
[23:08:36] Error: process exited with code 1
    at ChildProcess.<anonymous> (/Users/janaka.abeywardhana/code-projects/vscode-extensions/omnisharp-vscode/node_modules/async-child-process/lib/join.js:25:22)
    at emitTwo (events.js:126:13)
    at ChildProcess.emit (events.js:214:7)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:198:12)
[23:08:36] 'test:integration' errored after 1 min
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! csharp@1.15.2 test:integration: `gulp test:integration`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the csharp@1.15.2 test:integration script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/janaka.abeywardhana/.npm/_logs/2018-06-14T22_08_36_754Z-debug.log```

rchande · 2018-06-15T16:25:41Z

@janaka That error means that when the test tried to get signature help, it didn't get a result back. Usually this indicates a problem with OmniSharp loading your project. If you run the integration tests and show the output window in the VS Code test host, what's the C#/o# log?

Interestingly, our travis run is showing your PR as green, so this might be a configuration issue on your machine (I also have green tests with your changes).

rchande · 2018-06-15T16:27:04Z

test/integrationTests/testAssets/slnWithCsproj/src/app/app.csproj

@@ -1,9 +1,4 @@
 <Project Sdk="Microsoft.NET.Sdk">
-
-  <ItemGroup>
-    <ProjectReference Include="..\lib\lib.csproj" />


Why remove this?

Honestly don't remember exactly why or if I even directly removed this, likely I did. I'll revert.

rchande · 2018-06-15T16:27:12Z

test/integrationTests/testAssets/slnWithCsproj/a_FirstInOrder_SlnFile.sln

@@ -7,8 +7,6 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "src", "src", "{D28FC441-C95
 EndProject
 Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "app", "src\app\app.csproj", "{D36E1CC9-62CA-45A3-8BD0-6ADC2767FFB3}"
 EndProject
-Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "lib", "src\lib\lib.csproj", "{717BE881-D74C-45FC-B55D-2085499E1BF8}"


janaka · 2018-06-22T12:09:27Z

@rchande once I've addressed those ^ and merged latest master in, what else do you need to to get this PR merged into master?

Maybe best to improve test automation on a different PR?

rchande · 2018-06-22T15:57:23Z

@janaka Yes, fix up those last couple of things and I'll get this merged for you. Thanks for your contribution!

janaka · 2018-06-22T17:50:27Z

Nice! I'm away from this evening for a week. Will get this done next weekend.

janaka · 2018-07-04T21:36:33Z

@rchande I've merged in upstream master and fixed the lib project ref in both sln files and the app.csproj. I've looked through the remaining changes since the merge and don't think I've broken anything new.

sorry it's a few days later than promised.

rchande · 2018-07-05T16:59:47Z

@janaka No problem. Thanks for your contribution, and sorry it took us so long to get this merged. This LGTM-- @akshita31 want to take a look too?

akshita31 · 2018-07-05T18:36:34Z

test/integrationTests/testAssets/singleCsproj/.gitignore

@@ -0,0 +1,2 @@
+**/obj/


Do we need this?

This ignores the obj directory for integrationtests, which I think is fine.

Ok. Just merged master. Will merge once travis approves :)

akshita31 · 2018-07-05T22:04:39Z

@janaka Thanks a lot for your contribution. If you are interested, you could do an update in changelog log otherwise, I will be happy to do it.

janaka · 2018-07-05T22:07:48Z

Under 1.16 > project system?

akshita31 · 2018-07-05T22:10:44Z

@janaka Yup, that would be good.

Fix: broken link formatting

janaka · 2018-07-05T22:29:49Z

Done.

akshita31 · 2018-07-05T23:31:40Z

Thanks!

janaka added 14 commits December 30, 2017 20:02

ADD: ability to specifiy in settings.json the default *.sln file when…

6d2f205

… there are multiple

Fix logic

fad50e8

- To store default LaunchTarget (true/false) as property. - Apply the configured default when Omnisharp starts.

Change pre-release version number

6aafccf

Refactor: change config key to be more generalised.

db973e8

Fix: integertion tests: some issues with the multi sln test project

fab697a

Fix: up launch config and gitignore

e0edf51

- tweak bin and obj exlusion - refactor lauch config

Add: gitignores to test projects

baa827b

tweak gitignore

2a5056a

Merge: latest origin master

5c7d70a

Fix: gitignore

290ff42

package-lock.json changes

6de3809

remove none existant project from test sln

8a69508

Refactor: replace deprecated ${workspaceRoot} with ${workspaceFolder}

de1a8d2

Refactor: defaultLaunchTarget to defaultLaunchSolution

86b1b72

This was referenced Feb 20, 2018

Feature: Support configuring the default *.sln which is loaded when opening #2052

Closed

Multi Root Workspace Support #1889

Open

DustinCampbell reviewed Feb 27, 2018

View reviewed changes

rchande reviewed Feb 27, 2018

View reviewed changes

rchande suggested changes Feb 27, 2018

View reviewed changes

gregg-miskelly reviewed Feb 28, 2018

View reviewed changes

remove accidental addition

dfe1974

Merge pull request #1 from rchande/forJanaka

f4bce55

Cleanup/simplify

rchande reviewed Jun 15, 2018

View reviewed changes

Merge branch 'master' into config-default-sln

d5cd8fc

Janaka Abeywardhana added 3 commits July 2, 2018 20:56

Merge: latest upstream master

f801b8e

Fix: missing lib ref in test projet sln files

c80dd09

Fix: test project missing lib ref in csproj file

1e0e8fa

Merge branch 'master' into config-default-sln

72ccaad

rchande approved these changes Jul 5, 2018

View reviewed changes

akshita31 reviewed Jul 5, 2018

View reviewed changes

Merge branch 'master' into config-default-sln

dcfa0c6

Update: CHANGELOG with PR details

e34bf5a

Fix: broken link formatting

akshita31 merged commit b989062 into dotnet:master Jul 5, 2018

janaka deleted the config-default-sln branch July 7, 2018 13:43

per-samuelsson mentioned this pull request Dec 10, 2018

Feature request: Omnisharp server project/solution free scan mode. OmniSharp/omnisharp-roslyn#1206

Open

Add support for configuring default startup *.sln in settings.json #2053

Add support for configuring default startup *.sln in settings.json #2053

Conversation

janaka commented Feb 18, 2018 • edited Loading

Current Behaviour

Proposed Solution

Testing

dnfclas commented Feb 18, 2018 • edited Loading

TheRealPiotrP commented Feb 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janaka Apr 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rchande left a comment

Choose a reason for hiding this comment

janaka commented Feb 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janaka commented Mar 26, 2018

rchande commented Jun 14, 2018

janaka commented Jun 14, 2018

janaka commented Jun 14, 2018 • edited Loading

rchande commented Jun 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janaka commented Jun 22, 2018

rchande commented Jun 22, 2018

janaka commented Jun 22, 2018

janaka commented Jul 4, 2018

rchande commented Jul 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akshita31 commented Jul 5, 2018

janaka commented Jul 5, 2018

akshita31 commented Jul 5, 2018

janaka commented Jul 5, 2018

akshita31 commented Jul 5, 2018

janaka commented Feb 18, 2018 •

edited

Loading

dnfclas commented Feb 18, 2018 •

edited

Loading

janaka Apr 7, 2018 •

edited

Loading

janaka commented Jun 14, 2018 •

edited

Loading