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.

Sign up for GitHub

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

Jump to bottom

Revert task types updates #9373

Closed
wants to merge 3 commits into from
Closed

Revert task types updates #9373

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2097d7b
Revert "Consider taskConfig.type when running tasks."
alvsan09 Apr 17, 2021
be9b008
Revert "Use task.type first to look up task runner"
alvsan09 Apr 17, 2021
77babff
Revert "Fix task type is modified."
alvsan09 Apr 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/plugin-ext/src/plugin/tasks/tasks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ export class TasksExtImpl implements TasksExt {
// If this task is a custom execution, then we need to save it away
// in the provided custom execution map that is cleaned up after the
// task is executed.
if (CustomExecution.is(task.execution!)) {
if (taskDto.type === 'customExecution') {
this.addCustomExecution(taskDto, false);
}
const executionDto = await this.proxy.$executeTask(taskDto);
Expand All @@ -177,7 +177,7 @@ export class TasksExtImpl implements TasksExt {
return adapter.provideTasks(token).then(tasks => {
if (tasks) {
for (const task of tasks) {
if (task.type === 'customExecution' || task.taskType === 'customExecution') {
if (task.type === 'customExecution') {
this.addCustomExecution(task, true);
}
}
Expand Down
11 changes: 6 additions & 5 deletions packages/plugin-ext/src/plugin/type-converters.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -750,15 +750,15 @@ export function fromTask(task: theia.Task): TaskDto | undefined {
return taskDto;
}

if (taskDefinition.taskType === 'shell' || types.ShellExecution.is(execution)) {
if (taskDefinition.type === 'shell' || types.ShellExecution.is(execution)) {
return fromShellExecution(<theia.ShellExecution>execution, taskDto);
}

if (taskDefinition.taskType === 'process' || types.ProcessExecution.is(execution)) {
if (taskDefinition.type === 'process' || types.ProcessExecution.is(execution)) {
return fromProcessExecution(<theia.ProcessExecution>execution, taskDto);
}

if (taskDefinition.taskType === 'customExecution' || types.CustomExecution.is(execution)) {
if (taskDefinition.type === 'customExecution' || types.CustomExecution.is(execution)) {
return fromCustomExecution(<theia.CustomExecution>execution, taskDto);
}

Expand All @@ -770,7 +770,7 @@ export function toTask(taskDto: TaskDto): theia.Task {
throw new Error('Task should be provided for converting');
}

const { type, taskType, label, source, scope, problemMatcher, detail, command, args, options, group, presentation, ...properties } = taskDto;
const { type, label, source, scope, problemMatcher, detail, command, args, options, group, presentation, ...properties } = taskDto;
const result = {} as theia.Task;
result.name = label;
result.source = source;
Expand All @@ -788,8 +788,9 @@ export function toTask(taskDto: TaskDto): theia.Task {
result.scope = scope;
}

const taskType = type;
const taskDefinition: theia.TaskDefinition = {
type: type
type: taskType
};

result.definition = taskDefinition;
Expand Down
11 changes: 7 additions & 4 deletions packages/plugin-ext/src/plugin/types-impl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1911,18 +1911,21 @@ export class Task {
private updateDefinitionBasedOnExecution(): void {
if (this.taskExecution instanceof ProcessExecution) {
Object.assign(this.taskDefinition, {
type: 'process',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should ever update the type field: in Eclipse Che, we register a TaskRunnerfor the task type 'che'. Once the type field is overwritten, we can't get it back.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This update has been there for two years before the introduction of customExecution,
I am trying to understand how this has not caused problems for 'che' then ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thomas, I think @alvsan09 is right: I provided these changes 2 years ago, please see

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alvsan09 It's not a problem if you don't have an execution set in the task (which happens to be the case in Che), but otherwise, you'll never match the task type. So while the problem might not immediately manifest, it's still there.
Generally, I think #9377 offers a cleaner way forward.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tsmaeder, this PR would be a reset to base functionality in case a more complete solution is not available in time,
I will test and review #9377

id: this.taskExecution.computeId(),
taskType: 'process'
taskType: this.taskDefinition!.type
});
} else if (this.taskExecution instanceof ShellExecution) {
Object.assign(this.taskDefinition, {
type: 'shell',
id: this.taskExecution.computeId(),
taskType: 'shell'
taskType: this.taskDefinition!.type
});
} else if (this.taskExecution instanceof CustomExecution) {
Object.assign(this.taskDefinition, {
id: this.taskDefinition.id ? this.taskDefinition.id : this.taskExecution.computeId(),
taskType: 'customExecution'
type: 'customExecution',
id: this.taskExecution.computeId(),
taskType: this.taskDefinition!.type
});
} else {
Object.assign(this.taskDefinition, {
Expand Down
3 changes: 1 addition & 2 deletions packages/task/src/browser/process/process-task-resolver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,7 @@ export class ProcessTaskResolver implements TaskResolver {
* sane default values. Also, resolve all known variables, e.g. `${workspaceFolder}`.
*/
async resolveTask(taskConfig: TaskConfiguration): Promise<TaskConfiguration> {
const type = taskConfig.taskType || taskConfig.type;
if (type !== 'process' && type !== 'shell') {
if (taskConfig.type !== 'process' && taskConfig.type !== 'shell') {
throw new Error('Unsupported task configuration type.');
}
const context = typeof taskConfig._scope === 'string' ? new URI(taskConfig._scope) : undefined;
Expand Down
2 changes: 1 addition & 1 deletion packages/task/src/node/process/process-task-runner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ export class ProcessTaskRunner implements TaskRunner {
*/
let commandLine: string | undefined;

if ((taskConfig.taskType || taskConfig.type) === 'shell') {
if (taskConfig.type === 'shell') {
// When running a shell task, we have to spawn a shell process somehow,
// and tell it to run the command the user wants to run inside of it.
//
Expand Down
18 changes: 6 additions & 12 deletions packages/task/src/node/task-runner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,20 +84,14 @@ export class TaskRunnerRegistry {
}

/**
* Looks for a registered {@link TaskRunner} for each of the task types in sequence and returns the first that is found
* If no task runner is registered for any of the types, the default runner is returned.
* @param types the task types.
* Retrieves the {@link TaskRunner} registered for the specified Task type.
* @param type the task type.
*
* @returns the registered {@link TaskRunner} or a default runner if none is registered for the specified types.
* @returns the registered {@link TaskRunner} or a default runner if none is registered for the specified type.
*/
getRunner(...types: string[]): TaskRunner {
for (const type of types) {
const runner = this.runners.get(type);
if (runner) {
return runner;
}
}
return this.defaultRunner;
getRunner(type: string): TaskRunner {
const runner = this.runners.get(type);
return runner ? runner : this.defaultRunner;
}

/**
Expand Down
2 changes: 1 addition & 1 deletion packages/task/src/node/task-server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ export class TaskServerImpl implements TaskServer, Disposable {
}

async run(taskConfiguration: TaskConfiguration, ctx?: string, option?: RunTaskOption): Promise<TaskInfo> {
const runner = this.runnerRegistry.getRunner(taskConfiguration.type, taskConfiguration.taskType);
const runner = this.runnerRegistry.getRunner(taskConfiguration.type);
const task = await runner.run(taskConfiguration, ctx);

if (!this.toDispose.has(task.id)) {
Expand Down