Clean up handling of 'taskType' field #9377

tsmaeder · 2021-04-20T10:13:16Z

Signed-off-by: Thomas Mäder tmader@redhat.com

What it does

This PR cleans up a couple of the issues introduced by Implement CustomExecution extension API #9189 while retaining the custom execution API.

The approach is to strictly use the "taskType" field in a task configuration to mean the "task execution type". The magic in types-impl.ts (Task#updateTypeBasedOnExecution) has been removed and the translation of Task.execution into TaskDTO.taskType and vice-versa has been moved to the type converter.

I have removed a couple of instances where taskType was used erroneously when the declared type was meant and not the execution type.

I have removed some filtering of dependent tasks based on "source". It did not work (configured tasks have the folder they are in as "source") and I don't think the source field should be considered part of the task identity.

I have also removed the "garbage collection" of callback functions for custom executions. There is no explicit lifecycle for those callback functions that I could find in the VS Code API. I believe there are supposed to be only a fixed, small number of callback functions. So I'm retaining these functions based on identity. We could clean up the callbacks whenever we call provideTasks and after call to the executeTask API, but I don't think it is necessary. Convince me otherwise :-)

How to test

Make sure non-contributed tasks still work, aka process and shell tasks.

Make sure we can run and configure both custom execution tasks (see gradle tasks: #8767) and the shell tasks contributed by @vince-fugnitto's test plugin (see #9366)

Make sure we can run dependent tasks based on label as well as based on structured task identifier.

Review checklist

as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

as a reviewer, I agree to behave in accordance with the review guidelines

RomanNikitenko

@tsmaeder
thank you for the provided fix, I tested the changes and found that they don't fix #9207 (review).
It looks like we still have some problem with provided tasks:

The use case works well with these changes #9373:

RomanNikitenko · 2021-04-20T12:13:36Z

Also, there is still a problem from #9365.
It's number 3 in #9365:

It looks like after configuring a task the configuration from tasks.json file is not taken into account at all - so a user select a configured configuration for running, but the corresponding detected configuration is used instead.

So, I have configured a task, changed the configuration, but task system runs the corresponding detected task, not configured task configuration...

tsmaeder · 2021-04-20T12:30:51Z

@RomanNikitenko there is https://github.com/eclipse-theia/theia/pull/9207/files for the problem matchers.

RomanNikitenko · 2021-04-20T12:42:40Z

@RomanNikitenko there is https://github.com/eclipse-theia/theia/pull/9207/files for the problem matchers.

What do you mean?
is it related to my first comment #9377 (review)?
If so - I have checked that behavior for your changes and the same for these changes #9373

tsmaeder · 2021-04-20T12:48:14Z

is it related to my first comment #9377 (review)?

Yes, there is an open PR for that issue.

RomanNikitenko · 2021-04-20T12:52:06Z

is it related to my first comment #9377 (review)?

Yes, there is an open PR for that issue.

I don't understand what you mean.
I tested the use case described here #9207 (review) using:

your changes - it doesn't work
theses changes Revert task types updates #9373 - it works well

Maybe someone could check it as well - to confirm that behavior...

tsmaeder · 2021-04-20T12:54:43Z

@RomanNikitenko so you're doing this:

Configure the "tsc: build" task in a workspace containing theia source
Run the task.
Observe you get output in a terminal named 'npm'
Add a "presentation" object to the configuration
Change "panel" to "new"
Change "echo" to "false"
Run the task
Observe: the output of the command is shown in the same terminal as before

You expectation would be that there is no output or that there is a new terminal being opened and that the command line being executed is not shown in the terminal?

RomanNikitenko · 2021-04-20T12:58:26Z

@RomanNikitenko so you're doing this:

Configure the "tsc: build" task in a workspace containing theia source

Run the task.

Observe you get output in a terminal named 'npm'

Add a "presentation" object to the configuration

Change "panel" to "new"

Change "echo" to "false"

Run the task

Observe: the output of the command is shown in the same terminal as before

You expectation would be that there is no output or that there is a new terminal being opened and that the command line being executed is not shown in the terminal?

My expectation is: the configured task (not detected) is running when I run configured task

In the use case above:

the new panel should be used for running
no echo

as such changes I provided for the configured task, you can use another way to check it

colin-grant-work

I'll need a bit of time to get a better understanding of all of the contingencies in play, but here are a couple of very minor comments from my first read-through of the changes.

colin-grant-work · 2021-04-20T13:15:08Z

packages/plugin-ext/src/plugin/tasks/tasks.ts

-        this.providedCustomExecutions = new Map<number, CustomExecution>();
-        this.notProvidedCustomExecutions = new Set<number>();
-        this.activeCustomExecutions = new Map<number, CustomExecution>();
+        this.customExecutionIds = new Map();


[Minor]: some map fields are initialized as properties above (adaptersMap, executions); others are initialized here. I'd vote for moving these three, which don't need information from the constructor arguments, up with the rest and initializing with the property syntax.

colin-grant-work · 2021-04-20T13:23:53Z

packages/plugin-ext/src/main/browser/tasks-main.ts

@@ -234,6 +234,7 @@ export class TasksMainImpl implements TasksMain, Disposable {
        return {
            ...common,
            ...partialDto,
+            taskType: taskType,


[Minor]: Could be just taskType.

or #9377 (comment)

so, it looks like we don't need the change here

tsmaeder · 2021-04-20T14:43:48Z

@RomanNikitenko

My expectation is: the configured task (not detected) is running when I run configured task

We are running the configured task. However, we pass the configured task to taskProvider.resolve() of the typescript built-in extension, which returns a new task which has an empty presentation object.

I suspect this "worked" before because we were using the "taskType" based on the task execution (which is process or shell) to look up the task resolver. We would then not get the plugin task resolver, but the fallback "ProcessTaskResolver", which doesn't empty the presentation field.

Not sure what the right thing is here: I suspect that TaskProvider#resolve() and TaskResolver#resolve() are not the same thing.

tsmaeder · 2021-04-20T15:03:51Z

Turns out that VSCode overrides the configured properties after calling the task resolver: https://github.com/microsoft/vscode/blob/10c56b598de01119b80a5c53871003f868fd41fb/src/vs/workbench/parts/tasks/node/taskConfiguration.ts#L1473

alvsan09 · 2021-04-20T16:14:58Z

I found the following issue when testing with;
https://github.com/microsoft/vscode-extension-samples/tree/main/task-provider-sample

The output from custom task e.g. task 'custombuildscript: 32 -incremental' does not show the extensions output in the terminal,
you can compare the expected and actual results in the following images when running the task twice in a row.

alvsan09 · 2021-04-20T16:45:03Z

I have tried @RomanNikitenko case but using "gradle: clean", I updated the presentation options under the configured task (tasks.json) and confirmed that the presentation options don't take effect.

alvsan09

Nice improvement !
it would be great to rename all taskType instances that mean execution type, so taskType names that refer to actual TaskDefintion.type would not be confused with execution type.

packages/plugin-ext/src/common/plugin-api-rpc.ts

alvsan09 · 2021-04-20T22:21:58Z

packages/plugin-ext/src/main/browser/tasks-main.ts

@@ -219,7 +219,7 @@ export class TasksMainImpl implements TasksMain, Disposable {
    }

    protected fromTaskConfiguration(task: TaskConfiguration): TaskDto {
-        const { group, presentation, _scope, _source, ...common } = task;
+        const { group, presentation, _scope, _source, taskType, ...common } = task;


taskType does not seem to be converted, so we can leave it as part of ...comon, right ?

tsmaeder · 2021-04-21T06:46:12Z

TaskProvider.resolveTask() is not invoked with the changes in https://github.com/eclipse-theia/theia/pull/9373/files, so it's broken in both cases.

tsmaeder · 2021-04-21T07:14:01Z

I checked that in master, the presentation options of the configured tsc: build task are not used either. So at least it's no worse.

tsmaeder · 2021-04-21T07:16:06Z

I found the following issue when testing with;
https://github.com/microsoft/vscode-extension-samples/tree/main/task-provider-sample

@alvsan09 Sorry, but I don't understand what issue you're describing here: could you give steps to reproduce?

RomanNikitenko · 2021-04-21T09:14:06Z

I checked that in master, the presentation options of the configured tsc: build task are not used either.

I believe it worked well before merging #9189.

We have a lot of regressions in master after merging #9189:

tsmaeder · 2021-04-21T11:42:32Z

I believe all of those are fixed, except #9207 (review) (for which there is a separate PR already open).

As for the presentation options not being used, IMO that only ever worked because TaskProvider#resolve() was never invoked. So it was not working, just broken in a different way.

see #9377 (comment)

alvsan09 · 2021-04-21T12:42:10Z

I found the following issue when testing with;
https://github.com/microsoft/vscode-extension-samples/tree/main/task-provider-sample

@alvsan09 Sorry, but I don't understand what issue you're describing here: could you give steps to reproduce?

Install the extension mentioned above
In theia (any project) run the plugin provided task 'custombuildscript: 32 incremental' i.e. Terminal -> Run task ...
run it twice and see that the output in terminal is not complete i.e. it does not show the expected plugin provided messages highlighted in the red square.

alvsan09 · 2021-04-21T12:57:46Z

As for the presentation options not being used, IMO that only ever worked because TaskProvider#resolve() was never invoked. So it was not working, just broken in a different way.

@tsmaeder, could you please clarify what you mean with it was broken in a different way ?
From a user's perspective it's a regression, right ?

tsmaeder · 2021-04-21T13:46:28Z

@tsmaeder, could you please clarify what you mean with it was broken in a different way ?
From a user's perspective it's a regression, right ?

Well, if you cared about configuring the presentation options, it was a regression, but if you cared about TaskProvider.resolve() bein called, it was a bugfix :-) But anyway...with my latest commits, I believe both codepaths are covered now.

alvsan09 · 2021-04-21T13:51:48Z

@tsmaeder, could you please clarify what you mean with it was broken in a different way ?
From a user's perspective it's a regression, right ?

Well, if you cared about configuring the presentation options, it was a regression, but if you cared about TaskProvider.resolve() bein called, it was a bugfix :-) But anyway...with my latest commits, I believe both codepaths are covered now.

I can confirm that the presentation options work for me taken the latest branch of this PR

tsmaeder · 2021-04-21T14:02:58Z

I've added two commits that do the following:

Reapply task customisations after calling TaskProvider.resolve(). This should take care of the case where customising a tsc: build task was not working.
Introduced the concept of the TaskExcecutionResolver. This came from the observation that we want to apply the ProcessTaskResolver to any task that has a shell or process task execution, not just the ones that have the shell|process task type. ProcessTaskResolver is responsible for doing stuff like substituting variables in for the cwd option, for example.

To test the latest change, try configuring the cwd option for a tsc: build task.

I'm re-requesting reviews, since the latest two changes are non-trivial and should be looked at.

RomanNikitenko

I tested the changes and can confirm that they fix the following issues for me:

But I still not able to run a task according to #9207 (review)
The error is: task ERROR Error resolving task 'test': TypeError: Cannot read property 'group' of undefined

alvsan09 · 2021-04-27T11:17:36Z

I found the following issue when testing with;
https://github.com/microsoft/vscode-extension-samples/tree/main/task-provider-sample

@alvsan09 Sorry, but I don't understand what issue you're describing here: could you give steps to reproduce?
* Install the extension mentioned above

* In theia (any project) run the plugin provided task 'custombuildscript: 32 incremental' i.e. Terminal -> Run task ...

* run it twice and see that the output in terminal is not complete i.e. it does not show the expected plugin provided messages highlighted in the red square.

Any news on this issue ?

vince-fugnitto · 2021-04-28T12:11:18Z

@tsmaeder with the release tentatively scheduled for tomorrow (29th), how far are we from fixing regressions caused by #9189? Should we think about preparing a revert, or perhaps delaying the release?

cc @marcdumais-work

tsmaeder · 2021-04-28T12:43:12Z

IMO, the PR as it stands now is closer to a correct implementation than reverting the change.

tsmaeder · 2021-04-28T16:01:22Z

Any news on this issue ?

The problem here is that we're not registering custom execution callbacks for custom executions added during the resolveTask phase. I have pushed a fix.

alvsan09 · 2021-04-28T17:12:12Z

Any news on this issue ?

The problem here is that we're not registering custom execution callbacks for custom executions added during the resolveTask phase. I have pushed a fix.

I can confirm that the fix is for this issue works !!, Thanks Thomas !

alvsan09

I see only a pending issue for #9207 which is still under review
I know my vote does not count for the merge, but I would like to reflect my opinion
as this fixes many outstanding issues present in master.

RomanNikitenko · 2021-04-29T06:08:23Z

As workaround for the use case #9377 (review)
we could consider these changes 2aa654d

You can use my branch for testing - it contains all commits from the current PR + my workaround.

Update:
VS Code considers that resolveTask method can return undefined, you can see it here, for example.
The documentation says the same (please see a description for resolveTask() method):

A valid default implementation for the resolveTask method is to return undefined

I created a separate issue for that problem: #9411

So, maybe more clear solution is: 5d1efe3
The branch for testing is: https://github.com/eclipse-theia/theia/compare/fixTaskResolver

tsmaeder · 2021-04-29T07:51:38Z

Just by the way...the doc for resolveTask also says that:

Note that when filling in the properties of task, you must be sure to use the exact same TaskDefinition and not create a new one. Other properties may be changed.

So the implementation in the task provider sample from the vscode-examples project is actually bogus: it returns new task objects. The fix still applies, though: customized tasks may have their execution replaced.

RomanNikitenko · 2021-04-29T08:35:56Z

@tsmaeder

Just by the way...the doc for resolveTask also says that:

Note that when filling in the properties of task, you must be sure to use the exact same TaskDefinition and not create a new one. Other properties may be changed.

So the implementation in the task provider sample from the vscode-examples project is actually bogus: it returns new task objects. The fix still applies, though: customized tasks may have their execution replaced.

Do you mean that there is no problem for the use case if it it returns:

the exact same TaskDefinition and not create a new one

?

Sorry, I didn't have a lot of time for a deep investigation, but for me it looks like it's a separate problem.
I found that resolveTask method can return undefined and tasks system doesn't take it in account.
I provided a fix for that.

It's true that there is a note in the VS Code documentation

Note that when filling in the properties of task, you must be sure to use the exact same TaskDefinition and not create a new one. Other properties may be changed.

But for me it looks like it's not related to undefined and the fix that I provided.
Please be more precise, what influence that has within Theia tasks system.
I'll really appreciate if you provide some steps to reproduce or a use case.

tsmaeder · 2021-04-29T08:38:52Z

@RomanNikitenko it's a completely unrelated problem. I just wanted to point out that we should not take the vscode samples as our bible: we should stick to what the documentation says, not what the samples do.

RomanNikitenko · 2021-04-29T08:49:44Z

@RomanNikitenko it's a completely unrelated problem. I just wanted to point out that we should not take the vscode samples as our bible: we should stick to what the documentation says, not what the samples do.

I see, thank you!

RomanNikitenko

I tested the changes and can confirm that they fix the following issues for me:

I'm able to run a task according to #9207 (review) using current PR changes + my changes: 5d1efe3, please see #9377 (comment)

tsmaeder · 2021-04-29T13:27:39Z

@colin-grant-work @RomanNikitenko @alvsan09 if no-one has any last minute objections, I would go ahead and clean up and merge this one then. Thanks for the reviews & testing.

colin-grant-work · 2021-04-29T14:23:48Z

@colin-grant-work @RomanNikitenko @alvsan09 if no-one has any last minute objections, I would go ahead and clean up and merge this one then. Thanks for the reviews & testing.

I think that sounds good to me, as long as @RomanNikitenko code suggestions are taken into account.

RomanNikitenko · 2021-04-29T16:09:50Z

as long as @RomanNikitenko code suggestions are taken into account.

Do you mean these changes 5d1efe3 ?
if so - they were not reviewed/tested.

I can not open a separate PR for them as the changes depend on the current PR.
So, maybe the true way is:

squash and merge the current PR
I align my branch with master
I open the PR for 5d1efe3
review ^
merge if it looks good

If you mean something else or have a better way - please let me know

vince-fugnitto · 2021-04-29T16:37:44Z

squash and merge the current PR

I align my branch with master

I open the PR for 5d1efe3

review ^

merge if it looks good

@RomanNikitenko @tsmaeder I propose we go forward with that approach, let's squash the current pull-request and merge (for today's release) and continue with 5d1efe3 in a separate pull-request.

Signed-off-by: Thomas Mäder <tmader@redhat.com>

tsmaeder requested review from RomanNikitenko, vince-fugnitto and colin-grant-work April 20, 2021 10:13

RomanNikitenko reviewed Apr 20, 2021

View reviewed changes

colin-grant-work reviewed Apr 20, 2021

View reviewed changes

tsmaeder mentioned this pull request Apr 20, 2021

Revert task types updates #9373

Closed

1 task

alvsan09 reviewed Apr 20, 2021

View reviewed changes

tsmaeder requested review from RomanNikitenko and colin-grant-work April 21, 2021 14:03

RomanNikitenko self-requested a review April 27, 2021 05:58

RomanNikitenko reviewed Apr 27, 2021

View reviewed changes

alvsan09 approved these changes Apr 28, 2021

View reviewed changes

vince-fugnitto added tasks issues related to the task system vscode issues related to VSCode compatibility labels Apr 28, 2021

RomanNikitenko mentioned this pull request Apr 29, 2021

TaskResolver should respect VS Code documentation #9411

Open

RomanNikitenko approved these changes Apr 29, 2021

View reviewed changes

Clean up handling of 'taskType' field

b502bb6

Signed-off-by: Thomas Mäder <tmader@redhat.com>

tsmaeder force-pushed the 9189_cleanup_task_type branch from 7047f57 to b502bb6 Compare April 29, 2021 17:01

tsmaeder merged commit 6439a6d into eclipse-theia:master Apr 29, 2021

vince-fugnitto added this to the 1.13.0 milestone Apr 29, 2021

This was referenced Apr 30, 2021

Configuring tasks are broken #9365

Open

Task type is not displayed for detected tasks #9341

Open

vince-fugnitto mentioned this pull request May 14, 2021

task: unable to run provided tasks #9366

Closed

tsmaeder mentioned this pull request May 20, 2021

8822-task-type #8860

Closed

1 task

danarad05 mentioned this pull request May 26, 2021

Task type in task definition from execution is not the original type #8822

Closed

Clean up handling of 'taskType' field #9377

Clean up handling of 'taskType' field #9377

Conversation

tsmaeder commented Apr 20, 2021

What it does

How to test

Review checklist

Reminder for reviewers

RomanNikitenko left a comment • edited Loading

Choose a reason for hiding this comment

RomanNikitenko commented Apr 20, 2021 • edited Loading

tsmaeder commented Apr 20, 2021

RomanNikitenko commented Apr 20, 2021

tsmaeder commented Apr 20, 2021

RomanNikitenko commented Apr 20, 2021 • edited Loading

tsmaeder commented Apr 20, 2021

RomanNikitenko commented Apr 20, 2021 • edited Loading

colin-grant-work left a comment

Choose a reason for hiding this comment

colin-grant-work Apr 20, 2021

Choose a reason for hiding this comment

colin-grant-work Apr 20, 2021

Choose a reason for hiding this comment

RomanNikitenko Apr 26, 2021

Choose a reason for hiding this comment

RomanNikitenko Apr 26, 2021

Choose a reason for hiding this comment

tsmaeder commented Apr 20, 2021 • edited Loading

tsmaeder commented Apr 20, 2021

alvsan09 commented Apr 20, 2021

alvsan09 commented Apr 20, 2021

alvsan09 left a comment

Choose a reason for hiding this comment

alvsan09 Apr 20, 2021

Choose a reason for hiding this comment

tsmaeder commented Apr 21, 2021

tsmaeder commented Apr 21, 2021

tsmaeder commented Apr 21, 2021

RomanNikitenko commented Apr 21, 2021 • edited Loading

tsmaeder commented Apr 21, 2021

alvsan09 commented Apr 21, 2021

alvsan09 commented Apr 21, 2021

tsmaeder commented Apr 21, 2021

alvsan09 commented Apr 21, 2021

tsmaeder commented Apr 21, 2021

RomanNikitenko left a comment • edited Loading

Choose a reason for hiding this comment

alvsan09 commented Apr 27, 2021

vince-fugnitto commented Apr 28, 2021

tsmaeder commented Apr 28, 2021

tsmaeder commented Apr 28, 2021

alvsan09 commented Apr 28, 2021

alvsan09 left a comment

Choose a reason for hiding this comment

RomanNikitenko commented Apr 29, 2021 • edited Loading

tsmaeder commented Apr 29, 2021 • edited Loading

RomanNikitenko commented Apr 29, 2021 • edited Loading

tsmaeder commented Apr 29, 2021

RomanNikitenko commented Apr 29, 2021

RomanNikitenko left a comment • edited Loading

Choose a reason for hiding this comment

tsmaeder commented Apr 29, 2021

colin-grant-work commented Apr 29, 2021

RomanNikitenko commented Apr 29, 2021

vince-fugnitto commented Apr 29, 2021

RomanNikitenko left a comment •

edited

Loading

RomanNikitenko commented Apr 20, 2021 •

edited

Loading

RomanNikitenko commented Apr 20, 2021 •

edited

Loading

RomanNikitenko commented Apr 20, 2021 •

edited

Loading

tsmaeder commented Apr 20, 2021 •

edited

Loading

RomanNikitenko commented Apr 21, 2021 •

edited

Loading

RomanNikitenko left a comment •

edited

Loading

RomanNikitenko commented Apr 29, 2021 •

edited

Loading

tsmaeder commented Apr 29, 2021 •

edited

Loading

RomanNikitenko commented Apr 29, 2021 •

edited

Loading

RomanNikitenko left a comment •

edited

Loading