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

plugin-dev - fix run/debug flow to work on windows + code clean #5608

Merged
merged 1 commit into from
Jul 2, 2019
Merged

plugin-dev - fix run/debug flow to work on windows + code clean #5608

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
7 changes: 7 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,12 @@
# Change Log

## v0.9.0

Breaking changes:

- [plugin] fixed typo in 'HostedInstanceState' enum from RUNNNING to RUNNING in `plugin-dev` extension
- [plugin] removed member `processOptions` from `AbstractHostedInstanceManager` as it is not initialized or used

## v0.8.0

- [core] added bépo keyboard layout
Expand Down
2 changes: 1 addition & 1 deletion packages/filesystem/src/common/filesystem.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ export interface FileSystem extends JsonRpcServer<FileSystemClient> {
* If the URI is not a file URI, undefined is returned.
*
* USE WITH CAUTION: You should always prefer URIs to paths if possible, as they are
* portable and platform independent. Pathes should only be used in cases you directly
* portable and platform independent. Paths should only be used in cases you directly
* interact with the OS, e.g. when running a command on the shell.
*/
getFsPath(uri: string): Promise<string | undefined>
Expand Down
3 changes: 3 additions & 0 deletions packages/plugin-dev/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,10 @@
"typings": "lib/common/index.d.ts",
"dependencies": {
"@theia/core": "^0.8.0",
"@theia/workspace": "^0.8.0",
"@theia/filesystem": "^0.8.0",
"@theia/plugin-ext": "^0.8.0",
"@theia/debug": "^0.8.0",
"@theia/preferences": "^0.8.0",
"ps-tree": "1.1.0"
},
Expand Down
8 changes: 4 additions & 4 deletions packages/plugin-dev/src/browser/hosted-plugin-controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ export class HostedPluginController implements FrontendApplicationContribution {
this.hostedPluginManagerClient.onStateChanged(e => {
if (e.state === HostedInstanceState.STARTING) {
this.onHostedPluginStarting();
} else if (e.state === HostedInstanceState.RUNNNING) {
} else if (e.state === HostedInstanceState.RUNNING) {
this.onHostedPluginRunning();
} else if (e.state === HostedInstanceState.STOPPED) {
this.onHostedPluginStopped();
Expand Down Expand Up @@ -149,7 +149,7 @@ export class HostedPluginController implements FrontendApplicationContribution {
* Display status bar element for running plugin.
*/
protected async onHostedPluginRunning(): Promise<void> {
this.pluginState = HostedInstanceState.RUNNNING;
this.pluginState = HostedInstanceState.RUNNING;

let entryText: string;
if (this.hostedPluginPreferences['hosted-plugin.watchMode'] && this.watcherSuccess) {
Expand Down Expand Up @@ -216,7 +216,7 @@ export class HostedPluginController implements FrontendApplicationContribution {
* @param event hosted instance state change event
*/
protected async handleWatchers(event: HostedInstanceData): Promise<void> {
if (event.state === HostedInstanceState.RUNNNING) {
if (event.state === HostedInstanceState.RUNNING) {
if (this.hostedPluginPreferences['hosted-plugin.watchMode']) {
await this.runWatchCompilation(event.pluginLocation.toString());
// update status bar
Expand Down Expand Up @@ -289,7 +289,7 @@ export class HostedPluginController implements FrontendApplicationContribution {
commands
});

if (this.pluginState === HostedInstanceState.RUNNNING) {
if (this.pluginState === HostedInstanceState.RUNNING) {
this.addCommandsForRunningPlugin(commands, menu);
} else if (this.pluginState === HostedInstanceState.STOPPED || this.pluginState === HostedInstanceState.FAILED) {
this.addCommandsForStoppedPlugin(commands, menu);
Expand Down
6 changes: 3 additions & 3 deletions packages/plugin-dev/src/browser/hosted-plugin-manager-client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ export namespace HostedPluginCommands {
export enum HostedInstanceState {
STOPPED = 'stopped',
STARTING = 'starting',
RUNNNING = 'running',
vince-fugnitto marked this conversation as resolved.
Show resolved Hide resolved
RUNNING = 'running',
STOPPING = 'stopping',
FAILED = 'failed'
}
Expand Down Expand Up @@ -161,7 +161,7 @@ export class HostedPluginManagerClient {
await this.openPluginWindow();

this.messageService.info('Hosted instance is running at: ' + this.pluginInstanceURL);
this.stateChanged.fire({ state: HostedInstanceState.RUNNNING, pluginLocation: this.pluginLocation });
this.stateChanged.fire({ state: HostedInstanceState.RUNNING, pluginLocation: this.pluginLocation });
} catch (error) {
this.messageService.error('Failed to run hosted plugin instance: ' + this.getErrorMessage(error));
this.stateChanged.fire({ state: HostedInstanceState.FAILED, pluginLocation: this.pluginLocation });
Expand Down Expand Up @@ -224,7 +224,7 @@ export class HostedPluginManagerClient {
this.pluginInstanceURL = await this.hostedPluginServer.runHostedPluginInstance(this.pluginLocation!.toString());
await this.openPluginWindow();
this.messageService.info('Hosted instance is running at: ' + this.pluginInstanceURL);
this.stateChanged.fire({ state: HostedInstanceState.RUNNNING, pluginLocation: this.pluginLocation! });
this.stateChanged.fire({ state: HostedInstanceState.RUNNING, pluginLocation: this.pluginLocation! });
return;
} catch (error) {
lastError = error;
Expand Down
25 changes: 13 additions & 12 deletions packages/plugin-dev/src/node/hosted-instance-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import { FileUri } from '@theia/core/lib/node/file-uri';
import { LogType } from '@theia/plugin-ext/lib/common/types';
import { HostedPluginSupport } from '@theia/plugin-ext/lib/hosted/node/hosted-plugin';
import { MetadataScanner } from '@theia/plugin-ext/lib/hosted/node/metadata-scanner';

const processTree = require('ps-tree');

export const HostedInstanceManager = Symbol('HostedInstanceManager');
Expand Down Expand Up @@ -97,8 +98,7 @@ delete PROCESS_OPTIONS.env.ELECTRON_RUN_AS_NODE;
@injectable()
export abstract class AbstractHostedInstanceManager implements HostedInstanceManager {
protected hostedInstanceProcess: cp.ChildProcess;
protected processOptions: cp.SpawnOptions;
Copy link
Member

Choose a reason for hiding this comment

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

Why is processOptions removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

it is not used. should i keep it anyway?

Copy link
Member

Choose a reason for hiding this comment

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

Same reason as #5608 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

as this member is never initialized or used I guess no one should use it. Updated the changelog.

Copy link
Member

@akosyakov akosyakov Jul 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

You cannot know it. Theia is a framework, there are plenty clients ouside of this repo. Please keep changes related to an issue. It seems to be one-liner to use FileUri.fsPath instead of toString.

protected isPluginRunnig: boolean = false;
protected isPluginRunning: boolean = false;
protected instanceUri: URI;
protected pluginUri: URI;
protected instanceOptions: object;
Expand All @@ -110,7 +110,7 @@ export abstract class AbstractHostedInstanceManager implements HostedInstanceMan
protected readonly metadata: MetadataScanner;

isRunning(): boolean {
return this.isPluginRunnig;
return this.isPluginRunning;
}

async run(pluginUri: URI, port?: number): Promise<URI> {
Expand All @@ -122,7 +122,7 @@ export abstract class AbstractHostedInstanceManager implements HostedInstanceMan
}

private async doRun(pluginUri: URI, port?: number, debugConfig?: DebugConfiguration): Promise<URI> {
if (this.isPluginRunnig) {
if (this.isPluginRunning) {
this.hostedPluginSupport.sendLog({ data: 'Hosted plugin instance is already running.', type: LogType.Info });
throw new Error('Hosted instance is already running.');
}
Expand All @@ -131,7 +131,8 @@ export abstract class AbstractHostedInstanceManager implements HostedInstanceMan
let processOptions: cp.SpawnOptions;
if (pluginUri.scheme === 'file') {
processOptions = { ...PROCESS_OPTIONS };
processOptions.env.HOSTED_PLUGIN = pluginUri.path.toString();
// get filesystem path that work cross operating systems
processOptions.env.HOSTED_PLUGIN = FileUri.fsPath(pluginUri.toString());

// Disable all the other plugins on this instance
processOptions.env.THEIA_PLUGINS = '';
Expand All @@ -154,7 +155,7 @@ export abstract class AbstractHostedInstanceManager implements HostedInstanceMan
}

terminate(): void {
if (this.isPluginRunnig) {
if (this.isPluginRunning) {
// tslint:disable-next-line:no-any
processTree(this.hostedInstanceProcess.pid, (err: Error, children: Array<any>) => {
// tslint:disable-next-line:no-any
Expand All @@ -168,14 +169,14 @@ export abstract class AbstractHostedInstanceManager implements HostedInstanceMan
}

getInstanceURI(): URI {
if (this.isPluginRunnig) {
if (this.isPluginRunning) {
return this.instanceUri;
}
throw new Error('Hosted plugin instance is not running.');
}

getPluginURI(): URI {
if (this.isPluginRunnig) {
if (this.isPluginRunning) {
return this.pluginUri;
}
throw new Error('Hosted plugin instance is not running.');
Expand Down Expand Up @@ -290,7 +291,7 @@ export abstract class AbstractHostedInstanceManager implements HostedInstanceMan
}

protected runHostedPluginTheiaInstance(command: string[], options: cp.SpawnOptions): Promise<URI> {
this.isPluginRunnig = true;
this.isPluginRunning = true;
return new Promise((resolve, reject) => {
let started = false;
const outputListener = (data: string | Buffer) => {
Expand All @@ -304,8 +305,8 @@ export abstract class AbstractHostedInstanceManager implements HostedInstanceMan
};

this.hostedInstanceProcess = cp.spawn(command.shift()!, command, options);
this.hostedInstanceProcess.on('error', () => { this.isPluginRunnig = false; });
this.hostedInstanceProcess.on('exit', () => { this.isPluginRunnig = false; });
this.hostedInstanceProcess.on('error', () => { this.isPluginRunning = false; });
this.hostedInstanceProcess.on('exit', () => { this.isPluginRunning = false; });
this.hostedInstanceProcess.stdout.addListener('data', outputListener);

this.hostedInstanceProcess.stdout.addListener('data', data => {
Expand All @@ -318,7 +319,7 @@ export abstract class AbstractHostedInstanceManager implements HostedInstanceMan
setTimeout(() => {
if (!started) {
this.terminate();
this.isPluginRunnig = false;
this.isPluginRunning = false;
reject(new Error('Timeout.'));
}
}, HOSTED_INSTANCE_START_TIMEOUT_MS);
Expand Down