Fix loading platforms with malformed boards.txt (#1326)

* Fix loading platforms with malformed boards.txt

* [skip changelog] Fix legacy tests

* [skip changelog] Remove unecessary sentence from documentation

Co-authored-by: per1234 <[email protected]>

* [skip changelog] Fix some docs and comments

* Fix board skipping logic

* [skip changelog] Fix unit tests

* Fix panic if a board has a property that starts with menu

Co-authored-by: per1234 <[email protected]>

Author: silvanocerza

Diff for: arduino/cores/packagemanager/loader.go

@@ -19,6 +19,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"strings"
 
 	"github.com/arduino/arduino-cli/arduino/cores"
 	"github.com/arduino/arduino-cli/configuration"
@@ -250,8 +251,7 @@ func (pm *PackageManager) loadPlatform(targetPackage *cores.Package, platformPat
 			pm.Log.Infof("Package is built-in")
 		}
 		if err := pm.loadPlatformRelease(release, platformPath); err != nil {
-			return status.Newf(codes.FailedPrecondition, "loading platform release: %s", err)
-
+			return status.Newf(codes.FailedPrecondition, "loading platform release %s: %s", release, err)
 		}
 		pm.Log.WithField("platform", release).Infof("Loaded platform")
 
@@ -279,7 +279,7 @@ func (pm *PackageManager) loadPlatform(targetPackage *cores.Package, platformPat
 			}
 			release := platform.GetOrCreateRelease(version)
 			if err := pm.loadPlatformRelease(release, versionDir); err != nil {
-				return status.Newf(codes.FailedPrecondition, "loading platform release: %s", err)
+				return status.Newf(codes.FailedPrecondition, "loading platform release %s: %s", release, err)
 			}
 			pm.Log.WithField("platform", release).Infof("Loaded platform")
 		}
@@ -341,7 +341,7 @@ func (pm *PackageManager) loadPlatformRelease(platform *cores.PlatformRelease, p
 	}
 
 	if err := pm.loadBoards(platform); err != nil {
-		return err
+		return fmt.Errorf("loading boards: %s", err)
 	}
 
 	return nil
@@ -374,14 +374,47 @@ func (pm *PackageManager) loadBoards(platform *cores.PlatformRelease) error {
 
 	propertiesByBoard := boardsProperties.FirstLevelOf()
 
-	platform.Menus = propertiesByBoard["menu"]
+	if menus, ok := propertiesByBoard["menu"]; ok {
+		platform.Menus = menus
+	} else {
+		platform.Menus = properties.NewMap()
+	}
 
-	delete(propertiesByBoard, "menu") // TODO: check this one
+	// This is not a board id so we remove it to correctly
+	// set all other boards properties
+	delete(propertiesByBoard, "menu")
 
+	skippedBoards := []string{}
 	for boardID, boardProperties := range propertiesByBoard {
-		boardProperties.Set("_id", boardID) // TODO: What is that for??
-		board := platform.GetOrCreateBoard(boardID)
+		var board *cores.Board
+		for key := range boardProperties.AsMap() {
+			if !strings.HasPrefix(key, "menu.") {
+				continue
+			}
+			// Menu keys are formed like this:
+			//     menu.cache.off=false
+			//     menu.cache.on=true
+			// so we assume that the a second element in the slice exists
+			menuName := strings.Split(key, ".")[1]
+			if !platform.Menus.ContainsKey(menuName) {
+				fqbn := fmt.Sprintf("%s:%s:%s", platform.Platform.Package.Name, platform.Platform.Architecture, boardID)
+				skippedBoards = append(skippedBoards, fqbn)
+				goto next_board
+			}
+		}
+		// The board's ID must be available in a board's properties since it can
+		// be used in all configuration files for several reasons, like setting compilation
+		// flags depending on the board id.
+		// For more info:
+		// https://arduino.github.io/arduino-cli/dev/platform-specification/#global-predefined-properties
+		boardProperties.Set("_id", boardID)
+		board = platform.GetOrCreateBoard(boardID)
 		board.Properties.Merge(boardProperties)
+	next_board:
+	}
+
+	if len(skippedBoards) > 0 {
+		return fmt.Errorf("skipping loading of boards %s: malformed custom board options", strings.Join(skippedBoards, ", "))
 	}
 
 	return nil

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

@@ -228,8 +228,10 @@ 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")
-	errs := pm.LoadHardware()
-	require.Len(t, errs, 0)
+	// 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
+	pm.LoadHardware()
 	esp32, err := pm.FindBoardWithFQBN("esp32:esp32:esp32")
 	require.NoError(t, err)
 	esptool231 := pm.FindToolDependency(&cores.ToolDependency{

Diff for: legacy/builder/test/user_hardware/my_avr_platform/avr/boards.txt

@@ -1,3 +1,5 @@
+menu.cpu=Processor
+
 custom_yun.name=Arduino Yún
 custom_yun.upload.via_ssh=true
 

Diff for: test/test_core.py

@@ -14,6 +14,7 @@
 # a commercial license, send an email to [email protected].
 import os
 import datetime
+import shutil
 import time
 import platform
 import pytest
@@ -675,3 +676,39 @@ def test_core_list_platform_without_platform_txt(run_command, data_dir):
     core = cores[0]
     assert core["id"] == "some-packager:some-arch"
     assert core["name"] == "some-packager-some-arch"
+
+
+def test_core_with_wrong_custom_board_options_is_loaded(run_command, data_dir):
+    test_platform_name = "platform_with_wrong_custom_board_options"
+    platform_install_dir = Path(data_dir, "hardware", "arduino-beta-dev", test_platform_name)
+    platform_install_dir.mkdir(parents=True)
+
+    # Install platform in Sketchbook hardware dir
+    shutil.copytree(
+        Path(__file__).parent / "testdata" / test_platform_name, platform_install_dir, dirs_exist_ok=True,
+    )
+
+    assert run_command("update")
+
+    res = run_command("core list --format json")
+    assert res.ok
+
+    cores = json.loads(res.stdout)
+    mapped = {core["id"]: core for core in cores}
+    assert len(mapped) == 1
+    # Verifies platform is loaded except excluding board with wrong options
+    assert "arduino-beta-dev:platform_with_wrong_custom_board_options" in mapped
+    boards = {b["fqbn"]: b for b in mapped["arduino-beta-dev:platform_with_wrong_custom_board_options"]["boards"]}
+    assert len(boards) == 1
+    # Verify board with malformed options is not loaded
+    assert "arduino-beta-dev:platform_with_wrong_custom_board_options:nessuno" not in boards
+    # Verify other board is loaded
+    assert "arduino-beta-dev:platform_with_wrong_custom_board_options:altra" in boards
+    # Verify warning is shown to user
+    assert (
+        "Error initializing instance: "
+        + "loading platform release arduino-beta-dev:[email protected]: "
+        + "loading boards: "
+        + "skipping loading of boards arduino-beta-dev:platform_with_wrong_custom_board_options:nessuno: "
+        + "malformed custom board options"
+    ) in res.stderr

Diff for: test/testdata/platform_with_wrong_custom_board_options/boards.txt

@@ -0,0 +1,63 @@
+menu.cpu=Processor
+
+nessuno.name=Arduino Nessuno
+nessuno.vid.0=0x2341
+nessuno.pid.0=0x0043
+nessuno.vid.1=0x2341
+nessuno.pid.1=0x0001
+nessuno.vid.2=0x2A03
+nessuno.pid.2=0x0043
+nessuno.vid.3=0x2341
+nessuno.pid.3=0x0243
+nessuno.upload.tool=avrdude
+nessuno.upload.protocol=arduino
+nessuno.upload.maximum_size=32256
+nessuno.upload.maximum_data_size=2048
+nessuno.upload.speed=115200
+nessuno.bootloader.tool=avrdude
+nessuno.bootloader.low_fuses=0xFF
+nessuno.bootloader.high_fuses=0xDE
+nessuno.bootloader.extended_fuses=0xFD
+nessuno.bootloader.unlock_bits=0x3F
+nessuno.bootloader.lock_bits=0x0F
+nessuno.bootloader.file=optiboot/optiboot_atmega328.hex
+nessuno.build.mcu=atmega328p
+nessuno.build.f_cpu=16000000L
+nessuno.build.board=AVR_NESSUNO
+nessuno.build.core=arduino
+nessuno.build.variant=standard
+
+nessuno.menu.cache.on=Enabled
+nessuno.menu.cache.on.build.cache_flags=-DENABLE_CACHE
+nessuno.menu.cache.off=Disabled
+nessuno.menu.cache.off.build.cache_flags=
+
+altra.name=Arduino Altra
+altra.vid.0=0x2341
+altra.pid.0=0x0043
+altra.vid.1=0x2341
+altra.pid.1=0x0001
+altra.vid.2=0x2A03
+altra.pid.2=0x0043
+altra.vid.3=0x2341
+altra.pid.3=0x0243
+altra.upload.tool=avrdude
+altra.upload.protocol=arduino
+altra.upload.maximum_size=32256
+altra.upload.maximum_data_size=2048
+altra.upload.speed=115200
+altra.bootloader.tool=avrdude
+altra.bootloader.low_fuses=0xFF
+altra.bootloader.high_fuses=0xDE
+altra.bootloader.extended_fuses=0xFD
+altra.bootloader.unlock_bits=0x3F
+altra.bootloader.lock_bits=0x0F
+altra.bootloader.file=optiboot/optiboot_atmega328.hex
+altra.build.mcu=atmega328p
+altra.build.f_cpu=16000000L
+altra.build.board=AVR_ALTRA
+altra.build.core=arduino
+altra.build.variant=standard
+altra.menu.cpu.atmega328=ATmega328P
+altra.menu.cpu.atmega168=ATmega168
+altra.menufoo=bar

Diff for: test/testdata/platform_with_wrong_custom_board_options/platform.txt

@@ -0,0 +1,2 @@
+name=Arduino Test Boards
+version=4.2.0