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

[plugin] Add missing return statements #6073

Merged
merged 1 commit into from
Sep 2, 2019
Merged

[plugin] Add missing return statements #6073

merged 1 commit into from
Sep 2, 2019

Conversation

vinokurig
Copy link
Contributor

@vinokurig vinokurig commented Aug 30, 2019

What it does

Add missing return statements to VsCodePluginDeployerResolver.
Since 2d4c845 was merged Plugins install their dependent plugins automatically.
VsCode Github plugin includes built in vscode git extension in it's dependent extensions which is not present in the vscode marketplace. This causes an error witch is not properly handled. The return statement in the catch error section allows to skip the installation of dependent extension in case of failure.

fixes eclipse-che/che#14319

How to test

  1. Remove git from theia/examples/browser/package.json.
  2. Download vscode git extension.
  3. Download vscode github-pull-request extension.
  4. Create plugins folder in Theia's root and paste the downloaded plugins to it.
  5. Start Theia

Without the fix the vscode git and Github plugins fail to start.

Review checklist

Reminder for reviewers

@vinokurig vinokurig added the plug-in system issues related to the plug-in system label Aug 30, 2019
@vinokurig vinokurig requested a review from a team August 30, 2019 14:44
Signed-off-by: Igor Vinokur <ivinokur@redhat.com>
@akosyakov
Copy link
Member

@vinokurig does it fix #5993?

@akosyakov akosyakov added this to the 0.10.x milestone Sep 2, 2019
@akosyakov
Copy link
Member

@vinokurig I think this fix is fine. Although I wonder: do you have git extension deployed already right? then we should not try to resolve it again, we have a check for the current deployment: https://github.com/theia-ide/theia/blob/master/packages/plugin-ext/src/main/node/plugin-deployer-impl.ts#L145 Maybe we should extend it and have a look into deployed state, i.e. into PluginDeployerHandler. If it is so please file an issue.

@vinokurig
Copy link
Contributor Author

@akosyakov

@vinokurig does it fix #5993?

It partially fixes the issue, now the error is:

root ERROR Failed to resolve plugins from 'vscode:extension/eclipse-cdt.cdt-gdb-vscode' Error: No extension
    at Request._callback (/home/ivinokur/projects/theia/packages/plugin-ext-vscode/lib/node/plugin-vscode-resolver.js:121:44)
    at Request.init.self.callback (/home/ivinokur/projects/theia/node_modules/request/request.js:185:22)
    at Request.emit (events.js:198:13)
    at Request.<anonymous> (/home/ivinokur/projects/theia/node_modules/request/request.js:1161:10)
    at Request.emit (events.js:198:13)
    at IncomingMessage.<anonymous> (/home/ivinokur/projects/theia/node_modules/request/request.js:1083:12)
    at Object.onceWrapper (events.js:286:20)
    at IncomingMessage.emit (events.js:203:15)
    at endReadableNT (_stream_readable.js:1145:12)
    at process._tickCallback (internal/process/next_tick.js:63:19)

Although I wonder: do you have git extension deployed already right? then we should not try to resolve it again, we have a check for the current deployment: https://github.com/theia-ide/theia/blob/master/packages/plugin-ext/src/main/node/plugin-deployer-impl.ts#L145 Maybe we should extend it and have a look into deployed state, i.e. into PluginDeployerHandler. If it is so please file an issue.

The initial problem is fixed now by 1a3048d but I think the return is still needed in that part of code.

@akosyakov
Copy link
Member

akosyakov commented Sep 2, 2019

@vinokurig yes, let's merge, but i think we also should prevent Failed to resolve plugins from by checking already deployed plugins if possible. Could you comment findings on #5993 please?

@vinokurig vinokurig merged commit c704c74 into master Sep 2, 2019
@vinokurig vinokurig deleted the pluginReturn branch September 2, 2019 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plug-in system issues related to the plug-in system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Github plugin installation fails
3 participants