Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

[breaking] daemon: Fix concurrency and streamline access to PackageManager #1828

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 17 commits into from
Aug 26, 2022
Merged

[breaking] daemon: Fix concurrency and streamline access to PackageManager #1828

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
7ce9fd4
legacy: Removed ToolsLoader step
cmaglie Aug 5, 2022
8bec66e
Fixed some function comments
cmaglie Aug 22, 2022
1d3b99e
Thread-safe protect access to instances map
cmaglie Aug 3, 2022
ea01f19
Removed state-altering methods from PackageManager
cmaglie Aug 5, 2022
1b8a35c
Created packagemanager.Explorer to query PackageManager data
cmaglie Aug 5, 2022
6d6210c
PlatformInstall/Uninstall must release PackageManager.Explorer before…
cmaglie Aug 6, 2022
3b39d7c
Moved commands.InstanceContainer -> rpc.InstanceCommand
cmaglie Aug 11, 2022
4db186e
Created a coreInstancesContainer
cmaglie Aug 11, 2022
dd0414f
Made CoreInstance.PackageManager field private
cmaglie Aug 11, 2022
007a0ff
Moved the reminder of PackageManager functions to Explorer or Builder
cmaglie Aug 18, 2022
316d1d3
Now GetPackageManager accepts an rpc.InstanceCommand
cmaglie Aug 21, 2022
dbd6b01
Now GetLibraryManager accepts an rpc.InstanceCommand
cmaglie Aug 21, 2022
6438e58
Refactored automatic builtin-tool installation
cmaglie Aug 21, 2022
e8ef962
Added gRPC LibraryUpgrade call and fixed 'lib upgrade' command
cmaglie Aug 21, 2022
46eeee2
Explorer and Builder should not extend PackageManager
cmaglie Aug 22, 2022
04e70c5
Updated documentation
cmaglie Aug 23, 2022
df28c7c
Apply suggestions from code review
cmaglie Aug 24, 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
Prev Previous commit
Next Next commit
Updated documentation
  • Loading branch information
@cmaglie
cmaglie committed Aug 23, 2022
commit 04e70c53f24c0b3738cbdfbf685aa8a3ca635217
5 changes: 2 additions & 3 deletions 5 arduino/cores/packagemanager/package_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,9 +117,8 @@ func (pm *PackageManager) NewBuilder() (builder *Builder, commit func()) {

// NewExplorer creates an Explorer for this PackageManager.
// The Explorer will keep a read-lock on the underlying PackageManager,
// and a "release" callback function to release the lock is returned.
// The "release" function must be called when the Explorer is no more
// to correctly dispose it.
// the user must call the "release" callback function to release the lock
// when the Explorer is no more needed.
func (pm *PackageManager) NewExplorer() (explorer *Explorer, release func()) {
pm.packagesLock.RLock()
return &Explorer{
Expand Down
264 changes: 264 additions & 0 deletions 264 docs/UPGRADING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,270 @@

Here you can find a list of migration guides to handle breaking changes between releases of the CLI.

## 0.27.0

### Breaking changes in golang API `github.com/arduino/arduino-cli/arduino/cores/packagemanager.PackageManager`

The `PackageManager` API has been heavily refactored to correctly handle multitasking and concurrency. Many fields in
the PackageManager object are now private. All the `PackageManager` methods have been moved into other objects. In
particular:

- the methods that query the `PackageManager` without changing its internal state, have been moved into the new
`Explorer` object
- the methods that change the `PackageManager` internal state, have been moved into the new `Builder` object.

The `Builder` object must be used to create a new `PackageManager`. Previously the function `NewPackageManager` was used
to get a clean `PackageManager` object and then use the `LoadHardware*` methods to build it. Now the function
`NewBuilder` must be used to create a `Builder`, run the `LoadHardware*` methods to load platforms, and finally call the
`Builder.Build()` method to obtain the final `PackageManager`.

Previously we did:

```go
pm := packagemanager.NewPackageManager(...)
err = pm.LoadHardware()
err = pm.LoadHardwareFromDirectories(...)
err = pm.LoadHardwareFromDirectory(...)
err = pm.LoadToolsFromPackageDir(...)
err = pm.LoadToolsFromBundleDirectories(...)
err = pm.LoadToolsFromBundleDirectory(...)
pack = pm.GetOrCreatePackage("packagername")
// ...use `pack` to tweak or load more hardware...
err = pm.LoadPackageIndex(...)
err = pm.LoadPackageIndexFromFile(...)
err = pm.LoadHardwareForProfile(...)

// ...use `pm` to implement business logic...
```

Now we must do:

```go
var pm *packagemanager.PackageManager

{
pmb := packagemanager.Newbuilder(...)
err = pmb.LoadHardware()
err = pmb.LoadHardwareFromDirectories(...)
err = pmb.LoadHardwareFromDirectory(...)
err = pmb.LoadToolsFromPackageDir(...)
err = pmb.LoadToolsFromBundleDirectories(...)
err = pmb.LoadToolsFromBundleDirectory(...)
pack = pmb.GetOrCreatePackage("packagername")
// ...use `pack` to tweak or load more hardware...
err = pmb.LoadPackageIndex(...)
err = pmb.LoadPackageIndexFromFile(...)
err = pmb.LoadHardwareForProfile(...)
pm = pmb.Build()
}

// ...use `pm` to implement business logic...
```

It's not mandatory but highly recommended, to drop the `Builder` object once it has built the `PackageManager` (that's
why in the example the `pmb` builder is created in a limited scope between braces).

To query the `PackagerManager` now it is required to obtain an `Explorer` object through the
`PackageManager.NewExplorer()` method.

Previously we did:

```go
func DoStuff(pm *packagemanager.PackageManager, ...) {
// ...implement business logic through PackageManager methods...
... := pm.Packages
... := pm.CustomGlobalProperties
... := pm.FindPlatform(...)
... := pm.FindPlatformRelease(...)
... := pm.FindPlatformReleaseDependencies(...)
... := pm.DownloadToolRelease(...)
... := pm.DownloadPlatformRelease(...)
... := pm.IdentifyBoard(...)
... := pm.DownloadAndInstallPlatformUpgrades(...)
... := pm.DownloadAndInstallPlatformAndTools(...)
... := pm.InstallPlatform(...)
... := pm.InstallPlatformInDirectory(...)
... := pm.RunPostInstallScript(...)
... := pm.IsManagedPlatformRelease(...)
... := pm.UninstallPlatform(...)
... := pm.InstallTool(...)
... := pm.IsManagedToolRelease(...)
... := pm.UninstallTool(...)
... := pm.IsToolRequired(...)
... := pm.LoadDiscoveries(...)
... := pm.GetProfile(...)
... := pm.GetEnvVarsForSpawnedProcess(...)
... := pm.DiscoveryManager(...)
... := pm.FindPlatformReleaseProvidingBoardsWithVidPid(...)
... := pm.FindBoardsWithVidPid(...)
... := pm.FindBoardsWithID(...)
... := pm.FindBoardWithFQBN(...)
... := pm.ResolveFQBN(...)
... := pm.Package(...)
... := pm.GetInstalledPlatformRelease(...)
... := pm.GetAllInstalledToolsReleases(...)
... := pm.InstalledPlatformReleases(...)
... := pm.InstalledBoards(...)
... := pm.FindToolsRequiredFromPlatformRelease(...)
... := pm.GetTool(...)
... := pm.FindToolsRequiredForBoard(...)
... := pm.FindToolDependency(...)
... := pm.FindDiscoveryDependency(...)
... := pm.FindMonitorDependency(...)
}
```

Now we must obtain the `Explorer` object to access the same methods, moreover, we must call the `release` callback
function once we complete the task:

```go
func DoStuff(pm *packagemanager.PackageManager, ...) {
pme, release := pm.NewExplorer()
defer release()

... := pme.GetPackages()
... := pme.GetCustomGlobalProperties()
... := pme.FindPlatform(...)
... := pme.FindPlatformRelease(...)
... := pme.FindPlatformReleaseDependencies(...)
... := pme.DownloadToolRelease(...)
... := pme.DownloadPlatformRelease(...)
... := pme.IdentifyBoard(...)
... := pme.DownloadAndInstallPlatformUpgrades(...)
... := pme.DownloadAndInstallPlatformAndTools(...)
... := pme.InstallPlatform(...)
... := pme.InstallPlatformInDirectory(...)
... := pme.RunPostInstallScript(...)
... := pme.IsManagedPlatformRelease(...)
... := pme.UninstallPlatform(...)
... := pme.InstallTool(...)
... := pme.IsManagedToolRelease(...)
... := pme.UninstallTool(...)
... := pme.IsToolRequired(...)
... := pme.LoadDiscoveries(...)
... := pme.GetProfile(...)
... := pme.GetEnvVarsForSpawnedProcess(...)
... := pme.DiscoveryManager(...)
... := pme.FindPlatformReleaseProvidingBoardsWithVidPid(...)
... := pme.FindBoardsWithVidPid(...)
... := pme.FindBoardsWithID(...)
... := pme.FindBoardWithFQBN(...)
... := pme.ResolveFQBN(...)
... := pme.Package(...)
... := pme.GetInstalledPlatformRelease(...)
... := pme.GetAllInstalledToolsReleases(...)
... := pme.InstalledPlatformReleases(...)
... := pme.InstalledBoards(...)
... := pme.FindToolsRequiredFromPlatformRelease(...)
... := pme.GetTool(...)
... := pme.FindToolsRequiredForBoard(...)
... := pme.FindToolDependency(...)
... := pme.FindDiscoveryDependency(...)
... := pme.FindMonitorDependency(...)
}
```

The `Explorer` object keeps a read-lock on the underlying `PackageManager` that must be released once the task is done
by calling the `release` callback function. This ensures that no other task will change the status of the
`PackageManager` while the current task is in progress.

The `PackageManager.Clean()` method has been removed and replaced by the methods:

- `PackageManager.NewBuilder() (*Builder, commit func())`
- `Builder.BuildIntoExistingPackageManager(target *PackageManager)`
cmaglie marked this conversation as resolved.
Show resolved Hide resolved

Previously, to update a `PackageManager` instance we did:

```go
func Reload(pm *packagemanager.PackageManager) {
pm.Clear()
... = pm.LoadHardware(...)
// ...other pm.Load* calls...
}
```

now we have two options:

```go
func Reload(pm *packagemanager.PackageManager) {
// Create a new builder and build a package manager
pmb := packagemanager.NewBuilder(.../* config params */)
... = pmb.LoadHardware(...)
// ...other pmb.Load* calls...

// apply the changes to the original pm
pmb.BuildIntoExistingPackageManager(pm)
}
```

in this case, we create a new `Builder` with the given config params and once the package manager is built we apply the
changes atomically with `BuildIntoExistingPackageManager`. This procedure may be even more simplified with:

```go
func Reload(pm *packagemanager.PackageManager) {
// Create a new builder using the same config params
// as the original package manager
pmb, commit := pm.NewBuilder()

// build the new package manager
... = pmb.LoadHardware(...)
// ...other pmb.Load* calls...

// apply the changes to the original pm
commit()
}
```

In this case, we don't even need to bother to provide the configuration parameters because they are taken from the
previous `PackageManager` instance.

### Some gRPC-mapped methods now accepts the gRPC request instead of the instance ID as parameter

The following methods in subpackages of `github.com/arduino/arduino-cli/commands/*`:

```go
func Watch(instanceID int32) (<-chan *rpc.BoardListWatchResponse, func(), error) { ... }
func LibraryUpgradeAll(instanceID int32, downloadCB rpc.DownloadProgressCB, taskCB rpc.TaskProgressCB) error { ... }
func LibraryUpgrade(instanceID int32, libraryNames []string, downloadCB rpc.DownloadProgressCB, taskCB rpc.TaskProgressCB) error { ... }
```

have been changed to:

```go
func Watch(req *rpc.BoardListWatchRequest) (<-chan *rpc.BoardListWatchResponse, func(), error) { ... ]
cmaglie marked this conversation as resolved.
Show resolved Hide resolved
func LibraryUpgradeAll(req *rpc.LibraryUpgradeAllRequest, downloadCB rpc.DownloadProgressCB, taskCB rpc.TaskProgressCB) error { ... }
func LibraryUpgrade(ctx context.Context, req *rpc.LibraryUpgradeRequest, downloadCB rpc.DownloadProgressCB, taskCB rpc.TaskProgressCB) error { ... }
```

The following methods in package `github.com/arduino/arduino-cli/commands`

```go
func GetInstance(id int32) *CoreInstance { ... }
func GetPackageManager(id int32) *packagemanager.PackageManager { ... }
func GetLibraryManager(instanceID int32) *librariesmanager.LibrariesManager { ... }
```

have been changed to:

```go
func GetPackageManager(instance rpc.InstanceCommand) *packagemanager.PackageManager { ... } // Deprecated
func GetPackageManagerExplorer(req rpc.InstanceCommand) (explorer *packagemanager.Explorer, release func()) { ... ]
cmaglie marked this conversation as resolved.
Show resolved Hide resolved
func GetLibraryManager(req rpc.InstanceCommand) *librariesmanager.LibrariesManager { ... }
```

Old code using passing the `instanceID` inside the gRPC request must be changed to pass directly the whole gRPC request,
cmaglie marked this conversation as resolved.
Show resolved Hide resolved
for example:

```go
eventsChan, closeWatcher, err := board.Watch(req.Instance.Id)
```

must be changed to:

```go
eventsChan, closeWatcher, err := board.Watch(req)
```

## 0.26.0

### `github.com/arduino/arduino-cli/commands.DownloadToolRelease`, and `InstallToolRelease` functions have been removed
Expand Down
Morty Proxy This is a proxified and sanitized view of the page, visit original site.