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

AlbyIanna · 2022-07-11T10:41:55Z

Motivation

The first time the IDE is launched, it has a mechanism in place to automatically install the most common libraries and platforms, in order to deliver a better out-of-the-box experience for a majority of users.
Right now though, this process would overwrite libraries and platforms that were previously installed by the user.

Change description

when installing libraries and platforms at first start-up, use the --no-overwrite flag of the arduino-cli
[refactor] move the code that manages the initialization of these libraries to a new contribution.

Other information

Reviewer checklist

PR addresses a single concern.
The PR has no duplicates (please search among the Pull Requests before creating one)
PR title and description are properly filled.
Docs have been added / updated (for bug fixes / features)

kittaakos

The code looks good to me.

Thank you for the cleanup and extracting the logic into a standalone contribution. 👍

per1234

@AlbyIanna it is working great for me. I did run into a problem while testing, but it turned out to be a pre-existing bug not related to the changes made here (#1192).

I do have one question though: I notice that if the "Arduino AVR Boards" platform installation fails due to there being a pre-existing installation, the installation of the "Arduino_BuiltIn" library never happens:

Failed to install platform: arduino:avr.
Error: 2 UNKNOWN: Platform arduino:avr@1.8.4 already installed: could not overwrite

The primary purpose of this installation is to provide the expected boards and libraries on a fresh install. If there is already a pre-existing installation of "Arduino AVR Boards" then it is not a fresh install and so the library installation is not so important. That, as well as the unresolved issue with those libraries unexpectedly getting priority under certain conditions (#1055) makes me fine with the libraries installation being skipped in this case, so I don't think this needs to be addressed.

However, I would be concerned if the reverse of that behavior (skipping the "Arduino AVR Boards" platform installation if the "Arduino_BuiltIn" library installation failed) could occur, since the platform installation is more important and also no longer potentially disruptive after the change made in this PR. Is it possible that could happen, or is the order of the platform installation followed by the library installation guaranteed by the code? I have not produced such behavior in my tests.

AlbyIanna · 2022-07-14T07:18:13Z

@per1234

Is it possible that could happen, or is the order of the platform installation followed by the library installation guaranteed by the code?

I can confirm that the order of the platform installation followed by the library installation, so this scenario shouldn't be possible.

I notice that if the "Arduino AVR Boards" platform installation fails due to there being a pre-existing installation, the installation of the "Arduino_BuiltIn" library never happens

But this behavior was not expected. It should be easy to fix 🔨

per1234

My sole concern has been allayed and everything worked just fine during my testing. So this is a 👍 from me.

Thanks Alberto!

kittaakos · 2022-07-14T07:38:46Z

Please use 0.25.0-rc1. Thank you!

AlbyIanna · 2022-07-14T08:11:21Z

I've upgraded arduino-cli to 0.0.25-rc1 and the problem about skipping installation of the libraries. Please @kittaakos check if the new code is okay with you.

kittaakos · 2022-07-14T08:34:46Z

arduino-ide-extension/src/browser/contributions/init-libs-platforms.ts

+      await this.localStorageService.setData(
+        InitLibsPlatforms.INIT_LIBS_AND_PACKAGES,
+        true
+      );


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?

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.

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.

I was just doing some investigation into this.

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

Intended failure when the installation would result in an overwrite

Unintended failure due to something like no Internet access (Communicate first run Internet access requirement to user #784)

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.

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!

per1234

UPDATE: Fixed by 6c4482f

There is a missing line break in the output:
Failed to install platform: arduino:avr. Error: 2 UNKNOWN: Platform arduino:avr@1.8.4 already installed: could not overwriteFailed to install library: Arduino_BuiltIn:1.0.0. Error: 2 UNKNOWN: Library SD@1.2.4 is already installed, but with a different version: SD@1.2.3

Expected:

Failed to install platform: arduino:avr. Error: 2 UNKNOWN: Platform arduino:avr@1.8.4 already installed: could not overwrite Failed to install library: Arduino_BuiltIn:1.0.0. Error: 2 UNKNOWN: Library SD@1.2.4 is already installed, but with a different version: SD@1.2.3

AlbyIanna · 2022-07-14T20:52:39Z

I've added some logic to better handle errors here.

Now I'm setting INIT_LIBS_AND_PACKAGES to true only if both the library and platform are installed successfully or if they are already installed. If another error is raised, I'm not setting INIT_LIBS_AND_PACKAGES to true, so that on the next IDE start-up it will try to install them again.

Please @per1234 @kittaakos check it again and tell me if this is what you expected 🙏

I've also added a new line in the error messages printed in the output panel.

arduino-ide-extension/src/browser/contributions/init-libs-platforms.ts

…installed

per1234

In addition to resolving #798, this also partially fixes #784

Thanks Alberto!

kittaakos

Thank you!

AlbyIanna requested review from kittaakos and per1234 July 11, 2022 10:41

AlbyIanna force-pushed the init-libs-platforms branch from 1db02e9 to 69de5e5 Compare July 11, 2022 10:43

kittaakos approved these changes Jul 11, 2022

View reviewed changes

per1234 added topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project labels Jul 12, 2022

per1234 linked an issue Jul 12, 2022 that may be closed by this pull request

New installs overwrite existing libraries in the <sketchbook folder>/libraries #798

Closed

per1234 reviewed Jul 12, 2022

View reviewed changes

per1234 approved these changes Jul 14, 2022

View reviewed changes

Alberto Iannaccone added 4 commits July 14, 2022 09:53

move initialization of libs and platforms into new contribution

121aa67

use noOverwrite when install built-in libraries and platform

42283ba

catch errors when installing platforms and libraries at first start-up

9236f2e

arduino-cli version 0.25.0-rc1

1b214e7

AlbyIanna force-pushed the init-libs-platforms branch from 69de5e5 to 1b214e7 Compare July 14, 2022 08:08

AlbyIanna requested a review from kittaakos July 14, 2022 08:09

kittaakos reviewed Jul 14, 2022

View reviewed changes

per1234 requested changes Jul 14, 2022

View reviewed changes

per1234 mentioned this pull request Jul 14, 2022

First run "Arduino_BuiltIn" installation fails: "dir ... already exists" #1192

Closed

3 tasks

Alberto Iannaccone added 2 commits July 14, 2022 22:42

refine platforms and libraries initialization in case of errors

cd9719e

add trailing newline when libraries and platform installation fail

6c4482f

AlbyIanna requested review from kittaakos and per1234 July 14, 2022 20:44

per1234 requested changes Jul 15, 2022

View reviewed changes

arduino-ide-extension/src/browser/contributions/init-libs-platforms.ts Outdated Show resolved Hide resolved

Alberto Iannaccone added 2 commits July 15, 2022 09:27

use regex to check error if builtin library dependencies are already …

68bc78a

…installed

rename contribution

28b5732

AlbyIanna requested a review from per1234 July 15, 2022 07:31

per1234 approved these changes Jul 15, 2022

View reviewed changes

kittaakos approved these changes Jul 15, 2022

View reviewed changes

AlbyIanna merged commit 73835ec into main Jul 15, 2022

AlbyIanna deleted the init-libs-platforms branch July 15, 2022 14:06

per1234 mentioned this pull request Jul 18, 2022

Communicate first run Internet access requirement to user #784

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

AlbyIanna commented Jul 11, 2022

kittaakos left a comment

per1234 left a comment •

edited

Loading

AlbyIanna commented Jul 14, 2022

per1234 left a comment

kittaakos commented Jul 14, 2022

AlbyIanna commented Jul 14, 2022

kittaakos Jul 14, 2022

AlbyIanna Jul 14, 2022 •

edited

Loading

kittaakos Jul 14, 2022

per1234 Jul 14, 2022 •

edited

Loading

AlbyIanna Jul 14, 2022

per1234 left a comment •

edited

Loading

AlbyIanna commented Jul 14, 2022

per1234 left a comment

kittaakos left a comment

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

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

Conversation

AlbyIanna commented Jul 11, 2022

Motivation

Change description

Other information

Reviewer checklist

kittaakos left a comment

Choose a reason for hiding this comment

per1234 left a comment • edited Loading

Choose a reason for hiding this comment

AlbyIanna commented Jul 14, 2022

per1234 left a comment

Choose a reason for hiding this comment

kittaakos commented Jul 14, 2022

AlbyIanna commented Jul 14, 2022

kittaakos Jul 14, 2022

Choose a reason for hiding this comment

AlbyIanna Jul 14, 2022 • edited Loading

Choose a reason for hiding this comment

kittaakos Jul 14, 2022

Choose a reason for hiding this comment

per1234 Jul 14, 2022 • edited Loading

Choose a reason for hiding this comment

AlbyIanna Jul 14, 2022

Choose a reason for hiding this comment

per1234 left a comment • edited Loading

Choose a reason for hiding this comment

AlbyIanna commented Jul 14, 2022

per1234 left a comment

Choose a reason for hiding this comment

kittaakos left a comment

Choose a reason for hiding this comment

per1234 left a comment •

edited

Loading

AlbyIanna Jul 14, 2022 •

edited

Loading

per1234 Jul 14, 2022 •

edited

Loading

per1234 left a comment •

edited

Loading