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.

Sign up for GitHub

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

fix plugins are not loaded when staring theia without internet access #6056

Closed
wants to merge 1 commit into from
Closed

fix plugins are not loaded when staring theia without internet access #6056

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/plugin-ext-vscode/src/node/scanner-vscode.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ export class VsCodePluginScanner extends TheiaPluginScanner implements PluginSca
const result: PluginModel = {
// see id definition: https://github.com/microsoft/vscode/blob/15916055fe0cb9411a5f36119b3b012458fe0a1d/src/vs/platform/extensions/common/extensions.ts#L167-L169
id: `${plugin.publisher.toLowerCase()}.${plugin.name.toLowerCase()}`,
uniqueId: `${this.VSCODE_PREFIX}${plugin.publisher.toLowerCase()}.${plugin.name.toLowerCase()}`,
name: plugin.name,
publisher: plugin.publisher,
version: plugin.version,
Expand Down Expand Up @@ -63,6 +64,6 @@ export class VsCodePluginScanner extends TheiaPluginScanner implements PluginSca
* to an array of deployable extension dependencies
*/
private getDeployableDependencies(dependencies: string[]): string[] {
return dependencies.map(dep => this.VSCODE_PREFIX + dep);
return dependencies.map(dep => this.VSCODE_PREFIX + dep.toLowerCase());
}
}
1 change: 1 addition & 0 deletions packages/plugin-ext/src/common/plugin-api-rpc.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@ export const emptyPlugin: Plugin = {
},
model: {
id: 'emptyPlugin',
uniqueId: 'emptyPlugin',
name: 'emptyPlugin',
publisher: 'Theia',
version: 'empty',
Expand Down
1 change: 1 addition & 0 deletions packages/plugin-ext/src/common/plugin-protocol.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,7 @@ export interface PluginDeployerDirectoryHandlerContext {
*/
export interface PluginModel {
id: string;
uniqueId: string;
name: string;
publisher: string;
version: string;
Expand Down
1 change: 1 addition & 0 deletions packages/plugin-ext/src/hosted/node/scanners/scanner-theia.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ export class TheiaPluginScanner implements PluginScanner {
const result: PluginModel = {
// see id definition: https://github.com/microsoft/vscode/blob/15916055fe0cb9411a5f36119b3b012458fe0a1d/src/vs/platform/extensions/common/extensions.ts#L167-L169
id: `${plugin.publisher.toLowerCase()}.${plugin.name.toLowerCase()}`,
uniqueId: `${plugin.publisher.toLowerCase()}.${plugin.name.toLowerCase()}`,
name: plugin.name,
publisher: plugin.publisher,
version: plugin.version,
Expand Down
6 changes: 3 additions & 3 deletions packages/plugin-ext/src/main/node/plugin-deployer-impl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ export class PluginDeployerImpl implements PluginDeployer {
const queue = [...pluginEntries];
while (queue.length) {
const current = queue.shift()!;
if (visited.has(current)) {
if (visited.has(current) || pluginsToDeploy.has(current)) {
Copy link
Member

@akosyakov akosyakov Aug 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

current is deployment entry, it is not a plugin id

One can only check by a plugin id after resolution without it we don't know anything what is behind current.

continue;
}
visited.add(current);
Expand All @@ -131,8 +131,8 @@ export class PluginDeployerImpl implements PluginDeployer {

for (const deployerEntry of pluginDeployerEntries) {
const metadata = await this.pluginDeployerHandler.getPluginMetadata(deployerEntry);
if (metadata && !pluginsToDeploy.has(metadata.model.id)) {
pluginsToDeploy.set(metadata.model.id, deployerEntry);
if (metadata && !pluginsToDeploy.has(metadata.model.uniqueId)) {
Copy link
Member

@akosyakov akosyakov Aug 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

metadata.model.id is already unique, the check is already in place

Why it is not enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for metadata.model.extensionDependencies will add a prefix of vscode:extension/
see https://github.com/theia-ide/theia/blob/85b75768f702656db4ee8ff6eaad303be8102643/packages/plugin-ext-vscode/src/node/scanner-vscode.ts#L66

Copy link
Member

Choose a reason for hiding this comment

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

But at the end the extension dependency will be resolved to publisher.name and should be filtered out by pluginsToDeploy.has(metadata.model.id), since there is already an extension with such publisher and name installed from local vsix.

metadata.model.id has to be unique identifier among all extensions, there should not be another

Copy link
Contributor Author

@tom-shan tom-shan Aug 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry I misunderstood your question.

metadata.model.id is already unique.

That's right.

The reason I use metadata.model.uniqueId is that:
if using pluginsToDeploy.set(metadata.model.id, deployerEntry), considering queue.push(...metadata.model.extensionDependencies),
then queue will have some extensionDependencies elements which have the 'vscode:extension/' suffix,
and if (visited.has(current) || pluginsToDeploy.has(current)) { will not be true.

Because the keys of pluginsToDeploy and the elements of queue will never be equal even for the extensionDependencies.

Copy link
Member

Choose a reason for hiding this comment

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

It is expected.

Copy link
Contributor Author

@tom-shan tom-shan Aug 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

But we should not continue to resolve the extensionDependencies when pluginsToDeploy contains them.

Copy link
Member

Choose a reason for hiding this comment

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

Why not?

Copy link
Contributor Author

@tom-shan tom-shan Aug 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Because it will resolve twice for the same plugin.

pluginsToDeploy.set(metadata.model.uniqueId, deployerEntry);
if (metadata.model.extensionDependencies) {
queue.push(...metadata.model.extensionDependencies);
}
Expand Down