Fix core upgrading not uninstalling unused tools (#1219)

Author: silvanocerza

commands/core/install.go

@@ -101,19 +101,34 @@ func installPlatform(pm *packagemanager.PackageManager,
 	for _, tool := range toolsToInstall {
 		err := commands.InstallToolRelease(pm, tool, taskCB)
 		if err != nil {
-			// TODO: handle error
+			return err
 		}
 	}
 
-	// Are we installing or upgrading?
-	platform := platformRelease.Platform
-	installed := pm.GetInstalledPlatformRelease(platform)
+	installed := pm.GetInstalledPlatformRelease(platformRelease.Platform)
+	installedTools := []*cores.ToolRelease{}
 	if installed == nil {
+		// No version of this platform is installed
 		log.Info("Installing platform")
 		taskCB(&rpc.TaskProgress{Name: "Installing " + platformRelease.String()})
 	} else {
-		log.Info("Updating platform " + installed.String())
-		taskCB(&rpc.TaskProgress{Name: "Updating " + installed.String() + " with " + platformRelease.String()})
+		// A platform with a different version is already installed
+		log.Info("Upgrading platform " + installed.String())
+		taskCB(&rpc.TaskProgress{Name: "Upgrading " + installed.String() + " with " + platformRelease.String()})
+		platformRef := &packagemanager.PlatformReference{
+			Package:              platformRelease.Platform.Package.Name,
+			PlatformArchitecture: platformRelease.Platform.Architecture,
+			PlatformVersion:      installed.Version,
+		}
+
+		// Get a list of tools used by the currently installed platform version.
+		// This must be done so tools used by the currently installed version are
+		// removed if not used also by the newly installed version.
+		var err error
+		_, installedTools, err = pm.FindPlatformReleaseDependencies(platformRef)
+		if err != nil {
+			return fmt.Errorf("can't find dependencies for platform %s: %w", platformRef, err)
+		}
 	}
 
 	// Install
@@ -129,17 +144,25 @@ func installPlatform(pm *packagemanager.PackageManager,
 
 		// In case of error try to rollback
 		if errUn != nil {
-			log.WithError(errUn).Error("Error updating platform.")
-			taskCB(&rpc.TaskProgress{Message: "Error updating platform: " + err.Error()})
+			log.WithError(errUn).Error("Error upgrading platform.")
+			taskCB(&rpc.TaskProgress{Message: "Error upgrading platform: " + err.Error()})
 
 			// Rollback
 			if err := pm.UninstallPlatform(platformRelease); err != nil {
 				log.WithError(err).Error("Error rolling-back changes.")
 				taskCB(&rpc.TaskProgress{Message: "Error rolling-back changes: " + err.Error()})
 			}
 
-			return fmt.Errorf("updating platform: %s", errUn)
+			return fmt.Errorf("upgrading platform: %s", errUn)
 		}
+
+		// Uninstall unused tools
+		for _, tool := range installedTools {
+			if !pm.IsToolRequired(tool) {
+				uninstallToolRelease(pm, tool, taskCB)
+			}
+		}
+
 	}
 
 	// Perform post install

commands/core/uninstall.go

@@ -63,7 +63,7 @@ func PlatformUninstall(ctx context.Context, req *rpc.PlatformUninstallReq, taskC
 
 	for _, tool := range tools {
 		if !pm.IsToolRequired(tool) {
-			uninstallToolReleasse(pm, tool, taskCB)
+			uninstallToolRelease(pm, tool, taskCB)
 		}
 	}
 
@@ -90,7 +90,7 @@ func uninstallPlatformRelease(pm *packagemanager.PackageManager, platformRelease
 	return nil
 }
 
-func uninstallToolReleasse(pm *packagemanager.PackageManager, toolRelease *cores.ToolRelease, taskCB commands.TaskProgressCB) error {
+func uninstallToolRelease(pm *packagemanager.PackageManager, toolRelease *cores.ToolRelease, taskCB commands.TaskProgressCB) error {
 	log := pm.Log.WithField("Tool", toolRelease)
 
 	log.Info("Uninstalling tool")

commands/core/upgrade.go

@@ -64,7 +64,6 @@ func upgradePlatform(pm *packagemanager.PackageManager, platformRef *packagemana
 	}
 
 	// Search the latest version for all specified platforms
-	toInstallRefs := []*packagemanager.PlatformReference{}
 	platform := pm.FindPlatform(platformRef)
 	if platform == nil {
 		return fmt.Errorf("platform %s not found", platformRef)
@@ -78,17 +77,15 @@ func upgradePlatform(pm *packagemanager.PackageManager, platformRef *packagemana
 		return ErrAlreadyLatest
 	}
 	platformRef.PlatformVersion = latest.Version
-	toInstallRefs = append(toInstallRefs, platformRef)
 
-	for _, platformRef := range toInstallRefs {
-		platform, tools, err := pm.FindPlatformReleaseDependencies(platformRef)
-		if err != nil {
-			return fmt.Errorf("platform %s is not installed", platformRef)
-		}
-		err = installPlatform(pm, platform, tools, downloadCB, taskCB, skipPostInstall)
-		if err != nil {
-			return err
-		}
+	platformRelease, tools, err := pm.FindPlatformReleaseDependencies(platformRef)
+	if err != nil {
+		return fmt.Errorf("platform %s is not installed", platformRef)
+	}
+	err = installPlatform(pm, platformRelease, tools, downloadCB, taskCB, skipPostInstall)
+	if err != nil {
+		return err
 	}
+
 	return nil
 }

commands/instances.go

@@ -519,6 +519,17 @@ func Upgrade(ctx context.Context, req *rpc.UpgradeReq, downloadCB DownloadProgre
 				}
 
 				ref := &packagemanager.PlatformReference{
+					Package:              installedRelease.Platform.Package.Name,
+					PlatformArchitecture: installedRelease.Platform.Architecture,
+					PlatformVersion:      installedRelease.Version,
+				}
+				// Get list of installed tools needed by the currently installed version
+				_, installedTools, err := pm.FindPlatformReleaseDependencies(ref)
+				if err != nil {
+					return err
+				}
+
+				ref = &packagemanager.PlatformReference{
 					Package:              latest.Platform.Package.Name,
 					PlatformArchitecture: latest.Platform.Architecture,
 					PlatformVersion:      latest.Version,
@@ -590,6 +601,24 @@ func Upgrade(ctx context.Context, req *rpc.UpgradeReq, downloadCB DownloadProgre
 					}
 				}
 
+				// Uninstall unused tools
+				for _, toolRelease := range installedTools {
+					if !pm.IsToolRequired(toolRelease) {
+						log := pm.Log.WithField("Tool", toolRelease)
+
+						log.Info("Uninstalling tool")
+						taskCB(&rpc.TaskProgress{Name: "Uninstalling " + toolRelease.String() + ", tool is no more required"})
+
+						if err := pm.UninstallTool(toolRelease); err != nil {
+							log.WithError(err).Error("Error uninstalling")
+							return err
+						}
+
+						log.Info("Tool uninstalled")
+						taskCB(&rpc.TaskProgress{Message: toolRelease.String() + " uninstalled", Completed: true})
+					}
+				}
+
 				// Perform post install
 				if !req.SkipPostInstall {
 					logrus.Info("Running post_install script")

test/test_core.py

@@ -435,3 +435,37 @@ def test_core_list_updatable_all_flags(run_command, data_dir):
     assert expected_core_id in mapped
     assert "Arduino AVR Boards" == mapped[expected_core_id]["Name"]
     assert "1.8.3" == mapped[expected_core_id]["Latest"]
+
+
+def test_core_upgrade_removes_unused_tools(run_command, data_dir):
+    assert run_command("update")
+
+    # Installs a core
+    assert run_command("core install arduino:[email protected]")
+
+    # Verifies expected tool is installed
+    tool_path = Path(data_dir, "packages", "arduino", "tools", "avr-gcc", "7.3.0-atmel3.6.1-arduino5")
+    assert tool_path.exists()
+
+    # Upgrades core
+    assert run_command("core upgrade arduino:avr")
+
+    # Verifies tool is uninstalled since it's not used by newer core version
+    assert not tool_path.exists()
+
+
+def test_core_install_removes_unused_tools(run_command, data_dir):
+    assert run_command("update")
+
+    # Installs a core
+    assert run_command("core install arduino:[email protected]")
+
+    # Verifies expected tool is installed
+    tool_path = Path(data_dir, "packages", "arduino", "tools", "avr-gcc", "7.3.0-atmel3.6.1-arduino5")
+    assert tool_path.exists()
+
+    # Installs newer version of already installed core
+    assert run_command("core install arduino:[email protected]")
+
+    # Verifies tool is uninstalled since it's not used by newer core version
+    assert not tool_path.exists()

test/test_upgrade.py

@@ -65,3 +65,20 @@ def test_upgrade_using_library_with_invalid_version(run_command, data_dir):
     res = run_command("upgrade")
     assert res.ok
     assert "WiFi101" in res.stdout
+
+
+def test_upgrade_unused_core_tools_are_removed(run_command, data_dir):
+    assert run_command("update")
+
+    # Installs a core
+    assert run_command("core install arduino:[email protected]")
+
+    # Verifies expected tool is installed
+    tool_path = Path(data_dir, "packages", "arduino", "tools", "avr-gcc", "7.3.0-atmel3.6.1-arduino5")
+    assert tool_path.exists()
+
+    # Upgrades everything
+    assert run_command("upgrade")
+
+    # Verifies tool is uninstalled since it's not used by newer core version
+    assert not tool_path.exists()