microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
163.52k stars 28.99k forks source link

Extend tasks API to allow programmatic problem matching #59337

Open g-arjones opened 6 years ago

g-arjones commented 6 years ago

The new tasks APIs (fetchTask(), executeTask(), task events...) makes it more convenient to use vscode's own task system to spawn child process (as opposed to using something like child_process, for example). Still, AFAICT, there are two bits missing in the API:

  1. A method to get the underlying Terminal instance of a running task (maybe through TaskExecution?);

  2. Events that would be fired whenever something is written in the process' stdout/stderr (that would probably go in the Terminal instance itself)

This would be very useful to implement UI feedback (i.e progess notifications) while using vscode's task system. Right now, the only way I see to do that is to spawn my own proces, create an output channel and handle problem matching myself...

Does that make sense?

g-arjones commented 6 years ago

Just found out that there is already a onDidWrite proposed in Terminal. And that there is a onDidOpenTerminal in window. Now I just need a way to match Terminals to Tasks..

I think I could provide a PR that would either expose a Terminal from Task or a Task from a Terminal? Which one do you guys think it would make more sense? @Tyriar @dbaeumer

Tyriar commented 6 years ago

@g-arjones I think you can match the process ID of the terminal against the task

g-arjones commented 6 years ago

I thought about that but it didn't feel like a very safe thing to do. I mean, which one is triggered first onDidOpenTerminal or onDidStartTaskProcess and is this order guaranteed (a.k.a won't change in the future)? Or would you suggest another approach other than using these events to do the matching?

dbaeumer commented 6 years ago

Actually that tasks are executed in a terminal is still an implementation details (there are even legacy tasks which don't even run in a terminal). In addition there are requests to be able to run a task in a external terminal. As soon as we would add such a API we are basically hooked into this. This is why I feel very reluctant of adding this in the API.

Adding access to stdout / stderr is a request we do have and I fully support having in the API.

g-arjones commented 6 years ago

that tasks are executed in a terminal is still an implementation details (there are even legacy tasks which don't even run in a terminal). In addition there are requests to be able to run a task in a external terminal. As soon as we would add such a API we are basically hooked into this.

Even though it would be totally valid IMO to have Task.terminal === undefined in case the task is running in an external terminal, it's absolutely legit to keep the terminal instance as an implementation detail.

Adding access to stdout / stderr is a request we do have and I fully support having in the API.

That is good to know, it would be very useful for extension developers. But, how would that work with external terminals?

That all said, right now I don't have a good way to match Terminals and Tasks to properly parse what comes from onDidWrite API, would you have a suggestion?

dbaeumer commented 6 years ago

Capturing stdout / stderr is a indeed challenging for external terminals and it would depend on the terminal used. Under Mac there is for example support to tee the output for some terminals. We prototyped this once to run problem matchers in the output of external terminals.

Regarding:

That all said, right now I don't have a good way to match Terminals and Tasks to properly parse what comes from onDidWrite API, would you have a suggestion?

All I can think of right now is to use the process ID. There are some assumptions you can make:

So you could after the task of interest starts listen to all terminal creations, remember all of them, wait for the process start and then use the process id to correlate them.

g-arjones commented 6 years ago

@dbaeumer Hmmm.. That would fail occasionally, wouldn't it? I mean, in general, terminals are shared/reused by multiple tasks. In such cases, I will get a task start event but that won't ever be followed by a terminal open event

dbaeumer commented 6 years ago

@g-arjones good catch. Absolutely right.

g-arjones commented 6 years ago

Even worse, the processId property of the Terminal object is not updated when another process is started so this property is invalid most of the time... @Tyriar

Tyriar commented 6 years ago

@g-arjones that sounds like a bug if it is the case, looking through the code quickly it looks like it should be happening for reused terminals 😕. Could you create a new issue to track this?

g-arjones commented 6 years ago

@Tyriar Sure, no problem. Also, would you consider passing the Terminal object or the processId to onDidWrite's callback? The api is still proposed so I guess now is the perfect time for something like that.. Would help a lot those interested in this api

Tyriar commented 6 years ago

@g-arjones but you call onDidWriteData on the Terminal? Can't you just do this?

const term = window.createTerminal();
term.onDidWriteData(d => {
  d; // data
  term; // Terminal
});
g-arjones commented 6 years ago

@Tyriar what I meant was that I would rather subscribe to all terminals at once. Right now I have to subscribe to onOpenTerminal and then call onDidWrite there. But maybe that's just my use case

Tyriar commented 6 years ago

@g-arjones I have mixed feelings about this API, it was actually intentionally designed to disallow subscribing to all at once because there is a performance overhead associated with sending all that data over to the extension host. The data only travels when at least one extension uses it.

g-arjones commented 6 years ago

@Tyriar I see.. It seems that this really isin't suited to get stdout from a task started in an integrated terminal... :(

I will open the issue for the processId bug, I guess I can work around the other issues if I have a reliable way of getting the PID of the process currently running in the terminal

PEZ commented 5 years ago

@dbaeumer :

Adding access to stdout / stderr is a request we do have and I fully support having in the API.

I am looking for that request to put a thumbs up on it. Can't find it, though. I really need this access.

dbaeumer commented 5 years ago

Actually there is already proposed API that allows you to start your own process and hook it up to the terminal. It is called CustomTask but will change before it will be final.

g-arjones commented 5 years ago

@dbaeumer can I use that to execute a task that is not provided by a task provider ?

PEZ commented 5 years ago

I wonder the same as @g-arjones wonders.

I'm trying to find information on CustomTask but my google-fu does not suffice. Any pointers?

alexr00 commented 5 years ago

@PEZ Since custom task execution is still proposed API there's no documentation for it yet, but you can see an example of it in our samples repository.

@g-arjones with custom task execution you'll still need to write a task provider for the task.

PEZ commented 5 years ago

@alexr00 Thanks for the link to the sample! I have looked at it a while now, but am still wondering if it is of any help for my use case. Before I file a new feature request, I'll try to describe what I need here.

My extension starts an nREPL server (a bit like a language server for Clojure applications) connected to the application under development. When the server has started I then connect my extension to the server.

This pseudo code is similar to what I do today, with the exception that instead of being able to react on the stdout of the process/shell command, I watch for file changes.

function executeJackInTask(projectType: connector.ProjectType, executable: string, args: any) {
    const env = { ...process.env, ...state.config().jackInEnv } as {
        [key: string]: string;
    };
    const execution = isWin ?
        new vscode.ProcessExecution(executable, args, {
            cwd: utilities.getProjectDir(),
            env: env,
        }) :
        new vscode.ShellExecution(executable, args, {
            cwd: utilities.getProjectDir(),
            env: env,
        });
    const taskDefinition: vscode.TaskDefinition = {
        type: isWin ? "process" : "shell",
        label: "Calva Jack-in"
    };
    const folder = vscode.workspace.workspaceFolders[0];
    const task = new vscode.Task(taskDefinition, folder, "Calva Jack-in", "Calva", execution);

    vscode.tasks.executeTask(task).onOutput((output: string) => {
        if (output.includes("nREPL server started")) {
            connector.connect();
        }
    });
}

I want to use the Task API for this because then VS Code provides the user with UI to follow and control the process and also helps me with house keeping. I have not looked into TaskProviders, but they seem to be serving some other purpose than what I need.

Things work pretty well with the file system watcher. But it is a bit brittle, and I also want to be able to react on other output from the task, including things on stderr.

EDIT: Please not that I am not suggesting an API design here. Just trying to express what I would want to be able to do.

alexr00 commented 5 years ago

@PEZ I'm not a big fan of creating adhoc tasks like this. Any task that you want to create should be provided by a using a task provider so that VS Code has the tasks source of truth. This also allows users to modify the presentation properties of task and use it for debug prelaunchTask. Any tasks that exist should be provided by a task provider.

In terms of functionality, I think what you are looking for is a hybrid of custom task (you want to execute a callback when your task is run and be able to do things programatically on output), and process/shell execution (you want the easy process or script execution that tasks provides).

If you wish to use a task, the best fit for you is the custom task. However, we have no plans to add API around process management for that because you already have the tools to do that yourself. It also seems like you might be able to use a terminal for what you're doing instead of a file watcher.

If what you want to do here will be best served by additional VS Code API then please do open a new issue for it!.

PEZ commented 5 years ago

@alexr00 : I hear you about using Tasks properly. I thought Providers were for another purpose than I use Task for here, but I'll put it on my todo to check it out better. And thanks for the pointers! :heart:

We'll see if there is a need for a feature request about the API. :smile:

g-arjones commented 5 years ago

@alexr00 My goal is to have tasks that would be launched prior to a debugging session, just like a prelaunch task would. The caveat is that I don't want to make the user write a tasks.json for that purpose since all information required for these tasks are already available in the DebugConfiguration. Also, ideally, I didn't want these tasks showing up in tasks related UI (although I guess I could live with that).

Do you have any tips? Last time I checked, it wasn't possible to use tasks provided solely by a task provider as a prelaunch task (the user had to write a tasks.json for that to be possible). Is this still the case? Also, how would you pass data from the debug provider to the task provider?

g-arjones commented 5 years ago

It would be cool if I could provide an actual task definition in DebugConfiguration.prelaunchTask rather than just the task name. What do you think abou that? @alexr00 @weinand @isidorn @dbaeumer

alexr00 commented 5 years ago

@g-arjones there is no restriction on having a task in tasks.json for it to be used as a prelaunch task. You can use extension contributed tasks.

There is no way to pass info from the debug provider to the tasks provider.

If you feel strongly about https://github.com/microsoft/vscode/issues/59337#issuecomment-520163912 please open another issue for it.

piomis commented 6 months ago

Are there any plans in regards to this feature (possibility pass to 'custom problem matcher' to vscode.Task constructor)?