Add ConfigurationProvider API, ${workspaceRoot} -> ${workspaceFolder} #1801

WardenGnaw · 2017-10-23T21:38:30Z

Adding ConfigurationProvider API

Changing ${workspaceRoot} to ${workspaceFolder}

tasks.json to version 2.0.0 and updating associated tests.

DustinCampbell

Could you check my comment about WorkspaceInformationResponse.MsBuild being null? This can happen for project.json.

DustinCampbell · 2017-10-24T16:05:00Z

src/configurationProvider.ts

+     * except the first in a workspace. 
+     */
+    private checkWorkspaceInformationMatchesWorkspaceFolder(folder: vscode.WorkspaceFolder, info: WorkspaceInformationResponse): boolean {
+        return info.MsBuild.SolutionPath === folder.uri.fsPath;


Note: It is entirely possible for MsBuild to be null.

Other than this, the change looks good to me. Note that this can be null when the project is a project.json file.

DustinCampbell · 2017-10-24T16:08:07Z

src/configurationProvider.ts

+    /**
+     * TODO: Remove function. 
+     * 
+     * Note: serverUtils.requestWorkspaceInformation only retrieves one project. Therefore, generator will be incorrect for all folders


serverUtils.requestWorkspaceInformation only retrieves one project.

This isn't true.

Is there a way to make it retrieve all projects in the current workspace for VsCode?

It will retrieve all projects loaded by OmniSharp. Currently, OmniSharp can only target a single folder. However, that folder contains multiple .csproj projects or an .sln with multiple projects, it will return them. We're going to be doing some work in omnisharp-roslyn to make OmniSharp support multiple folders: OmniSharp/omnisharp-roslyn#909

DustinCampbell · 2017-10-25T00:07:09Z

If this requires 1.18, we have to wait until the next VS Code is out before shipping, correct?

gregg-miskelly · 2017-10-25T02:50:53Z

Correct.

DustinCampbell · 2017-10-25T13:23:00Z

Do we want to get this in for 1.13 then? IIRC, the PR you just merged works around the issue that the debug.start command is being removed in 1.18. So, if we hold 1.13 that for this change, there'll be a window of time where things won't really work.

DustinCampbell · 2017-10-25T14:17:48Z

Also, what API are we talking about? Debug configuration providers have been present for a little while.

WardenGnaw · 2017-10-25T14:24:28Z

During my testing, the current stable version of VsCode (1.17.2) was not providing initial configurations with the DebugConfigurationProvider.

DustinCampbell · 2017-10-25T15:21:55Z

During my testing, the current stable version of VsCode (1.17.2) was not providing initial configurations with the DebugConfigurationProvider.

Got it -- thanks! In that case, how do we want to coordinate this change? Should we hold it until after 1.13?

gregg-miskelly · 2017-10-25T16:23:40Z

@WardenGnaw: is it possible to keep things working in 17.2 - it wouldn't have new functionality, but the old functionality would still be there?

If not, we will definitely need to wait till post 1.13.

WardenGnaw · 2017-10-25T17:21:46Z

@gregg-miskelly Yep. We would only need to put back initialConfigurations in launch.json.

gregg-miskelly · 2017-10-25T17:29:29Z

@WardenGnaw would that still retain the benefits in 1.18? (Will they ignore initialConfigurations if we have a provider)

WardenGnaw · 2017-10-25T17:33:29Z

@gregg-miskelly No. We are using it as a JSON object literal, so it will not be ignored.

WardenGnaw · 2017-10-25T20:53:09Z

@DustinCampbell @gregg-miskelly I lied. This feature should work for 1.17.2. I'll be reverting the package.json to 1.17.0.

I was testing the C/C++ extension at the same time as this one. They do some overwriting of the activation events which made me lose the "onDebug" event which is required for DebugConfigurationProvider.

DustinCampbell · 2017-10-25T21:05:10Z

You shouldn't lie. 😄

gregg-miskelly · 2017-10-26T16:01:30Z

            const generator = new AssetGenerator(info);

This will currently just check the workspace root for a launch.json/tasks.json. Do we need to check all the root folders instead?

Refers to: src/assets.ts:500 in b3b56af. [](commit_id = b3b56af, deletion_comment = False)

gregg-miskelly · 2017-10-26T16:07:29Z

src/configurationProvider.ts

+    /**
+	 * Returns a list of initial debug configurations based on contextual information, e.g. package.json or folder.
+	 */
+    provideDebugConfigurations(folder: vscode.WorkspaceFolder | undefined, token?: vscode.CancellationToken): vscode.ProviderResult<vscode.DebugConfiguration[]> {


undefined [](start = 64, length = 9)

Can this really be undefined? If so, we don't seem to handle it.

@WardenGnaw did you see question?

Whoops I did see the question, VsCode had the function defined as that in their interface and I copied it over. So I added a check for folder but it got removed during other changes.

gregg-miskelly · 2017-10-26T16:40:23Z

src/configurationProvider.ts

+    }
+
+    /**
+     * TODO: Remove function. 


TODO [](start = 7, length = 4)

Can we remove this now?

Can I retrieve other workspace folder information with the change in #1806? Or this is still waiting on OmniSharp/omnisharp-roslyn#909.

@DustinCampbell should this work now?

You're waiting on OmniSharp/omnisharp-roslyn#909. OmniSharp is not being launched on multiple workspace folders. Instead, the user can now launch OmniSharp on targets within multiple workspace folders.

In that case I would suggest:

Add a link the bug

Instead of checking if the solution path is the same, seems like we should be checking if the folder is what we launched OmniSharp against.

Yes, I hadn't spotted that we were checking the SolutionPath against the folder name. That's going to be wrong. Note that you can have an .sln file and OmniSharp can be launched directly on that file. In that case SolutionPath will point to the .sln.

gregg-miskelly

Otherwise LGTM, but I think I would like to take a second look

New tasks schema from 0.1.0 to 2.0.0

package.json's initialConfiguraitons have been deprecated. It uses DebugConfigurationProvider instead. C# extension uses generate assets initially to create launch.json and tasks.json. However, if a user desides to dismiss the message and generate via f5, the prompt will initiate the DebugConfigurationProvider.

${workspaceRoot} is now ${workspaceFolder} ConfigurationProvider activates on f5. This will also trigger generating a tasks file. tasks args are now 'build <path>', updating tests for these changes.

gregg-miskelly

Otherwise LGTM

gregg-miskelly · 2017-10-27T20:59:09Z

src/configurationProvider.ts

+        {
+            serverFolder = path.dirname(solutionPathOrFolder);
+        }
+        return folder && folder.uri && (folder.uri.fsPath === serverFolder);


I think you want to check that solutionPathOrFolder is under folder.uir.fsPath rather than they exactly match. Otherwise if you open "c:\MyEnlistment" and your solution is in "c:\MyEnlistment\src" I believe your check will fail.

gregg-miskelly · 2017-10-27T20:59:51Z

src/configurationProvider.ts

+    /**
+	 * Returns a list of initial debug configurations based on contextual information, e.g. package.json or folder.
+	 */
+    provideDebugConfigurations(folder: vscode.WorkspaceFolder | undefined, token?: vscode.CancellationToken): vscode.ProviderResult<vscode.DebugConfiguration[]> {


@WardenGnaw did you see question?

gregg-miskelly · 2017-10-27T21:13:54Z

src/assets.ts

@@ -327,7 +325,7 @@ function getBuildTasks(tasksConfiguration: tasks.TaskConfiguration): tasks.TaskD

    function findBuildTask(tasksDescriptions: tasks.TaskDescription[]) {


What is VSCode's behavior if the user has an old style tasks.json? Should we be ignoring their tasks.json, replacing it, or is it really okay to add a new style task to an old style file?

@gregg-miskelly The current behavior if a user has 0.1.0 in the version field of tasks.json is only an issue if they decide to use workspace folders.

Therefore, if we see an old tasks.json that is nonempty, we should be ignoring it. We could add a warning message saying to upgrade it if they wish to use workspace folders?

Checking if folder.uri.fsPath is a subfolder of GetSolutionPathOrFolder to generate assets.

TheRealPiotrP · 2017-10-30T19:05:53Z

typings/vscode-tasks.d.ts

@@ -32,25 +32,21 @@ declare module "vscode-tasks" {
    export interface BaseTaskConfiguration {


Why do we have a copy of these typings? In the 1.1.6 version of the vscode package I see that these types are already defined in vscode.d.ts.

Not sure why we originally had a copy of this. @DustinCampbell may be able to answer that question. However, the current typings do not have the old Tasks schema, specifically isBuildCommand.

Back in the day, it wasn't included in vscode.d.ts.

TheRealPiotrP · 2017-10-30T19:08:22Z

src/configurationProvider.ts

+                fs.ensureDirSync(dotVscodeFolder);
+
+                // if the file does not exist, addTasksJson
+                const addTasksJson: boolean = !fs.existsSync(tasksJsonPath);


I'm preparing a PR to take advantage of the taskProvider API. We'll want to keep these two implementations in-sync. I'm also adding quite a bit of test infrastructure that we can use to ensure that they are in sync.

WardenGnaw · 2017-11-01T21:21:08Z

@DustinCampbell, when you have the time. Can you review this PR again?

DustinCampbell

I have a few comments, and I'd like to be sure that we test the new feature extensively. However, I sign off on the change.

DustinCampbell · 2017-11-02T18:34:22Z

package.json

@@ -193,6 +193,7 @@
    "vscode": "^1.17.0"
  },
  "activationEvents": [
+    "onDebug",


Does this mean that the C# extension will activate whenever the user starts debugging any non-CS file? If so, wouldn't that be a bad experience if the user installs the C# extension, tries to debug JS and the C# extension starts downloading OmniSharp and the debugger?

This activation event is required for DebugConfigurationProviders.

https://code.visualstudio.com/updates/v1_17#_debug-contributions-in-packagejson

Sure, but that's not what I asked. 😄

Sorry, I forgot to mention that I will still look into if the C# extension is activated in a node project. That was an FYI of why its there. :)

Clever! In that case, this seems like a reasonable approach.

@gregg-miskelly: Do you think we should do this for 1.13, or are you OK with doing it in 1.14, Same question for @TheRealPiotrP

@DustinCampbell do you mean the change all up or the switch to use the '*' activation event?

If change all up - @WardenGnaw how broken are we in the VS Code 1.18 (the one that is just about to come out in stable channel) without this? Are the things we are replacing just deprecated or are the broken?

If the '*' activation - I don't have a strong opinion. I am happy to wait for 1.14 on that part if you would like.

I meant using the '*' activation event. I'm OK with waiting for 1.14 for this.

Without the '*' activation event, we are not broken.

This was to fix the rare scenarios of people installing the C# extension, not opening a C# project for a while, then going to a non-C# project and then F5'ing would case the C# dependencies to start downloading.

DustinCampbell · 2017-11-02T18:37:00Z

src/assets.ts

+export function createWebLaunchConfiguration(programPath: string, workingDirectory: string): string {
+    return `
+{
+"name": ".NET Core Launch (web)",


Any reason not to indent this like you did below to make it a bit more readable?

Oops. Lost indentation when copying.

DustinCampbell · 2017-11-02T18:40:08Z

src/assets.ts

@@ -294,7 +292,7 @@ function findExecutableProjectJsonProjects(projects: protocol.DotNetProject[], c
    return result;
 }

-function containsDotNetCoreProjects(workspaceInfo: protocol.WorkspaceInformationResponse) {
+export function containsDotNetCoreProjects(workspaceInfo: protocol.WorkspaceInformationResponse) {


It feels like this helper should move someplace else if it needs to be accessed by more than just the assets generation. Perhaps src/omnisharp/protocol.ts? There's already other helpers like this there: https://github.com/OmniSharp/omnisharp-vscode/blob/master/src/omnisharp/protocol.ts#L589-L592.

DustinCampbell · 2017-11-02T18:42:16Z

src/common.ts

+ * @param subfolder subfolder to check if it is part of the folder parameter
+ * @param folder folder to check aganist
+ */
+export function isSubfolderOf(subfolder: string, folder:string): boolean {


Does this work if the paths differ by case? Does it matter?

Nit: space after folder:

@DustinCampbell Case sensitivity matters for Linux OS. Should I make it insensitive for Windows and Mac?

Not necessarily. If both values are expected to have the correct casing, it shouldn't matter. However, if they come from different sources, it might be a problem.

DustinCampbell · 2017-11-02T18:45:39Z

src/configurationProvider.ts

+                const tasksJsonPath: string = path.join(dotVscodeFolder, 'tasks.json');
+
+                // Make sure .vscode folder exists, addTasksJsonIfNecessary will fail to create tasks.json if the folder does not exist. 
+                fs.ensureDirSync(dotVscodeFolder);


I'm seeing a lot of synchronous file I/O mixed with async code. Any concerns with that?

I'm not too experienced with async programming patterns. I used the synchronous version of ensureDir and exist to make sure the .vscode folder exists and to make sure the .vscode/tasks.json does not exist before calling addTasksJsonIfNecessary. Is there a better way to do this?

I don't think there's any concern for these small checks. I just wanted to make note of it.

FWIW, NodeJS async isn't really challenging. Someday, maybe you can spend some time digging into it since the platform really hangs on asynchronous patterns. It's not necessary now.

DustinCampbell · 2017-11-02T18:47:24Z

src/configurationProvider.ts

+            }
+            else {
+                // Error to write default C# configurations.
+                throw new Error("Does not contain dotnet core projects.");


If this message gets surfaced to the user, that should be ".NET Core", not "dotnet core"

It does not get surfaced to the user since its caught by the .catch(), but I will still change it.

gregg-miskelly · 2017-11-03T15:58:55Z

src/common.ts

+ * @param folder folder to check aganist
+ */
+export function isSubfolderOf(subfolder: string, folder: string): boolean {
+    if (os.platform() !== 'win32')


Did you mean to do something here?

Assuming your idea was to try and solve the case sensitivity problem, here is my thought - I don't think we should try to solve it. From what I can tell, I think the paths we are dealing with are all from the VS Code search APIs using the same roots they are providing to us, so it seems like it would be very surprising if we saw the same path but with different case. Solving the casing problem in java script is likely a hard problem. For one because on both OSX and Linux its possible to have either case sensitivity or insensitivity (though OSX is generally insensitive and Linux is sensitive). For another because I don't know how closely the java script casing tables are actually going to match what the OS uses.

My suggestion: Just add a note to the header that the two paths must have consistent casing.

My suggestion: Just add a note to the header that the two paths must have consistent casing.

yup. That's what I was thinking too.

gregg-miskelly · 2017-11-03T16:42:35Z

src/coreclr-debug/install.ts

@@ -71,6 +71,28 @@ export class DebugInstaller {
        let manifestString = fs.readFileSync(manifestPath, 'utf8');
        let manifestObject = JSON.parse(manifestString);

+        manifestObject.activationEvents = [


Per Dustin's comment, this can wait for the next PR, but this needs to go to a different spot - I think we need to rewrite activationEvents at the very start of our activation handler if it is still set to [ "*" ] to make sure that we will not continuously try to download things in the case that we don't succeed in downloading the debugger.

gregg-miskelly

Otherwise LGTM assuming we back out the last commit.

Removing empty if and assuming paths have consistent casing.

WardenGnaw requested review from DustinCampbell, gregg-miskelly and rajkumar42 October 23, 2017 21:38

dnfclas added the cla-already-signed label Oct 23, 2017

DustinCampbell requested changes Oct 24, 2017

View reviewed changes

WardenGnaw force-pushed the master branch from bf323df to feff4e0 Compare October 24, 2017 22:38

gregg-miskelly reviewed Oct 26, 2017

View reviewed changes

gregg-miskelly requested changes Oct 26, 2017

View reviewed changes

WardenGnaw added 6 commits October 26, 2017 11:07

Updating tasks.json schema

8164618

New tasks schema from 0.1.0 to 2.0.0

Adding ConfigurationProvider

9fc23ab

${workspaceRoot} is now ${workspaceFolder} ConfigurationProvider activates on f5. This will also trigger generating a tasks file. tasks args are now 'build <path>', updating tests for these changes.

API requires October 2017 Drop (1.18)

eff8a1c

Fixing version requirement error

f740432

Addressing PR issues

f792ee8

WardenGnaw force-pushed the master branch from b3b56af to f792ee8 Compare October 26, 2017 23:54

gregg-miskelly approved these changes Oct 27, 2017

View reviewed changes

Detect older tasks.json

305d04c

Checking if folder.uri.fsPath is a subfolder of GetSolutionPathOrFolder to generate assets.

TheRealPiotrP reviewed Oct 30, 2017

View reviewed changes

gregg-miskelly approved these changes Oct 30, 2017

View reviewed changes

TheRealPiotrP mentioned this pull request Oct 31, 2017

Add TaskProvider + integration test infrastructure #1826

Closed

DustinCampbell approved these changes Nov 2, 2017

View reviewed changes

WardenGnaw added 3 commits November 2, 2017 12:57

Fixing PR issues

42c21d3

Upgrading fs-extra for promises

4461f39

Rewriting package.json for activation events

24203a2

gregg-miskelly reviewed Nov 3, 2017

View reviewed changes

gregg-miskelly approved these changes Nov 3, 2017

View reviewed changes

WardenGnaw added 2 commits November 3, 2017 09:51

Reverting start activationEvents

dcbd612

Removing empty if and assuming paths have consistent casing.

Merge branch 'master' into master

e076de4

WardenGnaw merged commit 03b89c4 into dotnet:master Nov 3, 2017

		@@ -327,7 +325,7 @@ function getBuildTasks(tasksConfiguration: tasks.TaskConfiguration): tasks.TaskD

		function findBuildTask(tasksDescriptions: tasks.TaskDescription[]) {

		@@ -32,25 +32,21 @@ declare module "vscode-tasks" {
		export interface BaseTaskConfiguration {

Add ConfigurationProvider API, ${workspaceRoot} -> ${workspaceFolder} #1801

Add ConfigurationProvider API, ${workspaceRoot} -> ${workspaceFolder} #1801

Conversation

WardenGnaw commented Oct 23, 2017

DustinCampbell left a 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

DustinCampbell commented Oct 25, 2017

gregg-miskelly commented Oct 25, 2017

DustinCampbell commented Oct 25, 2017

DustinCampbell commented Oct 25, 2017

WardenGnaw commented Oct 25, 2017

DustinCampbell commented Oct 25, 2017

gregg-miskelly commented Oct 25, 2017 • edited Loading

WardenGnaw commented Oct 25, 2017

gregg-miskelly commented Oct 25, 2017

WardenGnaw commented Oct 25, 2017

WardenGnaw commented Oct 25, 2017

DustinCampbell commented Oct 25, 2017

gregg-miskelly commented Oct 26, 2017

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

WardenGnaw Oct 26, 2017 • 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

DustinCampbell Oct 26, 2017 • edited Loading

Choose a reason for hiding this comment

gregg-miskelly left a comment

Choose a reason for hiding this comment

gregg-miskelly left a 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

WardenGnaw commented Nov 1, 2017

DustinCampbell left a 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

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

gregg-miskelly left a comment

Choose a reason for hiding this comment

gregg-miskelly commented Oct 25, 2017 •

edited

Loading

WardenGnaw Oct 26, 2017 •

edited

Loading

DustinCampbell Oct 26, 2017 •

edited

Loading