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

Enable Workspace-Scoped Tasks #8917

Conversation

colin-grant-work
Copy link
Contributor

@colin-grant-work colin-grant-work commented Jan 4, 2021

What it does

Fixes #8519
Fixes #8991 (thanks for noting that, @RomanNikitenko)
This PR sets up the necessary infrastructure to read and write tasks to a workspace file. In particular it:

  • modifies the way a WorkspaceFilePreferenceProvider reads and writes non-settings preferences. (breaking changes to bring Theia's workspace file schema into harmony with VSCode's)
  • modifies helper methods in workspace-service.ts for writing workspace files to accommodate fields other than folders and settings (breaking changes for anyone calling those file-builder methods)
  • removes some explicit references to the preference system in the workspace package.
  • adds a WorkspaceSchemaUpdater service to allow packages that contribute extensions to the schema of the workspace file (tasks, debug) to register their extensions
  • injects workspace scope related functionality into the TaskConfigurationManager

How to test

  1. Add a properly formatted tasks field to a workspace file. ("tasks": {"tasks": [...]} - has be double "tasks", one for the 'file' label (tasks.json) and one for the tasks inside)
  2. Open that file as your workspace.
  3. Open the Terminal menu and select Run Task...
  4. Observe that your tasks are available and run (or fail) as expected
  5. Modify the file (e.g. add a new task configuration)
  6. Repeat 3-4, and observe that the task list updated correctly.

  1. Use task quick picks (commands tasks configure, tasks run) and confirm that they still work in each scope in single- and multi-root workspaces.

NB: running very short-lived tasks (e.g. a shell task that just runs echo) can behave unexpectedly, with no terminal ever opening. This behavior is also found on master.

Changes

This PR has grown to touch quite a number of files, so it might be useful for reviewers if I explain why each change was necessary:

  • Core
    • preference-configurations.ts: add an isAnyConfig convenience method equivalent to checking isSectionName() || isConfigName()
    • preference-contribution.ts: change schema update logic to respect the superset / subset relationships among scopes. See Enable Workspace-Scoped Tasks #8917 (comment).
    • preference-provider.ts, preference-service.ts: add section name argument to getConfigUri and getContainingConfigUri to allow callers to find the URI of all preference files, not just settings.json. This is more systematic than the current implementation, and obviates workarounds using PreferenceService.resolve to find the URI for non-settings sections.
  • Debug
    • debug-schema-updater.ts: add launch to the workspace file schema.
  • Preferences
    • folders-preference-provider.ts, user-configs-preference-provider.ts, workspace-preference-provider: update getConfigUri and getContainingConfigUri to accommodate section name.
    • section-preference-provider.ts: minor cleanup to remove a non-null assertion.
    • workspace-file-preference-provider.ts: changes to handle sections other than settings, and to accommodate sections both inside the settings object and outside.
  • Tasks
    • quick-open-tasks.ts: refactor the configure method to accommodate workspace-scoped tasks.
    • task-configuration-manager.ts: rewrite logic to accommodate workspace-scoped tasks.
    • task-configuration-model.ts: make scope protected because in the case of workspace scope it becomes unreliable (sometimes pointing to folder, sometimes pointing to workspace file). It wasn't being accessed, and the only way to get access to a model is to know the scope you're interested in, so you shouldn't have to ask the model for that information.
    • task-configurations.ts: change to allow configure to work in workspace scope.
    • task-preferences.ts: better default value for preferences.
    • task-schema-updater.ts: make the basic schema for tasks publicly accessible (it is for preferences) so that it can be used for any defaulting. Also add tasks the workspace-file schema.
  • Workspace
    • workspace-commands.ts, workspace-frontend-contribution.ts: add a command to open the workspace file. It exists in VSCode and is parallel to commands to open e.g. the user-scope tasks.json.
    • workspace-schema-updater.ts, workspace-frontend-module.ts, workspace-service.ts: add a WorkspaceSchemaUpdater that can be accessed and updated by external packages (debug, tasks, preferences) to allow them to register their subschemas for the workspace file. Modify helper functions that assume that the schema will only ever include settings.

Review checklist

Reminder for reviewers

@colin-grant-work colin-grant-work added tasks issues related to the task system multi-root issues related to multi-root support labels Jan 4, 2021
@colin-grant-work colin-grant-work marked this pull request as draft January 4, 2021 19:01
@colin-grant-work colin-grant-work force-pushed the feature/workspace-tasks branch 6 times, most recently from 4aa6c90 to c88b5db Compare January 6, 2021 22:40
@colin-grant-work
Copy link
Contributor Author

@tsmaeder, I think you took the last thorough pass through the tasks system - I wonder if you'd be willing to take a look at this? I've made some changes to logic in the TaskConfigurationManager that may have some side effects that I haven't anticipated. In particular, there were some fallbacks to user scope if a find failed in folder scope, and I've removed that behavior since it seemed odd to tell a caller that tasks were in one scope when they were in another, but maybe that was hasty of me.

@colin-grant-work colin-grant-work marked this pull request as ready for review January 6, 2021 23:23
Copy link
Contributor

@kenneth-marut-work kenneth-marut-work left a comment

Choose a reason for hiding this comment

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

This seems to be working for me under the testing steps outlined and through experimenting with various other workspace configurations and setups. I haven't found any issues with your updates except that I'm not getting any sort of auto-suggest behavior inside the workspace file for tasks. Tried to track it down but was unsuccessful, not sure why that behavior isn't being enabled.

Otherwise had some minor comments 👌

@tsmaeder
Copy link
Contributor

@colin-grant-work am I understanding correctly that this would also enable workspace-level launch configurations, not just tasks?

@colin-grant-work
Copy link
Contributor Author

@tsmaeder, workspace-scoped launch configurations are actually already supported, but they're formatted differently than VSCode expects. Instead of

{
"settings": {...}
"launch": {"configurations": [...]}
}

The current system will do something like this:

{
"settings": {
  "launch": {...}
  }
}

This PR would adjust the way launch preferences are handled to agree with the format of VSCode's workspace file.

@colin-grant-work colin-grant-work force-pushed the feature/workspace-tasks branch 2 times, most recently from 224091a to d205c81 Compare January 20, 2021 16:55
@colin-grant-work
Copy link
Contributor Author

@kenneth-marut-work, thanks for the review. I've implemented your suggestions and tracked down the bug with the auto-suggestion functionality.

I haven't found any issues with your updates except that I'm not getting any sort of auto-suggest behavior inside the workspace file for tasks. Tried to track it down but was unsuccessful, not sure why that behavior isn't being enabled.

Turned out there was a race condition in the schema update logic. I've implemented a queue to handle it, and you should see suggestions for tasks, launch, folders (if it isn't already present), and settings.

@RomanNikitenko
Copy link
Contributor

workspace-scoped launch configurations are actually already supported, but they're formatted differently than VSCode expects.

Looks like VS Code supports both formats

settings_launch

@colin-grant-work
Copy link
Contributor Author

@RomanNikitenko, good catch. That's a bit annoying. The good news is (and please confirm that you also see this behavior), that you can do it only one way at a time. If there are launch configs outside settings, they take precedence over those inside settings:

image

image

Arranging for either...or is much easier than arranging for both...and. I'll modify the code accordingly.

@colin-grant-work
Copy link
Contributor Author

I've pushed code that should replicate the behavior seen in VSCode: it will read sections (launch, tasks) that exist inside settings, and if the user programmatically modifies those sections, it will perform the modifications inside settings. If it finds sections outside settings, it will favor that data over data inside settings, and any programmatic modifications will target the independent section.

Copy link
Contributor

@RomanNikitenko RomanNikitenko left a comment

Choose a reason for hiding this comment

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

I tested a little the changes and can confirm - I'm able to run tasks configurations defined in the workspace configuration file - @colin-grant-work thanks a lot for such cool feature!

I faced with some issues within testing the PR, for example:

  • I got the error Global task cannot be customized at attempt to apply Configure Task action to a workspace level task. You could try reproduce it by pressing the gear icon from Terminal => Run task menu
  • Looks like workspace level tasks are not available from Terminal => Configure Tasks menu

@colin-grant-work
could you take a look when you have a chance

@RomanNikitenko
Copy link
Contributor

I noticed some strange behavior related to launch configurations.

Steps to reproduce:

  • multi-root project with 2 projects, I have theia and vscode, for example
  • add a launch config on workspace level
  • check that it's available for running for both projects from Debug panel
  • add another configuration to theia/.theia/launch.json file
  • go to the Debug panel again and check if the workspace level config is still available for running

As result I can see that the workspace launch config is available for running only for one project.

@colin-grant-work
Copy link
Contributor Author

colin-grant-work commented Jan 25, 2021

@RomanNikitenko, thanks for taking another look. I will take a closer look at what's happening with the Configure Task logic and fix that up.

I noticed some strange behavior related to launch configurations.
...

I'm seeing the same behavior on master. I'll check it out a bit, but if it's going to take much messing with the debug-internal logic, I'm probably inclined to keep it separate from this PR.

@RomanNikitenko
Copy link
Contributor

RomanNikitenko commented Jan 25, 2021

@RomanNikitenko, good catch. That's a bit annoying. The good news is (and please confirm that you also see this behavior), that you can do it only one way at a time. If there are launch configs outside settings, they take precedence over those inside settings:

@colin-grant-work
Yes, I see the same behavior that you mentioned.
I added comment about it here: #8836 (comment)

@colin-grant-work
Copy link
Contributor Author

@danarad05, I'm having some trouble, and it looks like you're the most recent person to touch the code. It looks like #8761 added this code to the PreferenceSchemaProvider:

private updateSchemaProps(key: string, property: PreferenceDataProperty): void {
this.combinedSchema.properties[key] = property;
switch (property.scope) {
case PreferenceScope.Workspace:
this.workspaceSchema.properties[key] = property;
break;
case PreferenceScope.Folder:
this.folderSchema.properties[key] = property;
break;
}
}

The tasks preferences are added in 'resource' (i.e. PreferenceScope.Folder) scope, and so they are not being added to the workspace-scoped schema. That means that I can get auto-complete help if I put tasks outside the settings object in the workspace file, but not inside settings, because tasks isn't in the settings schema for workspace files. Did you have a strategy in mind for preferences that should be added to both scopes, or should I cook one up?

@colin-grant-work
Copy link
Contributor Author

@kenneth-marut-work, thanks for taking another look. I think I've addressed the issue of the strange opening behavior. A funky workaround had been employed to trick the PreferenceService into revealing the URI of the file that needed to be made or modified, and that workaround involved setting tasks to {}. (the empty string in "": {} was my fault). Instead of that workaround, I've modified PreferenceService.getConfigUri to accept a sectionName argument (e.g. tasks) so that you can retrieve the URI of any preference file directly.

@colin-grant-work
Copy link
Contributor Author

@RomanNikitenko

Another issue (minor) is related to task schema support for Workspace-scoped tasks: task system suggests me all known types when I'm creating a new task on Workspace level. AFAIK it should be only shell or process task type for Workspace scoped tasks. But it's OK for me to create a separate issue for that problem.

That sounds reasonable, and it'll mean modifying the TasksSchemaUpdater to maintain multiple schemas, just like the PreferenceSchemaProvider maintains user-, workspace-, and folder-scoped schemas. If you're willing to defer that to another issue, I'd prefer that, as this one is already touching quite a bit.

@RomanNikitenko
Copy link
Contributor

@colin-grant-work
thanks for the update - I'm going to test the changes ASAP.

About the task type limitation for User level and Workspace scoped tasks - I created the separate issue: #9009

Please feel free to edit it or add other required info.
thanks!

Copy link
Contributor

@RomanNikitenko RomanNikitenko left a comment

Choose a reason for hiding this comment

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

I tested the following for a multi-root workspace:

  • Open Workspace Configuration File command
  • adding a task to the workspace config file
  • task schema support for Workspace scoped tasks
  • Problems view when there is an error in a Workspace scoped task configuration
  • Configure Task action
  • Configure Task => Configure new task
  • running a workspace task
  • terminate a workspace task
  • restart a workspace task
  • modify a workspace task configuration and check if the changes is applied for running

I also tested launch configs related logic:

  • adding a launch configuration inside and outside of settings field of the workspace config file
  • schema support for launch configurations at a configuration creation

It works well for me, except #8987 and #9009. But it's out of the scope of the issue.

The PR also fixes #8991

@colin-grant-work thanks a lot!

@RomanNikitenko
Copy link
Contributor

@kenneth-marut-work
could you review again when you have a chance #8917 (comment)
thanks in advance!

@kenneth-marut-work
Copy link
Contributor

@colin-grant-work
Looks great, tested again and could not find any issues other than those @RomanNikitenko mentioned. I also confirmed that the empty string issue "": {} is gone.

Thanks for the thorough description of changes above. Ended up touching a lot of files, but excited for this new addition.
LGTM, and don't forget the changelog entry

@colin-grant-work
Copy link
Contributor Author

@vince-fugnitto, @marechal-p, would one of you be willing to take a look at this to make sure I haven't made any unwise changes to API? The changes to the getConfigUri method, WorkspaceService helper methods, and WorkspaceFilePreferenceProvider should definitely go in the CHANGELOG as (potentially) breaking, and I'm happy to add them, but it would be good to a checkon the code with an eye to larger code base, as well.

Copy link
Contributor

@alvsan09 alvsan09 left a comment

Choose a reason for hiding this comment

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

It looks pretty good, I just spotted a regression that should be easy to fix.

It would also be good if you re-base it to latest master, as new configurations will now include a label and problem matchers if applicable.

Regards

packages/task/src/browser/quick-open-task.ts Outdated Show resolved Hide resolved
packages/task/src/browser/quick-open-task.ts Show resolved Hide resolved
@colin-grant-work colin-grant-work force-pushed the feature/workspace-tasks branch 2 times, most recently from b597a78 to 42ceb5b Compare February 22, 2021 16:11
@colin-grant-work
Copy link
Contributor Author

If there are no objections, I'll merge this tomorrow morning. @alvsan09, are you happy with it as is?

@alvsan09
Copy link
Contributor

alvsan09 commented Mar 1, 2021

Hi @colin-grant-work ,
Yes I am fine with the updates, and it looks good to be merged !
thanks !
Regards

Signed-off-by: Colin Grant <colin.grant@ericsson.com>
@colin-grant-work colin-grant-work merged commit a20b26d into eclipse-theia:master Mar 2, 2021
@colin-grant-work colin-grant-work deleted the feature/workspace-tasks branch March 2, 2021 15:15
protected readonly toDisposeOnDelegateChange = new DisposableCollection();
protected updateWorkspaceModel(): void {
const newDelegate = this.workspaceService.saved ? this.workspacePreferences : this.folderPreferences;
const effectiveScope = this.workspaceService.saved ? TaskScope.Workspace : this.workspaceService.tryGetRoots()[0]?.resource.toString();
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
Hello!
Could you share what use case here is handled.

I thought that the use case is Untittled (Workspace) - so unsaved:
untitled

but I get newDelegate = this.workspacePreferences and effectiveScope = TaskScope.Workspace for such use case.

What steps I should do to get:

  • newDelegate = this.folderPreferences;
  • effectiveScope = this.workspaceService.tryGetRoots()[0]?.resource.toString();

Thanks in advance!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey, @RomanNikitenko! As soon as a workspace file exists, the workspace is considered .saved, even if you haven't officially named it and saved it yourself. The case in which the delegate is folder preference is in a single-root unsaved workspace - i.e. the case in which the user uses 'Open folder...' to open a single folder as the workspace.

There was a slight hiccup with this logic where the TaskConfigurationManager wasn't switching to the workspace preferences delegate as soon as a workspace file was created, but it that was fixed recently in #9331. It looks like you're seeing that change in action: as soon as you have a workspace file, whether you've titled it or not, the TaskConfigurationManager starts considering the workspace file to be the source of information for workspace-scoped tasks.

Copy link
Contributor

@RomanNikitenko RomanNikitenko Apr 21, 2021

Choose a reason for hiding this comment

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

thank you for the quick answer!
one more question:

The case in which the delegate is folder preference is in a single-root unsaved workspace - i.e. the case in which the user uses 'Open folder...' to open a single folder as the workspace.

If user has a single folder - then tasks are considered on the folder level and should be handled here
So, sorry - I still don't see what use case we handle here.

I'm not saying that there is a problem - I'm investigating some area related to multi-root workspace and have found this place - so I just want to clarify it for myself.

Maybe you could provide steps to reproduce, like:

  • open a folder
  • ....

thanks!

Copy link
Contributor Author

@colin-grant-work colin-grant-work Apr 21, 2021

Choose a reason for hiding this comment

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

You're right that all folder-scoped tasks, in both saved and unsaved workspaces, are handled by the folder preference provider. In the case of a single-root unsaved workspace, workspace scope also points to folder scope for the single folder. This parallels the preference system, which also collapses folder and workspace scope in the case of a single-root unsaved workspace. Since I'm not sure what your use case is, I'm not sure what steps to provide to 'reproduce' any particular behavior, but if you want to see how it works, you can do something like this:

  1. Create a command that does something like this:
{
    execute: () => {
        console.log("User scope:", this.taskConfigurationManager.getTasks(TaskScope.Global));
        console.log("Workspace scope:", this.taskConfigurationManager.getTasks(TaskScope.Workspace));
        for (const root of this.workspaceService.tryGetRoots()) {
            console.log(`Folder scope (${root.resource.path.base}):`, this.taskConfigurationManager.getTasks(root.resource.toString()));
        }
    }
}

Try that command in different kinds of workspaces. In a single-root unsaved workspace, the logs for workspace and folder scope should be the same (same _scope fields, too). In a saved workspace, they should be different. There aren't very many services that interface directly with the TaskConfigurationManager - mostly its output gets filtered through the TaskService, which is able to identify the fact that workspace scope and folder scope are duplicated in a single-root workspace (because the label and _scope fields are identical), so consumers of the data end up seeing what they should see: one set of folder-scoped tasks.

Conceivably, in a single-root unsaved workspace, we could just return an empty array for workspace tasks, but I prefer to keep the behavior of the various preference-related functionality areas consistent. Since the preferences service collapses workspace scope and folder scope in single-root workspaces, I think tasks (and launch), should do so, as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

thank you for the detailed answer.

I think I was confused for the single-root use case(user have opened just a single folder) because
before your explanation I thought that:

  • for the single-root use case tasks should be handled here on the Folder level - there is no need to create TaskConfigurationModel for the Workspace scope.
  • there should be no Workspace scoped tasks for the such use case - as they are duplicated - in the example that you provided above taskConfigurationManager returns the same tasks for the Workspace scope and for the Folder level

Thanks again for your time and efforts - now it's more clear for me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
multi-root issues related to multi-root support tasks issues related to the task system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Workspace configuration using API works incorrectly Add "tasks" to .theia-workspace file
6 participants