Add prioritization to tool selection in absence of explicit dependencies (#1887)

* Add prioritization to tool selection in absence of explicit dependencies

* Made tests working on windows too

* Updated docs

* Added tool selection from referenced platform

* Added some explanatory comments

* Moved integration tests into 'compile_3' group

arduino/cores/packagemanager/package_manager.go

@@ -614,29 +614,58 @@ func (pme *Explorer) GetTool(toolID string) *cores.Tool {
 	}
 }
 
-// FindToolsRequiredForBoard FIXMEDOC
-func (pme *Explorer) FindToolsRequiredForBoard(board *cores.Board) ([]*cores.ToolRelease, error) {
-	pme.log.Infof("Searching tools required for board %s", board)
-
-	// core := board.Properties["build.core"]
-	platform := board.PlatformRelease
-
-	// maps "PACKAGER:TOOL" => ToolRelease
-	foundTools := map[string]*cores.ToolRelease{}
-
-	// a Platform may not specify required tools (because it's a platform that comes from a
-	// user/hardware dir without a package_index.json) then add all available tools
-	for _, targetPackage := range pme.packages {
-		for _, tool := range targetPackage.Tools {
-			rel := tool.GetLatestInstalled()
-			if rel != nil {
-				foundTools[rel.Tool.Name] = rel
+// FindToolsRequiredForBuild returns the list of ToolReleases needed to build for the specified
+// plaftorm. The buildPlatform may be different depending on the selected board.
+func (pme *Explorer) FindToolsRequiredForBuild(platform, buildPlatform *cores.PlatformRelease) ([]*cores.ToolRelease, error) {
+
+	// maps tool name => all available ToolRelease
+	allToolsAlternatives := map[string][]*cores.ToolRelease{}
+	for _, tool := range pme.GetAllInstalledToolsReleases() {
+		alternatives := allToolsAlternatives[tool.Tool.Name]
+		alternatives = append(alternatives, tool)
+		allToolsAlternatives[tool.Tool.Name] = alternatives
+	}
+
+	// selectBest select the tool with best matching, applying the following rules
+	// in order of priority:
+	// - the tool comes from the requested packager
+	// - the tool comes from the build platform packager
+	// - the tool has the greatest version
+	// - the tool packager comes first in alphabetic order
+	packagerPriority := map[string]int{}
+	packagerPriority[platform.Platform.Package.Name] = 2
+	if buildPlatform != nil {
+		packagerPriority[buildPlatform.Platform.Package.Name] = 1
+	}
+	selectBest := func(tools []*cores.ToolRelease) *cores.ToolRelease {
+		selected := tools[0]
+		for _, tool := range tools[1:] {
+			if packagerPriority[tool.Tool.Package.Name] != packagerPriority[selected.Tool.Package.Name] {
+				if packagerPriority[tool.Tool.Package.Name] > packagerPriority[selected.Tool.Package.Name] {
+					selected = tool
+				}
+				continue
+			}
+			if !tool.Version.Equal(selected.Version) {
+				if tool.Version.GreaterThan(selected.Version) {
+					selected = tool
+				}
+				continue
+			}
+			if tool.Tool.Package.Name < selected.Tool.Package.Name {
+				selected = tool
 			}
 		}
+		return selected
 	}
 
-	// replace the default tools above with the specific required by the current platform
+	// First select the specific tools required by the current platform
 	requiredTools := []*cores.ToolRelease{}
+	// The Sorting of the tool dependencies is required because some platforms may depends
+	// on more than one version of the same tool. For example adafruit:samd has both
+	// bossac@1.8.0-48-gb176eee and bossac@1.7.0. To allow the runtime property
+	// {runtime.tools.bossac.path} to be correctly set to the 1.8.0 version we must ensure
+	// that the returned array is sorted by version.
 	platform.ToolDependencies.Sort()
 	for _, toolDep := range platform.ToolDependencies {
 		pme.log.WithField("tool", toolDep).Infof("Required tool")
@@ -645,11 +674,15 @@ func (pme *Explorer) FindToolsRequiredForBoard(board *cores.Board) ([]*cores.Too
 			return nil, fmt.Errorf(tr("tool release not found: %s"), toolDep)
 		}
 		requiredTools = append(requiredTools, tool)
-		delete(foundTools, tool.Tool.Name)
+		delete(allToolsAlternatives, tool.Tool.Name)
 	}
 
-	for _, toolRel := range foundTools {
-		requiredTools = append(requiredTools, toolRel)
+	// Since a Platform may not specify the required tools (because it's a platform that comes
+	// from a user/hardware dir without a package_index.json) then add all available tools giving
+	// priority to tools coming from the same packager or referenced packager
+	for _, tools := range allToolsAlternatives {
+		tool := selectBest(tools)
+		requiredTools = append(requiredTools, tool)
 	}
 	return requiredTools, nil
 }

arduino/cores/packagemanager/package_manager_test.go

@@ -287,6 +287,8 @@ func TestFindToolsRequiredForBoard(t *testing.T) {
 	loadIndex("https://dl.espressif.com/dl/package_esp32_index.json")
 	loadIndex("http://arduino.esp8266.com/stable/package_esp8266com_index.json")
 	loadIndex("https://adafruit.github.io/arduino-board-index/package_adafruit_index.json")
+	loadIndex("https://test.com/package_test_index.json") // this is not downloaded, it just picks the "local cached" file package_test_index.json
+
 	// We ignore the errors returned since they might not be necessarily blocking
 	// but just warnings for the user, like in the case a board is not loaded
 	// because of malformed menus
@@ -310,8 +312,13 @@ func TestFindToolsRequiredForBoard(t *testing.T) {
 	})
 	require.NotNil(t, esptool0413)
 
+	testPlatform := pme.FindPlatformRelease(&packagemanager.PlatformReference{
+		Package:              "test",
+		PlatformArchitecture: "avr",
+		PlatformVersion:      semver.MustParse("1.1.0")})
+
 	testConflictingToolsInDifferentPackages := func() {
-		tools, err := pme.FindToolsRequiredForBoard(esp32)
+		tools, err := pme.FindToolsRequiredForBuild(esp32.PlatformRelease, nil)
 		require.NoError(t, err)
 		require.Contains(t, tools, esptool231)
 		require.NotContains(t, tools, esptool0413)
@@ -331,10 +338,44 @@ func TestFindToolsRequiredForBoard(t *testing.T) {
 	testConflictingToolsInDifferentPackages()
 	testConflictingToolsInDifferentPackages()
 
+	{
+		// Test buildPlatform dependencies
+		arduinoBossac180 := pme.FindToolDependency(&cores.ToolDependency{
+			ToolPackager: "arduino",
+			ToolName:     "bossac",
+			ToolVersion:  semver.ParseRelaxed("1.8.0-48-gb176eee"),
+		})
+		require.NotNil(t, arduinoBossac180)
+		testBossac175 := pme.FindToolDependency(&cores.ToolDependency{
+			ToolPackager: "test",
+			ToolName:     "bossac",
+			ToolVersion:  semver.ParseRelaxed("1.7.5"),
+		})
+		require.NotNil(t, testBossac175)
+
+		tools, err := pme.FindToolsRequiredForBuild(esp32.PlatformRelease, nil)
+		require.NoError(t, err)
+		require.Contains(t, tools, esptool231)
+		require.NotContains(t, tools, esptool0413)
+		// When building without testPlatform dependency, arduino:bossac should be selected
+		// since it has the higher version
+		require.NotContains(t, tools, testBossac175)
+		require.Contains(t, tools, arduinoBossac180)
+
+		tools, err = pme.FindToolsRequiredForBuild(esp32.PlatformRelease, testPlatform)
+		require.NoError(t, err)
+		require.Contains(t, tools, esptool231)
+		require.NotContains(t, tools, esptool0413)
+		// When building with testPlatform dependency, test:bossac should be selected
+		// because it has dependency priority
+		require.Contains(t, tools, testBossac175)
+		require.NotContains(t, tools, arduinoBossac180)
+	}
+
 	feather, err := pme.FindBoardWithFQBN("adafruit:samd:adafruit_feather_m0_express")
 	require.NoError(t, err)
 	require.NotNil(t, feather)
-	featherTools, err := pme.FindToolsRequiredForBoard(feather)
+	featherTools, err := pme.FindToolsRequiredForBuild(feather.PlatformRelease, nil)
 	require.NoError(t, err)
 	require.NotNil(t, featherTools)
 

arduino/cores/packagemanager/testdata/data_dir_1/package_test_index.json

+{
+  "packages": [
+    {
+      "name": "test",
+      "websiteURL": "https://test.com",
+      "email": "test@test.com",
+      "help": {
+        "online": "https://test.com"
+      },
+      "maintainer": "Test",
+      "platforms": [
+        {
+          "architecture": "avr",
+          "boards": [],
+          "name": "Test AVR Boards",
+          "category": "Test",
+          "version": "1.1.0",
+          "archiveFileName": "test-1.1.0.tar.bz2",
+          "checksum": "SHA-256:4e72d4267d9a8d86874edcd021dc661854a5136c0eed947a6fe10366bc51e373",
+          "help": {
+            "online": "https://test.com"
+          },
+          "url": "https://test.com/test-1.1.0.tar.bz2",
+          "toolsDependencies": [],
+          "size": "12345"
+        }
+      ],
+      "tools": [
+        {
+          "name": "bossac",
+          "version": "1.7.5",
+          "systems": [
+            {
+              "host": "i386-apple-darwin11",
+              "checksum": "MD5:603bcce8e59683ac27054b3197a53254",
+              "size": "96372129",
+              "archiveFileName": "bossac.tar.bz2",
+              "url": "https://test.com/bossac.tar.bz2"
+            }
+          ]
+        }
+      ]
+    }
+  ]
+}

commands/debug/debug_info.go

@@ -66,7 +66,7 @@ func getDebugProperties(req *debug.DebugConfigRequest, pme *packagemanager.Explo
 	}
 
 	// Find target board and board properties
-	_, platformRelease, board, boardProperties, referencedPlatformRelease, err := pme.ResolveFQBN(fqbn)
+	_, platformRelease, _, boardProperties, referencedPlatformRelease, err := pme.ResolveFQBN(fqbn)
 	if err != nil {
 		return nil, &arduino.UnknownFQBNError{Cause: err}
 	}
@@ -98,7 +98,7 @@ func getDebugProperties(req *debug.DebugConfigRequest, pme *packagemanager.Explo
 	for _, tool := range pme.GetAllInstalledToolsReleases() {
 		toolProperties.Merge(tool.RuntimeProperties())
 	}
-	if requiredTools, err := pme.FindToolsRequiredForBoard(board); err == nil {
+	if requiredTools, err := pme.FindToolsRequiredForBuild(platformRelease, referencedPlatformRelease); err == nil {
 		for _, requiredTool := range requiredTools {
 			logrus.WithField("tool", requiredTool).Info("Tool required for debug")
 			toolProperties.Merge(requiredTool.RuntimeProperties())

commands/upload/upload.go

@@ -295,7 +295,7 @@ func runProgramAction(pme *packagemanager.Explorer,
 	for _, tool := range pme.GetAllInstalledToolsReleases() {
 		uploadProperties.Merge(tool.RuntimeProperties())
 	}
-	if requiredTools, err := pme.FindToolsRequiredForBoard(board); err == nil {
+	if requiredTools, err := pme.FindToolsRequiredForBuild(boardPlatform, buildPlatform); err == nil {
 		for _, requiredTool := range requiredTools {
 			logrus.WithField("tool", requiredTool).Info("Tool required for upload")
 			if requiredTool.IsInstalled() {

docs/UPGRADING.md

@@ -87,6 +87,23 @@ plus `Name`, `Version`, and an `UpToDate` boolean flag.
 
 `InstallZipLib` method `archivePath` is now a `paths.Path` instead of a `string`.
 
+### golang API change in `github.com/arduino/arduino-cli/rduino/cores/packagemanager.Explorer`
+
+The `packagemanager.Explorer` method `FindToolsRequiredForBoard`:
+
+```go
+func (pme *Explorer) FindToolsRequiredForBoard(board *cores.Board) ([]*cores.ToolRelease, error) { ... }
+```
+
+has been renamed to `FindToolsRequiredForBuild:
+
+```go
+func (pme *Explorer) FindToolsRequiredForBuild(platform, buildPlatform *cores.PlatformRelease) ([]*cores.ToolRelease, error) { ... }
+```
+
+moreover it now requires the `platform` and the `buildPlatform` (a.k.a. the referenced platform core used for the
+compile) instead of the `board`. Usually these two value are obtained from the `Explorer.ResolveFQBN(...)` method.
+
 ## 0.29.0
 
 ### Removed gRPC API: `cc.arduino.cli.commands.v1.UpdateCoreLibrariesIndex`, `Outdated`, and `Upgrade`

docs/package_index_json-specification.md

@@ -176,17 +176,6 @@ The other fields are:
   macOS you can use the command `shasum -a 256 filename` to generate SHA-256 checksums. There are free options for
   Windows, including md5deep. There are also online utilities for generating checksums.
 
-##### How a tool's path is determined in platform.txt
-
-When the IDE needs a tool, it downloads the corresponding archive file and unpacks the content into a private folder
-that can be referenced from `platform.txt` using one of the following properties:
-
-- `{runtime.tools.TOOLNAME-VERSION.path}`
-- `{runtime.tools.TOOLNAME.path}`
-
-For example, to obtain the avr-gcc 4.8.1 folder we can use `{runtime.tools.avr-gcc-4.8.1-arduino5.path}` or
-`{runtime.tools.avr-gcc.path}`.
-
 ### Platforms definitions
 
 Finally, let's see how `PLATFORMS` are made.
@@ -270,6 +259,38 @@ rules Arduino IDE follows for parsing versions
 Note: if you miss a bracket in the JSON index, then add the URL to your Preferences, and open Boards Manager it can
 cause the Arduino IDE to no longer load until you have deleted the file from your arduino15 folder.
 
+#### How a tool's path is determined in platform.txt
+
+When the IDE needs a tool, it downloads the corresponding archive file and unpacks the content into a private folder
+that can be referenced from `platform.txt` using one of the following properties:
+
+- `{runtime.tools.TOOLNAME-VERSION.path}`
+- `{runtime.tools.TOOLNAME.path}`
+
+For example, to obtain the avr-gcc 4.8.1 folder we can use `{runtime.tools.avr-gcc-4.8.1.path}` or
+`{runtime.tools.avr-gcc.path}`.
+
+In general the same tool may be provided by different packagers (for example the Arduino packager may provide an
+`arduino:avr-gcc` and another 3rd party packager may provide their own `3rdparty:avr-gcc`). The rules to disambiguate
+are as follows:
+
+- The property `{runtime.tools.TOOLNAME.path}` points, in order of priority, to:
+
+  1. the tool, version and packager specified via `toolsDependencies` in the `package_index.json`
+  1. the highest version of the tool provided by the packager of the current platform
+  1. the highest version of the tool provided by the packager of the referenced platform used for compile (see
+     ["Referencing another core, variant or tool"](platform-specification.md#referencing-another-core-variant-or-tool)
+     for more info)
+  1. the highest version of the tool provided by any other packager (in case of tie, the first packager in alphabetical
+     order wins)
+
+- The property `{runtime.tools.TOOLNAME-VERSION.path}` points, in order of priority, to:
+  1. the tool and version provided by the packager of the current platform
+  1. the tool and version provided by the packager of the referenced platform used for compile (see
+     ["Referencing another core, variant or tool"](platform-specification.md#referencing-another-core-variant-or-tool)
+     for more info)
+  1. the tool and version provided by any other packager (in case of tie, the first packager in alphabetical order wins)
+
 ### Example JSON index file
 
 ```json

docs/platform-specification.md

@@ -766,14 +766,18 @@ tools.avrdude.config.path={path}/etc/avrdude.conf
 tools.avrdude.upload.pattern="{cmd.path}" "-C{config.path}" -p{build.mcu} -c{upload.port.protocol} -P{upload.port.address} -b{upload.speed} -D "-Uflash:w:{build.path}/{build.project_name}.hex:i"
 ```
 
-A **{runtime.tools.TOOL_NAME.path}** and **{runtime.tools.TOOL_NAME-TOOL_VERSION.path}** property is generated for the
-tools of Arduino AVR Boards and any other platform installed via Boards Manager. **{runtime.tools.TOOL_NAME.path}**
-points to the latest version of the tool available.
-
 The tool configuration properties are available globally without the prefix. For example, the **tools.avrdude.cmd.path**
 property can be used as **{cmd.path}** inside the recipe, and the same happens for all the other avrdude configuration
 variables.
 
+### How to retrieve tools path via `{runtime.tools.*}` properties
+
+A **{runtime.tools.TOOLNAME.path}** and **{runtime.tools.TOOLNAME-TOOLVERSION.path}** property is generated for the
+tools provided by the current platform and for any other platform installed via Boards Manager.
+
+See [`{runtime.tools.*.path}` rules](package_index_json-specification.md#how-a-tools-path-is-determined-in-platformtxt)
+for details on how the runtime properties are determined.
+
 ### Environment variables
 
 All the tools launched to compile or upload a sketch will have the following environment variable set:

internal/integrationtest/compile_3/compile_test.go

+// This file is part of arduino-cli.
+//
+// Copyright 2022 ARDUINO SA (http://www.arduino.cc/)
+//
+// This software is released under the GNU General Public License version 3,
+// which covers the main part of arduino-cli.
+// The terms of this license can be found at:
+// https://www.gnu.org/licenses/gpl-3.0.en.html
+//
+// You can be released from the requirements of the above licenses by purchasing
+// a commercial license. Buying such a license is mandatory if you want to
+// modify or otherwise use the software for commercial activities involving the
+// Arduino software without disclosing the source code of your own applications.
+// To purchase a commercial license, send an email to license@arduino.cc.
+
+package compile_test
+
+import (
+	"testing"
+
+	"github.com/arduino/arduino-cli/internal/integrationtest"
+	"github.com/arduino/go-paths-helper"
+	"github.com/arduino/go-properties-orderedmap"
+	"github.com/stretchr/testify/require"
+)
+
+func TestRuntimeToolPropertiesGeneration(t *testing.T) {
+	env, cli := integrationtest.CreateArduinoCLIWithEnvironment(t)
+	defer env.CleanUp()
+
+	// Run update-index with our test index
+	_, _, err := cli.Run("core", "install", "arduino:avr@1.8.5")
+	require.NoError(t, err)
+
+	// Install test data into datadir
+	testdata := paths.New("testdata", "platforms_with_conflicting_tools")
+	hardwareDir := cli.DataDir().Join("packages")
+	err = testdata.Join("alice").CopyDirTo(hardwareDir.Join("alice"))
+	require.NoError(t, err)
+	err = testdata.Join("bob").CopyDirTo(hardwareDir.Join("bob"))
+	require.NoError(t, err)
+
+	sketch, err := paths.New("testdata", "bare_minimum").Abs()
+	require.NoError(t, err)
+
+	// As seen in https://github.com/arduino/arduino-cli/issues/73 the map randomess
+	// may make the function fail half of the times. Repeating the test 3 times
+	// greatly increases the chances to trigger the bad case.
+	for i := 0; i < 3; i++ {
+		stdout, _, err := cli.Run("compile", "-b", "alice:avr:alice", "--show-properties", sketch.String())
+		require.NoError(t, err)
+		res, err := properties.LoadFromBytes(stdout)
+		require.NoError(t, err)
+		// the tools coming from the same packager are selected
+		require.True(t, res.GetPath("runtime.tools.avr-gcc.path").EquivalentTo(hardwareDir.Join("alice", "tools", "avr-gcc", "50.0.0")))
+		require.True(t, res.GetPath("runtime.tools.avrdude.path").EquivalentTo(hardwareDir.Join("alice", "tools", "avrdude", "1.0.0")))
+
+		stdout, _, err = cli.Run("compile", "-b", "bob:avr:bob", "--show-properties", sketch.String())
+		require.NoError(t, err)
+		res, err = properties.LoadFromBytes(stdout)
+		require.NoError(t, err)
+		// the latest version available are selected
+		require.True(t, res.GetPath("runtime.tools.avr-gcc.path").EquivalentTo(hardwareDir.Join("alice", "tools", "avr-gcc", "50.0.0")))
+		require.True(t, res.GetPath("runtime.tools.avrdude.path").EquivalentTo(hardwareDir.Join("arduino", "tools", "avrdude", "6.3.0-arduino17")))
+
+		stdout, _, err = cli.Run("compile", "-b", "arduino:avr:uno", "--show-properties", sketch.String())
+		require.NoError(t, err)
+		res, err = properties.LoadFromBytes(stdout)
+		require.NoError(t, err)
+		// the selected tools are listed as platform dependencies from the index.json
+		require.True(t, res.GetPath("runtime.tools.avr-gcc.path").EquivalentTo(hardwareDir.Join("arduino", "tools", "avr-gcc", "7.3.0-atmel3.6.1-arduino7")))
+		require.True(t, res.GetPath("runtime.tools.avrdude.path").EquivalentTo(hardwareDir.Join("arduino", "tools", "avrdude", "6.3.0-arduino17")))
+	}
+}

internal/integrationtest/compile_3/testdata/bare_minimum/bare_minimum.ino

+void setup() {}
+void loop() {}

internal/integrationtest/compile_3/testdata/platforms_with_conflicting_tools/alice/hardware/avr/1.0.0/boards.txt

+alice.name=MyBoard
+alice.build.board=ALICE
\ No newline at end of file

internal/integrationtest/compile_3/testdata/platforms_with_conflicting_tools/bob/hardware/avr/1.0.0/boards.txt

+bob.name=Bob's Board
+bob.build.board=BOB
\ No newline at end of file

legacy/builder/target_board_resolver.go

@@ -25,7 +25,7 @@ import (
 type TargetBoardResolver struct{}
 
 func (s *TargetBoardResolver) Run(ctx *types.Context) error {
-	targetPackage, targetPlatform, targetBoard, buildProperties, actualPlatform, err := ctx.PackageManager.ResolveFQBN(ctx.FQBN)
+	targetPackage, targetPlatform, targetBoard, buildProperties, buildPlatform, err := ctx.PackageManager.ResolveFQBN(ctx.FQBN)
 	if err != nil {
 		return fmt.Errorf("%s: %w", tr("Error resolving FQBN"), err)
 	}
@@ -39,7 +39,7 @@ func (s *TargetBoardResolver) Run(ctx *types.Context) error {
 
 	if ctx.Verbose {
 		ctx.Info(tr("Using board '%[1]s' from platform in folder: %[2]s", targetBoard.BoardID, targetPlatform.InstallDir))
-		ctx.Info(tr("Using core '%[1]s' from platform in folder: %[2]s", core, actualPlatform.InstallDir))
+		ctx.Info(tr("Using core '%[1]s' from platform in folder: %[2]s", core, buildPlatform.InstallDir))
 	}
 
 	if buildProperties.Get("build.board") == "" {
@@ -50,7 +50,7 @@ func (s *TargetBoardResolver) Run(ctx *types.Context) error {
 			targetBoard.String(), "'build.board'", defaultBuildBoard))
 	}
 
-	requiredTools, err := ctx.PackageManager.FindToolsRequiredForBoard(targetBoard)
+	requiredTools, err := ctx.PackageManager.FindToolsRequiredForBuild(targetPlatform, buildPlatform)
 	if err != nil {
 		return err
 	}
@@ -60,7 +60,7 @@ func (s *TargetBoardResolver) Run(ctx *types.Context) error {
 	ctx.TargetBoardBuildProperties = buildProperties
 	ctx.TargetPlatform = targetPlatform
 	ctx.TargetPackage = targetPackage
-	ctx.ActualPlatform = actualPlatform
+	ctx.ActualPlatform = buildPlatform
 	ctx.RequiredTools = requiredTools
 	return nil
 }