vsx-registry: Skip extension resolution if already installed #10624

msujew · 2022-01-12T16:27:50Z

What it does

Closes #10560 by comparing the versions of already installed extensions with the to-be-resolved extension version. When the extension already exists and its version is greater or equal to the one that should be installed, its download is skipped.

How to test

Install the hbenl.test-adapter-converter and hbenl.vscode-test-explorer (in this exact order) extension from here. The hbenl.vscode-test-explorer version has been increased to 2.42.0 to recreate the original issue.
Search for the "Mocha Test Explorer" (hbenl.vscode-mocha-test-adapter) and install it. It requires the hbenl.version-test-explorer as an extension dependency.
In the console where Theia was started, the message [hbenl.vscode-test-explorer]: is already installed with the same or newer version '2.42.0' should appear.
Confirm that no old version of the test adapter extension has been downloaded into your <user>/.theia directory.

Repeat all of the above but instead of installing the extension from the .vsix file, unpack the extensions into your plugins folder. Additionally perform the test by adding the .vsix directly to the plugins folder without unpacking them.

Restart the application and also confirm here that no downgrade is performed.

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

msujew · 2022-01-12T16:30:20Z

cc @luettmaSICKAG this should fix the issue you were experiencing. Do you mind testing this PR to confirm that the resolution works as expected?

packages/vsx-registry/src/common/vsx-version.ts

luettmaSICKAG · 2022-01-13T16:15:26Z

cc @luettmaSICKAG this should fix the issue you were experiencing. Do you mind testing this PR to confirm that the resolution works as expected?

Hmm sadly not completely. I couldn't test it with your plugins and the public OpenVSX marketplace due to our company proxy and no support in Theia yet.
I tested it with this branch and our private OpenVSX marketplace though.

The issue we have is a little bit different to the test you described in the PR.
Our problem is that the dependency is downgraded (in your example the hbenl.test-adapter-converter, not the hbenl.vscode-test-explorer).

When installing the plugins manually (from vsix) everything seems to work correctly (no downgrades).
When the dependencies are built-in they're resolved correctly during installation, but after a restart it just tries to download/update all plugins and seems to ignore the check.

Logs
I have renamed the plugins/files in the log entries, but haven't removed anything.
So this is what happens when I have a PLUGIN_WITH_DEPENDENCIES which depends on DEPENDENCY_A and DEPENDENCY_B.
DEPENDENCY_A and DEPENDENCY_B are provided built-in.
PLUGIN_WITH_DEPENDENCIES is installed manually ("Install from vsix"), but is also available in the Marketplace.

First start and installing the PLUGIN_WITH_DEPENDENCIES: 1_plugin-install-fresh-installation.log
Second start where the downgrade happens: 2_plugin-install-second-start.log

msujew · 2022-01-13T16:27:19Z

@luettmaSICKAG I was able to reproduce the dependency downgrade issue with the exact configuration that I use here as my testing steps on master.

Basically the extension dependency situation is as follows:

hbenl.vscode-mocha-test-adapter -> hbenl.vscode-test-explorer -> hbenl.test-adapter-converter

When installing my custom version 2.42.0 of hbenl.vscode-test-explorer through a vsix file, and afterwards downloading the hbenl.vscode-mocha-test-adapter extension using the vsx-registry UI, Theia automatically downloads the 2.20.3 version (a downgrade) of hbenl.vscode-test-explorer (which is an extension dependency of hbenl.vscode-mocha-test-adapter) and replaces the previously running version.

The hbenl.test-adapter-converter extension doesn't really have anything to do with it, it is just needed by the hbenl.vscode-test-explorer extension, but doesn't get automatically downloaded when installing the test explorer from the vsix file.

My PR fixes this exact behavior by identifying whether an extension dependency already exists in the list of deployed plugins and stops the extension resolution if the fetched version is not a version upgrade.

I hope that clears things up. Am I still wrong in the assumption that it mimics your setup?

luettmaSICKAG · 2022-01-13T16:31:47Z

Oh right sorry, I missed that. No the setup is correct.

Do you mind trying to test it with hbenl.test-adapter-converter and hbenl.vscode-test-explorer as built-in extension again and afterwards downloading the "Mocha Test Explorer"?
That'd be basically the same situation where I was still experiencing the issue.

msujew · 2022-01-13T16:39:34Z

@luettmaSICKAG No worries :)

I just retested the fix with unpacking the vsix files first and adding them to the list of built-in extensions. It still works correctly. I added the additional testing step to the original PR comment 👍

packages/vsx-registry/src/node/vsx-extension-resolver.ts

colin-grant-work

I have confirmed the bug on master and that this change addresses the issue. I have some misgivings about what happens in cases when this code does not apply and we do download a new version of the plugin, but this code reduces the frequency of that event, so it's at least a step in the right direction. 👍

luettmaSICKAG · 2022-01-20T08:55:05Z

I just tested it again on my private device, with your setup and the public OVSX registry.
I could still reproduce the problem with built-in plugins.

Not quite sure if the problem is on my side, so I just recorded it :s

What I did

I built Theia using yarn && yarn build
I added the two plugins from "How to test, step 1" to the /plugins directory of the repo (without extracting)
Cleared the directories %USERPROFILE%/.theia/extensions and %LOCALAPPDATA%/Temp/vscode-unpacked
Started Theia (electron) yarn electron start
Installed the "Mocha Test Explorer" ("How to test, step 2")
Restart Theia
Now hbenl.vscode-test-explorer is downgraded and installed to %USERPROFILE%/.theia/extensions

msujew · 2022-01-20T10:48:17Z

@luettmaSICKAG Thanks for looking into this again. You're right, when restarting the app, the plugin resolving mechanism starts a second time, this time downloading the outdated version, since the plugins aren't deployed yet. I'll look into a solution for this 👍

msujew · 2022-01-20T12:02:39Z

@colin-grant-work Do you mind checking out step 6 of the testing steps? My newest changes fixes a restart-related downgrade issue. Previously, system and user extensions where resolved independently of each other when starting the plugin host, resulting in a downgrade of system extensions when a user extension depends on it. This only happens for packed extensions, since it is part of a racing condition.

colin-grant-work · 2022-01-21T15:21:14Z

packages/plugin-ext/src/main/node/plugin-deployer-impl.ts

        this.logMeasurement('Deploy plugin entry', startDeployTime);
    }

-    protected async deployMultipleEntries(pluginEntries: ReadonlyArray<string>, type: PluginType = PluginType.System): Promise<void> {


Is this (and resolvePlugins) the change that requires the introduction of the UnresolvedPluginEntry interface? It looks like previously we could only have a list of plugins with uniform plugin types, and now we want to have a list of plugin with mixed types. Otherwise, it looks like the same information was being passed around everywhere, just not necessarily in the same struct package?

Exactly, we need the mixed type since we need to resolve all user and builtin extensions in the first step. Though it's mostly resolvePlugins, since the main culprit of this issue is introduced here:

theia/packages/plugin-ext/src/main/node/plugin-deployer-impl.ts

Lines 121 to 124 in c56aa23

const [userPlugins, systemPlugins] = await Promise.all([

this.resolvePlugins(context.userEntries, PluginType.User),

this.resolvePlugins(context.systemEntries, PluginType.System)

]);

colin-grant-work

I confirm that the new changes also prevent the inappropriate loading of dependencies during startup - the existing builtin plugins are used to satisfy the dependencies of user plugins.

…-theia#10624) * Existing extensions are skipped on extension download * All extensions are considered for dependencies during startup Signed-off-by: Federico Bozzini <Federico.Bozzini@arm.com>

Skip extension resolution if already installed

74db1db

msujew added plug-in system issues related to the plug-in system vsx-registry Issues related to Open VSX Registry Integration labels Jan 12, 2022

colin-grant-work reviewed Jan 12, 2022

View reviewed changes

packages/vsx-registry/src/common/vsx-version.ts Outdated Show resolved Hide resolved

colin-grant-work reviewed Jan 13, 2022

View reviewed changes

packages/vsx-registry/src/node/vsx-extension-resolver.ts Outdated Show resolved Hide resolved

Use semver to compare versions

cdaee64

msujew force-pushed the msujew/vsx-check-version branch from 0095840 to cdaee64 Compare January 17, 2022 11:18

colin-grant-work self-requested a review January 19, 2022 20:50

colin-grant-work approved these changes Jan 19, 2022

View reviewed changes

Improve resolvePlugin order

fd77aa5

msujew requested a review from colin-grant-work January 20, 2022 12:02

colin-grant-work reviewed Jan 21, 2022

View reviewed changes

colin-grant-work approved these changes Jan 21, 2022

View reviewed changes

msujew merged commit d6e4971 into master Jan 21, 2022

msujew deleted the msujew/vsx-check-version branch January 21, 2022 21:37

github-actions bot added this to the 1.22.0 milestone Jan 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vsx-registry: Skip extension resolution if already installed #10624

vsx-registry: Skip extension resolution if already installed #10624

msujew commented Jan 12, 2022 •

edited

Loading

msujew commented Jan 12, 2022

luettmaSICKAG commented Jan 13, 2022

msujew commented Jan 13, 2022 •

edited

Loading

luettmaSICKAG commented Jan 13, 2022

msujew commented Jan 13, 2022

colin-grant-work left a comment

luettmaSICKAG commented Jan 20, 2022

msujew commented Jan 20, 2022 •

edited

Loading

msujew commented Jan 20, 2022 •

edited

Loading

colin-grant-work Jan 21, 2022 •

edited

Loading

msujew Jan 21, 2022

colin-grant-work left a comment

	const [userPlugins, systemPlugins] = await Promise.all([
	this.resolvePlugins(context.userEntries, PluginType.User),
	this.resolvePlugins(context.systemEntries, PluginType.System)
	]);

vsx-registry: Skip extension resolution if already installed #10624

vsx-registry: Skip extension resolution if already installed #10624

Conversation

msujew commented Jan 12, 2022 • edited Loading

What it does

How to test

Review checklist

Reminder for reviewers

msujew commented Jan 12, 2022

luettmaSICKAG commented Jan 13, 2022

msujew commented Jan 13, 2022 • edited Loading

luettmaSICKAG commented Jan 13, 2022

msujew commented Jan 13, 2022

colin-grant-work left a comment

Choose a reason for hiding this comment

luettmaSICKAG commented Jan 20, 2022

msujew commented Jan 20, 2022 • edited Loading

msujew commented Jan 20, 2022 • edited Loading

colin-grant-work Jan 21, 2022 • edited Loading

Choose a reason for hiding this comment

msujew Jan 21, 2022

Choose a reason for hiding this comment

colin-grant-work left a comment

Choose a reason for hiding this comment

msujew commented Jan 12, 2022 •

edited

Loading

msujew commented Jan 13, 2022 •

edited

Loading

msujew commented Jan 20, 2022 •

edited

Loading

msujew commented Jan 20, 2022 •

edited

Loading

colin-grant-work Jan 21, 2022 •

edited

Loading