Add support for compound launches #11444

martin-fleck-at · 2022-07-20T12:18:56Z

What it does

Fixes #11302

Extend debug model for compound launches
-- Support pre-launch task, stopAll, list of configurations
-- Properly parse compounds into configuration model
-- Support mapping to custom session option type
-- Support search of options by configuration, compound, and name
Show compound launches in configuration dropdown
-- Query all available options
-- Use JSON (de-)serialization to allow multiple options with same name
--- Previously custom string was used for identification
Extend session manager to start compound session options
Ensure compound sessions can also be found from DebugMain

I tried to keep backwards compatibility with interfaces and parameters as good as possible.

How to test

If you start Theia with Theia itself as a workspace, you should be able to see the already present compound launches Launch Electron Backend & Frontend and Launch Browser Backend & Frontend. But you can also add your own compound launches and play around with the options (stopAll, preLaunchTask).
To test the call from within a plugin, I added a small plugin that has a few commands to trigger the existing launch configurations (with or without workspace) or provide a custom launch configuration: https://github.com/martin-fleck-at/theia/tree/issue-11302-test

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

JonasHelming · 2022-07-21T08:12:54Z

@colin-grant-work It would help us a lot, if this could make it to the upcoming release. So if you have some cycles, we would be very thankful for a review. Thanks!

msujew

Functionality-wise this looks already quite good. I have a few comments on the code though.

packages/debug/src/common/debug-compound.ts

packages/debug/src/browser/debug-session-options.ts

packages/debug/src/browser/view/debug-configuration-select.tsx

packages/debug/src/browser/debug-configuration-manager.ts

packages/debug/src/browser/debug-configuration-model.ts

packages/debug/src/browser/debug-session-options.ts

packages/debug/src/browser/debug-session-manager.ts

martin-fleck-at · 2022-07-21T11:55:27Z

@msujew Thank you very much for your fast and detailed review! I added another commit to it so you can see the changes but I can also squash them if you feel everything is taken care of.

colin-grant-work · 2022-07-21T15:54:49Z

packages/debug/src/browser/debug-session-manager.ts

+        for (const configData of compound.configurations) {
+            const name = typeof configData === 'string' ? configData : configData.name;
+            if (name === compound.name) {
+                throw new Error(nls.localize("Launch configuration '{0}' contains a circle with itself", name));


This NLS call needs a key nls.localize(key: string, message: string, ...interpolations: string[]). This call just has the message and one interpolation.

Ah, good catch! I added the missing key to it, thank you!

colin-grant-work · 2022-07-21T16:00:35Z

packages/debug/src/browser/debug-session-options.ts

+    private stopEmitter = new Emitter<void>();
+    onDidSessionStop = this.stopEmitter.event;
+
+    sessionStopped(): void {


Rename to stopSession. sessionStopped sounds like a check, not an action.

I simply used the same name as in VS Code but I agree that stopSession is a better name.

colin-grant-work · 2022-07-21T18:19:50Z

packages/debug/src/common/debug-compound.ts

+
+import { TaskIdentifier } from '@theia/task/lib/common';
+
+export const defaultCompound: DebugCompound = { name: 'Compound', configurations: [] };


Appears to be unused.

True, I defined it and forgot to use it in the schema updater but now it is used there.

martin-fleck-at · 2022-07-22T07:45:08Z

@colin-grant-work Thank you very much for your review! I pushed it as a separate commit so you can see the changes at a glance. If need be, I can squash the changes and push the whole thing again.

vince-fugnitto

@martin-fleck-at something to try but I was unable to successfully start the Launch Electron Backend & Frontend debug configuration. The electron window did spawn but from the main window (where the config was launched from) I was not able to interact with the debug session (ex: stop it) since it did not seem to exist.

alvsan09

It's looking good !!

I am not sure if the following is related but
When I start a debug session (Browser Backend and Frontend), I am getting the following errors (TextEditor is disposed!) as per the image.
My setup is a theia electron app. debugging theia on browser running a 'hello_express' project). The two theia workspace folders are different.

martin-fleck-at · 2022-07-26T07:31:45Z

@vince-fugnitto @alvsan09 Thank you very much for your input!

I tested it a few times with Launch Electron Backend & Frontend and for me the debug sessions do seem to stay interactive, especially when I cancel the debug session early on:

The only one that sometimes causes problems is the Attach to Plugin Host which sometimes remains in the debug view. This can happen when I try to stop the debug sessions by stopping any of the debug sessions but also happens when I simply close the started Electron window. I can also see the following warning in the output sometimes: Did not receive terminated event in time.. @vince-fugnitto Is this what you are also experiencing? Do you experience the same problems when quickly starting one debug session after another manually?

For simple cases everything seems to work as expected, e.g.,

{
  "configurations": [{
      "command": "top",
      "name": "Run top",
      "request": "launch",
      "type": "node-terminal"
  }],
  "compounds": [{
      "name": "Multi-Run",
      "configurations": [
        "Run top",
        "Run top"
      ],
      "stopAll": false
    }]
}

I think it may has to do with how the debug adapters and their environment are set up or maybe it is a timing issue and some launch configurations need to wait for others to be completely started? I checked the VSCode code and there they also don't have any special waiting mechanism, they simply use Promise.all like we do.

@alvsan09 Unfortunately, I cannot reproduce the TextEditor is disposed message but I think my setup might not be exactly the same. Does it cause any failures on using any of the text editors? Do you experience the same problems when quickly starting one debug session after another manually?

martin-fleck-at · 2022-07-26T13:27:51Z

@colin-grant-work Could you please have a look if you believe the reported issues are connected to this PR or some other setup issue that is only revealed through this PR? I already tested it with the pre-built Gitpod but I am not sure how else I can proceed here so any help would be greatly appreciated.

alvsan09

I have tested running the updated code base to run a different Theia repository, and there is one combination as show below that causes errors.
I don't think it's related to this change but it would be great if all combinations below could work:

NOTE: I have also changed the configurations so theia runs 'pwa-chrome' instead of 'chrome'

Running Theia updated on browser,
then spawning Theia on electron
=== WORKS FINE ====

Running Theia updated on electron,
Changing electron launch backend config to: --remote-debugging-port=9223
Changing to port: 9223 on Attach to Electron Frontend
then spawning Theia on electron, gives different errors and ultimately crashes.
=== FAILURES ====

Running Theia updated on electron,
then spawning Theia on browser
=== WORKS FINE ===

Running Theia updated on browser,
Changing ports on backend / frontend launch configs.
then spawning Theia on browser
=== WORKS FINE ===

packages/debug/src/browser/debug-schema-updater.ts

packages/debug/src/browser/debug-configuration-manager.ts

packages/debug/src/browser/debug-session-options.ts

martin-fleck-at · 2022-07-28T07:28:12Z

@alvsan09 Thank you for your in-depth analysis! I agree that starting another Theia instance from Electron seems to cause a problem with the plugin host.

When I start the debug session I can see that the new Electron window opens normally and everything seems fine but after some time the 'Attach to Plugin Host' reports the following problem in the original window: Debug session initialization failed. See console for details.. Since we have the stopAll option set to true that stops all other debug sessions as well and the whole started Electron application closes/crashes. However, if we set stopAll to false, we can see that the window remains open as the other two launch configurations Launch Electron Backend and Attach to Electron Frontend do not have a problem. I know to little about the Plugin host to actually know how the attachment works and how that problem could be fixed if it has some deep underlying issue. I tried changing the port from 9339 to 9340 but that didn't solve the issue.

Still I would argue that this is a problem specifically to the plugin host, so maybe this could be considered as a separate bug?

martin-fleck-at · 2022-07-28T09:37:48Z

I just checked and the problem with 'Attach to Plugin Host' also happens when I start all three launch configurations manually. I therefore created a new bug for it here: #11475. I think I addressed all other comments so far so is there any chance we can still get this merged before the release? An adopter would really appreciate that.

alvsan09 · 2022-07-28T13:05:14Z

Hi @martin-fleck-at ,
Thanks for writing the issue above.

I just saw the following problem, when toggling stopAll in the editor,
in the following, the Auto save option is on, but changes to it does not always take effect on the next launch.

https://github.com/alvsan09/hello_express

martin-fleck-at · 2022-07-28T13:27:58Z

@alvsan09 Oh wow, great catch, thank you! Seems like I removed something when updating the current configuration and so changed properties were not correctly applied.

alvsan09 · 2022-07-28T13:52:39Z

@martin-fleck-at,
The latest change is breaking non compound configs, see:

martin-fleck-at · 2022-07-28T14:08:57Z

@alvsan09 Oh wow, you are right! I am very sorry for missing that! Seems like the typeguards were not good enough to detect a dynamic configuration vs a regular configuration. I just checked whether the property was there but not if that property was set.

alvsan09 · 2022-07-28T14:24:30Z

@martin-fleck-at ,
I saw one more problem,
we are no longer able to select dynamic configurations, see gif below:

martin-fleck-at · 2022-07-28T14:54:45Z

@alvsan09 I updated the search for the current configuration again, I think now all cases should be covered. Thank you very much for taking the time!

alvsan09 · 2022-07-28T16:59:43Z

@martin-fleck-at ,
The latest change fixes the previous issues,
unfortunately, I found new ones:

When starting a workspace used in a previous version of theia where that workspace had registered dynamic configurations in local storage, the configurations will be restored with a label of undefined.
From the debug view, selecting Add Configuration has no effect
When reloading the browser, sometimes the selection box in the debug view go out of bounds,
I tried reproducing this one on master but I couldn't, with the new change I had to try two or three times and it will happen.

See the gif below:

- Extend debug model for compound launches -- Support pre-launch task, stopAll, list of configurations -- Properly parse compounds into configuration model -- Support mapping to custom session option type -- Support search of options by configuration, compound, and name - Show compound launches in configuration dropdown -- Query all available options -- Use JSON (de-)serialization to allow multiple options with same name --- Previously custom string was used for identification - Extend session manager to start compound session options - Ensure compound sessions can also be found from DebugMain Fixes #11302 Signed-off-by: Martin Fleck <mfleck@eclipsesource.com>

- Use 'unknown' in type guard - Simplify parsing code for debug configuration model - Deprecate use of custom debug session options parsing - Remove unnecessary overload method in debug configuration manager

- Ensure proper NLS usage - Use default constant for schema updater - Rename 'sessionStopped' to 'stopSession'

- Localize further fields in debug schema - Move optional provider type to configuration type - Create custom dynamic options type for convenience

martin-fleck-at · 2022-07-29T08:48:50Z

@alvsan09 Thank you again for your feedback!

I fixed the naming issue, there was indeed a change as we now use a different serialization so the name was not properly recognized.
For me 'Add Configuration...' opens the launch.json file as expected both in Chrome and in Firefox.
I also cannot reproduce the select rendering issue. I tried several times in Chrome and in Firefox but it always renders to the bottom.

Could you please give it another try? I don't know what could have caused this but I cannot reproduce it at all. What browser are you using?

alvsan09 · 2022-07-29T13:38:42Z

@martin-fleck-at ,
I tried the correction for 'undefined' for the names on restored dynamic configs and it works fine now, 👍

Could you please give it another try? I don't know what could have caused this but I cannot reproduce it at all. What browser are you using?

For the two remaining issues, I managed to reproduce them using a mult-root workspace, and the amount of entries in the configuration is larger e.g.. all the ones provided by theia + all the ones provided by the hello_express example mentioned above.

The following clip shows the difference when having a folder vs a workspace project.

multi-root-debug-view-issues.mp4

- Prefer select options rendering to bottom in most cases - Using 'Add Configurations' in dropdown triggers focus-lost on pick service automatically closing the workspace selection, keep picking open

martin-fleck-at · 2022-08-01T13:02:33Z

@alvsan09 With the multi-root workspace service I can indeed reproduce the problem! Seems like when we need to select a workspace the update of the configurations dropdown takes focus and automatically closes the workspace select, I now keep it open even on focus lost.

As for the rendering "issue": This really depends on how much space is available at the top or bottom. Previously, if there was not enough space at the bottom, it was rendered to the top. I changed that so that it prefers bottom rendering unless more content can be shown when rendering to the top.

Hopefully this will resolve the remaining issues. In any case, I really appreciate you sticking with me on this ;-)

alvsan09 · 2022-08-01T17:08:09Z

@martin-fleck-at
I have done a regression test and almost everything seems to work as far as I can tell,
Except the following issues (related to recent dynamic configurations from old format)

The video starts showing a set of dynamic launch configurations which were created on a previous session with Theia master,
then we launch each of them and duplicates can sometimes be created.

duplicating_dynamic_launch.mp4

I saw a strange behaviour once before where a non dynamic config. managed to get inserted among the recent dynamic configurations, I am not sure how to reproduce it, although I managed to get a screen shot of the serialized data. See Attach to node process

alvsan09 · 2022-08-01T17:17:00Z

As for the rendering "issue": This really depends on how much space is available at the top or bottom. Previously, if there was not enough space at the bottom, it was rendered to the top. I changed that so that it prefers bottom rendering unless more content can be shown when rendering to the top.

@msujew, Do you agree with the change of approach affecting the custom Select Component widget ?

msujew · 2022-08-02T10:50:49Z

@alvsan09 @martin-fleck-at Yeah, the changes look reasonable to me. Everything in regards to the select component still works as expected or even better 👍

- Option name property was missing when restoring data - Fix proper update of current value -- Current value is not always perfectly in list of options due to serialization so we do a matching first

martin-fleck-at · 2022-08-02T20:00:08Z

@alvsan09 I hope I managed to remove the last issues now. When restoring the old dynamic configurations I now ensure that the options name is properly derived if it is not set already. I also filter for dynamic options to ensure nothing else gets into that list (even though I was not able to reproduce that locally).

Interestingly, the fix showed an issue which probably was the reason for the initial custom serialization: When we set the current value in the debug session manager, the value we have in current does not necessarily match exactly one of the options we have on the configuration drop down, mostly due to the fact that properties are in different order. So I now aim to match the current value to the current option before when updating the select. As far as I can tell, this now works for all option types and updates in the launch.json are also properly considered.

alvsan09

Hi @martin-fleck-at ,
I have done a full regression and it looks almost there !

I found the issue shown on the video clip below, where dynamic configurations are not restored as current, from local storage.

I have also added a question as an inline comment.

failing_to_restore_dynamic_to_current.mp4

alvsan09 · 2022-08-03T14:34:14Z

packages/debug/src/browser/debug-configuration-manager.ts

+        const dynamicOptions = options.map(option => {
+            option.name = option.name ?? option.configuration.name;
+            return option;
+        }).filter(DebugSessionOptions.isDynamic);


is it really necessary to filter for isDynamic ?
non dynamic configs should not be available under the received argument:
options?: DynamicDebugConfigurationSessionOptions[], right?

You are absolutely correct that it should not be necessary. I added this guard because you had the case where there was a regular configuration in the 'recentDynamicOptions'. I could not reproduce the case but during runtime we already have a type guard before updating recently used dynamic options so I reasoned that it probably was caused during loading some old data. Since the recent options are limited to 3, I thought the extra safety outweighs any potential performance cost. What do you think?

I am leaning towards removing it, as the case may have been caused while switching among different versions of this change.
If we ever see it again, it's not too bad for the user to ignore it or replace it by making a new selection, and then we could try to fix the actual source of the problem.

alvsan09 · 2022-08-04T13:45:22Z

Using the last commit I have verified that the functionality works as expected ! 👍

@vince-fugnitto and @colin-grant-work, any pending items on your side ?

I have successfully run the tests below:

Storing / Restoring configurations as current for (dynamic, not dynamic & compound)
Storing / Restoring recently used dynamic configurations
Restoring configurations from previous theia version
Launching configuration by 'debug ' command
Launching compound configurations
- stopAll [true, false]
- toggling stopAll takes effect at the next launch
- with a preLaunchTask
Launching not compound configurations
"Add configuration" works
The above works for multi-root as well as for project folders

vince-fugnitto

I don't have any objections to the changes 👍
I believe Alvaro reviewed thoroughly #11444 (comment).

martin-fleck-at · 2022-08-09T14:43:17Z

@colin-grant-work @msujew Any objections to merging this?

colin-grant-work · 2022-08-09T15:31:51Z

Any objections?

None from me.

JonasHelming requested a review from colin-grant-work July 21, 2022 08:12

msujew self-requested a review July 21, 2022 08:41

msujew reviewed Jul 21, 2022

View reviewed changes

vince-fugnitto added the debug issues that related to debug functionality label Jul 21, 2022

colin-grant-work reviewed Jul 21, 2022

View reviewed changes

vince-fugnitto reviewed Jul 25, 2022

View reviewed changes

alvsan09 reviewed Jul 25, 2022

View reviewed changes

alvsan09 reviewed Jul 27, 2022

View reviewed changes

packages/debug/src/browser/debug-schema-updater.ts Show resolved Hide resolved

packages/debug/src/browser/debug-configuration-manager.ts Outdated Show resolved Hide resolved

packages/debug/src/browser/debug-session-options.ts Outdated Show resolved Hide resolved

martin-fleck-at mentioned this pull request Jul 28, 2022

'Attach to Plugin Host' cannot be initialized when started from within running Theia instance #11475

Open

martin-fleck-at added 7 commits July 29, 2022 09:50

Incorporate review

8117512

- Use 'unknown' in type guard - Simplify parsing code for debug configuration model - Deprecate use of custom debug session options parsing - Remove unnecessary overload method in debug configuration manager

Incorporate review

c3908a1

- Ensure proper NLS usage - Use default constant for schema updater - Rename 'sessionStopped' to 'stopSession'

Fix inproper refresh of debug configuration selection

593619d

Integrate further feedback

e059a8c

- Localize further fields in debug schema - Move optional provider type to configuration type - Create custom dynamic options type for convenience

Fix missing update of current configuration, need to search

b96e332

Fix type guard of session options

45caa7d

martin-fleck-at added 2 commits July 29, 2022 09:50

Fix missing update of current configuration, need to search better

375f093

Fix name calculation of stored dynamic session options

5a8d61e

Fix select rendering and 'Add Configurations' in multi-root workspace

efb090d

- Prefer select options rendering to bottom in most cases - Using 'Add Configurations' in dropdown triggers focus-lost on pick service automatically closing the workspace selection, keep picking open

Fix incomplete restoring of old data

038423d

- Option name property was missing when restoring data - Fix proper update of current value -- Current value is not always perfectly in list of options due to serialization so we do a matching first

alvsan09 reviewed Aug 3, 2022

View reviewed changes

Ensure dynamic options are properly restored as 'current'

fc34582

martin-fleck-at requested review from vince-fugnitto and colin-grant-work August 9, 2022 12:21

vince-fugnitto approved these changes Aug 9, 2022

View reviewed changes

martin-fleck-at merged commit fe224bf into eclipse-theia:master Aug 11, 2022

martin-fleck-at deleted the issue-11302 branch August 11, 2022 08:38

vince-fugnitto added this to the 1.29.0 milestone Aug 25, 2022

colin-grant-work mentioned this pull request Dec 13, 2022

Workspace Selection QuickPick for Debug 'Add Configuration' ignores focus out #11992

Closed


		import { TaskIdentifier } from '@theia/task/lib/common';

		export const defaultCompound: DebugCompound = { name: 'Compound', configurations: [] };

Add support for compound launches #11444

Add support for compound launches #11444

Conversation

martin-fleck-at commented Jul 20, 2022 • edited Loading

What it does

How to test

Review checklist

Reminder for reviewers

JonasHelming commented Jul 21, 2022

msujew left a comment

Choose a reason for hiding this comment

martin-fleck-at commented Jul 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martin-fleck-at commented Jul 22, 2022

vince-fugnitto left a comment

Choose a reason for hiding this comment

alvsan09 left a comment

Choose a reason for hiding this comment

martin-fleck-at commented Jul 26, 2022

martin-fleck-at commented Jul 26, 2022

alvsan09 left a comment • edited Loading

Choose a reason for hiding this comment

martin-fleck-at commented Jul 28, 2022

martin-fleck-at commented Jul 28, 2022

alvsan09 commented Jul 28, 2022

martin-fleck-at commented Jul 28, 2022

alvsan09 commented Jul 28, 2022

martin-fleck-at commented Jul 28, 2022

alvsan09 commented Jul 28, 2022

martin-fleck-at commented Jul 28, 2022

alvsan09 commented Jul 28, 2022

martin-fleck-at commented Jul 29, 2022

alvsan09 commented Jul 29, 2022 • edited Loading

martin-fleck-at commented Aug 1, 2022

alvsan09 commented Aug 1, 2022 • edited Loading

alvsan09 commented Aug 1, 2022

msujew commented Aug 2, 2022

martin-fleck-at commented Aug 2, 2022

alvsan09 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvsan09 commented Aug 4, 2022

vince-fugnitto left a comment

Choose a reason for hiding this comment

martin-fleck-at commented Aug 9, 2022

colin-grant-work commented Aug 9, 2022

martin-fleck-at commented Jul 20, 2022 •

edited

Loading

alvsan09 left a comment •

edited

Loading

alvsan09 commented Jul 29, 2022 •

edited

Loading

alvsan09 commented Aug 1, 2022 •

edited

Loading

alvsan09 left a comment •

edited

Loading