Improve multi workspace handling #62

doudou · 2019-02-28T14:25:51Z

This pull request improves having multiple Autoproj workspaces in a single VSCode workspace. In addition, it workarounds an issue related to VSCode's handling of the very first folder in the workspace.

First the vscode issue. VSCode will restart the extension hosts if the toplevel folder in a VSCode workspace changes (because it needs to change the now-obsolete rootPath property). This leads to the extension attempting to start a Watch while there is already one. The first attempt was to make sure we deregister the watch task using the new Task execution APIs. This is a great addition in the first place, but proved fruitless as it seems that vscode doesn't shut down the extensions cleanly (I wouldn't get the extension's deactivate function or any of the dispose methods to be called). I then tried to use the taskExecutions property to find the watch task, but it is also empty - don't really know why.

In fine, I tried to see a way to make sure the first folder doesn't change this much. This led me to think about a problem I recently had, to use two autoproj workspaces in the same vscode workspace (comparing code). This was hell, as the packages would be named the same.

So, I went for:

always having the workspace's autoproj folder added to the vscode workspace, and name it as the workspace (e.g. autoproj (squidbot)).
use the autoproj folder as a guard, i.e. folders for the squidbot workspace would be placed after the autoproj (squidbot) folder. Moreover, they would be placed before the following folder that is not part of the workspace project.

Because the folder names can't be changed after they have been added, I also created a Rock: Add Workspace command for the initial add. The one corner case is if someone adds the autoproj folder directly.

Illustration:

…pace

This should improve the interaction between an autoproj workspace and additional folders and/or other workspaces. The folder packages are now added *after* the autoproj build configuration, and *before* a possible follow-up package that would not be part of the workspace. It ensures that the packages of a given workspace are grouped together, and that two open workspaces would not be interleaved.

The issue here was that the "autoproj (_WORKSPACE_NAME_)" name cannot be given after VSCode added the folder (e.g. cannot be done if the user uses "Add folder to workspace"). Create a "Add workspace" command for this.

…e's is undefined The "folders is undefined" is an artifact of VSCode API pre-multi root workspaces. We shouldn't need to deal with it. Since we have our own shim to VSCode's API, make it return an empty array when vscode's folders is undefined.

…t code Having it in the "Add package to workspace" code makes no sense. We really want to auto-add when a new folder gets added. This ensures a smooth-y transition to the Add Workspace command.

coveralls · 2019-02-28T14:37:49Z

Pull Request Test Coverage Report for Build 70

70 of 129 (54.26%) changed or added relevant lines in 7 files are covered.
6 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.08%) to 79.766%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/config.ts	0	1	0.0%
src/context.ts	1	2	50.0%
src/wrappers.ts	0	4	0.0%
src/autoproj.ts	2	7	28.57%
src/extension.ts	0	12	0.0%
src/commands.ts	35	48	72.92%
src/vscode_workspace_manager.ts	32	55	58.18%

Files with Coverage Reduction	New Missed Lines	%
src/extension.ts	6	0.0%

Totals
Change from base Build 65:	0.08%
Covered Lines:	1010
Relevant Lines:	1258

💛 - Coveralls

g-arjones · 2019-02-28T14:45:41Z

This leads to the extension attempting to start a Watch while there is already one

I handled this by:

Which seems to work fine, AFAICT.

Because the folder names can't be changed after they have been added

I don't think this is correct... You can rename the folder by doing:

vscode.workspace.updateWorkspaceFolders(0, index, { name: 'foo', uri: folderUri });

See this

Still, I like the idea of having the main configuration automatically added when a new pacakage is added (and removed when all packages from the workspace get removed)

src/commands.ts

package.json

doudou · 2019-02-28T16:03:59Z

(and removed when all packages from the workspace get removed)

Actually, that's something I really don't want to do.

doudou · 2019-02-28T16:08:40Z

2\. Killing watch tasks when the extension gets disposed

Well, then, I don't know why, but according to the tests I did yesterday, the extension doesn't get disposed in the case where the first workspace folder is changed. The PR implements the same task management behavior - but through tasks instead of relying on the PID file - and I still get the same errors. It appeared that the dispose function was not called.

g-arjones · 2019-02-28T16:17:09Z

but through tasks instead of relying on the PID file

Not file, just a Map() from workspacePath to pid.

dispose() does get called but you can't do anything async in it (i.e use vscode's own terminateTask api) because apparently the extension host won't get the chance to execute that

doudou · 2019-02-28T17:00:11Z

dispose() does get called but you can't do anything async in it (i.e use vscode's own terminateTask api) because apparently the extension host won't get the chance to execute that

That would explain.

doudou · 2019-03-01T12:08:48Z

dispose() does get called but you can't do anything async in it (i.e use vscode's own terminateTask api) because apparently the extension host won't get the chance to execute that

terminate() is not even async ... they really are not letting the extensions do any form of cleanup.

doudou · 2019-03-01T12:10:01Z

Anyways:

updated the description for devFolder
added a force-kill on dispose. I kept the "clean" terminate as well, it works in cases the workspace is disposed from within the normal workflow (i.e. last package removed from workspace)

There's no way to do it through the normal task-terminate mechanism, since when vscode shuts down an extension host, it does give us a way to wait on promises. We still attempt to terminate for the "normal" case, that is the case where all folders from a workspace have been removed.

g-arjones · 2019-03-01T14:09:49Z

Looks good to me.. I liked the idea of creating one disposable per watch task.
Cool stuff... 👍

doudou · 2019-03-01T14:33:33Z

I liked the idea of creating one disposable per watch task.

After working with vscode, I "stole" the disposable pattern in Roby itself. It's a really neat pattern.

doudou added 7 commits February 28, 2019 08:53

update vscode's engine to 1.30 to get the task API

05f4020

terminate the watch task when deregistering a workspace

6cea497

auto-insert the autoproj configuration folder at the top of the works…

12ffcc0

…pace

improve workflow to add an autoproj workspace to the environment

42dc78b

The issue here was that the "autoproj (_WORKSPACE_NAME_)" name cannot be given after VSCode added the folder (e.g. cannot be done if the user uses "Add folder to workspace"). Create a "Add workspace" command for this.

move the auto-adding of the autoproj/ folder into the new folder even…

3e06cb4

…t code Having it in the "Add package to workspace" code makes no sense. We really want to auto-add when a new folder gets added. This ensures a smooth-y transition to the Add Workspace command.

doudou added the wip label Feb 28, 2019

update README to mention the Rock: Add Workspace command

492e363

doudou requested a review from g-arjones February 28, 2019 14:39

doudou removed the wip label Feb 28, 2019

g-arjones reviewed Feb 28, 2019

View reviewed changes

src/commands.ts Show resolved Hide resolved

package.json Show resolved Hide resolved

better description for the devFolder setting

e60f3a6

doudou added 2 commits March 1, 2019 09:19

bump typescript version

af339d9

doudou force-pushed the improve_multi_workspace_handling branch from 7a2a6f1 to f7ceb83 Compare March 1, 2019 12:20

doudou merged commit dcb10fb into master Mar 1, 2019

doudou deleted the improve_multi_workspace_handling branch March 1, 2019 14:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve multi workspace handling #62

Improve multi workspace handling #62

doudou commented Feb 28, 2019

coveralls commented Feb 28, 2019 •

edited

Loading

g-arjones commented Feb 28, 2019

doudou commented Feb 28, 2019

doudou commented Feb 28, 2019 •

edited

Loading

g-arjones commented Feb 28, 2019

doudou commented Feb 28, 2019

doudou commented Mar 1, 2019

doudou commented Mar 1, 2019

g-arjones commented Mar 1, 2019

doudou commented Mar 1, 2019

Improve multi workspace handling #62

Improve multi workspace handling #62

Conversation

doudou commented Feb 28, 2019

coveralls commented Feb 28, 2019 • edited Loading

Pull Request Test Coverage Report for Build 70

💛 - Coveralls

g-arjones commented Feb 28, 2019

doudou commented Feb 28, 2019

doudou commented Feb 28, 2019 • edited Loading

g-arjones commented Feb 28, 2019

doudou commented Feb 28, 2019

doudou commented Mar 1, 2019

doudou commented Mar 1, 2019

g-arjones commented Mar 1, 2019

doudou commented Mar 1, 2019

coveralls commented Feb 28, 2019 •

edited

Loading

doudou commented Feb 28, 2019 •

edited

Loading