Expose output from processes on frontend #5597

westbury · 2019-06-27T13:14:01Z

I have been looking at moving various tasks into Theia's task framework. We have a number of tasks that provide feedback to progress monitors, output channels, and other destinations in the UI. Currently tasks that are run as processes (ie not terminals) do not provide support for extenders to do this (just start and exit messages courtesy of @elaihau in #5155).

These changes provide the required support. The changes involved are:

Output from processes are sent to the client. These are converted from 'chunks' to 'lines' before sending to the client.
Previously all tasks show a message both when the task starts and when it completes. If a process shows alternative feedback to the user then those messages may not be wanted. It is possible to change that behavior by rebinding TaskWatcher but that is very messy. This code therefore outputs the start and exit messages only if no custom code is bound to the process output.
The kill message is supported. This is easily invoked from the cancel token in progress monitors.

I have kept the changes fairly minimal. Ideally the 'process' and 'task' packages should be more decoupled. For example, although 'task' depends on 'process', there is lot code in 'process' that is supporting terminals. If that code were refactored out into the 'terminal' package then it would also be easy for extenders to extend stuff in 'process'.

ProgressManager currently maps ids to Process objects. Ideally it would map ids to objects (eg TaskInfo) that contain at least Process and the task type. For many uses of Process it is not necessary to put in the ProcessManager, so it makes more sense if ProcessManager were TaskManager in the tasks package. This would avoid the need to hook initClientConnection off Process as is done in this PR.

I am very happy to do the further changes too. However I cannot do that without guidance on how I can know what is considered API that can't be changed.

packages/process/src/node/process-backend-contribution.ts

packages/task/src/browser/task-service.ts

packages/task/src/common/task-protocol.ts

packages/process/src/node/process.ts

paul-marechal · 2019-06-27T15:31:49Z

For example, although 'task' depends on 'process', there is lot code in 'process' that is supporting terminals. If that code were refactored out into the 'terminal' package then it would also be easy for extenders to extend stuff in 'process'.

Can you detail a bit about this?

westbury · 2019-06-27T16:40:24Z

For example, although 'task' depends on 'process', there is lot code in 'process' that is supporting terminals. If that code were refactored out into the 'terminal' package then it would also be easy for extenders to extend stuff in 'process'.

Can you detail a bit about this?

Here is an example. We have backend requests that are not a single process. The backend may need to run a number of external processes to carry out a task. We also have a framework that runs these processes and provides overall progress reporting. We can move these requests into tasks by using a custom task type and custom task runner. This would require a custom hookup to the connection. For example, if a kill request arrives then only our custom task runner would know how to kill the task.

My comment was really that the process and tasks packages could be refactored so that support for terminal could be implemented entirely in the terminal package, without any references to 'terminal' in the process package. If the API allowed terminal to be implemented in this way then it is likely the API would also allow custom processes to be implemented. If, however, the terminal implementation cannot be done without lots of terminal code in the process package then it is less likely that custom processes can also be implemented without custom code in the process package. However I recognize that 'terminal' is a special case, different from custom processes, and it may not make sense to abstract it out in this way.

paul-marechal · 2019-07-02T15:19:22Z

This would require a custom hookup to the connection. For example, if a kill request arrives then only our custom task runner would know how to kill the task.

I like the idea, but since I am not 100% familiar with the task package, I didn't seem to find a way to kill a running task. Is it something that we are missing but that VS Code has?

My comment was really that the process and tasks packages could be refactored so that support for terminal could be implemented entirely in the terminal package, without any references to 'terminal' in the process package.

It is a confusing part, but the terminal reference in TerminalProcess only has to do with the way that the process is started, it is not directly tied to our console/terminal integration. IIRC VS Code runs tasks in ptys like we do with these TerminalProcess instances. And I think moving it wouldn't help either, as the hookup problem for different kind of "processes" would still happen.

westbury · 2019-07-04T14:22:47Z

On looking further at this, it seems to me that the task processes should be using TaskManager, not ProcessManager, to lookup the ids. So task ids are used which we use to lookup the Task. Making this change, it all falls into place. The custom hookup to the client is now done in the Task object, not the Process object, which is much better for a number of reasons. Not only was Process not the place for this, but this also supports Tasks that are not backed by a Process.

I do wonder if ProcessManager would not better be TerminalManager, containing only TerminalProcess objects. But that is another discussion.

I have updated the PR. Note that all changes are now in task package only.

westbury · 2019-07-04T14:31:00Z

I didn't seem to find a way to kill a running task. Is it something that we are missing but that VS Code has

@marechal-p the ability of the user to kill a running task is indeed something that VS Code has that is missing in Theia. However what is important, IMO, is that it is easy for extenders who are implementing Tasks to add custom requests (such as kill) to their tasks. I mentioned the 'kill' request only as an example, though perhaps it is not the best example because I included 'kill' support in the base implementation.

vince-fugnitto · 2019-07-04T14:43:53Z

I didn't seem to find a way to kill a running task. Is it something that we are missing but that VS Code has

@marechal-p the ability of the user to kill a running task is indeed something that VS Code has that is missing in Theia...

I'm confused, don't we have the kill method in the task-service?
https://github.com/theia-ide/theia/blob/20251eae573ae89f5cf3242131add17f0ef40956/packages/task/src/browser/task-service.ts#L367

westbury · 2019-07-04T15:18:34Z

I'm confused, don't we have the kill method in the task-service?

@vince-fugnitto Yes, you are right. Thanks for pointing that out. I originally implemented this using ProcessManager and Process, for which I believe we don't have the ability for users to kill from the frontend. I had therefore included support for killing the process in the connection.

I then moved the code from Process to Task (clearly a much better place). As you say, there was already kill support there, so there are now two ways of killing tasks. I should remove the code I added to support kill through the connection because I don't think there should be two ways of doing the same thing.

https://github.com/theia-ide/theia/blob/39cb9866a2352833be8e0cf984a3905dd9856f72/packages/task/src/node/process/process-task.ts#L81

paul-marechal

There is a build error now, see the CI :)

packages/task/src/node/process/process-task.ts

packages/task/src/node/task.ts

paul-marechal · 2019-07-09T14:45:18Z

Build error: https://travis-ci.org/theia-ide/theia/jobs/554273807#L1056

paul-marechal

Sorry for the lengthy review, just want to be sure that we are on the same frequency.

packages/task/src/browser/task-service.ts

packages/task/src/node/process/process-task.ts

packages/task/src/browser/task-service.ts

Signed-off-by: Nigel Westbury <nigelipse@miegel.org>

westbury · 2019-07-11T13:39:31Z

Sorry for the lengthy review, just want to be sure that we are on the same frequency.

I really appreciate the time you have taken to get this right. It's much better now than my original PR thanks to your comments.

westbury · 2019-07-11T13:46:02Z

More importantly, the branching is bogus... If event.code === 0 we want to suppress the message, if event.code !== 0 then we want to show the message. The else branch here that outputs "Invalid TaskExitedEvent ..." should not be there (it is already here a few lines below).

That was messed up during rebase merging. Sorry. The only change that should have been made was that no 'start' or 'exit' messages are sent to the message service if there is custom feedback. It's now fixed

packages/task/src/browser/task-service.ts

paul-marechal · 2019-07-29T21:22:44Z

packages/task/src/browser/task-service.ts

+            if (taskInfo.config.type === 'shell') {
+                this.attach(taskInfo.terminalId, taskInfo.taskId);
+            } else {
+                this.attachProcess(taskInfo);


In order to spawn processes and fetch the output in a different way than displaying it in a terminal widget, would creating a new task type solely for that be better? Or how should one hook onto one process but not the other, assuming that both process and shell tasks both should be displayed in a terminal widget by default?

paul-marechal · 2019-08-08T21:58:18Z

Ok so I made the change to align with VS Code: #5895

If you are still interested with the change in the current PR, can you tell me how this impacts you?

paul-marechal · 2022-03-01T21:22:02Z

PR is stale. Feel free to re-open if still willing to get this in.

vince-fugnitto reviewed Jun 27, 2019

View reviewed changes

packages/process/src/node/process-backend-contribution.ts Outdated Show resolved Hide resolved

vince-fugnitto reviewed Jun 27, 2019

View reviewed changes

packages/task/src/browser/task-service.ts Outdated Show resolved Hide resolved

vince-fugnitto reviewed Jun 27, 2019

View reviewed changes

packages/task/src/common/task-protocol.ts Outdated Show resolved Hide resolved

paul-marechal reviewed Jun 27, 2019

View reviewed changes

packages/process/src/node/process.ts Outdated Show resolved Hide resolved

westbury force-pushed the process-output branch from 2d500f6 to 39cb986 Compare July 4, 2019 14:13

paul-marechal reviewed Jul 4, 2019

View reviewed changes

packages/task/src/node/process/process-task.ts Outdated Show resolved Hide resolved

packages/task/src/node/task.ts Outdated Show resolved Hide resolved

paul-marechal reviewed Jul 9, 2019

View reviewed changes

packages/task/src/browser/task-service.ts Outdated Show resolved Hide resolved

packages/task/src/node/process/process-task.ts Outdated Show resolved Hide resolved

packages/task/src/browser/task-service.ts Show resolved Hide resolved

Expose output from tasks on frontend

57bf40c

Signed-off-by: Nigel Westbury <nigelipse@miegel.org>

westbury force-pushed the process-output branch from 39cb986 to 57bf40c Compare July 11, 2019 13:28

paul-marechal reviewed Jul 29, 2019

View reviewed changes

westbury mentioned this pull request Aug 18, 2019

support "showOutput" flag in task config #5974

Closed

akosyakov added output issues related to the output vscode issues related to VSCode compatibility labels Aug 18, 2019

JonasHelming assigned paul-marechal Feb 14, 2022

JonasHelming added the decision_needed issues that require decisions on ways forward (ex: if they should be updated or closed) label Feb 14, 2022

paul-marechal closed this Mar 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose output from processes on frontend #5597

Expose output from processes on frontend #5597

westbury commented Jun 27, 2019

paul-marechal commented Jun 27, 2019

westbury commented Jun 27, 2019

paul-marechal commented Jul 2, 2019

westbury commented Jul 4, 2019

westbury commented Jul 4, 2019

vince-fugnitto commented Jul 4, 2019

westbury commented Jul 4, 2019

paul-marechal left a comment

paul-marechal commented Jul 9, 2019

paul-marechal left a comment

westbury commented Jul 11, 2019

westbury commented Jul 11, 2019

paul-marechal Jul 29, 2019 •

edited

Loading

paul-marechal commented Aug 8, 2019

paul-marechal commented Mar 1, 2022

Expose output from processes on frontend #5597

Expose output from processes on frontend #5597

Conversation

westbury commented Jun 27, 2019

paul-marechal commented Jun 27, 2019

westbury commented Jun 27, 2019

paul-marechal commented Jul 2, 2019

westbury commented Jul 4, 2019

westbury commented Jul 4, 2019

vince-fugnitto commented Jul 4, 2019

westbury commented Jul 4, 2019

paul-marechal left a comment

Choose a reason for hiding this comment

paul-marechal commented Jul 9, 2019

paul-marechal left a comment

Choose a reason for hiding this comment

westbury commented Jul 11, 2019

westbury commented Jul 11, 2019

paul-marechal Jul 29, 2019 • edited Loading

Choose a reason for hiding this comment

paul-marechal commented Aug 8, 2019

paul-marechal commented Mar 1, 2022

paul-marechal Jul 29, 2019 •

edited

Loading