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: use includeAllVersions when resolving packs #12658

Merged
merged 1 commit into from
Jun 29, 2023

Conversation

vince-fugnitto
Copy link
Member

What it does

The commit re-introduces the includeAllVersions flag when querying for extension-packs in order to find a compatible version of a declared plugin. Before the change we were not properly iterating the versions of a plugin which resulted in a plugin failing to download if the latest version was not compatible to our version range (ex: ms-vscode.js-debug).

The change means that we we will properly iterate the versions of a plugin until we hit a version which is compatible with the framework (with respect to our declared supported API range).

How to test

  1. confirm that CI for the yarn download:plugins script is successful

Review checklist

Reminder for reviewers

@vince-fugnitto vince-fugnitto added theia-cli issues related to the theia-cli builtins Issues related to VS Code builtin extensions labels Jun 28, 2023
@vince-fugnitto vince-fugnitto self-assigned this Jun 28, 2023
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

This fixes the download during the download plugins script, but we also need to adjust the runtime plugin download. See:

const { extensions } = await client.query({ extensionId: id });

Copy link
Member

@paul-marechal paul-marechal left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

The commit adds the `includeAllVersions` flag when querying for
compatible plugins which are resolved for extension-packs, else the
framework might fail early when a plugin is too new compared to our
supported API range.

Signed-off-by: vince-fugnitto <[email protected]>
@vince-fugnitto
Copy link
Member Author

@msujew thank you for catching it, I managed to fix the following three bugs:

  • the resolution of plugins in packs compared to out supported api version
  • the download of plugins at runtime
  • the ability to open an extension information (vsx-editor) from the extensions-view

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Works better, but installing the current German Language Pack from the vsx extension view still fails for some reason. It does work through the Configure Display Language command though, so I'm not sure what's wrong there. Can you confirm Vince?

@vince-fugnitto
Copy link
Member Author

Can you confirm Vince?

@msujew I can confirm as well, the query we use https://open-vsx.org/api/-/query?extensionId=ms-ceintl.vscode-language-pack-de&includeAllVersions=true does not seem to produce any results oddly enough (even when I tried on their swagger ui page.

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

I can confirm as well, the query we use https://open-vsx.org/api/-/query?extensionId=ms-ceintl.vscode-language-pack-de&includeAllVersions=true does not seem to produce any results oddly enough

I see. In that case we're not really responsible for the issue, as it appears to be a regression from open-vsx. The main issue that this change covers is fixed anyway, so I'll approve as well.

@vince-fugnitto
Copy link
Member Author

@msujew actually, it might just be the fact the open-vsx is case-sensitive:

The following would produce results.

@msujew
Copy link
Member

msujew commented Jun 29, 2023

@vince-fugnitto Oh well, that seems to be new... Any chance you can quickly get the case sensitivity issue fixed in this PR as well?

@vince-fugnitto
Copy link
Member Author

@msujew it seems like supporting case-sensitive values for parameters will cause changes in a lot of areas for plugins, namely for how we scan and read them and also determine a plugin is installed, and how we rely on the toLowerCase(). I did ask the question to open-vsx if the changes are intended (eclipse/openvsx#768) (afaik the restriction was not present before). I'll keep investigating what might be necessary to get everything to work.

@msujew
Copy link
Member

msujew commented Jun 29, 2023

@vince-fugnitto Sounds good. Though it makes sense to merge this fix now to get it into today's release.

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 theia-cli issues related to the theia-cli
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants