[java-docker] use builtin and external ext #287

vince-fugnitto · 2020-01-21T18:31:40Z

What it does

Update the theia-java-docker:next image to include builtins and external VS Code extensions

How to test

Build and verify the changes (that plugins are downloaded and picked up when started)

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

Signed-off-by: Vincent Fugnitto vincent.fugnitto@ericsson.com

akosyakov · 2020-01-22T02:36:53Z

@marcdumais-work @vince-fugnitto Would be fine If i do theia-ide/theia?

One important thing is Electron. It is not clear how to bundle extensions yet. An end user cannot use env variables or cli options to pick up extensions, they should be inside the bundled app and picked up automatically.

vince-fugnitto · 2020-01-22T02:57:31Z

@marcdumais-work @vince-fugnitto Would be fine If i do theia-ide/theia?

Please go ahead :) I wanted to try it out for an example image (java) so I could iron out any details and see what needs to be done for other images.

One important thing is Electron. It is not clear how to bundle extensions yet. An end user cannot use env variables or cli options to pick up extensions, they should be inside the bundled app and picked up automatically.

I believe @lmcbout had some experience picking up plugins for Electron applications if we get blocked.

akosyakov · 2020-01-22T03:33:14Z

I believe @lmcbout had some experience picking up plugins for Electron applications if we get blocked.

I've seen the issue on Theia repo where it was recommended to configure an env variable, but for builtins it is not how it should work. A user should download distributable, run it and VS Code extensions should be there already, without changing any env variables. I have not seen anyone to figure it out and it will be important for all Electron based products if we remove textmate grammars Theia extension.

marcdumais-work · 2020-01-22T12:52:29Z

I believe @lmcbout had some experience picking up plugins for Electron applications if we get blocked.

I've seen the issue on Theia repo where it was recommended to configure an env variable, but for builtins it is not how it should work. A user should download distributable, run it and VS Code extensions should be there already, without changing any env variables. I have not seen anyone to figure it out and it will be important for all Electron based products if we remove textmate grammars Theia extension.

In our current internal application we do have the built-ins and some extra VS Code extensions part of the bundle (MSI installer for Window, AppImage for Linux). We have our own version of electron-main.js that's used as entry-point, and where we inject env var THEIA_DEFAULT_PLUGINS pointing it to the bundled plugins folder.

So it does rely on the env var mechanism but the user needs not set it or care about it. This leaves env var THEIA_PLUGINS to point to a folder on the host, where the user or eventually our extensions manager could install their own extensions.

akosyakov · 2020-01-22T17:33:55Z

We have our own version of electron-main.js that's used as entry-point, and where we inject env var THEIA_DEFAULT_PLUGINS pointing it to the bundled plugins folder.

Does not sound super clean :) Can we generalize it in Theia to a solution without using custom electron-main.js?

marcdumais-work · 2020-01-22T18:03:48Z

Does not sound super clean :) Can we generalize it in Theia to a solution without using custom electron-main.js?

Sure. Maybe the plugins system could look-for a plugins folder at a given path relative to the app, that would be used if present, without depending on any env var? Or maybe you have other ideas.

For reference, he're how we do it ATM in our custom electron-main.js:

process.env.THEIA_DEFAULT_PLUGINS = `local-dir:${resolve(__dirname, '..', '..', '..', '..', 'plugins')}`;

akosyakov · 2020-01-22T18:07:14Z

Maybe the plugins system could look-for a plugins folder at a given path relative to the app, that would be used if present, without depending on any env var?

No ideas, i don't have much experience with Electron. We can try whatever you propose. Maybe someone come up with a better idea on the PR.

akosyakov · 2020-01-23T02:08:31Z

Let's move a discussion here: eclipse-theia/theia#6946

vince-fugnitto · 2020-01-23T17:12:37Z

Unfortunately, we cannot successfully update the applications until @theia/cli is released.
We cannot include @theia/cli:next in the latest.package.json since it pulls @theia/application-package (used by many extensions) and will cause a multi injector error on startup.

We either need to:

release a hotfix with the update @theia/cli so we can update the images ahead of the release (gives us time to update the images, gives others extra time to adopt their applications to the builtins)
wait a week for the release and proceed with the general updates (with many other things to update before we can deprecate extensions anyway, maybe this approach is fine)

akosyakov · 2020-01-24T09:55:49Z

@vince-fugnitto yes, it won't work, but we can upgrade next images for now?

Unfortunately it won’t work either since I reference the download:plugins script in the Dockerfile. In the case of the latest Images this command will fail since it does not exist :(

marcdumais-work · 2020-01-28T16:22:19Z

theia-java-docker/next.package.json

@@ -15,20 +15,89 @@
        "@theia/file-search": "next",
        "@theia/getting-started": "next",
        "@theia/git": "next",
-        "@theia/java": "next",
        "@theia/json": "next",


I think maybe we're keeping this one because the json built-in is not working yet?

Correct, the vscode-builtin-json-language-features is not yet working so I could not add it at this time (eclipse-theia/theia#6623).

vscode-builtin-json is included to add JSON syntax highlighting and snippet support since @theia/textmate grammars is removed.

marcdumais-work · 2020-01-28T16:22:43Z

theia-java-docker/next.package.json

        "@theia/search-in-workspace": "next",
        "@theia/terminal": "next",
-        "@theia/textmate-grammars": "next",
-        "@theia/tslint": "next",
-        "@theia/typescript": "next",
        "typescript": "latest"


needed?

I wasn't sure, it looks like all images include typescript as a dependency.

marcdumais-work · 2020-01-28T16:41:23Z

Got these errors:

Maybe setting JAVA_HOME so it points to the JDK instead of the JRE would help: "/opt/ibm/java/" ? (line 7 in Dockerfile)

update: confirmed by setting java path through a preference, then the LS starts.
update2: maybe it would be best to leave JAVA_HOME as-is and add JDK_HOME? This seems to be what's suggested in the extension's documentation

marcdumais-work · 2020-01-28T18:27:01Z

I tested a bit with a few sample Java projects. Once the JDK path issue is fixed, the image seems to work decently well. So looking good so far.

theia-java-docker/latest.package.json

- updated the `theia-java-docker:latest` image to use builtins and external VS Code extensions. - updated the `theia-java-docker:next` image to use builtins and external VS Code extensions. - updated the `JAVA_HOME` environment variable Signed-off-by: Vincent Fugnitto <vincent.fugnitto@ericsson.com>

vince-fugnitto · 2020-02-03T18:42:37Z

@marcdumais-work I've updated based on previous feedback, and since @theia/cli has been released last week both the latest and next images can benefit from the download:plugins script (used for example to pull builtin plugins).

marcdumais-work

LGTM - quickly tested both next and latest - both compile and start, both successfully spawn the Java LS.

marcdumais-work · 2020-02-04T11:38:52Z

Merging

vince-fugnitto · 2020-02-06T16:01:02Z

@marcdumais-work @vince-fugnitto Would be fine If i do theia-ide/theia?

@akosyakov do you still plan on migrating the theia-docker image?

vince-fugnitto added the java label Jan 21, 2020

vince-fugnitto requested review from lmcbout and marcdumais-work January 21, 2020 18:31

vince-fugnitto self-assigned this Jan 21, 2020

vince-fugnitto force-pushed the vf/java-builtins branch from 7e4f8ae to 31f63a0 Compare January 21, 2020 19:39

akosyakov mentioned this pull request Jan 23, 2020

provide migration steps to builtins for Electron distributions eclipse-theia/theia#6946

Closed

vince-fugnitto mentioned this pull request Jan 24, 2020

[cpp] added the builtin ext to theia-cpp and remove deprecated exts #290

Merged

1 task

marcdumais-work reviewed Jan 28, 2020

View reviewed changes

vince-fugnitto force-pushed the vf/java-builtins branch from 31f63a0 to 7e427c4 Compare January 29, 2020 00:00

akosyakov reviewed Jan 30, 2020

View reviewed changes

theia-java-docker/latest.package.json Outdated Show resolved Hide resolved

vince-fugnitto force-pushed the vf/java-builtins branch from 7e427c4 to f30f9a7 Compare January 30, 2020 11:17

marcdumais-work approved these changes Feb 3, 2020

View reviewed changes

marcdumais-work merged commit 622d432 into master Feb 4, 2020

marcdumais-work deleted the vf/java-builtins branch February 4, 2020 11:39

marcdumais-work mentioned this pull request Feb 19, 2020

[Java, full] Language Server not working/starting #285

Closed

vince-fugnitto mentioned this pull request Apr 23, 2020

use vscode extensions instead of native #239

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[java-docker] use builtin and external ext #287

[java-docker] use builtin and external ext #287

vince-fugnitto commented Jan 21, 2020

akosyakov commented Jan 22, 2020

vince-fugnitto commented Jan 22, 2020

akosyakov commented Jan 22, 2020 •

edited

Loading

marcdumais-work commented Jan 22, 2020

akosyakov commented Jan 22, 2020

marcdumais-work commented Jan 22, 2020

akosyakov commented Jan 22, 2020

akosyakov commented Jan 23, 2020

vince-fugnitto commented Jan 23, 2020

akosyakov commented Jan 24, 2020 •

edited by vince-fugnitto

Loading

marcdumais-work Jan 28, 2020

vince-fugnitto Jan 28, 2020

marcdumais-work Jan 28, 2020

vince-fugnitto Jan 28, 2020

marcdumais-work commented Jan 28, 2020 •

edited

Loading

marcdumais-work commented Jan 28, 2020

vince-fugnitto commented Feb 3, 2020

marcdumais-work left a comment

marcdumais-work commented Feb 4, 2020

vince-fugnitto commented Feb 6, 2020

[java-docker] use builtin and external ext #287

[java-docker] use builtin and external ext #287

Conversation

vince-fugnitto commented Jan 21, 2020

What it does

How to test

Review checklist

Reminder for reviewers

akosyakov commented Jan 22, 2020

vince-fugnitto commented Jan 22, 2020

akosyakov commented Jan 22, 2020 • edited Loading

marcdumais-work commented Jan 22, 2020

akosyakov commented Jan 22, 2020

marcdumais-work commented Jan 22, 2020

akosyakov commented Jan 22, 2020

akosyakov commented Jan 23, 2020

vince-fugnitto commented Jan 23, 2020

akosyakov commented Jan 24, 2020 • edited by vince-fugnitto Loading

marcdumais-work Jan 28, 2020

Choose a reason for hiding this comment

vince-fugnitto Jan 28, 2020

Choose a reason for hiding this comment

marcdumais-work Jan 28, 2020

Choose a reason for hiding this comment

vince-fugnitto Jan 28, 2020

Choose a reason for hiding this comment

marcdumais-work commented Jan 28, 2020 • edited Loading

marcdumais-work commented Jan 28, 2020

vince-fugnitto commented Feb 3, 2020

marcdumais-work left a comment

Choose a reason for hiding this comment

marcdumais-work commented Feb 4, 2020

vince-fugnitto commented Feb 6, 2020

akosyakov commented Jan 22, 2020 •

edited

Loading

akosyakov commented Jan 24, 2020 •

edited by vince-fugnitto

Loading

marcdumais-work commented Jan 28, 2020 •

edited

Loading