Skip to content

Conversation

cmaglie
Copy link
Member

@cmaglie cmaglie commented Aug 26, 2022

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?
    Merge the feature branch daemon-fixes into master.
    No code review is required, it has already been done in the other PRs.

cmaglie and others added 2 commits August 10, 2022 14:48
* Improved streaming of pluggable-discoveries events (WIP)

Now the DiscoveryManager is able to start the discoveries and add/remove
them in a thread-safe way. Also the watchers may connect and disconnect
seamlessly at any time, the incoming events from the discovery are
broadcasted correctly to each active watcher.

This refactoring dramatically simplifies the DiscoveryManager design.

* Added discovery id in discovery.Event struct

* Cache active ports and transmit them when a new watcher connects

* Correctly handle discovery cleanup

* Fixed wrong test

* Correctly handle discovery cleanup and re-add

* Added some doc comments in the source code

* Move Unlock under defer

* Factored subrotuine into a function

it will be useful in the next commits.

* Do not cache ports in the DiscoveryClient

there is already a cache in the DiscoveryManager there is no need to
duplicate it.

* Discovery: eventChan must be protected by mutex when doing START_SYNC

otherwise the discovery may send some events before the eventChan is
setup (and those events will be lost)

* Increased error level for logging watchers that lags

* Updated discvoery_client to the latest API

* Report discovery start errors

* Update arduino/discovery/discovery_client/main.go

Co-authored-by: Umberto Baldi <34278123+umbynos@users.noreply.github.com>

Co-authored-by: Umberto Baldi <34278123+umbynos@users.noreply.github.com>
…nager (#1828)

* legacy: Removed ToolsLoader step

It has been splitted and merged into HardwareLoader and TargetBoardResolver
that are more appropriate.

* Fixed some function comments

* Thread-safe protect access to instances map

* Removed state-altering methods from PackageManager

They have been moved into a Builder object that has the ability to build
a new PackageManager. This allows to clearly separate subrotuines that
actually change the status of the PackageManager from subroutines that
just need to query it.

* Created packagemanager.Explorer to query PackageManager data

The Explorer object can be see as a read-only "view" to the underlying
PackageManager: we may ask the PackageManager to create an Explorer on
itself. The returned explorer will held a read-lock on the
PackageManager until it's disposed.

This architecture should prevent unwanted changes on the PackageManager
while it's being used, and viceversa, when the PackageManager is updated
it should be guaranteed that no Explorers are reading it.

* PlatformInstall/Uninstall must release PackageManager.Explorer before calling commands.Init

Otherwise, since Init will try to take a write-lock, it will block
indefinitely.

* Moved commands.InstanceContainer -> rpc.InstanceCommand

* Created a coreInstancesContainer

This container will handle all the atomic access to the instances map.

* Made CoreInstance.PackageManager field private

* Moved the reminder of PackageManager functions to Explorer or Builder

* Now GetPackageManager accepts an rpc.InstanceCommand

It has also been deprecated in favor of GetPackageManagerExplorer.

* Now GetLibraryManager accepts an rpc.InstanceCommand

* Refactored automatic builtin-tool installation

* Added gRPC LibraryUpgrade call and fixed 'lib upgrade' command

* Explorer and Builder should not extend PackageManager

Previuosly the methods PackageManager.NewBuilder and
PackageManager.NewExplorer were available also on Builder and Explorer.

Now Builder and Explorer does not inherith these methods anymore,
avoiding trivial errors like the one fixed in this commit in the
builder_utils package.

* Updated documentation

* Apply suggestions from code review

Co-authored-by: per1234 <accounts@perglass.com>
@cmaglie cmaglie changed the title Merge deamon-fix tracking branch into master Merge daemon-fixes tracking branch into master Aug 26, 2022
@codecov
Copy link

codecov bot commented Aug 26, 2022

Codecov Report

Merging #1851 (276b0cc) into master (312cfdb) will decrease coverage by 0.22%.
The diff coverage is 29.46%.

@@            Coverage Diff             @@
##           master    #1851      +/-   ##
==========================================
- Coverage   36.34%   36.12%   -0.23%     
==========================================
  Files         232      231       -1     
  Lines       19496    19664     +168     
==========================================
+ Hits         7086     7103      +17     
- Misses      11583    11731     +148     
- Partials      827      830       +3     
Flag Coverage Δ
unit 36.12% <29.46%> (-0.23%) ⬇️

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

Impacted Files Coverage Δ
arduino/cores/packagemanager/profiles.go 0.00% <0.00%> (ø)
arduino/cores/status.go 56.07% <ø> (ø)
arduino/discovery/discovery.go 19.63% <0.00%> (+0.67%) ⬆️
cli/arguments/completion.go 0.00% <0.00%> (ø)
cli/arguments/port.go 0.00% <0.00%> (ø)
cli/board/list.go 0.00% <0.00%> (ø)
cli/compile/compile.go 0.00% <0.00%> (ø)
cli/lib/upgrade.go 0.00% <0.00%> (ø)
cli/upload/upload.go 0.00% <0.00%> (ø)
commands/board/attach.go 0.00% <0.00%> (ø)
... and 60 more

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

@per1234 per1234 added topic: gRPC Related to the gRPC interface topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project labels Aug 26, 2022
@cmaglie cmaglie requested review from per1234 and umbynos and removed request for per1234 August 26, 2022 11:02
@kittaakos kittaakos self-requested a review August 26, 2022 14:00
@cmaglie cmaglie requested a review from ubidefeo August 26, 2022 14:02
Copy link
Contributor

@kittaakos kittaakos left a comment

Choose a reason for hiding this comment

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

IDE2 has been using the test_1 for a while.

As discussed with @cmaglie, to align master and test_1, we need these changes 👍

@cmaglie cmaglie merged commit 63f1e18 into master Aug 26, 2022
@cmaglie cmaglie deleted the daemon-fixes branch August 26, 2022 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: code Related to content of the project itself topic: gRPC Related to the gRPC interface type: imperfection Perceived defect in any part of project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants