Skip to content

Prevent overwriting existing libraries and platforms at first IDE start-up #1169

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

Merged
merged 8 commits into from
Jul 15, 2022
Merged

Prevent overwriting existing libraries and platforms at first IDE start-up #1169

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
121aa67
move initialization of libs and platforms into new contribution
Jul 7, 2022
42283ba
use noOverwrite when install built-in libraries and platform
Jul 11, 2022
9236f2e
catch errors when installing platforms and libraries at first start-up
Jul 14, 2022
1b214e7
arduino-cli version 0.25.0-rc1
Jul 14, 2022
cd9719e
refine platforms and libraries initialization in case of errors
Jul 14, 2022
6c4482f
add trailing newline when libraries and platform installation fail
Jul 14, 2022
68bc78a
use regex to check error if builtin library dependencies are already …
Jul 15, 2022
28b5732
rename contribution
Jul 15, 2022
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
2 changes: 1 addition & 1 deletion arduino-ide-extension/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@
],
"arduino": {
"cli": {
"version": "0.24.0"
"version": "0.25.0-rc1"
},
"fwuploader": {
"version": "2.2.0"
Expand Down
26 changes: 0 additions & 26 deletions arduino-ide-extension/src/browser/arduino-frontend-contribution.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import {
SketchesService,
ExecutableService,
Sketch,
LibraryService,
ArduinoDaemon,
} from '../common/protocol';
import { Mutex } from 'async-mutex';
Expand Down Expand Up @@ -77,7 +76,6 @@ import { IDEUpdater } from '../common/protocol/ide-updater';
import { FileSystemFrontendContribution } from '@theia/filesystem/lib/browser/filesystem-frontend-contribution';
import { HostedPluginEvents } from './hosted-plugin-events';

const INIT_LIBS_AND_PACKAGES = 'initializedLibsAndPackages';
export const SKIP_IDE_VERSION = 'skipIDEVersion';

@injectable()
Expand All @@ -98,9 +96,6 @@ export class ArduinoFrontendContribution
@inject(BoardsService)
private readonly boardsService: BoardsService;

@inject(LibraryService)
private readonly libraryService: LibraryService;

@inject(BoardsServiceProvider)
private readonly boardsServiceClientImpl: BoardsServiceProvider;

Expand Down Expand Up @@ -162,27 +157,6 @@ export class ArduinoFrontendContribution

@postConstruct()
protected async init(): Promise<void> {
const isFirstStartup = !(await this.localStorageService.getData(
INIT_LIBS_AND_PACKAGES
));
if (isFirstStartup) {
await this.localStorageService.setData(INIT_LIBS_AND_PACKAGES, true);
const avrPackage = await this.boardsService.getBoardPackage({
id: 'arduino:avr',
});
const builtInLibrary = (
await this.libraryService.search({
query: 'Arduino_BuiltIn',
})
)[0];

!!avrPackage && (await this.boardsService.install({ item: avrPackage }));
!!builtInLibrary &&
(await this.libraryService.install({
item: builtInLibrary,
installDependencies: true,
}));
}
if (!window.navigator.onLine) {
// tslint:disable-next-line:max-line-length
this.messageService.warn(
Expand Down
2 changes: 2 additions & 0 deletions arduino-ide-extension/src/browser/arduino-ide-frontend-module.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,6 +304,7 @@ import { WidgetManager as TheiaWidgetManager } from '@theia/core/lib/browser/wid
import { StartupTask } from './widgets/sketchbook/startup-task';
import { IndexesUpdateProgress } from './contributions/indexes-update-progress';
import { Daemon } from './contributions/daemon';
import { InitLibsPlatforms } from './contributions/init-libs-platforms';

MonacoThemingService.register({
id: 'arduino-theme',
Expand Down Expand Up @@ -699,6 +700,7 @@ export default new ContainerModule((bind, unbind, isBound, rebind) => {
Contribution.configure(bind, StartupTask);
Contribution.configure(bind, IndexesUpdateProgress);
Contribution.configure(bind, Daemon);
Contribution.configure(bind, InitLibsPlatforms);

// Disabled the quick-pick customization from Theia when multiple formatters are available.
// Use the default VS Code behavior, and pick the first one. In the IDE2, clang-format has `exclusive` selectors.
Expand Down
55 changes: 55 additions & 0 deletions arduino-ide-extension/src/browser/contributions/init-libs-platforms.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
import { LocalStorageService } from '@theia/core/lib/browser';
import { inject, injectable } from '@theia/core/shared/inversify';
import { BoardsService, LibraryService } from '../../common/protocol';
import { Contribution } from './contribution';

@injectable()
export class InitLibsPlatforms extends Contribution {
@inject(LocalStorageService)
private readonly localStorageService: LocalStorageService;
@inject(BoardsService)
private readonly boardsService: BoardsService;
@inject(LibraryService)
private readonly libraryService: LibraryService;

override async onReady(): Promise<void> {
const isFirstStartup = !(await this.localStorageService.getData(
InitLibsPlatforms.INIT_LIBS_AND_PACKAGES
));
if (isFirstStartup) {
await this.localStorageService.setData(
InitLibsPlatforms.INIT_LIBS_AND_PACKAGES,
true
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you changed the logic with the try{} catch{}; why do not IDE2 sets this flag after the installation.

Potential issue:

  • Is it the first start? yes,
  • Store that the libs and AVR were installed,
  • Install the libs and AVR,
  • It fails,
  • Next IDE2 start; is it the first start? No, but nothing is installed.

What do you think?

Copy link
Contributor Author

@AlbyIanna AlbyIanna Jul 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

  • Is it the first start? yes,
  • Store that the libs and AVR were installed,
  • Install the libs and AVR,
  • It fails,
  • Next IDE2 start; is it the first start? No, but nothing is installed.

This behavior is expected in my opinion.

As @per1234 stated here above:

The primary purpose of this installation is to provide the expected boards and libraries on a fresh install.

This, I believe, means that we should try to install built-in cores and libraries only once (during the first start-up), even if the installation fails because of pre-existing installations.

Does it make sense?

If we all agree with the behavior above, then I see no value in moving the setting of INIT_LIBS_AND_PACKAGES after the installation.

Copy link
Contributor

Choose a reason for hiding this comment

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

This behavior is expected in my opinion.

Another use case:

  • User has a fresh env and will open the IDE2 for the very first time,
  • No internet connection,
  • Install will fail but IDE2 stores that the install was already run,
  • User starts the IDE2 the second time with an Internet connection,
  • Nothing will happen.

If this is expected, please proceed with the merge.

Copy link
Contributor

@per1234 per1234 Jul 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I was just doing some investigation into this.

As Akos mentioned, there are two distinct types of failure during this process:

The installation should never be attempted again in the event of the former type of failure, but the IDE does really need to eventually be able to handle the latter gracefully. That could be in the form of clearly communicating about the problem to the user and leaving it up to them to use Boards and Library Manager to make the failed installations, or handling it automatically by trying again on the next restart.

The lack of handling for the latter type of failure was a pre-existing issue, so it is only the former type of failure that is of direct concern to this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, that makes sense! But in that case, moving the initialization of INIT_LIBS_AND_PACKAGES is not enough. I will work on a fix! Thanks!

const avrPackage = await this.boardsService.getBoardPackage({
id: 'arduino:avr',
});
const builtInLibrary = (
await this.libraryService.search({
query: 'Arduino_BuiltIn',
})
)[0];

if (avrPackage) {
try {
await this.boardsService.install({
item: avrPackage,
noOverwrite: true, // We don't want to automatically replace custom platforms the user might already have in place
});
} catch {} // If this fails, we still want to install the libraries
}
if (builtInLibrary) {
try {
await this.libraryService.install({
item: builtInLibrary,
installDependencies: true,
noOverwrite: true, // We don't want to automatically replace custom libraries the user might already have in place
});
} catch {}
}
}
}
}
export namespace InitLibsPlatforms {
export const INIT_LIBS_AND_PACKAGES = 'initializedLibsAndPackages';
}
1 change: 1 addition & 0 deletions arduino-ide-extension/src/common/protocol/installable.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ export interface Installable<T extends ArduinoComponent> {
item: T;
progressId?: string;
version?: Installable.Version;
noOverwrite?: boolean;
}): Promise<void>;

/**
Expand Down
1 change: 1 addition & 0 deletions arduino-ide-extension/src/common/protocol/library-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export interface LibraryService
progressId?: string;
version?: Installable.Version;
installDependencies?: boolean;
noOverwrite?: boolean;
}): Promise<void>;
installZip(options: {
zipUri: string;
Expand Down
2 changes: 2 additions & 0 deletions arduino-ide-extension/src/node/boards-service-impl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,6 +395,7 @@ export class BoardsServiceImpl
item: BoardsPackage;
progressId?: string;
version?: Installable.Version;
noOverwrite?: boolean;
}): Promise<void> {
const item = options.item;
const version = !!options.version
Expand All @@ -410,6 +411,7 @@ export class BoardsServiceImpl
req.setArchitecture(architecture);
req.setPlatformPackage(platform);
req.setVersion(version);
req.setNoOverwrite(Boolean(options.noOverwrite));

console.info('>>> Starting boards package installation...', item);

Expand Down
4 changes: 4 additions & 0 deletions arduino-ide-extension/src/node/cli-protocol/cc/arduino/cli/commands/v1/compile_pb.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,9 @@ export class CompileRequest extends jspb.Message {
getEncryptKey(): string;
setEncryptKey(value: string): CompileRequest;

getSkipLibrariesDiscovery(): boolean;
setSkipLibrariesDiscovery(value: boolean): CompileRequest;


serializeBinary(): Uint8Array;
toObject(includeInstance?: boolean): CompileRequest.AsObject;
Expand Down Expand Up @@ -133,6 +136,7 @@ export namespace CompileRequest {
keysKeychain: string,
signKey: string,
encryptKey: string,
skipLibrariesDiscovery: boolean,
}
}

Expand Down
32 changes: 31 additions & 1 deletion arduino-ide-extension/src/node/cli-protocol/cc/arduino/cli/commands/v1/compile_pb.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,8 @@ proto.cc.arduino.cli.commands.v1.CompileRequest.toObject = function(includeInsta
libraryList: (f = jspb.Message.getRepeatedField(msg, 24)) == null ? undefined : f,
keysKeychain: jspb.Message.getFieldWithDefault(msg, 25, ""),
signKey: jspb.Message.getFieldWithDefault(msg, 26, ""),
encryptKey: jspb.Message.getFieldWithDefault(msg, 27, "")
encryptKey: jspb.Message.getFieldWithDefault(msg, 27, ""),
skipLibrariesDiscovery: jspb.Message.getBooleanFieldWithDefault(msg, 28, false)
};

if (includeInstance) {
Expand Down Expand Up @@ -286,6 +287,10 @@ proto.cc.arduino.cli.commands.v1.CompileRequest.deserializeBinaryFromReader = fu
var value = /** @type {string} */ (reader.readString());
msg.setEncryptKey(value);
break;
case 28:
var value = /** @type {boolean} */ (reader.readBool());
msg.setSkipLibrariesDiscovery(value);
break;
default:
reader.skipField();
break;
Expand Down Expand Up @@ -482,6 +487,13 @@ proto.cc.arduino.cli.commands.v1.CompileRequest.serializeBinaryToWriter = functi
f
);
}
f = message.getSkipLibrariesDiscovery();
if (f) {
writer.writeBool(
28,
f
);
}
};


Expand Down Expand Up @@ -1016,6 +1028,24 @@ proto.cc.arduino.cli.commands.v1.CompileRequest.prototype.setEncryptKey = functi
};


/**
* optional bool skip_libraries_discovery = 28;
* @return {boolean}
*/
proto.cc.arduino.cli.commands.v1.CompileRequest.prototype.getSkipLibrariesDiscovery = function() {
return /** @type {boolean} */ (jspb.Message.getBooleanFieldWithDefault(this, 28, false));
};


/**
* @param {boolean} value
* @return {!proto.cc.arduino.cli.commands.v1.CompileRequest} returns this
*/
proto.cc.arduino.cli.commands.v1.CompileRequest.prototype.setSkipLibrariesDiscovery = function(value) {
return jspb.Message.setProto3BooleanField(this, 28, value);
};



/**
* List of repeated fields within this message type.
Expand Down
4 changes: 4 additions & 0 deletions arduino-ide-extension/src/node/cli-protocol/cc/arduino/cli/commands/v1/core_pb.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@ export class PlatformInstallRequest extends jspb.Message {
getSkipPostInstall(): boolean;
setSkipPostInstall(value: boolean): PlatformInstallRequest;

getNoOverwrite(): boolean;
setNoOverwrite(value: boolean): PlatformInstallRequest;


serializeBinary(): Uint8Array;
toObject(includeInstance?: boolean): PlatformInstallRequest.AsObject;
Expand All @@ -44,6 +47,7 @@ export namespace PlatformInstallRequest {
architecture: string,
version: string,
skipPostInstall: boolean,
noOverwrite: boolean,
}
}

Expand Down
32 changes: 31 additions & 1 deletion arduino-ide-extension/src/node/cli-protocol/cc/arduino/cli/commands/v1/core_pb.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,8 @@ proto.cc.arduino.cli.commands.v1.PlatformInstallRequest.toObject = function(incl
platformPackage: jspb.Message.getFieldWithDefault(msg, 2, ""),
architecture: jspb.Message.getFieldWithDefault(msg, 3, ""),
version: jspb.Message.getFieldWithDefault(msg, 4, ""),
skipPostInstall: jspb.Message.getBooleanFieldWithDefault(msg, 5, false)
skipPostInstall: jspb.Message.getBooleanFieldWithDefault(msg, 5, false),
noOverwrite: jspb.Message.getBooleanFieldWithDefault(msg, 6, false)
};

if (includeInstance) {
Expand Down Expand Up @@ -397,6 +398,10 @@ proto.cc.arduino.cli.commands.v1.PlatformInstallRequest.deserializeBinaryFromRea
var value = /** @type {boolean} */ (reader.readBool());
msg.setSkipPostInstall(value);
break;
case 6:
var value = /** @type {boolean} */ (reader.readBool());
msg.setNoOverwrite(value);
break;
default:
reader.skipField();
break;
Expand Down Expand Up @@ -462,6 +467,13 @@ proto.cc.arduino.cli.commands.v1.PlatformInstallRequest.serializeBinaryToWriter
f
);
}
f = message.getNoOverwrite();
if (f) {
writer.writeBool(
6,
f
);
}
};


Expand Down Expand Up @@ -574,6 +586,24 @@ proto.cc.arduino.cli.commands.v1.PlatformInstallRequest.prototype.setSkipPostIns
};


/**
* optional bool no_overwrite = 6;
* @return {boolean}
*/
proto.cc.arduino.cli.commands.v1.PlatformInstallRequest.prototype.getNoOverwrite = function() {
return /** @type {boolean} */ (jspb.Message.getBooleanFieldWithDefault(this, 6, false));
};


/**
* @param {boolean} value
* @return {!proto.cc.arduino.cli.commands.v1.PlatformInstallRequest} returns this
*/
proto.cc.arduino.cli.commands.v1.PlatformInstallRequest.prototype.setNoOverwrite = function(value) {
return jspb.Message.setProto3BooleanField(this, 6, value);
};





Expand Down
4 changes: 4 additions & 0 deletions arduino-ide-extension/src/node/cli-protocol/cc/arduino/cli/commands/v1/lib_pb.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,9 @@ export class LibraryInstallRequest extends jspb.Message {
getNoDeps(): boolean;
setNoDeps(value: boolean): LibraryInstallRequest;

getNoOverwrite(): boolean;
setNoOverwrite(value: boolean): LibraryInstallRequest;


serializeBinary(): Uint8Array;
toObject(includeInstance?: boolean): LibraryInstallRequest.AsObject;
Expand All @@ -96,6 +99,7 @@ export namespace LibraryInstallRequest {
name: string,
version: string,
noDeps: boolean,
noOverwrite: boolean,
}
}

Expand Down
Loading