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

CHE-9095: Improve Theia build stability on plugin list changes #9163

Merged
merged 4 commits into from Mar 23, 2018

Conversation

mmorhun
Copy link
Contributor

@mmorhun mmorhun commented Mar 21, 2018

What does this PR do?

Adds support for Theia native plugins from filesystem
Adds support for Theia native plugins which is hosted in git repository
Improves build stability. Skips Invalid plugins (where possible).

Plugin format

To set custom Theia plugins one should list them in THEIA_PLUGINS environment variable, each plugin should be separated by ,. For example: THEIA_PLUGINS=@theia/go,@theia/python

Plugin versions are supported and could be specified after :. Example: plugin-name:5.6.7

  • if a plugin is native Theia plugin, name and optionally version should be specified. For example: @theia/python:0.3.7
  • If a plugin is located on local file system (useful for developing and testing purposes), one should specify absolute path to the plugin folder itself (not parent folder with lerna.json) instead of version. Example: plugin-name:/path/my-plugin
  • If a plugin is located in git repository, the following format should be used:
    plugin-name:git://github.com/username/plugin-repository
    Note, field name in plugin's package.json should be the same as plugin-name at the beggining of plugin defining.

What issues does this PR fix or reference?

Partially fixes #9095

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>
Signed-off-by: Mykola Morhun <mmorhun@redhat.com>
Signed-off-by: Mykola Morhun <mmorhun@redhat.com>
@benoitf benoitf added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/bug Outline of a bug - must adhere to the bug report template. labels Mar 21, 2018
}

const rootPackageJson = require(pluginPath + 'package.json');
if (rootPackageJson['name'] === pluginName && !fs.existsSync(pluginPath + 'lerna.json')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

question: do we need to use all synchronous API or we could use async ? and then install in parallel ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@benoitf what we can do in parallel that's cloning of git repositories and building them if needed.
But this makes the logic of the script more complicated. We have to deal with paralleling and barrier then, which is not supported in nodejs natively. So, some kind of emulation is needed. We'll end up with batch of promises wrapped in timeouts and async await chain up to the 'main' function. More, not all the plugins are git-hosted, but to have similar code we'll have to use promises for other plugins where they aren't needed actually. But the main problem is stability of that code. Having parallelism emulation and all that stuff is error prone. Also it could be not such efficient as it looks like with this approach.
Bringing real parallelism with child process library also isn't trivial. Threads may interact via IPC or some libs for shared memory, which is require new dependency and again much more complicated and error prone code.
And taking into account, that we are going to have new plugins model - this script will be removed then. So, I think, that it is not needed for now.

}

const rootPackageJson = require(pluginPath + 'package.json');
if (rootPackageJson['name'] === pluginName && !fs.existsSync(pluginPath + 'lerna.json')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems in this case we need check if node_modules and lib exists and build plugin if needed. We can split build logic to separated method and reuse it here. @mmorhun What do you think?

Copy link
Contributor Author

@mmorhun mmorhun Mar 21, 2018

Choose a reason for hiding this comment

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

Well, we do not know how to build this plugin in general case. And most of the plagins (at least generated with Theai plugin generator, which is majority) use lerna (i.e. else block will be executed). I use this just to be able to include kinda 'ready' plugin (as you know, will work with the plugin's subdirectory). This can be used to speed up Theia run.
But if you think that we should do it I'll add the build here too.

const defaultTheiaPath = `/home/default/theia`;
const defaultConfig = require(`${defaultTheiaPath}/package.json`);
function copyDefaultTheiaBuild() {
cp.execSync(`cp -r ${defaultTheiaRoot}/node_modules ${theiaRoot} && cp ${defaultTheiaRoot}/yarn.lock ${defaultTheiaRoot}/package.json ${theiaRoot}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe split this commad:
cp.execSync(cp -r ${defaultTheiaRoot}/node_modules ${theiaRoot});
cp.execSync(cp ${defaultTheiaRoot}/yarn.lock ${defaultTheiaRoot}/package.json ${theiaRoot});
It should be a bit more readable. But up to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just don't want to spawn one more child process for this

function prepareTheia() {
process.chdir(theiaRoot);

const extraPlagins = getExtraTheiaPlugins();
Copy link
Contributor

Choose a reason for hiding this comment

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

extraPlagins -> extraPlugins

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>
@mmorhun mmorhun merged commit 6034d6e into master Mar 23, 2018
@mmorhun mmorhun deleted the CHE-9095 branch March 23, 2018 13:51
@benoitf benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Mar 23, 2018
@benoitf benoitf added this to the 6.3.0 milestone Mar 23, 2018
@mmorhun mmorhun restored the CHE-9095 branch March 26, 2018 14:32
@mmorhun mmorhun deleted the CHE-9095 branch March 26, 2018 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Outline of a bug - must adhere to the bug report template.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sometimes rebuild Theia with plugins applied by env THEIA_PLUGINS fails
5 participants