Skip to content

Commit df03ad7

Browse files
silvanocerzasilvanocerza
authored
Fix core upgrading not uninstalling unused tools (#1219)
1 parent 414ab00 commit df03ad7

File tree

6 files changed

+122
-22
lines changed

6 files changed

+122
-22
lines changed

‎commands/core/install.go

+32-9
Original file line numberDiff line numberDiff line change
@@ -101,19 +101,34 @@ func installPlatform(pm *packagemanager.PackageManager,
101101
for _, tool := range toolsToInstall {
102102
err := commands.InstallToolRelease(pm, tool, taskCB)
103103
if err != nil {
104-
// TODO: handle error
104+
return err
105105
}
106106
}
107107

108-
// Are we installing or upgrading?
109-
platform := platformRelease.Platform
110-
installed := pm.GetInstalledPlatformRelease(platform)
108+
installed := pm.GetInstalledPlatformRelease(platformRelease.Platform)
109+
installedTools := []*cores.ToolRelease{}
111110
if installed == nil {
111+
// No version of this platform is installed
112112
log.Info("Installing platform")
113113
taskCB(&rpc.TaskProgress{Name: "Installing " + platformRelease.String()})
114114
} else {
115-
log.Info("Updating platform " + installed.String())
116-
taskCB(&rpc.TaskProgress{Name: "Updating " + installed.String() + " with " + platformRelease.String()})
115+
// A platform with a different version is already installed
116+
log.Info("Upgrading platform " + installed.String())
117+
taskCB(&rpc.TaskProgress{Name: "Upgrading " + installed.String() + " with " + platformRelease.String()})
118+
platformRef := &packagemanager.PlatformReference{
119+
Package: platformRelease.Platform.Package.Name,
120+
PlatformArchitecture: platformRelease.Platform.Architecture,
121+
PlatformVersion: installed.Version,
122+
}
123+
124+
// Get a list of tools used by the currently installed platform version.
125+
// This must be done so tools used by the currently installed version are
126+
// removed if not used also by the newly installed version.
127+
var err error
128+
_, installedTools, err = pm.FindPlatformReleaseDependencies(platformRef)
129+
if err != nil {
130+
return fmt.Errorf("can't find dependencies for platform %s: %w", platformRef, err)
131+
}
117132
}
118133

119134
// Install
@@ -129,17 +144,25 @@ func installPlatform(pm *packagemanager.PackageManager,
129144

130145
// In case of error try to rollback
131146
if errUn != nil {
132-
log.WithError(errUn).Error("Error updating platform.")
133-
taskCB(&rpc.TaskProgress{Message: "Error updating platform: " + err.Error()})
147+
log.WithError(errUn).Error("Error upgrading platform.")
148+
taskCB(&rpc.TaskProgress{Message: "Error upgrading platform: " + err.Error()})
134149

135150
// Rollback
136151
if err := pm.UninstallPlatform(platformRelease); err != nil {
137152
log.WithError(err).Error("Error rolling-back changes.")
138153
taskCB(&rpc.TaskProgress{Message: "Error rolling-back changes: " + err.Error()})
139154
}
140155

141-
return fmt.Errorf("updating platform: %s", errUn)
156+
return fmt.Errorf("upgrading platform: %s", errUn)
142157
}
158+
159+
// Uninstall unused tools
160+
for _, tool := range installedTools {
161+
if !pm.IsToolRequired(tool) {
162+
uninstallToolRelease(pm, tool, taskCB)
163+
}
164+
}
165+
143166
}
144167

145168
// Perform post install

‎commands/core/uninstall.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ func PlatformUninstall(ctx context.Context, req *rpc.PlatformUninstallReq, taskC
6363

6464
for _, tool := range tools {
6565
if !pm.IsToolRequired(tool) {
66-
uninstallToolReleasse(pm, tool, taskCB)
66+
uninstallToolRelease(pm, tool, taskCB)
6767
}
6868
}
6969

@@ -90,7 +90,7 @@ func uninstallPlatformRelease(pm *packagemanager.PackageManager, platformRelease
9090
return nil
9191
}
9292

93-
func uninstallToolReleasse(pm *packagemanager.PackageManager, toolRelease *cores.ToolRelease, taskCB commands.TaskProgressCB) error {
93+
func uninstallToolRelease(pm *packagemanager.PackageManager, toolRelease *cores.ToolRelease, taskCB commands.TaskProgressCB) error {
9494
log := pm.Log.WithField("Tool", toolRelease)
9595

9696
log.Info("Uninstalling tool")

‎commands/core/upgrade.go

+8-11
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,6 @@ func upgradePlatform(pm *packagemanager.PackageManager, platformRef *packagemana
6464
}
6565

6666
// Search the latest version for all specified platforms
67-
toInstallRefs := []*packagemanager.PlatformReference{}
6867
platform := pm.FindPlatform(platformRef)
6968
if platform == nil {
7069
return fmt.Errorf("platform %s not found", platformRef)
@@ -78,17 +77,15 @@ func upgradePlatform(pm *packagemanager.PackageManager, platformRef *packagemana
7877
return ErrAlreadyLatest
7978
}
8079
platformRef.PlatformVersion = latest.Version
81-
toInstallRefs = append(toInstallRefs, platformRef)
8280

83-
for _, platformRef := range toInstallRefs {
84-
platform, tools, err := pm.FindPlatformReleaseDependencies(platformRef)
85-
if err != nil {
86-
return fmt.Errorf("platform %s is not installed", platformRef)
87-
}
88-
err = installPlatform(pm, platform, tools, downloadCB, taskCB, skipPostInstall)
89-
if err != nil {
90-
return err
91-
}
81+
platformRelease, tools, err := pm.FindPlatformReleaseDependencies(platformRef)
82+
if err != nil {
83+
return fmt.Errorf("platform %s is not installed", platformRef)
84+
}
85+
err = installPlatform(pm, platformRelease, tools, downloadCB, taskCB, skipPostInstall)
86+
if err != nil {
87+
return err
9288
}
89+
9390
return nil
9491
}

‎commands/instances.go

+29
Original file line numberDiff line numberDiff line change
@@ -519,6 +519,17 @@ func Upgrade(ctx context.Context, req *rpc.UpgradeReq, downloadCB DownloadProgre
519519
}
520520

521521
ref := &packagemanager.PlatformReference{
522+
Package: installedRelease.Platform.Package.Name,
523+
PlatformArchitecture: installedRelease.Platform.Architecture,
524+
PlatformVersion: installedRelease.Version,
525+
}
526+
// Get list of installed tools needed by the currently installed version
527+
_, installedTools, err := pm.FindPlatformReleaseDependencies(ref)
528+
if err != nil {
529+
return err
530+
}
531+
532+
ref = &packagemanager.PlatformReference{
522533
Package: latest.Platform.Package.Name,
523534
PlatformArchitecture: latest.Platform.Architecture,
524535
PlatformVersion: latest.Version,
@@ -590,6 +601,24 @@ func Upgrade(ctx context.Context, req *rpc.UpgradeReq, downloadCB DownloadProgre
590601
}
591602
}
592603

604+
// Uninstall unused tools
605+
for _, toolRelease := range installedTools {
606+
if !pm.IsToolRequired(toolRelease) {
607+
log := pm.Log.WithField("Tool", toolRelease)
608+
609+
log.Info("Uninstalling tool")
610+
taskCB(&rpc.TaskProgress{Name: "Uninstalling " + toolRelease.String() + ", tool is no more required"})
611+
612+
if err := pm.UninstallTool(toolRelease); err != nil {
613+
log.WithError(err).Error("Error uninstalling")
614+
return err
615+
}
616+
617+
log.Info("Tool uninstalled")
618+
taskCB(&rpc.TaskProgress{Message: toolRelease.String() + " uninstalled", Completed: true})
619+
}
620+
}
621+
593622
// Perform post install
594623
if !req.SkipPostInstall {
595624
logrus.Info("Running post_install script")

‎test/test_core.py

+34
Original file line numberDiff line numberDiff line change
@@ -435,3 +435,37 @@ def test_core_list_updatable_all_flags(run_command, data_dir):
435435
assert expected_core_id in mapped
436436
assert "Arduino AVR Boards" == mapped[expected_core_id]["Name"]
437437
assert "1.8.3" == mapped[expected_core_id]["Latest"]
438+
439+
440+
def test_core_upgrade_removes_unused_tools(run_command, data_dir):
441+
assert run_command("update")
442+
443+
# Installs a core
444+
assert run_command("core install arduino:avr@1.8.2")
445+
446+
# Verifies expected tool is installed
447+
tool_path = Path(data_dir, "packages", "arduino", "tools", "avr-gcc", "7.3.0-atmel3.6.1-arduino5")
448+
assert tool_path.exists()
449+
450+
# Upgrades core
451+
assert run_command("core upgrade arduino:avr")
452+
453+
# Verifies tool is uninstalled since it's not used by newer core version
454+
assert not tool_path.exists()
455+
456+
457+
def test_core_install_removes_unused_tools(run_command, data_dir):
458+
assert run_command("update")
459+
460+
# Installs a core
461+
assert run_command("core install arduino:avr@1.8.2")
462+
463+
# Verifies expected tool is installed
464+
tool_path = Path(data_dir, "packages", "arduino", "tools", "avr-gcc", "7.3.0-atmel3.6.1-arduino5")
465+
assert tool_path.exists()
466+
467+
# Installs newer version of already installed core
468+
assert run_command("core install arduino:avr@1.8.3")
469+
470+
# Verifies tool is uninstalled since it's not used by newer core version
471+
assert not tool_path.exists()

‎test/test_upgrade.py

+17
Original file line numberDiff line numberDiff line change
@@ -65,3 +65,20 @@ def test_upgrade_using_library_with_invalid_version(run_command, data_dir):
6565
res = run_command("upgrade")
6666
assert res.ok
6767
assert "WiFi101" in res.stdout
68+
69+
70+
def test_upgrade_unused_core_tools_are_removed(run_command, data_dir):
71+
assert run_command("update")
72+
73+
# Installs a core
74+
assert run_command("core install arduino:avr@1.8.2")
75+
76+
# Verifies expected tool is installed
77+
tool_path = Path(data_dir, "packages", "arduino", "tools", "avr-gcc", "7.3.0-atmel3.6.1-arduino5")
78+
assert tool_path.exists()
79+
80+
# Upgrades everything
81+
assert run_command("upgrade")
82+
83+
# Verifies tool is uninstalled since it's not used by newer core version
84+
assert not tool_path.exists()

0 commit comments

Comments
 (0)