From a42a487ba54fb9715c86c3b8f338728655b10d81 Mon Sep 17 00:00:00 2001 From: Luca Bianconi Date: Wed, 1 Feb 2023 16:45:47 +0100 Subject: [PATCH 1/9] fix: clear discovery manager when spawing a new one Co-authored-by: Cristian Maglie --- .../cores/packagemanager/package_manager.go | 6 ++++- arduino/discovery/discovery.go | 1 + .../discoverymanager/discoverymanager.go | 12 ++++++++-- commands/instances.go | 22 ++++++++++++++++++- 4 files changed, 37 insertions(+), 4 deletions(-) diff --git a/arduino/cores/packagemanager/package_manager.go b/arduino/cores/packagemanager/package_manager.go index df7dafb7842..b7d30f5a4f3 100644 --- a/arduino/cores/packagemanager/package_manager.go +++ b/arduino/cores/packagemanager/package_manager.go @@ -98,7 +98,11 @@ func (pmb *Builder) BuildIntoExistingPackageManager(target *PackageManager) { target.tempDir = pmb.tempDir target.packagesCustomGlobalProperties = pmb.packagesCustomGlobalProperties target.profile = pmb.profile - target.discoveryManager = pmb.discoveryManager + if target.discoveryManager != nil { + target.discoveryManager.Clear() + } else { + target.discoveryManager = pmb.discoveryManager + } target.userAgent = pmb.userAgent } diff --git a/arduino/discovery/discovery.go b/arduino/discovery/discovery.go index 4ecd27252e6..b8c3537b5f4 100644 --- a/arduino/discovery/discovery.go +++ b/arduino/discovery/discovery.go @@ -381,6 +381,7 @@ func (disc *PluggableDiscovery) Quit() { if _, err := disc.waitMessage(time.Second * 5); err != nil { logrus.Errorf("Quitting discovery %s: %s", disc.id, err) } + disc.stopSync() disc.killProcess() } diff --git a/arduino/discovery/discoverymanager/discoverymanager.go b/arduino/discovery/discoverymanager/discoverymanager.go index 5cf3f2d3aa1..266cac1d2d0 100644 --- a/arduino/discovery/discoverymanager/discoverymanager.go +++ b/arduino/discovery/discoverymanager/discoverymanager.go @@ -65,9 +65,15 @@ func (dm *DiscoveryManager) Clear() { logrus.Infof("Closed and removed discovery %s", d.GetID()) } } + dm.discoveriesRunning = false dm.discoveries = map[string]*discovery.PluggableDiscovery{} } +// AreDiscoveriesRunning returns a boolean representing the running status of discoveries +func (dm *DiscoveryManager) AreDiscoveriesRunning() bool { + return dm.discoveriesRunning +} + // IDs returns the list of discoveries' ids in this DiscoveryManager func (dm *DiscoveryManager) IDs() []string { ids := []string{} @@ -194,12 +200,14 @@ func (dm *DiscoveryManager) startDiscovery(d *discovery.PluggableDiscovery) (dis return fmt.Errorf("%s: %s", tr("starting discovery %s", d.GetID()), err) } - go func() { + go func(d *discovery.PluggableDiscovery) { // Transfer all incoming events from this discovery to the feed channel for ev := range eventCh { + // here from discovery to discovery manager dm.feed <- ev } - }() + logrus.Infof("Discovery event channel closed %s. Exiting goroutine.", d.GetID()) + }(d) return nil } diff --git a/commands/instances.go b/commands/instances.go index 2c8c44ade5e..af0a94b388d 100644 --- a/commands/instances.go +++ b/commands/instances.go @@ -261,13 +261,17 @@ func Init(req *rpc.InitRequest, responseCallback func(r *rpc.InitResponse)) erro }, }) } - + var shouldRestartDiscovery bool { // We need to rebuild the PackageManager currently in use by this instance // in case this is not the first Init on this instances, that might happen // after reinitializing an instance after installing or uninstalling a core. // If this is not done the information of the uninstall core is kept in memory, // even if it should not. + + // register whether the discoveries are running, if so we need to start them in + // order for the previous watchers to keep receiving events + shouldRestartDiscovery = areDiscoveriesRunning(instance.pm) pmb, commitPackageManager := instance.pm.NewBuilder() loadBuiltinTools := func() []error { @@ -463,6 +467,9 @@ func Init(req *rpc.InitRequest, responseCallback func(r *rpc.InitResponse)) erro responseError(s) } + if shouldRestartDiscovery { + pme.DiscoveryManager().Start() + } // Refreshes the locale used, this will change the // language of the CLI if the locale is different // after started. @@ -471,6 +478,19 @@ func Init(req *rpc.InitRequest, responseCallback func(r *rpc.InitResponse)) erro return nil } +func areDiscoveriesRunning(pm *packagemanager.PackageManager) bool { + if pm == nil { + return false + } + exp, release := pm.NewExplorer() + defer release() + + if exp.DiscoveryManager() != nil && exp.DiscoveryManager().AreDiscoveriesRunning() { + return true + } + return false +} + // Destroy FIXMEDOC func Destroy(ctx context.Context, req *rpc.DestroyRequest) (*rpc.DestroyResponse, error) { if ok := instances.RemoveID(req.GetInstance().GetId()); !ok { From 1b6d1c1c1540f7932d34afe0aaaa8f9afa0e4fa7 Mon Sep 17 00:00:00 2001 From: Luca Bianconi Date: Fri, 3 Feb 2023 15:15:01 +0100 Subject: [PATCH 2/9] test: fix failing test --- .../packagemanager/package_manager_test.go | 36 ++++++++++--------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/arduino/cores/packagemanager/package_manager_test.go b/arduino/cores/packagemanager/package_manager_test.go index adecae80f28..39a2d8d0c66 100644 --- a/arduino/cores/packagemanager/package_manager_test.go +++ b/arduino/cores/packagemanager/package_manager_test.go @@ -13,7 +13,7 @@ // Arduino software without disclosing the source code of your own applications. // To purchase a commercial license, send an email to license@arduino.cc. -package packagemanager_test +package packagemanager import ( "fmt" @@ -24,7 +24,6 @@ import ( "testing" "github.com/arduino/arduino-cli/arduino/cores" - "github.com/arduino/arduino-cli/arduino/cores/packagemanager" "github.com/arduino/arduino-cli/configuration" "github.com/arduino/go-paths-helper" "github.com/arduino/go-properties-orderedmap" @@ -39,7 +38,7 @@ var dataDir1 = paths.New("testdata", "data_dir_1") var extraHardware = paths.New("testdata", "extra_hardware") func TestFindBoardWithFQBN(t *testing.T) { - pmb := packagemanager.NewBuilder(customHardware, customHardware, customHardware, customHardware, "test") + pmb := NewBuilder(customHardware, customHardware, customHardware, customHardware, "test") pmb.LoadHardwareFromDirectory(customHardware) pm := pmb.Build() pme, release := pm.NewExplorer() @@ -57,7 +56,7 @@ func TestFindBoardWithFQBN(t *testing.T) { func TestResolveFQBN(t *testing.T) { // Pass nil, since these paths are only used for installing - pmb := packagemanager.NewBuilder(nil, nil, nil, nil, "test") + pmb := NewBuilder(nil, nil, nil, nil, "test") // Hardware from main packages directory pmb.LoadHardwareFromDirectory(dataDir1.Join("packages")) // This contains the arduino:avr core @@ -181,7 +180,7 @@ func TestResolveFQBN(t *testing.T) { } func TestBoardOptionsFunctions(t *testing.T) { - pmb := packagemanager.NewBuilder(customHardware, customHardware, customHardware, customHardware, "test") + pmb := NewBuilder(customHardware, customHardware, customHardware, customHardware, "test") pmb.LoadHardwareFromDirectory(customHardware) pm := pmb.Build() pme, release := pm.NewExplorer() @@ -221,13 +220,13 @@ func TestBoardOptionsFunctions(t *testing.T) { } func TestBoardOrdering(t *testing.T) { - pmb := packagemanager.NewBuilder(dataDir1, dataDir1.Join("packages"), nil, nil, "") + pmb := NewBuilder(dataDir1, dataDir1.Join("packages"), nil, nil, "") _ = pmb.LoadHardwareFromDirectories(paths.NewPathList(dataDir1.Join("packages").String())) pm := pmb.Build() pme, release := pm.NewExplorer() defer release() - pl := pme.FindPlatform(&packagemanager.PlatformReference{ + pl := pme.FindPlatform(&PlatformReference{ Package: "arduino", PlatformArchitecture: "avr", }) @@ -273,7 +272,7 @@ func TestBoardOrdering(t *testing.T) { func TestFindToolsRequiredForBoard(t *testing.T) { os.Setenv("ARDUINO_DATA_DIR", dataDir1.String()) configuration.Settings = configuration.Init("") - pmb := packagemanager.NewBuilder( + pmb := NewBuilder( dataDir1, configuration.PackagesDir(configuration.Settings), configuration.DownloadsDir(configuration.Settings), @@ -314,7 +313,7 @@ func TestFindToolsRequiredForBoard(t *testing.T) { }) require.NotNil(t, esptool0413) - testPlatform := pme.FindPlatformRelease(&packagemanager.PlatformReference{ + testPlatform := pme.FindPlatformRelease(&PlatformReference{ Package: "test", PlatformArchitecture: "avr", PlatformVersion: semver.MustParse("1.1.0")}) @@ -407,7 +406,7 @@ func TestFindToolsRequiredForBoard(t *testing.T) { } func TestIdentifyBoard(t *testing.T) { - pmb := packagemanager.NewBuilder(customHardware, customHardware, customHardware, customHardware, "test") + pmb := NewBuilder(customHardware, customHardware, customHardware, customHardware, "test") pmb.LoadHardwareFromDirectory(customHardware) pm := pmb.Build() pme, release := pm.NewExplorer() @@ -434,12 +433,12 @@ func TestIdentifyBoard(t *testing.T) { func TestPackageManagerClear(t *testing.T) { // Create a PackageManager and load the harware - pmb := packagemanager.NewBuilder(customHardware, customHardware, customHardware, customHardware, "test") + pmb := NewBuilder(customHardware, customHardware, customHardware, customHardware, "test") pmb.LoadHardwareFromDirectory(customHardware) pm := pmb.Build() // Creates another PackageManager but don't load the hardware - emptyPmb := packagemanager.NewBuilder(customHardware, customHardware, customHardware, customHardware, "test") + emptyPmb := NewBuilder(customHardware, customHardware, customHardware, customHardware, "test") emptyPm := emptyPmb.Build() // Verifies they're not equal @@ -448,7 +447,10 @@ func TestPackageManagerClear(t *testing.T) { // Clear the first PackageManager that contains loaded hardware emptyPmb.BuildIntoExistingPackageManager(pm) - // Verifies both PackageManagers are now equal + // the discovery manager is maintained + require.NotEqual(t, pm.discoveryManager, emptyPm.discoveryManager) + // Verifies all other fields are assigned to target + pm.discoveryManager = emptyPm.discoveryManager require.Equal(t, pm, emptyPm) } @@ -456,7 +458,7 @@ func TestFindToolsRequiredFromPlatformRelease(t *testing.T) { // Create all the necessary data to load discoveries fakePath := paths.New("fake-path") - pmb := packagemanager.NewBuilder(fakePath, fakePath, fakePath, fakePath, "test") + pmb := NewBuilder(fakePath, fakePath, fakePath, fakePath, "test") pack := pmb.GetOrCreatePackage("arduino") { @@ -559,13 +561,13 @@ func TestFindToolsRequiredFromPlatformRelease(t *testing.T) { } func TestFindPlatformReleaseDependencies(t *testing.T) { - pmb := packagemanager.NewBuilder(nil, nil, nil, nil, "test") + pmb := NewBuilder(nil, nil, nil, nil, "test") pmb.LoadPackageIndexFromFile(paths.New("testdata", "package_tooltest_index.json")) pm := pmb.Build() pme, release := pm.NewExplorer() defer release() - pl, tools, err := pme.FindPlatformReleaseDependencies(&packagemanager.PlatformReference{Package: "test", PlatformArchitecture: "avr"}) + pl, tools, err := pme.FindPlatformReleaseDependencies(&PlatformReference{Package: "test", PlatformArchitecture: "avr"}) require.NoError(t, err) require.NotNil(t, pl) require.Len(t, tools, 3) @@ -574,7 +576,7 @@ func TestFindPlatformReleaseDependencies(t *testing.T) { func TestLegacyPackageConversionToPluggableDiscovery(t *testing.T) { // Pass nil, since these paths are only used for installing - pmb := packagemanager.NewBuilder(nil, nil, nil, nil, "test") + pmb := NewBuilder(nil, nil, nil, nil, "test") // Hardware from main packages directory pmb.LoadHardwareFromDirectory(dataDir1.Join("packages")) pm := pmb.Build() From b73907b002f1435f7b80bd2ef0a8fa267bf66516 Mon Sep 17 00:00:00 2001 From: Luca Bianconi Date: Mon, 13 Feb 2023 09:20:26 +0100 Subject: [PATCH 3/9] chore: rm useless comment --- arduino/discovery/discoverymanager/discoverymanager.go | 1 - 1 file changed, 1 deletion(-) diff --git a/arduino/discovery/discoverymanager/discoverymanager.go b/arduino/discovery/discoverymanager/discoverymanager.go index 266cac1d2d0..4a31e9622e9 100644 --- a/arduino/discovery/discoverymanager/discoverymanager.go +++ b/arduino/discovery/discoverymanager/discoverymanager.go @@ -203,7 +203,6 @@ func (dm *DiscoveryManager) startDiscovery(d *discovery.PluggableDiscovery) (dis go func(d *discovery.PluggableDiscovery) { // Transfer all incoming events from this discovery to the feed channel for ev := range eventCh { - // here from discovery to discovery manager dm.feed <- ev } logrus.Infof("Discovery event channel closed %s. Exiting goroutine.", d.GetID()) From 27625c5be0806c35401ef86e7d3988377fde7af3 Mon Sep 17 00:00:00 2001 From: Luca Bianconi <71259950+Bikappa@users.noreply.github.com> Date: Tue, 28 Feb 2023 16:02:08 +0100 Subject: [PATCH 4/9] refactor: pr comments --- .../cores/packagemanager/package_manager.go | 1 + .../discoverymanager/discoverymanager.go | 17 +++++++++++------ commands/instances.go | 19 +------------------ 3 files changed, 13 insertions(+), 24 deletions(-) diff --git a/arduino/cores/packagemanager/package_manager.go b/arduino/cores/packagemanager/package_manager.go index b7d30f5a4f3..b9012a26f63 100644 --- a/arduino/cores/packagemanager/package_manager.go +++ b/arduino/cores/packagemanager/package_manager.go @@ -103,6 +103,7 @@ func (pmb *Builder) BuildIntoExistingPackageManager(target *PackageManager) { } else { target.discoveryManager = pmb.discoveryManager } + target.discoveryManager.AddAllDiscoveriesFrom(pmb.discoveryManager) target.userAgent = pmb.userAgent } diff --git a/arduino/discovery/discoverymanager/discoverymanager.go b/arduino/discovery/discoverymanager/discoverymanager.go index 4a31e9622e9..b8c85974f8b 100644 --- a/arduino/discovery/discoverymanager/discoverymanager.go +++ b/arduino/discovery/discoverymanager/discoverymanager.go @@ -65,15 +65,9 @@ func (dm *DiscoveryManager) Clear() { logrus.Infof("Closed and removed discovery %s", d.GetID()) } } - dm.discoveriesRunning = false dm.discoveries = map[string]*discovery.PluggableDiscovery{} } -// AreDiscoveriesRunning returns a boolean representing the running status of discoveries -func (dm *DiscoveryManager) AreDiscoveriesRunning() bool { - return dm.discoveriesRunning -} - // IDs returns the list of discoveries' ids in this DiscoveryManager func (dm *DiscoveryManager) IDs() []string { ids := []string{} @@ -283,3 +277,14 @@ func (dm *DiscoveryManager) List() []*discovery.Port { } return res } + +// AddAllDiscoveriesFrom transfers discoveries from src to the receiver +func (dm *DiscoveryManager) AddAllDiscoveriesFrom(src *DiscoveryManager) { + for id, d := range src.discoveries { + dm.discoveries[id] = d + } + + if src.discoveriesRunning { + dm.Start() + } +} diff --git a/commands/instances.go b/commands/instances.go index af0a94b388d..14b2f16cd2f 100644 --- a/commands/instances.go +++ b/commands/instances.go @@ -261,7 +261,7 @@ func Init(req *rpc.InitRequest, responseCallback func(r *rpc.InitResponse)) erro }, }) } - var shouldRestartDiscovery bool + { // We need to rebuild the PackageManager currently in use by this instance // in case this is not the first Init on this instances, that might happen @@ -271,7 +271,6 @@ func Init(req *rpc.InitRequest, responseCallback func(r *rpc.InitResponse)) erro // register whether the discoveries are running, if so we need to start them in // order for the previous watchers to keep receiving events - shouldRestartDiscovery = areDiscoveriesRunning(instance.pm) pmb, commitPackageManager := instance.pm.NewBuilder() loadBuiltinTools := func() []error { @@ -467,9 +466,6 @@ func Init(req *rpc.InitRequest, responseCallback func(r *rpc.InitResponse)) erro responseError(s) } - if shouldRestartDiscovery { - pme.DiscoveryManager().Start() - } // Refreshes the locale used, this will change the // language of the CLI if the locale is different // after started. @@ -478,19 +474,6 @@ func Init(req *rpc.InitRequest, responseCallback func(r *rpc.InitResponse)) erro return nil } -func areDiscoveriesRunning(pm *packagemanager.PackageManager) bool { - if pm == nil { - return false - } - exp, release := pm.NewExplorer() - defer release() - - if exp.DiscoveryManager() != nil && exp.DiscoveryManager().AreDiscoveriesRunning() { - return true - } - return false -} - // Destroy FIXMEDOC func Destroy(ctx context.Context, req *rpc.DestroyRequest) (*rpc.DestroyResponse, error) { if ok := instances.RemoveID(req.GetInstance().GetId()); !ok { From fbce83805eca7f8acc16752aefb1da0c9a1cf09f Mon Sep 17 00:00:00 2001 From: Luca Bianconi <71259950+Bikappa@users.noreply.github.com> Date: Thu, 9 Mar 2023 09:25:14 +0100 Subject: [PATCH 5/9] chore: removed comment --- commands/instances.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/commands/instances.go b/commands/instances.go index 14b2f16cd2f..2c8c44ade5e 100644 --- a/commands/instances.go +++ b/commands/instances.go @@ -268,9 +268,6 @@ func Init(req *rpc.InitRequest, responseCallback func(r *rpc.InitResponse)) erro // after reinitializing an instance after installing or uninstalling a core. // If this is not done the information of the uninstall core is kept in memory, // even if it should not. - - // register whether the discoveries are running, if so we need to start them in - // order for the previous watchers to keep receiving events pmb, commitPackageManager := instance.pm.NewBuilder() loadBuiltinTools := func() []error { From 147179e2aaa3c73d2a1fcc14c7911f093cee0c31 Mon Sep 17 00:00:00 2001 From: Luca Bianconi <71259950+Bikappa@users.noreply.github.com> Date: Thu, 9 Mar 2023 10:15:47 +0100 Subject: [PATCH 6/9] Update arduino/discovery/discoverymanager/discoverymanager.go Co-authored-by: Cristian Maglie --- arduino/discovery/discoverymanager/discoverymanager.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/arduino/discovery/discoverymanager/discoverymanager.go b/arduino/discovery/discoverymanager/discoverymanager.go index b8c85974f8b..bcc56eaee88 100644 --- a/arduino/discovery/discoverymanager/discoverymanager.go +++ b/arduino/discovery/discoverymanager/discoverymanager.go @@ -281,10 +281,6 @@ func (dm *DiscoveryManager) List() []*discovery.Port { // AddAllDiscoveriesFrom transfers discoveries from src to the receiver func (dm *DiscoveryManager) AddAllDiscoveriesFrom(src *DiscoveryManager) { for id, d := range src.discoveries { - dm.discoveries[id] = d - } - - if src.discoveriesRunning { - dm.Start() + dm.Add(d) } } From 35a4eea7cc7bf547f64618e486101f7ee23bca7f Mon Sep 17 00:00:00 2001 From: Luca Bianconi <71259950+Bikappa@users.noreply.github.com> Date: Thu, 9 Mar 2023 17:05:21 +0100 Subject: [PATCH 7/9] fix: unused var --- arduino/discovery/discoverymanager/discoverymanager.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arduino/discovery/discoverymanager/discoverymanager.go b/arduino/discovery/discoverymanager/discoverymanager.go index bcc56eaee88..7d33115ce31 100644 --- a/arduino/discovery/discoverymanager/discoverymanager.go +++ b/arduino/discovery/discoverymanager/discoverymanager.go @@ -280,7 +280,7 @@ func (dm *DiscoveryManager) List() []*discovery.Port { // AddAllDiscoveriesFrom transfers discoveries from src to the receiver func (dm *DiscoveryManager) AddAllDiscoveriesFrom(src *DiscoveryManager) { - for id, d := range src.discoveries { + for _, d := range src.discoveries { dm.Add(d) } } From 61ac16aaf663b48235e51a0368eb09dec8aeaf91 Mon Sep 17 00:00:00 2001 From: Luca Bianconi <71259950+Bikappa@users.noreply.github.com> Date: Mon, 13 Mar 2023 16:34:22 +0100 Subject: [PATCH 8/9] fix: initialize in builder --- .../cores/packagemanager/package_manager.go | 21 ++++++++++++------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/arduino/cores/packagemanager/package_manager.go b/arduino/cores/packagemanager/package_manager.go index b9012a26f63..f385faf2299 100644 --- a/arduino/cores/packagemanager/package_manager.go +++ b/arduino/cores/packagemanager/package_manager.go @@ -98,20 +98,25 @@ func (pmb *Builder) BuildIntoExistingPackageManager(target *PackageManager) { target.tempDir = pmb.tempDir target.packagesCustomGlobalProperties = pmb.packagesCustomGlobalProperties target.profile = pmb.profile - if target.discoveryManager != nil { - target.discoveryManager.Clear() - } else { - target.discoveryManager = pmb.discoveryManager - } + target.discoveryManager.Clear() target.discoveryManager.AddAllDiscoveriesFrom(pmb.discoveryManager) target.userAgent = pmb.userAgent } // Build builds a new PackageManager. func (pmb *Builder) Build() *PackageManager { - res := &PackageManager{} - pmb.BuildIntoExistingPackageManager(res) - return res + return &PackageManager{ + log: pmb.log, + packages: pmb.packages, + IndexDir: pmb.IndexDir, + PackagesDir: pmb.PackagesDir, + DownloadDir: pmb.DownloadDir, + tempDir: pmb.tempDir, + packagesCustomGlobalProperties: pmb.packagesCustomGlobalProperties, + profile: pmb.profile, + discoveryManager: pmb.discoveryManager, + userAgent: pmb.userAgent, + } } // NewBuilder creates a Builder with the same configuration From f661846c16b3c2d5744191e09b1c8a1d3163162b Mon Sep 17 00:00:00 2001 From: Luca Bianconi <71259950+Bikappa@users.noreply.github.com> Date: Mon, 13 Mar 2023 16:38:31 +0100 Subject: [PATCH 9/9] fix: merge error --- arduino/cores/packagemanager/package_manager_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arduino/cores/packagemanager/package_manager_test.go b/arduino/cores/packagemanager/package_manager_test.go index 39a2d8d0c66..eb63ba1315d 100644 --- a/arduino/cores/packagemanager/package_manager_test.go +++ b/arduino/cores/packagemanager/package_manager_test.go @@ -645,7 +645,7 @@ func TestLegacyPackageConversionToPluggableDiscovery(t *testing.T) { } func TestRunPostInstall(t *testing.T) { - pmb := packagemanager.NewBuilder(nil, nil, nil, nil, "test") + pmb := NewBuilder(nil, nil, nil, nil, "test") pm := pmb.Build() pme, release := pm.NewExplorer() defer release()