[breaking] Remove auto detection of Arduino IDE built-in libraries and tools / Allow gRPC install of built-in libraries #1817

cmaglie · 2022-07-27T16:46:29Z

Please check if the PR fulfills these requirements

The PR has no duplicates (please search among the Pull Requests
before creating one)
The PR follows
our contributing guidelines
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)
UPGRADING.md has been updated with a migration guide (for breaking changes)

What kind of change does this PR introduce?
Remove the check for the CLI being bundled in the Arduino IDE

What is the current behavior?
Arduino CLI checks if it's bundled with the Arduino IDE. This check allows the CLI
to automatically use the libraries and tools bundled in the Arduino IDE

What is the new behavior?
Now, this is not supported anymore unless the configuration keys directories.builtin.libraries/tools are set.

Does this PR introduce a breaking change, and is
titled accordingly?
Yes

Other information:
Fix Sourcing built-in libraries when not bundled w/ classic IDE is undocumented or unintended #1565

docs/sketch-build-process.md

configuration/directories.go

cmaglie · 2022-08-05T12:09:22Z

More work is going into this PR: the handling of the historic Arduino IDE bundled libraries will be implemented in the CLI.

codecov · 2022-08-27T13:59:46Z

Codecov Report

Merging #1817 (ad98097) into master (05d1446) will increase coverage by 0.19%.
The diff coverage is 56.55%.

@@            Coverage Diff             @@
##           master    #1817      +/-   ##
==========================================
+ Coverage   36.39%   36.58%   +0.19%     
==========================================
  Files         231      231              
  Lines       19573    19561      -12     
==========================================
+ Hits         7124     7157      +33     
+ Misses      11620    11578      -42     
+ Partials      829      826       -3

Flag	Coverage Δ
unit	`36.58% <56.55%> (+0.19%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
arduino/libraries/libraries_location.go	`42.66% <0.00%> (-5.10%)`	⬇️
arduino/libraries/librariesmanager/install.go	`14.97% <0.00%> (-0.14%)`	⬇️
cli/cache/clean.go	`0.00% <0.00%> (ø)`
cli/cli.go	`0.00% <0.00%> (ø)`
cli/config/validate.go	`0.00% <ø> (ø)`
cli/core/search.go	`0.00% <0.00%> (ø)`
cli/daemon/daemon.go	`0.00% <0.00%> (ø)`
commands/compile/compile.go	`0.00% <0.00%> (ø)`
commands/lib/install.go	`0.00% <0.00%> (ø)`
commands/lib/uninstall.go	`0.00% <0.00%> (ø)`
... and 15 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

cmaglie · 2022-08-29T13:20:44Z

The gRPC LibraryInstall now has an extra field called install_location that can have two values:

LIBRARY_INSTALL_LOCATION_USER (this is the default if not set)
LIBRARY_INSTALL_LOCATION_BUILTIN

This change allows the gRPC client (IDE) to install libraries in the "builtin libraries" directory.
The "builtin libraries" directory can be set via the CLI configuration key directories.builtin.libraries.

The library installation into the builtin directory is available only via gRPC. If the "builtin libraries" directory is not set, installing to the builtin library directory will fail.

configuration/defaults.go

docs/UPGRADING.md

docs/configuration.md

kittaakos · 2022-08-31T08:22:17Z

As reported in IDE2 code here, it does not work.

root ERROR Request install failed with error: 2 UNKNOWN: Builtin libraries directory not set Error: 2 UNKNOWN: Builtin libraries directory not set
    at Object.callErrorFromStatus (/Users/a.kitta/dev/git/arduino-ide/node_modules/@grpc/grpc-js/build/src/call.js:31:26)
    at Object.onReceiveStatus (/Users/a.kitta/dev/git/arduino-ide/node_modules/@grpc/grpc-js/build/src/client.js:349:49)
    at Object.onReceiveStatus (/Users/a.kitta/dev/git/arduino-ide/node_modules/@grpc/grpc-js/build/src/client-interceptors.js:328:181)
    at /Users/a.kitta/dev/git/arduino-ide/node_modules/@grpc/grpc-js/build/src/call-stream.js:187:78
    at processTicksAndRejections (node:internal/process/task_queues:78:11)

Why should the IDE2 set any path? We discussed that the default install_location which is LIBRARY_INSTALL_LOCATION_USER will install to #directories/user/libraries, the LIBRARY_INSTALL_LOCATION_BUILTIN will install into #directories/data/libraries.

cmaglie · 2022-08-31T10:29:13Z

Uhm... shall we make it default to .../Arduino15/libraries?

The rationale for this PR is that I see the "bundled" libraries as a legacy that we inherited from Arduino IDE 1.8.x, and I would like to separate them from the pure-CLI users. The bundled libraries can't be managed from the command line (there is no lib install --bundled flags right now). If we make the "bundled dir" available by default, the command-line user will see these "bundled" libraries appearing after the first run of the Arduino IDE.

@ubidefeo @per1234 any comments on this?

Why should the IDE2 set any path?

The IDE should be already setting the sketchbook path AFAIK, the bundled libs would be another path to set. But I'm open to defaulting it to $DATA_DIR/libraries if we decide to do so.

kittaakos · 2022-08-31T10:36:06Z

Why should the IDE2 set any path?

The IDE should be already setting the sketchbook path AFAIK

No, IDE2 is not setting anything. There is the default CLI config that is generated by the CLI when IDE2 starts, and it's missing. The sketchbook path is inferred from the CLI config: #directories/user. Whenever a user wants to modify the sketchbook path from IDE2, eventually, the CLI config's #directories/user location will change and IDE2 restarts the daemon.

cmaglie · 2022-08-31T12:05:56Z

Another solution may be to set the default path only when the CLI is running as a daemon, this way:

command-line users will not have to handle the weird "bundled" libraries
the IDE would not need to set up paths

cmaglie · 2022-08-31T12:40:34Z

I've implemented the default for the bundled library directory when running as a daemon, as I said in my last comment.

per1234

Describe the problem

cc.arduino.cli.commands.v1.ArduinoCoreService.LibraryUninstall uninstalls the library that was installed to directories.builtin.libraries by that instance.

To reproduce

Set up

$ arduino-cli version
arduino-cli.exe  Version: git-snapshot Commit: fb57a522 Date: 2022-08-31T14:09:52Z

$ arduino-cli lib install Servo
Downloading [email protected]...
[email protected] already downloaded
Installing [email protected]...
Installed [email protected]

$ arduino-cli daemon --debug
Daemon is now listening on 127.0.0.1:50051

Demo

Use grpcurl to run the following commands in another terminal:

$ grpcurl \
  -plaintext \
  -import-path ./rpc \
  -proto cc/arduino/cli/commands/v1/commands.proto \
  127.0.0.1:50051 \
  cc.arduino.cli.commands.v1.ArduinoCoreService.Create

{
  "instance": {
    "id": 1
  }
}

$ grpcurl \
  -plaintext \
  -import-path ./rpc \
  -proto cc/arduino/cli/commands/v1/commands.proto \
  -d '{"instance": {"id": 1}}' \
  127.0.0.1:50051 \
  cc.arduino.cli.commands.v1.ArduinoCoreService.Init

$ grpcurl \
  -plaintext \
  -import-path ./rpc \
  -proto cc/arduino/cli/commands/v1/commands.proto \
  -d '{"instance": {"id": 1}, "name": "Servo", "install_location": 1}' \
  127.0.0.1:50051 \
  cc.arduino.cli.commands.v1.ArduinoCoreService.LibraryInstall

{
  "taskProgress": {
    "name": "Downloading [email protected]"
  }
}
{
  "progress": {
    "file": "[email protected]",
    "completed": true
  }
}
{
  "taskProgress": {
    "completed": true
  }
}
{
  "taskProgress": {
    "name": "Installing [email protected]"
  }
}
{
  "taskProgress": {
    "message": "Installed [email protected]",
    "completed": true
  }
}
{

}

$ grpcurl \
  -plaintext \
  -import-path ./rpc \
  -proto cc/arduino/cli/commands/v1/commands.proto \
  -d '{"instance": {"id": 1}, "all": true, "name": "Servo"}' \
  127.0.0.1:50051 \
  cc.arduino.cli.commands.v1.ArduinoCoreService.LibraryList

{
  "installedLibraries": [
    {
      "library": {
        "name": "Servo",
        "author": "Michael Margolis, Arduino",
        "maintainer": "Arduino \[email protected]\u003e",
        "sentence": "Allows Arduino boards to control a variety of servo motors.",
        "paragraph": "This library can control a great number of servos.\u003cbr /\u003eIt makes careful use of timers: the library can control 12 servos using only 1 timer.\u003cbr /\u003eOn the Arduino Due you can control up to 60 servos.",
        "website": "https://www.arduino.cc/reference/en/libraries/servo/",
        "category": "Device Control",
        "architectures": [
          "avr",
          "megaavr",
          "sam",
          "samd",
          "nrf52",
          "stm32f4",
          "mbed",
          "mbed_nano",
          "mbed_portenta",
          "mbed_rp2040"
        ],
        "installDir": "C:\\Users\\per\\AppData\\Local\\Arduino15\\libraries\\Servo",
        "sourceDir": "C:\\Users\\per\\AppData\\Local\\Arduino15\\libraries\\Servo\\src",
        "realName": "Servo",
        "version": "1.1.8",
        "license": "Unspecified",
        "layout": "LIBRARY_LAYOUT_RECURSIVE",
        "examples": [
          "C:\\Users\\per\\AppData\\Local\\Arduino15\\libraries\\Servo\\examples\\Knob",
          "C:\\Users\\per\\AppData\\Local\\Arduino15\\libraries\\Servo\\examples\\Sweep"
        ],
        "providesIncludes": [
          "Servo.h"
        ]
      }
    },
    {
      "library": {
        "name": "Servo",
        "author": "Michael Margolis, Arduino",
        "maintainer": "Arduino \[email protected]\u003e",
        "sentence": "Allows Arduino boards to control a variety of servo motors.",
        "paragraph": "This library can control a great number of servos.\u003cbr /\u003eIt makes careful use of timers: the library can control 12 servos using only 1 timer.\u003cbr /\u003eOn the Arduino Due you can control up to 60 servos.",
        "website": "https://www.arduino.cc/reference/en/libraries/servo/",
        "category": "Device Control",
        "architectures": [
          "avr",
          "megaavr",
          "sam",
          "samd",
          "nrf52",
          "stm32f4",
          "mbed",
          "mbed_nano",
          "mbed_portenta",
          "mbed_rp2040"
        ],
        "installDir": "C:\\Users\\per\\Documents\\Arduino\\libraries\\Servo",
        "sourceDir": "C:\\Users\\per\\Documents\\Arduino\\libraries\\Servo\\src",
        "realName": "Servo",
        "version": "1.1.8",
        "license": "Unspecified",
        "location": "LIBRARY_LOCATION_USER",
        "layout": "LIBRARY_LAYOUT_RECURSIVE",
        "examples": [
          "C:\\Users\\per\\Documents\\Arduino\\libraries\\Servo\\examples\\Knob",
          "C:\\Users\\per\\Documents\\Arduino\\libraries\\Servo\\examples\\Sweep"
        ],
        "providesIncludes": [
          "Servo.h"
        ]
      }
    }
  ]
}

$ grpcurl \
  -plaintext \
  -import-path ./rpc \
  -proto cc/arduino/cli/commands/v1/commands.proto \
  -d '{"instance": {"id": 1}, "name": "Servo", "version": "1.1.8"}' \
  127.0.0.1:50051 \
  cc.arduino.cli.commands.v1.ArduinoCoreService.LibraryUninstall

{
  "taskProgress": {
    "name": "Uninstalling [email protected]"
  }
}
{
  "taskProgress": {
    "completed": true
  }
}
{

}

$ grpcurl \
  -plaintext \
  -import-path ./rpc \
  -proto cc/arduino/cli/commands/v1/commands.proto \
  -d '{"instance": {"id": 1}, "all": true, "name": "Servo"}' \
  127.0.0.1:50051 \
  cc.arduino.cli.commands.v1.ArduinoCoreService.LibraryList

{
  "installedLibraries": [
    {
      "library": {
        "name": "Servo",
        "author": "Michael Margolis, Arduino",
        "maintainer": "Arduino \[email protected]\u003e",
        "sentence": "Allows Arduino boards to control a variety of servo motors.",
        "paragraph": "This library can control a great number of servos.\u003cbr /\u003eIt makes careful use of timers: the library can control 12 servos using only 1 timer.\u003cbr /\u003eOn the Arduino Due you can control up to 60 servos.",
        "website": "https://www.arduino.cc/reference/en/libraries/servo/",
        "category": "Device Control",
        "architectures": [
          "avr",
          "megaavr",
          "sam",
          "samd",
          "nrf52",
          "stm32f4",
          "mbed",
          "mbed_nano",
          "mbed_portenta",
          "mbed_rp2040"
        ],
        "installDir": "C:\\Users\\per\\Documents\\Arduino\\libraries\\Servo",
        "sourceDir": "C:\\Users\\per\\Documents\\Arduino\\libraries\\Servo\\src",
        "realName": "Servo",
        "version": "1.1.8",
        "license": "Unspecified",
        "location": "LIBRARY_LOCATION_USER",
        "layout": "LIBRARY_LAYOUT_RECURSIVE",
        "examples": [
          "C:\\Users\\per\\Documents\\Arduino\\libraries\\Servo\\examples\\Knob",
          "C:\\Users\\per\\Documents\\Arduino\\libraries\\Servo\\examples\\Sweep"
        ],
        "providesIncludes": [
          "Servo.h"
        ]
      }
    }
  ]
}

🐛 The library in LIBRARY_LOCATION_BUILTIN was uninstalled instead of the library in LIBRARY_LOCATION_USER

Expected behavior

cc.arduino.cli.commands.v1.ArduinoCoreService.LibraryUninstall only uninstalls libraries from directories.user

Arduino CLI version

fb57a52

Operating system

Windows 10

Additional context

The bug does not occur if "version": "1.1.8" is removed from the cc.arduino.cli.commands.v1.ArduinoCoreService.LibraryUninstall request.

See arduino#1817 (review)

Co-authored-by: per1234 <[email protected]>

See arduino#1817 (review)

cmaglie · 2022-09-01T16:27:26Z

🐛 The library in LIBRARY_LOCATION_BUILTIN was uninstalled instead of the library in LIBRARY_LOCATION_USER

@per1234 I don't know how you manage to find these edge cases but it's amazing!

In this case, LibraryUninstall shows the problem only when it's called with the library version parameter. I've fixed and added the test.

per1234

I verified that the bug reported in my previous review was fixed by ad98097

Excellent work on this Cristian!

cmaglie self-assigned this Jul 28, 2022

cmaglie requested review from per1234 and umbynos July 28, 2022 08:53

cmaglie added type: enhancement Proposed improvement priority: medium Resolution is a medium priority topic: CLI Related to the command line interface criticality: low Of low impact labels Jul 28, 2022

umbynos approved these changes Jul 29, 2022

View reviewed changes

per1234 requested changes Jul 31, 2022

View reviewed changes

docs/sketch-build-process.md Outdated Show resolved Hide resolved

configuration/directories.go Outdated Show resolved Hide resolved

cmaglie force-pushed the byebye_bundles branch from eb6e52a to 696ca82 Compare August 1, 2022 13:04

umbynos approved these changes Aug 5, 2022

View reviewed changes

cmaglie marked this pull request as draft August 5, 2022 12:07

cmaglie force-pushed the byebye_bundles branch 2 times, most recently from 15f8111 to 1b3264a Compare August 27, 2022 13:54

cmaglie force-pushed the byebye_bundles branch 2 times, most recently from 3e68ac2 to 6b797cf Compare August 27, 2022 21:46

cmaglie changed the title ~~[breaking] Remove auto detection of Arduino IDE built-in libraries and tools~~ [breaking] Remove auto detection of Arduino IDE built-in libraries and tools / Allow gRPC install of built-in libraries Aug 29, 2022

cmaglie force-pushed the byebye_bundles branch from 6b797cf to 520de09 Compare August 29, 2022 12:59

cmaglie marked this pull request as ready for review August 29, 2022 13:16

cmaglie requested review from per1234, umbynos and kittaakos August 30, 2022 10:02

kittaakos mentioned this pull request Aug 30, 2022

Use dedicated location for "built-in" libraries arduino/arduino-ide#1055

Closed

3 tasks

per1234 requested changes Aug 30, 2022

View reviewed changes

configuration/defaults.go Outdated Show resolved Hide resolved

per1234 requested changes Aug 31, 2022

View reviewed changes

docs/UPGRADING.md Outdated Show resolved Hide resolved

docs/configuration.md Outdated Show resolved Hide resolved

cmaglie force-pushed the byebye_bundles branch from ba9117a to 79702f8 Compare August 31, 2022 12:11

per1234 requested changes Aug 31, 2022

View reviewed changes

kittaakos mentioned this pull request Aug 31, 2022

#1055 Install the Arduino_BuiltIn to built-in location arduino/arduino-ide#1370

Merged

4 tasks

cmaglie added a commit to cmaglie/arduino-cli that referenced this pull request Sep 1, 2022

Added test for regression

9b8ddcf

See arduino#1817 (review)

cmaglie and others added 13 commits September 1, 2022 18:24

Removed IDE-bundle autodetection

fd5dc83

Do not return bundled tools folder with HardwarePaths

006f710

Created configuration functions to obtain data dir and downloads dir

deda8e7

Bundled libraries configuration is now considere a single directory

49d4dba

Allow library installation on bundled libs dir via gRPC

a213270

Added integration test for library install in bundled directory

bc9c8cb

Fixed nil pointer exception in build options extraction

1bcfd73

Updated docs

a4c87f4

Removed useless custom env bindings

00f214a

Applied code review changes

aaea3be

Co-authored-by: per1234 <[email protected]>

Set builtin libraries dir by default when running as a daemon

3c19b77

Added test for regression

be777ad

See arduino#1817 (review)

Fixed regression: 'lib uninstall' must operate only on user dir

ad98097

cmaglie force-pushed the byebye_bundles branch from 499b963 to ad98097 Compare September 1, 2022 16:24

per1234 approved these changes Sep 1, 2022

View reviewed changes

cmaglie merged commit 4a4d1fd into arduino:master Sep 2, 2022

cmaglie deleted the byebye_bundles branch September 2, 2022 07:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[breaking] Remove auto detection of Arduino IDE built-in libraries and tools / Allow gRPC install of built-in libraries #1817

[breaking] Remove auto detection of Arduino IDE built-in libraries and tools / Allow gRPC install of built-in libraries #1817

cmaglie commented Jul 27, 2022

cmaglie commented Aug 5, 2022

codecov bot commented Aug 27, 2022 •

edited

Loading

cmaglie commented Aug 29, 2022 •

edited

Loading

kittaakos commented Aug 31, 2022 •

edited

Loading

cmaglie commented Aug 31, 2022

kittaakos commented Aug 31, 2022

cmaglie commented Aug 31, 2022

cmaglie commented Aug 31, 2022

per1234 left a comment •

edited

Loading

cmaglie commented Sep 1, 2022

per1234 left a comment

[breaking] Remove auto detection of Arduino IDE built-in libraries and tools / Allow gRPC install of built-in libraries #1817

[breaking] Remove auto detection of Arduino IDE built-in libraries and tools / Allow gRPC install of built-in libraries #1817

Conversation

cmaglie commented Jul 27, 2022

cmaglie commented Aug 5, 2022

codecov bot commented Aug 27, 2022 • edited Loading

Codecov Report

cmaglie commented Aug 29, 2022 • edited Loading

kittaakos commented Aug 31, 2022 • edited Loading

cmaglie commented Aug 31, 2022

kittaakos commented Aug 31, 2022

cmaglie commented Aug 31, 2022

cmaglie commented Aug 31, 2022

per1234 left a comment • edited Loading

Choose a reason for hiding this comment

Describe the problem

To reproduce

Set up

Demo

Expected behavior

Arduino CLI version

Operating system

Additional context

cmaglie commented Sep 1, 2022

per1234 left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 27, 2022 •

edited

Loading

cmaglie commented Aug 29, 2022 •

edited

Loading

kittaakos commented Aug 31, 2022 •

edited

Loading

per1234 left a comment •

edited

Loading