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

Extend tasks API to allow programmatic problem matching #59337

Open
g-arjones opened this issue Sep 25, 2018 · 27 comments
Open

Extend tasks API to allow programmatic problem matching #59337

g-arjones opened this issue Sep 25, 2018 · 27 comments
Assignees
Labels
api feature-request Request for new features or functionality tasks Task system issues
Milestone

Comments

@g-arjones
Copy link
Contributor

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?

@vscodebot vscodebot bot added the tasks Task system issues label Sep 25, 2018
@g-arjones
Copy link
Contributor Author

g-arjones commented Sep 25, 2018

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
Copy link
Member

Tyriar commented Sep 25, 2018

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

@g-arjones
Copy link
Contributor Author

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
Copy link
Member

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.

@dbaeumer dbaeumer added the feature-request Request for new features or functionality label Sep 26, 2018
@dbaeumer dbaeumer added this to the On Deck milestone Sep 26, 2018
@g-arjones
Copy link
Contributor Author

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
Copy link
Member

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:

  • task start is fired before process start
  • task start should be fired (I haven't verified that but from what I know how our code works) before the terminal instance creation is fired.

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
Copy link
Contributor Author

@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
Copy link
Member

@g-arjones good catch. Absolutely right.

@g-arjones
Copy link
Contributor Author

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
Copy link
Member

Tyriar commented Sep 28, 2018

@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
Copy link
Contributor Author

@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
Copy link
Member

Tyriar commented Sep 28, 2018

@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
Copy link
Contributor Author

@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
Copy link
Member

Tyriar commented Sep 28, 2018

@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
Copy link
Contributor Author

@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
Copy link
Contributor

PEZ commented Aug 7, 2019

@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
Copy link
Member

dbaeumer commented Aug 7, 2019

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
Copy link
Contributor Author

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

@PEZ
Copy link
Contributor

PEZ commented Aug 7, 2019

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?

@Tyriar Tyriar assigned alexr00 and unassigned dbaeumer Aug 7, 2019
@alexr00
Copy link
Member

alexr00 commented Aug 7, 2019

@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.

@alexr00 alexr00 changed the title Extend tasks API Extend tasks API to allow programmatic problem matching Aug 7, 2019
@PEZ
Copy link
Contributor

PEZ commented Aug 7, 2019

@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
Copy link
Member

alexr00 commented Aug 9, 2019

@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
Copy link
Contributor

PEZ commented Aug 10, 2019

@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! ❤️

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

@g-arjones
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Copy link
Member

alexr00 commented Aug 12, 2019

@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 #59337 (comment) please open another issue for it.

@alexr00 alexr00 assigned meganrogge and unassigned alexr00 Jun 30, 2022
@meganrogge meganrogge added the api label Jun 30, 2022
@piomis
Copy link

piomis commented Apr 11, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api feature-request Request for new features or functionality tasks Task system issues
Projects
None yet
Development

No branches or pull requests

7 participants