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

[cli] Keep downloaded .vsix plugins as they are #7093

Merged
merged 1 commit into from
Feb 6, 2020

Conversation

marcdumais-work
Copy link
Contributor

@marcdumais-work marcdumais-work commented Feb 6, 2020

What it does

With this patch the downloaded VS Code extensions are kept in their original .vsix format. This should
make it possible for the plugins folder and contained extensions to work even if they are read-only, even if one or more extension writes inside their own folder at runtime, like e.g. the Java extension by Red Hat does.

Concretely this will permit packaging Electron Theia applications along with a set of VS Code extensions considered "default", like the builtins and other wanted extensions. Most operating system packagers install the packages globally on a machine, and have the files writable only by root. By keeping the .vsix in their original format, they will, at app startup, be unzipped in the host's temporary folder under ownership of the current user and so be writable by them. This way, if any extension writes to its own folder, it will not fail.

How to test

$> yarn

Then verify the content of the plugins folder. The downloaded extensions should be in the .vsix format rather than unzipped.

Run the example applications and confirm that plugins are detected and work like before

Review checklist

Reminder for reviewers

Keep the vs code extensions in their original .vsix format. This should
make it possible for the plugins folder and contained extensions to work
even if they are read-only, even if one or more extension writes inside
their own folder at runtime, like e.g. the Java extension by Red Hat does.

Concretely this will permit packaging Electron Theia applications along
with a set of VS Code extensions considered "default", like the builtins
and other wanted extensions. Most operating system packagers install the
packages globally on a machine, and have the files writable only by
root. By keeping the .vsix in their original format, they will, at app
startup, be unzipped in the host's temporary folder under ownership of
the current user and so be writable by them. This way, if any extension
writes to its own folder, it will not fail.

Signed-off-by: Marc Dumais <marc.dumais@ericsson.com>
@vince-fugnitto vince-fugnitto added the builtins Issues related to VS Code builtin extensions label Feb 6, 2020
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

The changes look good to me, I've verified that:

  • the plugins are kept in .vsix form when calling yarn download:plugins.
  • the plugins still work correctly (ex: vscode-builtin-npm contributes a view to the Explorer).
  • the already downloaded plugins are skipped properly.
  • the above mentioned checks work correctly in Electron as well.

Copy link
Contributor

@benoitf benoitf left a comment

Choose a reason for hiding this comment

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

ok. they'll expanded later but then we see files instead of folders in plugins folder

@marcdumais-work
Copy link
Contributor Author

ok. they'll expanded later but then we see files instead of folders in plugins folder

Yes, that's correct. They'll be expanded when the Theia application starts, saved under /tmp/vscode-unpacked/ .

@marcdumais-work
Copy link
Contributor Author

Thanks for the review @vince-fugnitto and @benoitf - merging

@marcdumais-work marcdumais-work merged commit d54c2f0 into master Feb 6, 2020
@marcdumais-work marcdumais-work deleted the plugins-kept-zipped branch February 6, 2020 15:27
Copy link
Contributor

@lmcbout lmcbout left a comment

Choose a reason for hiding this comment

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

Tested on Ubuntu 18.04
Keeping the file as ".vsix" allows to build an AppImage with JAVA and to run OK with the language server which tries to write to the plugin. If we unzip the plugins before, running an AppImage with JAVA did not execute properly with the language server since the permission were set to "read only" for the end-user.
Works nicely
Thanks @marcdumais-work

@akosyakov
Copy link
Member

It is significantly increased deployment time around 10x from 300ms to 3s. We should rather work on keeping all extracted extensions in one place under user product directory and let this script to extract it there at build time. I hope the code doing such extraction can be reused between plugin-ext and script.

@marcdumais-work
Copy link
Contributor Author

marcdumais-work commented Feb 10, 2020

It is significantly increased deployment time around 10x from 300ms to 3s.

Sounds about right. This should happen only the first time however, afterwards the /tmp/vscode-unpacked folder should be reused (until next reboot I guess).

We should rather work on keeping all extracted extensions in one place under user product directory and let this script to extract it there at build time. I hope the code doing such extraction can be reused between plugin-ext and script.

Can you clarify what you mean by "user product directory"?

We encountered an issue when packaging an Electron app: the app and extracted extensions are read-only for all but most primitives packagings (.tar.gz), and we found at least one extension that relies on creating files/folders under itself: Red Hat Java. AppImage format is really interesting/convenient on Linux, but all the content is read-only / static - even playing with permissions I do not think we can change that.

@vince-fugnitto suggests we could have the script unpack the extensions by default but have the option to leave them as .vsix if wanted (update: I see you suggest the same thing on a theia-apps PR so it looks like a good way forward)

@akosyakov
Copy link
Member

@marcdumais-work I mean for instance there is a Theia product based alpha, then all unpacked extensions should leave under ${user_home}/.alpha/extensions. At runtime Theia should load from there and install there. If an extension is already present there with the greater version installation should no do anything. The plugin download script should install there by default as well with the same logic. With such approach there is no startup cost, extensions are not lost between application restarts and it is a good default for both, browser and electron, targets. Custom products though should be able to disable such default if it is required.

@marcdumais-work
Copy link
Contributor Author

I mean for instance there is a Theia product based alpha, then all unpacked extensions should leave under ${user_home}/.alpha/extensions. At runtime Theia should load from there and install there. If an extension is already present there with the greater version installation should no do anything.

Sounds good.

The plugin download script should install there by default as well with the same logic.

Does that imply that the "default" extensions (defined in package.json) will be installed at runtime by running the download script? At least the first time a user starts a given packaged Electron Theia application?

With such approach there is no startup cost, extensions are not lost between application restarts and it is a good default for both, browser and electron, targets. Custom products though should be able to disable such default if it is required.

+1

@akosyakov
Copy link
Member

akosyakov commented Feb 10, 2020

Does that imply that the "default" extensions (defined in package.json) will be installed at runtime by running the download script? At least the first time a user starts a given packaged Electron Theia application?

I thought this script could be moved to be a binary exposed by the plugin-ext package to reuse installation code, and then we call run installation at build time to avoid any unpacking on first start.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builtins Issues related to VS Code builtin extensions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants