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

Add task#runOptions to API. #11759

Merged
merged 2 commits into from
Nov 2, 2022
Merged

Add task#runOptions to API. #11759

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c712f75
Add `task#runOptions` to API.
sgraband Oct 12, 2022
fd36854
Address review feedback
sgraband Oct 17, 2022
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
9 changes: 9 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,15 @@

- [Previous Changelogs](https://github.com/eclipse-theia/theia/tree/master/doc/changelogs/)

## v1.32.0

- [plugin] added `Task#runOptions` field and `RunOptions` interface [#11759](https://github.com/eclipse-theia/theia/pull/11759) - Contributed on behalf of STMicroelectronics
- [tasks] added support for `reevaluateOnRerun` run option [#11759](https://github.com/eclipse-theia/theia/pull/11759) - Contributed on behalf of STMicroelectronics
Copy link
Contributor

Choose a reason for hiding this comment

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

As suggested by @tsmaeder, please add a note about the change in behavior for tasks in the breaking changes area.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Could you recheck?


<a name="breaking_changes_1.32.0">[Breaking Changes:](#breaking_changes_1.32.0)</a>

- [tasks] if the variables of a task should be reevaluated on a rerun (this was the behavior until now) the `reevaluateOnRerun` run option in the task description needs to be set to `true` from now on [#11759](https://github.com/eclipse-theia/theia/pull/11759) - Contributed on behalf of STMicroelectronics

## v1.31.0 - 10/27/2022

- [debug] added confirmation message for debug exit [#11546](https://github.com/eclipse-theia/theia/pull/11546)
Expand Down
5 changes: 5 additions & 0 deletions packages/plugin-ext/src/common/plugin-api-rpc.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1456,10 +1456,15 @@ export interface TaskDto {
group?: string;
detail?: string;
presentation?: TaskPresentationOptionsDTO;
runOptions?: RunOptionsDTO;
// eslint-disable-next-line @typescript-eslint/no-explicit-any
[key: string]: any;
}

export interface RunOptionsDTO {
reevaluateOnRerun?: boolean;
}

export interface TaskPresentationOptionsDTO {
reveal?: number;
focus?: boolean;
Expand Down
3 changes: 2 additions & 1 deletion packages/plugin-ext/src/main/browser/tasks-main.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ export class TasksMainImpl implements TasksMain, Disposable {
}

protected toTaskConfiguration(taskDto: TaskDto): TaskConfiguration {
const { group, presentation, scope, source, ...common } = taskDto ?? {};
const { group, presentation, scope, source, runOptions, ...common } = taskDto ?? {};
const partialConfig: Partial<TaskConfiguration> = {};
if (presentation) {
partialConfig.presentation = this.convertTaskPresentation(presentation);
Expand All @@ -213,6 +213,7 @@ export class TasksMainImpl implements TasksMain, Disposable {
return {
...common,
...partialConfig,
runOptions,
_scope: scope,
_source: source,
};
Expand Down
19 changes: 17 additions & 2 deletions packages/plugin-ext/src/plugin/type-converters.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,10 @@ describe('Type converters:', () => {
reveal: 3,
focus: true
},
group: groupDto
group: groupDto,
runOptions: {
reevaluateOnRerun: false
}
};

const shellTaskDtoWithCommandLine: TaskDto = {
Expand All @@ -213,7 +216,10 @@ describe('Type converters:', () => {
scope: 2,
command: commandLine,
options: { cwd },
additionalProperty
additionalProperty,
runOptions: {
reevaluateOnRerun: false
}
};

const shellPluginTask: theia.Task = {
Expand All @@ -235,6 +241,9 @@ describe('Type converters:', () => {
options: {
cwd
}
},
runOptions: {
reevaluateOnRerun: false
}
};

Expand Down Expand Up @@ -264,6 +273,9 @@ describe('Type converters:', () => {
options: {
cwd
}
},
runOptions: {
reevaluateOnRerun: false
}
};

Expand Down Expand Up @@ -291,6 +303,9 @@ describe('Type converters:', () => {
options: {
cwd
}
},
runOptions: {
reevaluateOnRerun: false
}
};

Expand Down
5 changes: 4 additions & 1 deletion 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 @@ -816,6 +816,8 @@ export function fromTask(task: theia.Task): TaskDto | undefined {
taskDto.label = task.name;
taskDto.source = task.source;

taskDto.runOptions = { reevaluateOnRerun: task.runOptions.reevaluateOnRerun };

if ((task as types.Task).hasProblemMatchers) {
taskDto.problemMatcher = task.problemMatchers;
}
Expand Down Expand Up @@ -877,10 +879,11 @@ 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, taskType, label, source, scope, problemMatcher, detail, command, args, options, group, presentation, runOptions, ...properties } = taskDto;
const result = {} as theia.Task;
result.name = label;
result.source = source;
result.runOptions = runOptions ?? {};
if (detail) {
result.detail = detail;
}
Expand Down
12 changes: 12 additions & 0 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 @@ -2057,6 +2057,7 @@ export class Task {
private taskSource: string;
private taskGroup: TaskGroup | undefined;
private taskPresentationOptions: theia.TaskPresentationOptions;
private taskRunOptions: theia.RunOptions;
constructor(
taskDefinition: theia.TaskDefinition,
scope: theia.WorkspaceFolder | theia.TaskScope.Global | theia.TaskScope.Workspace,
Expand Down Expand Up @@ -2229,6 +2230,17 @@ export class Task {
}
this.taskPresentationOptions = value;
}

get runOptions(): theia.RunOptions {
return this.taskRunOptions;
}

set runOptions(value: theia.RunOptions) {
if (value === null || value === undefined) {
value = Object.create(null);
}
this.taskRunOptions = value;
}
}

@es5ClassCompat
Expand Down
15 changes: 15 additions & 0 deletions packages/plugin/src/theia.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11532,6 +11532,16 @@ export module '@theia/plugin' {
clear?: boolean;
}

/**
* Run options for a task.
*/
export interface RunOptions {
/**
* Controls whether task variables are re-evaluated on rerun.
*/
reevaluateOnRerun?: boolean;
}

export class Task {

/**
Expand Down Expand Up @@ -11618,6 +11628,11 @@ export module '@theia/plugin' {
* array.
*/
problemMatchers?: string[];

/**
* Run options for the task
*/
runOptions: RunOptions;
}

/**
Expand Down
2 changes: 1 addition & 1 deletion packages/task/src/browser/task-configurations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,7 @@ export class TaskConfigurations implements Disposable {
console.error('Detected / Contributed tasks should have a task definition.');
return;
}
const customization: TaskCustomization = { type: task.type };
const customization: TaskCustomization = { type: task.type, runOptions: task.runOptions };
definition.properties.all.forEach(p => {
if (task[p] !== undefined) {
customization[p] = task[p];
Expand Down
25 changes: 16 additions & 9 deletions packages/task/src/browser/task-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,13 +87,18 @@ export interface TaskEndedInfo {
value: number | boolean | undefined
}

export interface LastRunTaskInfo {
resolvedTask?: TaskConfiguration;
option?: RunTaskOption
}

@injectable()
export class TaskService implements TaskConfigurationClient {

/**
* The last executed task.
*/
protected lastTask: { source: string, taskLabel: string, scope: TaskConfigurationScope } | undefined = undefined;
protected lastTask: LastRunTaskInfo = {resolvedTask: undefined, option: undefined};
protected cachedRecentTasks: TaskConfiguration[] = [];
protected runningTasks = new Map<number, {
exitCode: Deferred<number | undefined>,
Expand Down Expand Up @@ -470,7 +475,7 @@ export class TaskService implements TaskConfigurationClient {
*
* @returns the last executed task or `undefined`.
*/
getLastTask(): { source: string, taskLabel: string, scope: TaskConfigurationScope } | undefined {
getLastTask(): LastRunTaskInfo {
return this.lastTask;
}

Expand All @@ -496,11 +501,14 @@ export class TaskService implements TaskConfigurationClient {
* @param token The cache token for the user interaction in progress
*/
async runLastTask(token: number): Promise<TaskInfo | undefined> {
if (!this.lastTask) {
if (!this.lastTask?.resolvedTask) {
return;
}
const { source, taskLabel, scope } = this.lastTask;
return this.run(token, source, taskLabel, scope);
if (!this.lastTask.resolvedTask.runOptions?.reevaluateOnRerun) {
Copy link
Contributor

@colin-grant-work colin-grant-work Oct 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

If this flag is set to false no variables will be resolved again. Therefore lastTask can now be re-triggered without resolving all variables.

It appears that reevaluateOnRerun = false is being treated as the default. That's a change in the expectations for internal tasks, which in the past would always have been resolved when rerun. Is there any value in treating internal and external tasks differently in this respect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes i was wondering this as well. I would find it odd, if the default behavior is reevaluateOnRerun = true as this would mean, that we need to interpret missing options as true. I am not sure, how internal tasks are handled, but would it be a option to add this runOption to all internal tasks? This would keep the current behavior and explicitly explain for each task how it should be handled. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

@colin-grant-work by "internal", do you mean tasks defined in tasks.json or contributed by non-plugin API? If we need to treat tasks different, I would much prefer putting the difference where we create the tasks (for example by setting a flag in the contructor) and not where we process the tasks. Locality of reference for teh win!

Copy link
Contributor

Choose a reason for hiding this comment

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

Re: tasks.json, we should likely follow what VSCode does, though there's scope for making our own choice there. Mostly I mean other tasks created internally, but I'm not very familiar with the behavior or UI of 'rerun' tasks (as opposed to running the same task again, but not as a rerun), so I'm not sure how disruptive it would be to change from resolving to not resolving variables in the task description.

One solution might be to modify the plugin system to supply false by default for tasks originating from plugins, and then have a flag in the task service for whether to use false or true as the default value there so that application implementers can easily toggle it if they have a use case where true is appropriate. On the other hand, it sounds like @tsmaeder favors something on the task description itself (in which case maybe setting false in the plugin system and using true as a default otherwise would be the best course?). In any case, I don't know if it really warrants too much concern.

Copy link
Contributor

@tsmaeder tsmaeder Oct 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I thought about this some more: what I really would like to avoid is a "if tasksource = plugin, do x, else do y". I'm also not a fan of setting variables that are client API in order to simulate the "right" default behaviour: if anything, we should set a flag : task.defaultRerunOnReevaluate= false that is not part of the API in the plugin task provider implementation and default to true in clients to preserve the current behaviour.
But t.b.h, I would just make the change to the VS Code behavior: it's a breaking change, and while we try not to frivolously break API, we also should not unduly complicate the system to keep the API stable.
My vote is: put the code in as it is and make a note of it in the "breaking changes". Clients can trivially update their code if they rely on the behaviour. @colin-grant-work can you live with that?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with this approach.

return this.runResolvedTask(this.lastTask.resolvedTask, this.lastTask.option);
}
const { _source, label, _scope } = this.lastTask.resolvedTask;
return this.run(token, _source, label, _scope);
}

/**
Expand Down Expand Up @@ -737,7 +745,7 @@ export class TaskService implements TaskConfigurationClient {
try {
// resolve problemMatchers
if (!option && task.problemMatcher) {
const customizationObject: TaskCustomization = { type: task.taskType, problemMatcher: task.problemMatcher };
const customizationObject: TaskCustomization = { type: task.taskType, problemMatcher: task.problemMatcher, runOptions: task.runOptions };
const resolvedMatchers = await this.resolveProblemMatchers(task, customizationObject);
option = {
customization: { ...customizationObject, ...{ problemMatcher: resolvedMatchers } }
Expand Down Expand Up @@ -949,7 +957,7 @@ export class TaskService implements TaskConfigurationClient {
}

protected async getTaskCustomization(task: TaskConfiguration): Promise<TaskCustomization> {
const customizationObject: TaskCustomization = { type: '', _scope: task._scope };
const customizationObject: TaskCustomization = { type: '', _scope: task._scope, runOptions: task.runOptions };
const customizationFound = this.taskConfigurations.getCustomizationForTask(task);
if (customizationFound) {
Object.assign(customizationObject, customizationFound);
Expand Down Expand Up @@ -982,12 +990,11 @@ export class TaskService implements TaskConfigurationClient {
* @param option options to run the resolved task
*/
protected async runResolvedTask(resolvedTask: TaskConfiguration, option?: RunTaskOption): Promise<TaskInfo | undefined> {
const source = resolvedTask._source;
const taskLabel = resolvedTask.label;
let taskInfo: TaskInfo | undefined;
try {
taskInfo = await this.taskServer.run(resolvedTask, this.getContext(), option);
this.lastTask = { source, taskLabel, scope: resolvedTask._scope };
this.lastTask = {resolvedTask, option };
this.logger.debug(`Task created. Task id: ${taskInfo.taskId}`);

/**
Expand Down
6 changes: 6 additions & 0 deletions packages/task/src/common/task-protocol.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,8 @@ export interface TaskCustomization {
/** The order the dependsOn tasks should be executed in. */
dependsOrder?: DependsOrder;

runOptions?: RunOptions;

// eslint-disable-next-line @typescript-eslint/no-explicit-any
[name: string]: any;
}
Expand Down Expand Up @@ -228,6 +230,10 @@ export interface RunTaskOption {
customization?: TaskCustomizationData;
}

export interface RunOptions {
reevaluateOnRerun?: boolean;
}

/** Event sent when a task has concluded its execution */
export interface TaskExitedEvent {
readonly taskId: number;
Expand Down