From 5ec191500028582e687c950b78966e4b7aefaad8 Mon Sep 17 00:00:00 2001 From: dankeboy36 <111981763+dankeboy36@users.noreply.github.com> Date: Mon, 10 Mar 2025 09:20:22 +0100 Subject: [PATCH] fix(plugin): decouple state update from the LS (#2643) * fix(plugin): decouple state update from the LS To enhance the reliability of Arduino IDE extensions, the update process for `ArduinoState` has been modified to ensure independence from the language server's availability. This change addresses issues caused by `compileSummary` being `undefined` due to potential startup failures of the Arduino Language Server, as noted in https://github.com/dankeboy36/esp-exception-decoder/issues/28#issuecomment-2681800772. The `compile` command now resolves with a `CompileSummary` rather than `void`, facilitating a more reliable way for extensions to access necessary data. Furthermore, the command has been adjusted to allow resolution with `undefined` when the compiled data is partial. By transitioning to direct usage of the resolved compile value for state updates, the reliance on executed commands for extensions is eliminated. This update also moves the VSIX command execution to the frontend without altering existing IDE behavior. Closes arduino/arduino-ide#2642 Signed-off-by: dankeboy36 * fix: install missing libx11-dev and libxkbfile-dev Signed-off-by: dankeboy36 * fix: pick better GH step name Signed-off-by: dankeboy36 * fix: install the required dependencies on Linux Signed-off-by: dankeboy36 * fix(revert): do not manually install deps on Linux Signed-off-by: dankeboy36 * chore: pin `ubuntu-22.04` for linux actions * fix: restore accidentally removed dispose on finally Signed-off-by: dankeboy36 * fix(test): align mock naming :lipstick: Signed-off-by: dankeboy36 * fix: let the ino contribution notify the LS + event emitter dispatches the new state. Signed-off-by: dankeboy36 * fix(test): emit the new compiler summary state Signed-off-by: dankeboy36 * chore(revert): unpin linux version, use latest revert of https://github.com/arduino/arduino-ide/commit/b11bde1c473322c86dfde2dd3cec3ebbf92011fa Signed-off-by: dankeboy36 --------- Signed-off-by: dankeboy36 Co-authored-by: Giacomo Cusinato <7659518+giacomocusinato@users.noreply.github.com> --- .../browser/arduino-ide-frontend-module.ts | 7 ++- .../src/browser/contributions/ino-language.ts | 39 +++++++++++++ .../contributions/update-arduino-state.ts | 25 ++++---- .../browser/contributions/verify-sketch.ts | 39 +++++++++++-- .../src/common/protocol/core-service.ts | 2 +- .../src/node/core-service-impl.ts | 55 +++++------------- .../test/browser/update-arduino-state.test.ts | 18 ++++-- .../test/node/core-service-impl.slow-test.ts | 58 +++---------------- 8 files changed, 132 insertions(+), 111 deletions(-) diff --git a/arduino-ide-extension/src/browser/arduino-ide-frontend-module.ts b/arduino-ide-extension/src/browser/arduino-ide-frontend-module.ts index d6779c30..342516c0 100644 --- a/arduino-ide-extension/src/browser/arduino-ide-frontend-module.ts +++ b/arduino-ide-extension/src/browser/arduino-ide-frontend-module.ts @@ -131,7 +131,10 @@ import { OpenSketch } from './contributions/open-sketch'; import { Close } from './contributions/close'; import { SaveAsSketch } from './contributions/save-as-sketch'; import { SaveSketch } from './contributions/save-sketch'; -import { VerifySketch } from './contributions/verify-sketch'; +import { + CompileSummaryProvider, + VerifySketch, +} from './contributions/verify-sketch'; import { UploadSketch } from './contributions/upload-sketch'; import { CommonFrontendContribution } from './theia/core/common-frontend-contribution'; import { EditContributions } from './contributions/edit-contributions'; @@ -788,6 +791,8 @@ export default new ContainerModule((bind, unbind, isBound, rebind) => { Contribution.configure(bind, BoardsDataMenuUpdater); Contribution.configure(bind, AutoSelectProgrammer); + bind(CompileSummaryProvider).toService(VerifySketch); + bindContributionProvider(bind, StartupTaskProvider); bind(StartupTaskProvider).toService(BoardsServiceProvider); // to reuse the boards config in another window diff --git a/arduino-ide-extension/src/browser/contributions/ino-language.ts b/arduino-ide-extension/src/browser/contributions/ino-language.ts index 4f336ef3..4f42d399 100644 --- a/arduino-ide-extension/src/browser/contributions/ino-language.ts +++ b/arduino-ide-extension/src/browser/contributions/ino-language.ts @@ -8,6 +8,7 @@ import { ArduinoDaemon, BoardIdentifier, BoardsService, + CompileSummary, ExecutableService, isBoardIdentifierChangeEvent, sanitizeFqbn, @@ -23,6 +24,7 @@ import { HostedPluginEvents } from '../hosted/hosted-plugin-events'; import { NotificationCenter } from '../notification-center'; import { CurrentSketch } from '../sketches-service-client-impl'; import { SketchContribution, URI } from './contribution'; +import { CompileSummaryProvider } from './verify-sketch'; interface DaemonAddress { /** @@ -107,6 +109,8 @@ export class InoLanguage extends SketchContribution { private readonly notificationCenter: NotificationCenter; @inject(BoardsDataStore) private readonly boardDataStore: BoardsDataStore; + @inject(CompileSummaryProvider) + private readonly compileSummaryProvider: CompileSummaryProvider; private readonly toDispose = new DisposableCollection(); private readonly languageServerStartMutex = new Mutex(); @@ -173,6 +177,13 @@ export class InoLanguage extends SketchContribution { } } }), + this.compileSummaryProvider.onDidChangeCompileSummary( + (compileSummary) => { + if (compileSummary) { + this.fireBuildDidComplete(compileSummary); + } + } + ), ]); Promise.all([ this.boardsServiceProvider.ready, @@ -317,4 +328,32 @@ export class InoLanguage extends SketchContribution { params ); } + + // Execute the a command contributed by the Arduino Tools VSIX to send the `ino/buildDidComplete` notification to the language server + private async fireBuildDidComplete( + compileSummary: CompileSummary + ): Promise { + const params = { + ...compileSummary, + }; + console.info( + `Executing 'arduino.languageserver.notifyBuildDidComplete' with ${JSON.stringify( + params.buildOutputUri + )}` + ); + + try { + await this.commandService.executeCommand( + 'arduino.languageserver.notifyBuildDidComplete', + params + ); + } catch (err) { + console.error( + `Unexpected error when firing event on build did complete. ${JSON.stringify( + params.buildOutputUri + )}`, + err + ); + } + } } diff --git a/arduino-ide-extension/src/browser/contributions/update-arduino-state.ts b/arduino-ide-extension/src/browser/contributions/update-arduino-state.ts index 767fbbf8..bce3fa85 100644 --- a/arduino-ide-extension/src/browser/contributions/update-arduino-state.ts +++ b/arduino-ide-extension/src/browser/contributions/update-arduino-state.ts @@ -1,13 +1,11 @@ import { DisposableCollection } from '@theia/core/lib/common/disposable'; import URI from '@theia/core/lib/common/uri'; import { inject, injectable } from '@theia/core/shared/inversify'; -import { HostedPluginSupport } from '../hosted/hosted-plugin-support'; import type { ArduinoState } from 'vscode-arduino-api'; import { + BoardsConfig, BoardsService, CompileSummary, - isCompileSummary, - BoardsConfig, PortIdentifier, resolveDetectedPort, } from '../../common/protocol'; @@ -18,8 +16,10 @@ import { } from '../../common/protocol/arduino-context-mapper'; import { BoardsDataStore } from '../boards/boards-data-store'; import { BoardsServiceProvider } from '../boards/boards-service-provider'; +import { HostedPluginSupport } from '../hosted/hosted-plugin-support'; import { CurrentSketch } from '../sketches-service-client-impl'; import { SketchContribution } from './contribution'; +import { CompileSummaryProvider } from './verify-sketch'; /** * (non-API) exported for tests @@ -43,6 +43,8 @@ export class UpdateArduinoState extends SketchContribution { private readonly boardsDataStore: BoardsDataStore; @inject(HostedPluginSupport) private readonly hostedPluginSupport: HostedPluginSupport; + @inject(CompileSummaryProvider) + private readonly compileSummaryProvider: CompileSummaryProvider; private readonly toDispose = new DisposableCollection(); @@ -60,14 +62,13 @@ export class UpdateArduinoState extends SketchContribution { this.configService.onDidChangeSketchDirUri((userDirUri) => this.updateUserDirPath(userDirUri) ), - this.commandService.onDidExecuteCommand(({ commandId, args }) => { - if ( - commandId === 'arduino.languageserver.notifyBuildDidComplete' && - isCompileSummary(args[0]) - ) { - this.updateCompileSummary(args[0]); + this.compileSummaryProvider.onDidChangeCompileSummary( + (compilerSummary) => { + if (compilerSummary) { + this.updateCompileSummary(compilerSummary); + } } - }), + ), this.boardsDataStore.onDidChange((event) => { const selectedFqbn = this.boardsServiceProvider.boardsConfig.selectedBoard?.fqbn; @@ -88,6 +89,10 @@ export class UpdateArduinoState extends SketchContribution { this.updateSketchPath(this.sketchServiceClient.tryGetCurrentSketch()); this.updateUserDirPath(this.configService.tryGetSketchDirUri()); this.updateDataDirPath(this.configService.tryGetDataDirUri()); + const { compileSummary } = this.compileSummaryProvider; + if (compileSummary) { + this.updateCompileSummary(compileSummary); + } } onStop(): void { diff --git a/arduino-ide-extension/src/browser/contributions/verify-sketch.ts b/arduino-ide-extension/src/browser/contributions/verify-sketch.ts index 4d8b445e..22693085 100644 --- a/arduino-ide-extension/src/browser/contributions/verify-sketch.ts +++ b/arduino-ide-extension/src/browser/contributions/verify-sketch.ts @@ -1,7 +1,7 @@ -import { Emitter } from '@theia/core/lib/common/event'; +import { Emitter, Event } from '@theia/core/lib/common/event'; import { nls } from '@theia/core/lib/common/nls'; import { inject, injectable } from '@theia/core/shared/inversify'; -import type { CoreService } from '../../common/protocol'; +import type { CompileSummary, CoreService } from '../../common/protocol'; import { ArduinoMenus } from '../menu/arduino-menus'; import { CurrentSketch } from '../sketches-service-client-impl'; import { ArduinoToolbar } from '../toolbar/arduino-toolbar'; @@ -15,6 +15,12 @@ import { } from './contribution'; import { CoreErrorHandler } from './core-error-handler'; +export const CompileSummaryProvider = Symbol('CompileSummaryProvider'); +export interface CompileSummaryProvider { + readonly compileSummary: CompileSummary | undefined; + readonly onDidChangeCompileSummary: Event; +} + export type VerifySketchMode = /** * When the user explicitly triggers the verify command from the primary UI: menu, toolbar, or keybinding. The UI shows the output, updates the toolbar items state, etc. @@ -46,13 +52,20 @@ export interface VerifySketchParams { type VerifyProgress = 'idle' | VerifySketchMode; @injectable() -export class VerifySketch extends CoreServiceContribution { +export class VerifySketch + extends CoreServiceContribution + implements CompileSummaryProvider +{ @inject(CoreErrorHandler) private readonly coreErrorHandler: CoreErrorHandler; private readonly onDidChangeEmitter = new Emitter(); private readonly onDidChange = this.onDidChangeEmitter.event; + private readonly onDidChangeCompileSummaryEmitter = new Emitter< + CompileSummary | undefined + >(); private verifyProgress: VerifyProgress = 'idle'; + private _compileSummary: CompileSummary | undefined; override registerCommands(registry: CommandRegistry): void { registry.registerCommand(VerifySketch.Commands.VERIFY_SKETCH, { @@ -117,6 +130,21 @@ export class VerifySketch extends CoreServiceContribution { super.handleError(error); } + get compileSummary(): CompileSummary | undefined { + return this._compileSummary; + } + + private updateCompileSummary( + compileSummary: CompileSummary | undefined + ): void { + this._compileSummary = compileSummary; + this.onDidChangeCompileSummaryEmitter.fire(this._compileSummary); + } + + get onDidChangeCompileSummary(): Event { + return this.onDidChangeCompileSummaryEmitter.event; + } + private async verifySketch( params?: VerifySketchParams ): Promise { @@ -141,7 +169,7 @@ export class VerifySketch extends CoreServiceContribution { return options; } - await this.doWithProgress({ + const compileSummary = await this.doWithProgress({ progressText: nls.localize( 'arduino/sketch/compile', 'Compiling sketch...' @@ -160,6 +188,9 @@ export class VerifySketch extends CoreServiceContribution { nls.localize('arduino/sketch/doneCompiling', 'Done compiling.'), { timeout: 3000 } ); + + this.updateCompileSummary(compileSummary); + // Returns with the used options for the compilation // so that follow-up tasks (such as upload) can reuse the compiled code. // Note that the `fqbn` is already decorated with the board settings, if any. diff --git a/arduino-ide-extension/src/common/protocol/core-service.ts b/arduino-ide-extension/src/common/protocol/core-service.ts index 2b4a0765..f4b0d8f0 100644 --- a/arduino-ide-extension/src/common/protocol/core-service.ts +++ b/arduino-ide-extension/src/common/protocol/core-service.ts @@ -171,7 +171,7 @@ export interface CoreService { compile( options: CoreService.Options.Compile, cancellationToken?: CancellationToken - ): Promise; + ): Promise; upload( options: CoreService.Options.Upload, cancellationToken?: CancellationToken diff --git a/arduino-ide-extension/src/node/core-service-impl.ts b/arduino-ide-extension/src/node/core-service-impl.ts index 285c05f7..2e2eb21a 100644 --- a/arduino-ide-extension/src/node/core-service-impl.ts +++ b/arduino-ide-extension/src/node/core-service-impl.ts @@ -1,7 +1,6 @@ import { type ClientReadableStream } from '@grpc/grpc-js'; import { ApplicationError } from '@theia/core/lib/common/application-error'; import type { CancellationToken } from '@theia/core/lib/common/cancellation'; -import { CommandService } from '@theia/core/lib/common/command'; import { Disposable, DisposableCollection, @@ -69,15 +68,13 @@ export class CoreServiceImpl extends CoreClientAware implements CoreService { private readonly responseService: ResponseService; @inject(MonitorManager) private readonly monitorManager: MonitorManager; - @inject(CommandService) - private readonly commandService: CommandService; @inject(BoardDiscovery) private readonly boardDiscovery: BoardDiscovery; async compile( options: CoreService.Options.Compile, cancellationToken?: CancellationToken - ): Promise { + ): Promise { const coreClient = await this.coreClient; const { client, instance } = coreClient; const request = this.compileRequest(options, instance); @@ -91,7 +88,7 @@ export class CoreServiceImpl extends CoreClientAware implements CoreService { ); const toDisposeOnFinally = new DisposableCollection(handler); - return new Promise((resolve, reject) => { + return new Promise((resolve, reject) => { let hasRetried = false; const handleUnexpectedError = (error: Error) => { @@ -164,50 +161,26 @@ export class CoreServiceImpl extends CoreClientAware implements CoreService { call .on('data', handler.onData) .on('error', handleError) - .on('end', resolve); + .on('end', () => { + if (isCompileSummary(compileSummary)) { + resolve(compileSummary); + } else { + console.error( + `Have not received the full compile summary from the CLI while running the compilation. ${JSON.stringify( + compileSummary + )}` + ); + resolve(undefined); + } + }); }; startCompileStream(); }).finally(() => { toDisposeOnFinally.dispose(); - if (!isCompileSummary(compileSummary)) { - if (cancellationToken && cancellationToken.isCancellationRequested) { - // NOOP - return; - } - console.error( - `Have not received the full compile summary from the CLI while running the compilation. ${JSON.stringify( - compileSummary - )}` - ); - } else { - this.fireBuildDidComplete(compileSummary); - } }); } - // This executes on the frontend, the VS Code extension receives it, and sends an `ino/buildDidComplete` notification to the language server. - private fireBuildDidComplete(compileSummary: CompileSummary): void { - const params = { - ...compileSummary, - }; - console.info( - `Executing 'arduino.languageserver.notifyBuildDidComplete' with ${JSON.stringify( - params.buildOutputUri - )}` - ); - this.commandService - .executeCommand('arduino.languageserver.notifyBuildDidComplete', params) - .catch((err) => - console.error( - `Unexpected error when firing event on build did complete. ${JSON.stringify( - params.buildOutputUri - )}`, - err - ) - ); - } - private compileRequest( options: CoreService.Options.Compile & { exportBinaries?: boolean; diff --git a/arduino-ide-extension/src/test/browser/update-arduino-state.test.ts b/arduino-ide-extension/src/test/browser/update-arduino-state.test.ts index 31a534f3..fbbf1751 100644 --- a/arduino-ide-extension/src/test/browser/update-arduino-state.test.ts +++ b/arduino-ide-extension/src/test/browser/update-arduino-state.test.ts @@ -31,6 +31,7 @@ import { UpdateArduinoState, UpdateStateParams, } from '../../browser/contributions/update-arduino-state'; +import { CompileSummaryProvider } from '../../browser/contributions/verify-sketch'; import { NotificationCenter } from '../../browser/notification-center'; import { CurrentSketch, @@ -61,10 +62,12 @@ describe('update-arduino-state', function () { let currentSketchMock: CurrentSketch | undefined; let sketchDirUriMock: URI | undefined; let dataDirUriMock: URI | undefined; + let compileSummaryMock: CompileSummary | undefined; let onCurrentSketchDidChangeEmitter: Emitter; let onDataDirDidChangeEmitter: Emitter; let onSketchDirDidChangeEmitter: Emitter; let onDataStoreDidChangeEmitter: Emitter; + let compileSummaryDidChangeEmitter: Emitter; beforeEach(async () => { toDisposeAfterEach = new DisposableCollection(); @@ -76,15 +79,18 @@ describe('update-arduino-state', function () { currentSketchMock = undefined; sketchDirUriMock = undefined; dataDirUriMock = undefined; + compileSummaryMock = undefined; onCurrentSketchDidChangeEmitter = new Emitter(); onDataDirDidChangeEmitter = new Emitter(); onSketchDirDidChangeEmitter = new Emitter(); onDataStoreDidChangeEmitter = new Emitter(); + compileSummaryDidChangeEmitter = new Emitter(); toDisposeAfterEach.pushAll([ onCurrentSketchDidChangeEmitter, onDataDirDidChangeEmitter, onSketchDirDidChangeEmitter, onDataStoreDidChangeEmitter, + compileSummaryDidChangeEmitter, ]); const container = createContainer(); @@ -418,10 +424,8 @@ describe('update-arduino-state', function () { buildPlatform: undefined, buildOutputUri: 'file:///path/to/build', }; - await commandRegistry.executeCommand( - 'arduino.languageserver.notifyBuildDidComplete', - summary - ); + compileSummaryMock = summary; + compileSummaryDidChangeEmitter.fire(compileSummaryMock); await wait(50); const params = stateUpdateParams.filter( @@ -585,6 +589,12 @@ describe('update-arduino-state', function () { new ContainerModule((bind, unbind, isBound, rebind) => { bindSketchesContribution(bind, unbind, isBound, rebind); bind(UpdateArduinoState).toSelf().inSingletonScope(); + bind(CompileSummaryProvider).toConstantValue({ + get compileSummary(): CompileSummary | undefined { + return compileSummaryMock; + }, + onDidChangeCompileSummary: compileSummaryDidChangeEmitter.event, + }); rebind(BoardsService).toConstantValue({ getDetectedPorts() { return {}; diff --git a/arduino-ide-extension/src/test/node/core-service-impl.slow-test.ts b/arduino-ide-extension/src/test/node/core-service-impl.slow-test.ts index dd80bca5..52b0d044 100644 --- a/arduino-ide-extension/src/test/node/core-service-impl.slow-test.ts +++ b/arduino-ide-extension/src/test/node/core-service-impl.slow-test.ts @@ -1,12 +1,11 @@ -import { CancellationTokenSource } from '@theia/core/lib/common/cancellation'; -import { CommandRegistry } from '@theia/core/lib/common/command'; import { DisposableCollection } from '@theia/core/lib/common/disposable'; import { isWindows } from '@theia/core/lib/common/os'; import { FileUri } from '@theia/core/lib/node/file-uri'; -import { Container, injectable } from '@theia/core/shared/inversify'; +import { Container } from '@theia/core/shared/inversify'; import { expect } from 'chai'; import { BoardsService, + CompileSummary, CoreService, SketchesService, isCompileSummary, @@ -36,11 +35,9 @@ describe('core-service-impl', () => { this.timeout(testTimeout); const coreService = container.get(CoreService); const sketchesService = container.get(SketchesService); - const commandService = - container.get(TestCommandRegistry); const sketch = await sketchesService.createNewSketch(); - await coreService.compile({ + const compileSummary = await coreService.compile({ fqbn: uno, sketch, optimizeForDebug: false, @@ -48,18 +45,9 @@ describe('core-service-impl', () => { verbose: true, }); - const executedBuildDidCompleteCommands = - commandService.executedCommands.filter( - ([command]) => - command === 'arduino.languageserver.notifyBuildDidComplete' - ); - expect(executedBuildDidCompleteCommands.length).to.be.equal(1); - const [, args] = executedBuildDidCompleteCommands[0]; - expect(args.length).to.be.equal(1); - const arg = args[0]; - expect(isCompileSummary(arg)).to.be.true; - expect('buildOutputUri' in arg).to.be.true; - expect(arg.buildOutputUri).to.be.not.undefined; + expect(isCompileSummary(compileSummary)).to.be.true; + expect((compileSummary).buildOutputUri).to.be.not + .undefined; const tempBuildPaths = await sketchesService.getBuildPath(sketch); if (isWindows) { @@ -68,7 +56,7 @@ describe('core-service-impl', () => { expect(tempBuildPaths.length).to.be.equal(1); } - const { buildOutputUri } = arg; + const { buildOutputUri } = compileSummary; const buildOutputPath = FileUri.fsPath(buildOutputUri).toString(); expect(tempBuildPaths.includes(buildOutputPath)).to.be.true; }); @@ -91,35 +79,5 @@ async function start( } async function createContainer(): Promise { - return createBaseContainer({ - additionalBindings: (bind, rebind) => { - bind(TestCommandRegistry).toSelf().inSingletonScope(); - rebind(CommandRegistry).toService(TestCommandRegistry); - }, - }); -} - -@injectable() -class TestCommandRegistry extends CommandRegistry { - // eslint-disable-next-line @typescript-eslint/no-explicit-any - readonly executedCommands: [string, any[]][] = []; - - override async executeCommand( - commandId: string, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - ...args: any[] - ): Promise { - const { token } = new CancellationTokenSource(); - this.onWillExecuteCommandEmitter.fire({ - commandId, - args, - token, - waitUntil: () => { - // NOOP - }, - }); - this.executedCommands.push([commandId, args]); - this.onDidExecuteCommandEmitter.fire({ commandId, args }); - return undefined; - } + return createBaseContainer(); }