[skip-changelog] refactoring: Added LibrariesManager.Clone() / Auto-scan libs on LibrariesManager.Build() (#2491)

* Added LibrariesManager.Clone() / Auto-scan libraries on LibrariesManager.Build()

* Ensure AddLibrariesDir do not share input parameters

* Fixed wrong loop... ooops

Author: cmaglie

commands/instances.go

@@ -308,7 +308,7 @@ func Init(req *rpc.InitRequest, responseCallback func(r *rpc.InitResponse)) erro
 	for _, pack := range pme.GetPackages() {
 		for _, platform := range pack.Platforms {
 			if platformRelease := pme.GetInstalledPlatformRelease(platform); platformRelease != nil {
-				lmb.AddLibrariesDir(&librariesmanager.LibrariesDir{
+				lmb.AddLibrariesDir(librariesmanager.LibrariesDir{
 					PlatformRelease: platformRelease,
 					Path:            platformRelease.GetLibrariesDir(),
 					Location:        libraries.PlatformBuiltIn,
@@ -336,14 +336,14 @@ func Init(req *rpc.InitRequest, responseCallback func(r *rpc.InitResponse)) erro
 	if profile == nil {
 		// Add directories of libraries bundled with IDE
 		if bundledLibsDir := configuration.IDEBuiltinLibrariesDir(configuration.Settings); bundledLibsDir != nil {
-			lmb.AddLibrariesDir(&librariesmanager.LibrariesDir{
+			lmb.AddLibrariesDir(librariesmanager.LibrariesDir{
 				Path:     bundledLibsDir,
 				Location: libraries.IDEBuiltIn,
 			})
 		}
 
 		// Add libraries directory from config file
-		lmb.AddLibrariesDir(&librariesmanager.LibrariesDir{
+		lmb.AddLibrariesDir(librariesmanager.LibrariesDir{
 			Path:     configuration.LibrariesDir(configuration.Settings),
 			Location: libraries.User,
 		})
@@ -383,23 +383,18 @@ func Init(req *rpc.InitRequest, responseCallback func(r *rpc.InitResponse)) erro
 				taskCallback(&rpc.TaskProgress{Completed: true})
 			}
 
-			lmb.AddLibrariesDir(&librariesmanager.LibrariesDir{
+			lmb.AddLibrariesDir(librariesmanager.LibrariesDir{
 				Path:     libRoot,
 				Location: libraries.User,
 			})
 		}
 	}
 
-	lm := lmb.Build()
+	lm, libsLoadingWarnings := lmb.Build()
 	_ = instances.SetLibraryManager(instance, lm) // should never fail
-
-	{
-		lmi, release := lm.NewInstaller()
-		for _, status := range lmi.RescanLibraries() {
-			logrus.WithError(status.Err()).Warnf("Error loading library")
-			// TODO: report as warning: responseError(err)
-		}
-		release()
+	for _, status := range libsLoadingWarnings {
+		logrus.WithError(status.Err()).Warnf("Error loading library")
+		// TODO: report as warning: responseError(err)
 	}
 
 	// Refreshes the locale used, this will change the

commands/internal/instances/instances.go

@@ -126,9 +126,12 @@ func Create(dataDir, packagesDir, downloadsDir *paths.Path, extraUserAgent ...st
 	}
 	tempDir := dataDir.Join("tmp")
 
+	pm := packagemanager.NewBuilder(dataDir, packagesDir, downloadsDir, tempDir, userAgent).Build()
+	lm, _ := librariesmanager.NewBuilder().Build()
+
 	instance := &coreInstance{
-		pm: packagemanager.NewBuilder(dataDir, packagesDir, downloadsDir, tempDir, userAgent).Build(),
-		lm: librariesmanager.NewBuilder().Build(),
+		pm: pm,
+		lm: lm,
 		li: librariesindex.EmptyIndex,
 	}
 

internal/arduino/builder/internal/detector/detector.go

@@ -598,7 +598,7 @@ func LibrariesLoader(
 	if useCachedLibrariesResolution {
 		// Since we are using the cached libraries resolution
 		// the library manager is not needed.
-		lm = librariesmanager.NewBuilder().Build()
+		lm, _ = librariesmanager.NewBuilder().Build()
 	}
 	if librariesManager == nil {
 		lmb := librariesmanager.NewBuilder()
@@ -608,20 +608,20 @@ func LibrariesLoader(
 			if err := builtInLibrariesFolders.ToAbs(); err != nil {
 				return nil, nil, nil, err
 			}
-			lmb.AddLibrariesDir(&librariesmanager.LibrariesDir{
+			lmb.AddLibrariesDir(librariesmanager.LibrariesDir{
 				Path:     builtInLibrariesFolders,
 				Location: libraries.IDEBuiltIn,
 			})
 		}
 
 		if actualPlatform != targetPlatform {
-			lmb.AddLibrariesDir(&librariesmanager.LibrariesDir{
+			lmb.AddLibrariesDir(librariesmanager.LibrariesDir{
 				PlatformRelease: actualPlatform,
 				Path:            actualPlatform.GetLibrariesDir(),
 				Location:        libraries.ReferencedPlatformBuiltIn,
 			})
 		}
-		lmb.AddLibrariesDir(&librariesmanager.LibrariesDir{
+		lmb.AddLibrariesDir(librariesmanager.LibrariesDir{
 			PlatformRelease: targetPlatform,
 			Path:            targetPlatform.GetLibrariesDir(),
 			Location:        libraries.PlatformBuiltIn,
@@ -632,35 +632,31 @@ func LibrariesLoader(
 			return nil, nil, nil, err
 		}
 		for _, folder := range librariesFolders {
-			lmb.AddLibrariesDir(&librariesmanager.LibrariesDir{
+			lmb.AddLibrariesDir(librariesmanager.LibrariesDir{
 				Path:     folder,
 				Location: libraries.User, // XXX: Should be libraries.Unmanaged?
 			})
 		}
 
 		for _, dir := range libraryDirs {
-			lmb.AddLibrariesDir(&librariesmanager.LibrariesDir{
+			lmb.AddLibrariesDir(librariesmanager.LibrariesDir{
 				Path:            dir,
 				Location:        libraries.Unmanaged,
 				IsSingleLibrary: true,
 			})
 		}
 
-		lm = lmb.Build()
-
-		{
-			lmi, release := lm.NewInstaller()
-			for _, status := range lmi.RescanLibraries() {
-				// With the refactoring of the initialization step of the CLI we changed how
-				// errors are returned when loading platforms and libraries, that meant returning a list of
-				// errors instead of a single one to enhance the experience for the user.
-				// I have no intention right now to start a refactoring of the legacy package too, so
-				// here's this shitty solution for now.
-				// When we're gonna refactor the legacy package this will be gone.
-				verboseOut.Write([]byte(status.Message()))
-			}
-			release()
+		newLm, libsLoadingWarnings := lmb.Build()
+		for _, status := range libsLoadingWarnings {
+			// With the refactoring of the initialization step of the CLI we changed how
+			// errors are returned when loading platforms and libraries, that meant returning a list of
+			// errors instead of a single one to enhance the experience for the user.
+			// I have no intention right now to start a refactoring of the legacy package too, so
+			// here's this shitty solution for now.
+			// When we're gonna refactor the legacy package this will be gone.
+			verboseOut.Write([]byte(status.Message()))
 		}
+		lm = newLm
 	}
 
 	allLibs := lm.FindAllInstalled()

internal/arduino/libraries/librariesmanager/librariesmanager.go

@@ -65,6 +65,7 @@ type LibrariesDir struct {
 	Location        libraries.LibraryLocation
 	PlatformRelease *cores.PlatformRelease
 	IsSingleLibrary bool // true if Path points directly to a library instad of a dir of libraries
+	scanned         bool
 }
 
 var tr = i18n.Tr
@@ -95,14 +96,19 @@ func NewBuilder() *Builder {
 	}
 }
 
-// NewBuilder creates a Builder with the same configuration of this
-// LibrariesManager. A "commit" function callback is returned: calling
-// this function will write the new configuration into this LibrariesManager.
-func (lm *LibrariesManager) NewBuilder() (*Builder, func()) {
+// Clone creates a Builder starting with a copy of the same configuration
+// of this LibrariesManager. At the moment of the Build() only the added
+// libraries directories will be scanned, keeping the existing directories
+// "cached" to optimize scan. If you need to do a full rescan you must use
+// the RescanLibraries method of the Installer.
+func (lm *LibrariesManager) Clone() *Builder {
 	lmb := NewBuilder()
-	return lmb, func() {
-		lmb.BuildIntoExistingLibrariesManager(lm)
+	lmb.librariesDir = append(lmb.librariesDir, lm.librariesDir...)
+	for libName, libAlternatives := range lm.libraries {
+		// TODO: Maybe we should deep clone libAlternatives...
+		lmb.libraries[libName] = append(lmb.libraries[libName], libAlternatives...)
 	}
+	return lmb
 }
 
 // NewExplorer returns a new Explorer. The returned function must be called
@@ -120,10 +126,18 @@ func (lm *LibrariesManager) NewInstaller() (*Installer, func()) {
 }
 
 // Build builds a new LibrariesManager.
-func (lmb *Builder) Build() *LibrariesManager {
+func (lmb *Builder) Build() (*LibrariesManager, []*status.Status) {
+	var statuses []*status.Status
 	res := &LibrariesManager{}
+	for _, dir := range lmb.librariesDir {
+		if !dir.scanned {
+			if errs := lmb.loadLibrariesFromDir(dir); len(errs) > 0 {
+				statuses = append(statuses, errs...)
+			}
+		}
+	}
 	lmb.BuildIntoExistingLibrariesManager(res)
-	return res
+	return res, statuses
 }
 
 // BuildIntoExistingLibrariesManager will overwrite the given LibrariesManager instead
@@ -138,7 +152,7 @@ func (lmb *Builder) BuildIntoExistingLibrariesManager(old *LibrariesManager) {
 // AddLibrariesDir adds path to the list of directories
 // to scan when searching for libraries. If a path is already
 // in the list it is ignored.
-func (lmb *Builder) AddLibrariesDir(libDir *LibrariesDir) {
+func (lmb *Builder) AddLibrariesDir(libDir LibrariesDir) {
 	if libDir.Path == nil {
 		return
 	}
@@ -151,7 +165,7 @@ func (lmb *Builder) AddLibrariesDir(libDir *LibrariesDir) {
 		WithField("location", libDir.Location.String()).
 		WithField("isSingleLibrary", libDir.IsSingleLibrary).
 		Info("Adding libraries dir")
-	lmb.librariesDir = append(lmb.librariesDir, libDir)
+	lmb.librariesDir = append(lmb.librariesDir, &libDir)
 }
 
 // RescanLibraries reload all installed libraries in the system.
@@ -184,9 +198,11 @@ func (lm *LibrariesManager) getLibrariesDir(installLocation libraries.LibraryLoc
 
 // loadLibrariesFromDir loads all libraries in the given directory. Returns
 // nil if the directory doesn't exists.
-func (lmi *Installer) loadLibrariesFromDir(librariesDir *LibrariesDir) []*status.Status {
+func (lm *LibrariesManager) loadLibrariesFromDir(librariesDir *LibrariesDir) []*status.Status {
 	statuses := []*status.Status{}
 
+	librariesDir.scanned = true
+
 	var libDirs paths.PathList
 	if librariesDir.IsSingleLibrary {
 		libDirs.Add(librariesDir.Path)
@@ -212,9 +228,9 @@ func (lmi *Installer) loadLibrariesFromDir(librariesDir *LibrariesDir) []*status
 			continue
 		}
 		library.ContainerPlatform = librariesDir.PlatformRelease
-		alternatives := lmi.libraries[library.Name]
+		alternatives := lm.libraries[library.Name]
 		alternatives.Add(library)
-		lmi.libraries[library.Name] = alternatives
+		lm.libraries[library.Name] = alternatives
 	}
 
 	return statuses

internal/arduino/libraries/librariesmanager/librariesmanager_test.go

@@ -21,17 +21,25 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-func Test_RescanLibrariesCallClear(t *testing.T) {
+func TestLibrariesBuilderScanCloneRescan(t *testing.T) {
 	lmb := NewBuilder()
 	lmb.libraries["testLibA"] = libraries.List{}
 	lmb.libraries["testLibB"] = libraries.List{}
-	lm := lmb.Build()
+	lm, warns := lmb.Build()
+	require.Empty(t, warns)
+	require.Len(t, lm.libraries, 2)
 
+	// Cloning should keep existing libraries
+	lm2, warns2 := lm.Clone().Build()
+	require.Empty(t, warns2)
+	require.Len(t, lm2.libraries, 2)
+
+	// Full rescan should update libs
 	{
-		lmi, release := lm.NewInstaller()
-		lmi.RescanLibraries()
+		lmi2, release := lm2.NewInstaller()
+		lmi2.RescanLibraries()
 		release()
 	}
-
-	require.Len(t, lm.libraries, 0)
+	require.Len(t, lm.libraries, 2) // Ensure deep-coping worked as expected...
+	require.Len(t, lm2.libraries, 0)
 }