Skip to content

Commit b894e12

Browse files
cmagliecmaglie
authored
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
1 parent 4b70e02 commit b894e12

File tree

17 files changed

+286
-45
lines changed

17 files changed

+286
-45
lines changed

Diff for: ‎arduino/cores/packagemanager/package_manager.go

+54-21
Original file line numberDiff line numberDiff line change
@@ -614,29 +614,58 @@ func (pme *Explorer) GetTool(toolID string) *cores.Tool {
614614
}
615615
}
616616

617-
// FindToolsRequiredForBoard FIXMEDOC
618-
func (pme *Explorer) FindToolsRequiredForBoard(board *cores.Board) ([]*cores.ToolRelease, error) {
619-
pme.log.Infof("Searching tools required for board %s", board)
620-
621-
// core := board.Properties["build.core"]
622-
platform := board.PlatformRelease
623-
624-
// maps "PACKAGER:TOOL" => ToolRelease
625-
foundTools := map[string]*cores.ToolRelease{}
626-
627-
// a Platform may not specify required tools (because it's a platform that comes from a
628-
// user/hardware dir without a package_index.json) then add all available tools
629-
for _, targetPackage := range pme.packages {
630-
for _, tool := range targetPackage.Tools {
631-
rel := tool.GetLatestInstalled()
632-
if rel != nil {
633-
foundTools[rel.Tool.Name] = rel
617+
// FindToolsRequiredForBuild returns the list of ToolReleases needed to build for the specified
618+
// plaftorm. The buildPlatform may be different depending on the selected board.
619+
func (pme *Explorer) FindToolsRequiredForBuild(platform, buildPlatform *cores.PlatformRelease) ([]*cores.ToolRelease, error) {
620+
621+
// maps tool name => all available ToolRelease
622+
allToolsAlternatives := map[string][]*cores.ToolRelease{}
623+
for _, tool := range pme.GetAllInstalledToolsReleases() {
624+
alternatives := allToolsAlternatives[tool.Tool.Name]
625+
alternatives = append(alternatives, tool)
626+
allToolsAlternatives[tool.Tool.Name] = alternatives
627+
}
628+
629+
// selectBest select the tool with best matching, applying the following rules
630+
// in order of priority:
631+
// - the tool comes from the requested packager
632+
// - the tool comes from the build platform packager
633+
// - the tool has the greatest version
634+
// - the tool packager comes first in alphabetic order
635+
packagerPriority := map[string]int{}
636+
packagerPriority[platform.Platform.Package.Name] = 2
637+
if buildPlatform != nil {
638+
packagerPriority[buildPlatform.Platform.Package.Name] = 1
639+
}
640+
selectBest := func(tools []*cores.ToolRelease) *cores.ToolRelease {
641+
selected := tools[0]
642+
for _, tool := range tools[1:] {
643+
if packagerPriority[tool.Tool.Package.Name] != packagerPriority[selected.Tool.Package.Name] {
644+
if packagerPriority[tool.Tool.Package.Name] > packagerPriority[selected.Tool.Package.Name] {
645+
selected = tool
646+
}
647+
continue
648+
}
649+
if !tool.Version.Equal(selected.Version) {
650+
if tool.Version.GreaterThan(selected.Version) {
651+
selected = tool
652+
}
653+
continue
654+
}
655+
if tool.Tool.Package.Name < selected.Tool.Package.Name {
656+
selected = tool
634657
}
635658
}
659+
return selected
636660
}
637661

638-
// replace the default tools above with the specific required by the current platform
662+
// First select the specific tools required by the current platform
639663
requiredTools := []*cores.ToolRelease{}
664+
// The Sorting of the tool dependencies is required because some platforms may depends
665+
// on more than one version of the same tool. For example adafruit:samd has both
666+
// [email protected] and [email protected]. To allow the runtime property
667+
// {runtime.tools.bossac.path} to be correctly set to the 1.8.0 version we must ensure
668+
// that the returned array is sorted by version.
640669
platform.ToolDependencies.Sort()
641670
for _, toolDep := range platform.ToolDependencies {
642671
pme.log.WithField("tool", toolDep).Infof("Required tool")
@@ -645,11 +674,15 @@ func (pme *Explorer) FindToolsRequiredForBoard(board *cores.Board) ([]*cores.Too
645674
return nil, fmt.Errorf(tr("tool release not found: %s"), toolDep)
646675
}
647676
requiredTools = append(requiredTools, tool)
648-
delete(foundTools, tool.Tool.Name)
677+
delete(allToolsAlternatives, tool.Tool.Name)
649678
}
650679

651-
for _, toolRel := range foundTools {
652-
requiredTools = append(requiredTools, toolRel)
680+
// Since a Platform may not specify the required tools (because it's a platform that comes
681+
// from a user/hardware dir without a package_index.json) then add all available tools giving
682+
// priority to tools coming from the same packager or referenced packager
683+
for _, tools := range allToolsAlternatives {
684+
tool := selectBest(tools)
685+
requiredTools = append(requiredTools, tool)
653686
}
654687
return requiredTools, nil
655688
}

Diff for: ‎arduino/cores/packagemanager/package_manager_test.go

+43-2
Original file line numberDiff line numberDiff line change
@@ -287,6 +287,8 @@ func TestFindToolsRequiredForBoard(t *testing.T) {
287287
loadIndex("https://dl.espressif.com/dl/package_esp32_index.json")
288288
loadIndex("http://arduino.esp8266.com/stable/package_esp8266com_index.json")
289289
loadIndex("https://adafruit.github.io/arduino-board-index/package_adafruit_index.json")
290+
loadIndex("https://test.com/package_test_index.json") // this is not downloaded, it just picks the "local cached" file package_test_index.json
291+
290292
// We ignore the errors returned since they might not be necessarily blocking
291293
// but just warnings for the user, like in the case a board is not loaded
292294
// because of malformed menus
@@ -310,8 +312,13 @@ func TestFindToolsRequiredForBoard(t *testing.T) {
310312
})
311313
require.NotNil(t, esptool0413)
312314

315+
testPlatform := pme.FindPlatformRelease(&packagemanager.PlatformReference{
316+
Package: "test",
317+
PlatformArchitecture: "avr",
318+
PlatformVersion: semver.MustParse("1.1.0")})
319+
313320
testConflictingToolsInDifferentPackages := func() {
314-
tools, err := pme.FindToolsRequiredForBoard(esp32)
321+
tools, err := pme.FindToolsRequiredForBuild(esp32.PlatformRelease, nil)
315322
require.NoError(t, err)
316323
require.Contains(t, tools, esptool231)
317324
require.NotContains(t, tools, esptool0413)
@@ -331,10 +338,44 @@ func TestFindToolsRequiredForBoard(t *testing.T) {
331338
testConflictingToolsInDifferentPackages()
332339
testConflictingToolsInDifferentPackages()
333340

341+
{
342+
// Test buildPlatform dependencies
343+
arduinoBossac180 := pme.FindToolDependency(&cores.ToolDependency{
344+
ToolPackager: "arduino",
345+
ToolName: "bossac",
346+
ToolVersion: semver.ParseRelaxed("1.8.0-48-gb176eee"),
347+
})
348+
require.NotNil(t, arduinoBossac180)
349+
testBossac175 := pme.FindToolDependency(&cores.ToolDependency{
350+
ToolPackager: "test",
351+
ToolName: "bossac",
352+
ToolVersion: semver.ParseRelaxed("1.7.5"),
353+
})
354+
require.NotNil(t, testBossac175)
355+
356+
tools, err := pme.FindToolsRequiredForBuild(esp32.PlatformRelease, nil)
357+
require.NoError(t, err)
358+
require.Contains(t, tools, esptool231)
359+
require.NotContains(t, tools, esptool0413)
360+
// When building without testPlatform dependency, arduino:bossac should be selected
361+
// since it has the higher version
362+
require.NotContains(t, tools, testBossac175)
363+
require.Contains(t, tools, arduinoBossac180)
364+
365+
tools, err = pme.FindToolsRequiredForBuild(esp32.PlatformRelease, testPlatform)
366+
require.NoError(t, err)
367+
require.Contains(t, tools, esptool231)
368+
require.NotContains(t, tools, esptool0413)
369+
// When building with testPlatform dependency, test:bossac should be selected
370+
// because it has dependency priority
371+
require.Contains(t, tools, testBossac175)
372+
require.NotContains(t, tools, arduinoBossac180)
373+
}
374+
334375
feather, err := pme.FindBoardWithFQBN("adafruit:samd:adafruit_feather_m0_express")
335376
require.NoError(t, err)
336377
require.NotNil(t, feather)
337-
featherTools, err := pme.FindToolsRequiredForBoard(feather)
378+
featherTools, err := pme.FindToolsRequiredForBuild(feather.PlatformRelease, nil)
338379
require.NoError(t, err)
339380
require.NotNil(t, featherTools)
340381

Diff for: ‎arduino/cores/packagemanager/testdata/data_dir_1/package_test_index.json

+45
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
{
2+
"packages": [
3+
{
4+
"name": "test",
5+
"websiteURL": "https://test.com",
6+
"email": "[email protected]",
7+
"help": {
8+
"online": "https://test.com"
9+
},
10+
"maintainer": "Test",
11+
"platforms": [
12+
{
13+
"architecture": "avr",
14+
"boards": [],
15+
"name": "Test AVR Boards",
16+
"category": "Test",
17+
"version": "1.1.0",
18+
"archiveFileName": "test-1.1.0.tar.bz2",
19+
"checksum": "SHA-256:4e72d4267d9a8d86874edcd021dc661854a5136c0eed947a6fe10366bc51e373",
20+
"help": {
21+
"online": "https://test.com"
22+
},
23+
"url": "https://test.com/test-1.1.0.tar.bz2",
24+
"toolsDependencies": [],
25+
"size": "12345"
26+
}
27+
],
28+
"tools": [
29+
{
30+
"name": "bossac",
31+
"version": "1.7.5",
32+
"systems": [
33+
{
34+
"host": "i386-apple-darwin11",
35+
"checksum": "MD5:603bcce8e59683ac27054b3197a53254",
36+
"size": "96372129",
37+
"archiveFileName": "bossac.tar.bz2",
38+
"url": "https://test.com/bossac.tar.bz2"
39+
}
40+
]
41+
}
42+
]
43+
}
44+
]
45+
}

Diff for: ‎arduino/cores/packagemanager/testdata/data_dir_1/packages/test/hardware/avr/1.1.0/boards.txt

Whitespace-only changes.

Diff for: ‎arduino/cores/packagemanager/testdata/data_dir_1/packages/test/tools/bossac/1.7.5/.keep

Whitespace-only changes.

Diff for: ‎commands/debug/debug_info.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ func getDebugProperties(req *debug.DebugConfigRequest, pme *packagemanager.Explo
6666
}
6767

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

Diff for: ‎commands/upload/upload.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -295,7 +295,7 @@ func runProgramAction(pme *packagemanager.Explorer,
295295
for _, tool := range pme.GetAllInstalledToolsReleases() {
296296
uploadProperties.Merge(tool.RuntimeProperties())
297297
}
298-
if requiredTools, err := pme.FindToolsRequiredForBoard(board); err == nil {
298+
if requiredTools, err := pme.FindToolsRequiredForBuild(boardPlatform, buildPlatform); err == nil {
299299
for _, requiredTool := range requiredTools {
300300
logrus.WithField("tool", requiredTool).Info("Tool required for upload")
301301
if requiredTool.IsInstalled() {

Diff for: ‎docs/UPGRADING.md

+17
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,23 @@ plus `Name`, `Version`, and an `UpToDate` boolean flag.
8787

8888
`InstallZipLib` method `archivePath` is now a `paths.Path` instead of a `string`.
8989

90+
### golang API change in `github.com/arduino/arduino-cli/rduino/cores/packagemanager.Explorer`
91+
92+
The `packagemanager.Explorer` method `FindToolsRequiredForBoard`:
93+
94+
```go
95+
func (pme *Explorer) FindToolsRequiredForBoard(board *cores.Board) ([]*cores.ToolRelease, error) { ... }
96+
```
97+
98+
has been renamed to `FindToolsRequiredForBuild:
99+
100+
```go
101+
func (pme *Explorer) FindToolsRequiredForBuild(platform, buildPlatform *cores.PlatformRelease) ([]*cores.ToolRelease, error) { ... }
102+
```
103+
104+
moreover it now requires the `platform` and the `buildPlatform` (a.k.a. the referenced platform core used for the
105+
compile) instead of the `board`. Usually these two value are obtained from the `Explorer.ResolveFQBN(...)` method.
106+
90107
## 0.29.0
91108

92109
### Removed gRPC API: `cc.arduino.cli.commands.v1.UpdateCoreLibrariesIndex`, `Outdated`, and `Upgrade`

Diff for: ‎docs/package_index_json-specification.md

+32-11
Original file line numberDiff line numberDiff line change
@@ -176,17 +176,6 @@ The other fields are:
176176
macOS you can use the command `shasum -a 256 filename` to generate SHA-256 checksums. There are free options for
177177
Windows, including md5deep. There are also online utilities for generating checksums.
178178

179-
##### How a tool's path is determined in platform.txt
180-
181-
When the IDE needs a tool, it downloads the corresponding archive file and unpacks the content into a private folder
182-
that can be referenced from `platform.txt` using one of the following properties:
183-
184-
- `{runtime.tools.TOOLNAME-VERSION.path}`
185-
- `{runtime.tools.TOOLNAME.path}`
186-
187-
For example, to obtain the avr-gcc 4.8.1 folder we can use `{runtime.tools.avr-gcc-4.8.1-arduino5.path}` or
188-
`{runtime.tools.avr-gcc.path}`.
189-
190179
### Platforms definitions
191180

192181
Finally, let's see how `PLATFORMS` are made.
@@ -270,6 +259,38 @@ rules Arduino IDE follows for parsing versions
270259
Note: if you miss a bracket in the JSON index, then add the URL to your Preferences, and open Boards Manager it can
271260
cause the Arduino IDE to no longer load until you have deleted the file from your arduino15 folder.
272261

262+
#### How a tool's path is determined in platform.txt
263+
264+
When the IDE needs a tool, it downloads the corresponding archive file and unpacks the content into a private folder
265+
that can be referenced from `platform.txt` using one of the following properties:
266+
267+
- `{runtime.tools.TOOLNAME-VERSION.path}`
268+
- `{runtime.tools.TOOLNAME.path}`
269+
270+
For example, to obtain the avr-gcc 4.8.1 folder we can use `{runtime.tools.avr-gcc-4.8.1.path}` or
271+
`{runtime.tools.avr-gcc.path}`.
272+
273+
In general the same tool may be provided by different packagers (for example the Arduino packager may provide an
274+
`arduino:avr-gcc` and another 3rd party packager may provide their own `3rdparty:avr-gcc`). The rules to disambiguate
275+
are as follows:
276+
277+
- The property `{runtime.tools.TOOLNAME.path}` points, in order of priority, to:
278+
279+
1. the tool, version and packager specified via `toolsDependencies` in the `package_index.json`
280+
1. the highest version of the tool provided by the packager of the current platform
281+
1. the highest version of the tool provided by the packager of the referenced platform used for compile (see
282+
["Referencing another core, variant or tool"](platform-specification.md#referencing-another-core-variant-or-tool)
283+
for more info)
284+
1. the highest version of the tool provided by any other packager (in case of tie, the first packager in alphabetical
285+
order wins)
286+
287+
- The property `{runtime.tools.TOOLNAME-VERSION.path}` points, in order of priority, to:
288+
1. the tool and version provided by the packager of the current platform
289+
1. the tool and version provided by the packager of the referenced platform used for compile (see
290+
["Referencing another core, variant or tool"](platform-specification.md#referencing-another-core-variant-or-tool)
291+
for more info)
292+
1. the tool and version provided by any other packager (in case of tie, the first packager in alphabetical order wins)
293+
273294
### Example JSON index file
274295

275296
```json

Diff for: ‎docs/platform-specification.md

+8-4
Original file line numberDiff line numberDiff line change
@@ -766,14 +766,18 @@ tools.avrdude.config.path={path}/etc/avrdude.conf
766766
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"
767767
```
768768

769-
A **{runtime.tools.TOOL_NAME.path}** and **{runtime.tools.TOOL_NAME-TOOL_VERSION.path}** property is generated for the
770-
tools of Arduino AVR Boards and any other platform installed via Boards Manager. **{runtime.tools.TOOL_NAME.path}**
771-
points to the latest version of the tool available.
772-
773769
The tool configuration properties are available globally without the prefix. For example, the **tools.avrdude.cmd.path**
774770
property can be used as **{cmd.path}** inside the recipe, and the same happens for all the other avrdude configuration
775771
variables.
776772

773+
### How to retrieve tools path via `{runtime.tools.*}` properties
774+
775+
A **{runtime.tools.TOOLNAME.path}** and **{runtime.tools.TOOLNAME-TOOLVERSION.path}** property is generated for the
776+
tools provided by the current platform and for any other platform installed via Boards Manager.
777+
778+
See [`{runtime.tools.*.path}` rules](package_index_json-specification.md#how-a-tools-path-is-determined-in-platformtxt)
779+
for details on how the runtime properties are determined.
780+
777781
### Environment variables
778782

779783
All the tools launched to compile or upload a sketch will have the following environment variable set:

0 commit comments

Comments
 (0)