Fix electron extension host #96

sgraband · 2024-06-13T08:52:17Z

What it does

Before yarn electron start was invoked.
This does not work, as the cli tools will not be available when bundled.
Adjust the getStartCommand in AbstractHostedInstanceManager to work for both the browser and electron case.
Additionally, add --electronUserData to the electron case.
This will avoid odd behavior between the two instances running.
Use a timestamp folder in the tmp folder of the os in order to keep things clean on the machine and between different debugging sessions.

Fixes https://github.com/eclipse-theia/theia-blueprint/issues/ 281 (will remove the whitespace in the real PR)

How to test

Create a packaged electron version either via electron-builder or by linking the packages into the theia-ide
Create a workspace with a vscode extension with the yeoman generator
Open the workspace with the theia application
You will need to slightly adapt the launch.json, due to other bugs. Comment out the preLaunchTask line
Run the Run extension debug config.
Notice, that a extension host comes up that has the extension installed.
You can also test closing the host and restarting the debug config as well as that a new folder in your tmp folder is created everytime you start a new extension host.

Follow-ups

I will check if there are already tickets and will open tickets for any issues that i have found, that do not have one yet.
One example is that apparently ${defaultBuildTask}' is not supported in launch.json

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

Before `yarn electron start` was invoked. This does not work, as the cli tools will not be available when bundled. Adjust the `getStartCommand` in `AbstractHostedInstanceManager` to work for both the browser and electron case. Additionally, add `--electronUserData` to the electron case. This will avoid odd behavior between the two instances running. Use a timestamp folder in the tmp folder of the os in order to keep things clean on the machine and between different debugging sessions.

sgraband · 2024-06-13T08:54:13Z

@tsmaeder can you take a look and let me know what you think or if you have any objections/suggestions?

Note this only fixes the problem that the start command was wrong. Other issues with the debugging are not handled here.

tsmaeder · 2024-06-13T09:30:03Z

@sgraband sure. Might be next week, though...moving house on monday.

sgraband · 2024-06-27T07:49:49Z

@tsmaeder Could you take a look, this or next week?

tsmaeder · 2024-06-27T07:54:16Z

Sorry, got sidetracked with PRs for the release today. Feel free to nag me again tomorrow and every day if I don't get my fingers out of my butt.

sgraband · 2024-07-01T09:25:13Z

@tsmaeder No worries. If you would have time this week it would be great, as i would have time to respond to feedback/suggestions.

tsmaeder · 2024-07-01T10:00:00Z

packages/plugin-dev/src/node/hosted-instance-manager.ts

@@ -240,23 +241,22 @@ export abstract class AbstractHostedInstanceManager implements HostedInstanceMan
    }

    protected async getStartCommand(port?: number, debugConfig?: PluginDebugConfiguration): Promise<string[]> {
-
+        if (!port) {
+            port = process.env.HOSTED_PLUGIN_PORT ?


I tried running the plugin a second time, but that failed. My suspicion is that we're trying to run a second instance with the same port. We need some algorithm to assign unique, free ports per instance.

Is it actually intended, that we will ever get a second instance? I just tested it in VSCode and there i will only get one extension host. If i start the same config again, while the old one is still running it will just kill the first debug session.

Of course the handling here could be improved, but i am not sure if we need to handle mutliple ports? WDYT?

Sorry, but something just not working without any indication why is simply a bug. If VS Code jumped out the Window, would you follow? ;-) There might be good reasons why we can't implement this, but then we need to catch it and inform the user. Is there a good reason we can't allocate a new random port?

I do not have any objections to support mutliple Hosted Plugin instances, but the current implementation seems like it was build under the assumption that only one will be possible (e.g. the cli parameter to specify one port, or the UI in the parent instance). As this would require additional changes, outside the scope of this PR, i would suggest to handle this in a follow up ticket. Does that sound good to you?

tsmaeder · 2024-07-01T10:01:57Z

packages/plugin-dev/src/node/hosted-instance-manager.ts

 }

 @injectable()
 export class ElectronNodeHostedPluginRunner extends AbstractHostedInstanceManager {
+    protected override async getStartCommand(port?: number, debugConfig?: PluginDebugConfiguration): Promise<string[]> {
+        const command = await super.getStartCommand(port, debugConfig);
+        command.push(`--electronUserData="${this.getTempDir()}/theia-extension-host/${this.getTimestamp()}"`);


IMO, we should track this directory and remove it once the hosted instance exits.

You mean, once the hosted instance is removed/terminated?

tsmaeder · 2024-07-01T10:03:19Z

packages/plugin-dev/src/node/hosted-instance-manager.ts


+    protected getTempDir(): string {
+        const tempDir = os.tmpdir();
+        return process.platform === 'darwin' ? fs.realpathSync(tempDir) : tempDir;


This could use a comment. Why the special case?

tsmaeder · 2024-07-01T10:05:52Z

packages/plugin-dev/src/node/hosted-instance-manager.ts

+    }
+
+    protected getTimestamp(): string {
+        return `${Math.round(new Date().getTime() / 1000)}`;


Why not just move this into getTempDir()? Also: doesn't this limit the rate of temp dir creation to one per second? I would be more comfortable with a random directory + checking for existence.

tsmaeder · 2024-07-01T10:09:00Z

packages/plugin-dev/src/node/hosted-instance-manager.ts

+            // remove --port=X and --port X arguments if set
+            // remove --plugins arguments
+            if (arg.startsWith('--port') || args[index - 1] === '--port') {
+                return;


I know you didn't invent this code, but it would be much clearer if we just returned true/false instead of arg and undefined. "truthiness" is bogus enough without us adding to the confusion. Also, it might be wrong: the empty string is falsy, so if we actually have an empty argument, it borks! 🤦

tsmaeder reviewed Jul 1, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix electron extension host #96

Fix electron extension host #96

sgraband commented Jun 13, 2024 •

edited

Loading

sgraband commented Jun 13, 2024

tsmaeder commented Jun 13, 2024

sgraband commented Jun 27, 2024

tsmaeder commented Jun 27, 2024

sgraband commented Jul 1, 2024

tsmaeder Jul 1, 2024

sgraband Jul 2, 2024

tsmaeder Jul 2, 2024

sgraband Jul 5, 2024

tsmaeder Jul 1, 2024

sgraband Jul 2, 2024

tsmaeder Jul 2, 2024

tsmaeder Jul 1, 2024

tsmaeder Jul 1, 2024

tsmaeder Jul 1, 2024

Fix electron extension host #96

Are you sure you want to change the base?

Fix electron extension host #96

Conversation

sgraband commented Jun 13, 2024 • edited Loading

What it does

How to test

Follow-ups

Review checklist

Reminder for reviewers

sgraband commented Jun 13, 2024

tsmaeder commented Jun 13, 2024

sgraband commented Jun 27, 2024

tsmaeder commented Jun 27, 2024

sgraband commented Jul 1, 2024

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

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

sgraband commented Jun 13, 2024 •

edited

Loading