Vscode: CustomExecution2 Feedback: expose variable resolvers

[bwateratmsft]

One shortcoming I realized with CustomExecution2 is that each task would have to implement their own way of resolving common variables--e.g. ${workspaceFolder} or ${file}, etc.

Can we get an API to reach the variable resolvers e.g. ConfigurationResolverService? This way any variable input to CustomExecution2 tasks would not require the task to reimplement the wheel.

[bwateratmsft]

@alexr00 @philliphoff

[alexr00]

@bwateratmsft what if we just resolved variables for custom execution tasks instead of exposing the configuration resolver?

Edit: This changes the task ID, which causes a whole host of problems.

[alexr00]

Can you outline a situation in which you couldn't use VS Code API to get the current workspace folder or current file from within your task?

[bwateratmsft]

File and folder are just some of the available variables. There are commands, configs, prompts, environment variables, every imagineable thing about workspace/file/line...end users would expect all of these to work regardless of how the task is implemented behind the scenes. If the extension implementer had to implement support for all of these, it would be very difficult (and probably not even possible in some cases), _and_ it would essentially just end up being a reimplementation of what VSCode has already.

[alexr00]

I think I'm still missing something. Why would you need to use the ${} variables at all in custom execution tasks? The VS Code API should expose all of those things to you to use in your task without ${} variables.

[bwateratmsft]

Why would you _not_ need ${} variables? To take away that capability would be to artificially limit CustomExecution, and end users would not understand why, because they don't know or care how a task is implemented behind-the-scenes.

Let me give an example. In the Docker extension, we cannot realistically make ASP.NET Core Blazor web apps work without CustomExecution, because of the order of execution of a debug launch. We have to do some steps after the dotnet build task, which is not possible without either having CustomExecution, or else offloading them to an external process (ShellExecution, ProcessExecution) (which also makes for an ugly tasks.json, because resolveDebugConfiguration cannot resolve a task dynamically).

But, having CustomExecution does not suddenly mean that users don't need to configure the docker build and docker run tasks. There are a large selection of options available for configuring these tasks: docker-build and docker-run.

Users have already stated the need to be able to pass in ${env:xx}, and we ourselves already use ${workspaceFolder} and ${file}. We also have users who want a ${command} variable. Every flavor of ${relativeFile} etc. would also be useful to us.

So, unless we get an API that allows us to resolve task variables, here are our choices:

1. Never switch to CustomExecution, which means high perf cost of the constant calls to resolveTask, and no ability to ever support ASP.NET Core Blazor apps. But, users could use any built-in variable (or env/command/prompt/config/etc.).
2. Implement our own variable resolver, which will be a waste of work considering a variable resolver already exists in VSCode. It would also always lag behind the capabilities of the built-in one, and users will not care why--they will simply see it as broken functionality.

[philliphoff]

Just to add: the ability to resolve tokens is not exclusive to the use of CustomExecution. If the user makes use of tokens such as ${workspaceFolder} in any of a provider's custom properties, even "traditional" TaskProviders and DebugConfigurationProviders may need to resolve those tokens in order to properly resolve a task or debug configuration.

We're doing this already in our providers, for the tokens we know are being used. But, as the features gain adoption, we can be sure more tokens will be used, and we'd rather not reimplement the wheel.

[alexr00]

@bwateratmsft it sounds like you want to pass values from tasks.json into tasks, which is related https://github.com/microsoft/vscode/issues/58836. If we implement https://github.com/microsoft/vscode/issues/58836 then we can easily resolve variables in the passed in values. Would that be enough?

[bwateratmsft]

I think that that implementation might be insufficient. We are building extremely complicated docker build and docker run commands based on user input, universal defaults, language-specific defaults, and platform-specific defaults. Simple substitutions into a command line (like --port ${task:port} from #58836) would generally not work, which is why we need to be in control of building the command line ourselves.

As an example, this relatively simple docker-run task configuration for a .NET Core app:

        {
            "type": "docker-run",
            "label": "docker-run: debug",
            "dependsOn": [
                "docker-build: debug"
            ],
            "dockerRun": {
                "volumes": [
                    {
                        "containerPath": "/myvolume",
                        "localPath": "${workspaceFolder}",
                        "permissions": "ro"
                    }
                ]
            },
            "netCore": {
                "appProject": "${workspaceFolder}/TestWeb/TestWeb.csproj"
            }
        }

becomes this ShellExecution command:

docker run -dt -P --name 'testsln-dev' -e 'ASPNETCORE_ENVIRONMENT=Development' -e 'ASPNETCORE_URLS=https://+:443;http://+:80' --label 'com.microsoft.created-by=visual-studio-code' -v 'C:\Users\brandonw\source\repos\TestSln:/myvolume:ro' -v 'c:\Users\brandonw\source\repos\TestSln\TestWeb:/app:rw' -v 'c:\Users\brandonw\source\repos\TestSln:/src:rw' -v 'C:\Users\brandonw\.vsdbg:/remote_debugger:ro' -v 'C:\Users\brandonw\.nuget\packages:/root/.nuget/packages:ro' -v 'C:\Program Files\dotnet\sdk\NuGetFallbackFolder:/root/.nuget/fallbackpackages:ro' -v 'C:\Users\brandonw\AppData\Roaming\Microsoft\UserSecrets:/root/.microsoft/usersecrets:ro' -v 'C:\Users\brandonw\AppData\Roaming\ASP.NET\Https:/root/.aspnet/https:ro' 'testsln:dev'

As you can see, the input from the task cannot be trivially mapped into the command line. If we wanted a "simple" task definition that was just that huge command line, without all the complicated logic, then we may as well not implement a task at all. The goal is to make it easier for users to configure the docker build and docker run tasks without needing to deal with the difficult syntax, and without needing to know all the language- and platform-specific subtleties.

I don't really understand why this sounds so difficult to implement? The heart of the code is already all there, we just need to be able to access it as extension developers.

[alexr00]

Looks like we have an issue for exposing the configuration resolver API already: https://github.com/microsoft/vscode/issues/2809
Duplicate of that issue.

[vscodebot[bot]]

Thanks for creating this issue! We figured it's covering the same as another one we already have. Thus, we closed this one as a duplicate. You can search for existing issues here. See also our issue reporting guidelines.

Happy Coding!

[alexr00]

@bwateratmsft What if we resolved all the variables in your TaskDefinition and then passed that back into the custom execution callback?

    export class CustomExecution {
        /**
         * @param process The [Pseudoterminal](#Pseudoterminal) to be used by the task to display output.
         * @param callback The callback that will be called when the task is started by a user.
         */
        constructor(callback: (resolvedDefinition?: TaskDefinition) => Thenable<Pseudoterminal>);
    }

[bwateratmsft]

That sounds great, I can't think of any scenarios where that _wouldn't_ work. It gives extension developers the freedom to use the resolved config, or not use it if they're certain it isn't needed, or mix the two.

One question--is this something that has to ship with CustomExecution from the start, or can it be added later? My thinking is that if it meant delaying CustomExecution, I'd rather release what we have now and add this later, if that is possible.

[alexr00]

It's an optional parameter, so I would add it later. The new parameter would sit in vscode.proposed.d.ts for a few months then we would try to finalize it. CustomExecution would not be delayed because of this.

[alexr00]

Moving to November since we are taking October for issue grooming.

[alexr00]

@bwateratmsft I merged a proposed API for this. I don't think it's complete yet, as it only supports simple built in variables now (no input, command, or defaultBuildTask). The variable resolver on the extension host doesn't currently support more than predefined variables. Adding support for the other variables is more involved and can be done later since this is proposed API.

[bwateratmsft]

Awesome, thank you @alexr00 !

[philliphoff]

@alexr00 Any update on when this might go from "proposed" API to "official" API? We have a new extension that makes use of CustomExecution and customers have also asked for support of variable expansion in the task definition (vscode-dapr#94).

[alexr00]

There is no plan to finalize it at the moment. It requires a fair amount more work since it only works for simple variables (not input, command, and defaultBuildTask). I think it would be confusing for user if they try to use a non-supported variable since it wouldn't be clear why some variables work in some parts of a task and don't work in other parts.

[philliphoff]

Fair point, though that means that we'll likely have to implement variable resolution ourselves in order to satisfy our users in the meantime, and that could cause more confusion, as we'll not (have the time or resources to) implement all variables as well as allow for the possibility that our implementation diverges from VS Code's (for their in-built task properties).

Personally, using a common implementation, even for a subset of variables (which can be well documented as a current limitation of the API), is better than several inconsistent implementations.

[alexr00]

Less confusion is clearly better. I will see what folks at our API discussion think.

[bwateratmsft]

We just got our first GitHub issue from said confusion: https://github.com/microsoft/vscode-docker/issues/1961

[chris-sekira]

We just got our first GitHub issue from said confusion: microsoft/vscode-docker#1961

I got here from this, the inability to substitute environment variables into docker-build type tasks for buildArgs is really a letdown. I'd love to be able to use that task to inject Azure Artifacts auth tokens when trying to build/restore inside of a container, but I have to resort to manual shell tasks which doesn't play nice with the debugging capabilities.

[vglushkov]

Another important scenario is the ability to use promptString input variables in docker-build type tasks to tag image with a version with every new build.

[cecilphillip]

This issue has become very important for our audience. Right now we're forced to hardcode home directory paths in our tasks.json files which become problematic when working with source control. Being able to use ${env:HOME} would save us a ton of headache.

[alexr00]

There are several places folks want to be able to resolve built in ${} style variables in strings within VS Code:

• CustomExecution tasks
• Extension settings
• I think there is also a place in debug

Within VS Code, there are several places we resolve variables:

• Tasks (command, input, other variables that require user interaction, and "static" variables)
• Debug (same as tasks)
• Some select settings (only "static" variables because they must be sync)

Some examples of variables:

${cwd}
${fileBasename}
${input:myUserDefinedTaskInput}
${command:extension.commandId}

Since wanting to resolve variables is not unique to CustomExecution tasks, it makes sense to have a solution that isn't specific. Here's the proposal to that end:

    export namespace workspace {
                // Resolves and replaces all known ${} style variables in each element of `value`.
        // Any unknown variables will be left in place in the return value.
        export function resolveVariables(values: string[], scope?: Uri): Promise<string[]>;
    }

[bwateratmsft]

This should work for the Docker extension. Would it be possible to make it object-recursive? Otherwise we can pretty easily deal with the recursion ourselves.

[alexr00]

After much discussion, we are back to the tasks specific API proposal:

https://github.com/microsoft/vscode/blob/2d9ffda24619f1f9ae5d845f42f1fcee8aea2fba/src/vs/vscode.proposed.d.ts#L989-L999

Summary of discussion:

• Debug has a solution for resolving variables already. This is necessary because variable resolving in debug requires some debug context that can't easily be passed in from the extension on demand.
• Because debug has a separate solution and requires context, it would need to be specifically disallowed from calling any on demand resolving API to prevent inconsistencies.
• Tasks also requires some context when resolving variables. It will be easier for extensions to simply get the appropriately resolved variables at the appropriate time rather than for them to figure out the context correctly then resolve on demand.
• While a task specific solution doesn't solve the problem for settings, there are types of variables that tasks will want to resolve but with _never_ make sense for settings to resolve (input and command for example). Having a single solution that sometimes resolves those variables and sometimes doesn't is confusing.

[alexr00]

Folks who are interested in this API, I merged the change that will allow input and command variables to resolve with the resolvedDefinition in the callback for the proposal. Thinking about it a bit more, I might need to move where the task definition is resolved, but in the next insiders you should be able to try it out and give feedback.

[alexr00]

I will be finalizing this API this week, please speak now if this API doesn't fulfill your custom execution variable resolving needs! https://github.com/microsoft/vscode/blob/master/src/vs/vscode.proposed.d.ts#L1000-L1010

[kaysonwu]

Hi, @alexr00 Before it is implemented, how should I get the name of the currently opened ${file}?

[alexr00]

If your task is a CustomExecution task then all variables in the task definition will be resolved in the resolvedDefinition in the callback.

[kaysonwu]

resolvedDefinition I tried it, but it didn't resolve. Am I doing something wrong? @alexr00

[alexr00]

@kaysonwu what version of VS Code are you using? If you aren't using the latest Insiders build, please do try that since there are important changes there.

[kaysonwu]

I used 14.8.0

[alexr00]

@kaysonwu please try with the latest insiders: https://code.visualstudio.com/insiders/

[kaysonwu]

okay. thanks.

[philliphoff]

@alexr00 @bwateratmsft is on leave at the moment, so I don't know how soon we'll be able to check out the new API (cc @karolz-ms) That said, looking at it, it seems like it should work for the bulk of our needs in vscode-docker and vscode-dapr.

[karolz-ms]

I trust @philliphoff judgement on this ☺️

[bwateratmsft]

I'm back! I will take a look today.

Update: works great, thanks @alexr00!

My PR for it: https://github.com/microsoft/vscode-docker/pull/2250