From 68db33dbf9cbd72b0a8b3b628bfacdbd5205d2db Mon Sep 17 00:00:00 2001 From: Silvano Cerza Date: Thu, 17 Jun 2021 16:35:45 +0200 Subject: [PATCH 1/7] Fix loading platforms with malformed boards.txt --- arduino/cores/packagemanager/loader.go | 41 ++++++++++--- docs/platform-specification.md | 5 +- test/test_core.py | 37 ++++++++++++ .../boards.txt | 58 +++++++++++++++++++ .../platform.txt | 2 + 5 files changed, 133 insertions(+), 10 deletions(-) create mode 100644 test/testdata/platform_with_wrong_custom_board_options/boards.txt create mode 100644 test/testdata/platform_with_wrong_custom_board_options/platform.txt diff --git a/arduino/cores/packagemanager/loader.go b/arduino/cores/packagemanager/loader.go index ac1b82e7118..9f25a8c752f 100644 --- a/arduino/cores/packagemanager/loader.go +++ b/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,39 @@ 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.FirstLevelKeys() { + if key == "menu" && platform.Menus.Size() == 0 { + 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 settings compilation + // flags depending on the board id. + // For more info: + // https://arduino.github.io/arduino-cli/latest/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("skip loading of boards %s: malformed custom board options", strings.Join(skippedBoards, ", ")) } return nil diff --git a/docs/platform-specification.md b/docs/platform-specification.md index e056c9d01dc..f593ee6e320 100644 --- a/docs/platform-specification.md +++ b/docs/platform-specification.md @@ -839,8 +839,9 @@ custom menu we are going to create and must be declared at the beginning of the menu.cpu=Processor [.....] -in this case, the menu name is "Processor".
Now let's add, always in the boards.txt file, the default configuration -(common to all processors) for the duemilanove board: +in this case, the menu name is "Processor". This step is really important, if a board defines an option but the name of +that option is not defined the board won't be loaded.
Now let's add, always in the boards.txt file, the default +configuration (common to all processors) for the duemilanove board: menu.cpu=Processor [.....] diff --git a/test/test_core.py b/test/test_core.py index 8b255fbe1ec..a778f940917 100644 --- a/test/test_core.py +++ b/test/test_core.py @@ -14,6 +14,7 @@ # a commercial license, send an email to license@arduino.cc. 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:platform_with_wrong_custom_board_options@4.2.0: " + + "loading boards: " + + "skip loading of boards arduino-beta-dev:platform_with_wrong_custom_board_options:nessuno: " + + "malformed custom board options" + ) in res.stderr diff --git a/test/testdata/platform_with_wrong_custom_board_options/boards.txt b/test/testdata/platform_with_wrong_custom_board_options/boards.txt new file mode 100644 index 00000000000..87bbb3b86d8 --- /dev/null +++ b/test/testdata/platform_with_wrong_custom_board_options/boards.txt @@ -0,0 +1,58 @@ +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 diff --git a/test/testdata/platform_with_wrong_custom_board_options/platform.txt b/test/testdata/platform_with_wrong_custom_board_options/platform.txt new file mode 100644 index 00000000000..682f130ed49 --- /dev/null +++ b/test/testdata/platform_with_wrong_custom_board_options/platform.txt @@ -0,0 +1,2 @@ +name=Arduino Test Boards +version=4.2.0 \ No newline at end of file From 60a836999cc88b22a8361707911771d1ca35664b Mon Sep 17 00:00:00 2001 From: Silvano Cerza Date: Thu, 17 Jun 2021 17:11:37 +0200 Subject: [PATCH 2/7] [skip changelog] Fix legacy tests --- .../builder/test/user_hardware/my_avr_platform/avr/boards.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/legacy/builder/test/user_hardware/my_avr_platform/avr/boards.txt b/legacy/builder/test/user_hardware/my_avr_platform/avr/boards.txt index 35bd354871d..5b50b4dfaf1 100644 --- a/legacy/builder/test/user_hardware/my_avr_platform/avr/boards.txt +++ b/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 From 9dc0195ad9c5c3120f8ecf7cf66cbcb59a1cc40a Mon Sep 17 00:00:00 2001 From: Silvano Cerza <3314350+silvanocerza@users.noreply.github.com> Date: Thu, 17 Jun 2021 19:00:18 +0200 Subject: [PATCH 3/7] [skip changelog] Remove unecessary sentence from documentation Co-authored-by: per1234 --- docs/platform-specification.md | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/docs/platform-specification.md b/docs/platform-specification.md index f593ee6e320..e056c9d01dc 100644 --- a/docs/platform-specification.md +++ b/docs/platform-specification.md @@ -839,9 +839,8 @@ custom menu we are going to create and must be declared at the beginning of the menu.cpu=Processor [.....] -in this case, the menu name is "Processor". This step is really important, if a board defines an option but the name of -that option is not defined the board won't be loaded.
Now let's add, always in the boards.txt file, the default -configuration (common to all processors) for the duemilanove board: +in this case, the menu name is "Processor".
Now let's add, always in the boards.txt file, the default configuration +(common to all processors) for the duemilanove board: menu.cpu=Processor [.....] From 1106bb644c6677e5bb010587d8dd35b5cb26cd1f Mon Sep 17 00:00:00 2001 From: Silvano Cerza Date: Fri, 18 Jun 2021 09:58:35 +0200 Subject: [PATCH 4/7] [skip changelog] Fix some docs and comments --- arduino/cores/packagemanager/loader.go | 6 +++--- test/test_core.py | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/arduino/cores/packagemanager/loader.go b/arduino/cores/packagemanager/loader.go index 9f25a8c752f..2e56cdfb10a 100644 --- a/arduino/cores/packagemanager/loader.go +++ b/arduino/cores/packagemanager/loader.go @@ -395,10 +395,10 @@ func (pm *PackageManager) loadBoards(platform *cores.PlatformRelease) error { } } // 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 settings compilation + // 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/latest/platform-specification/#global-predefined-properties + // https://arduino.github.io/arduino-cli/dev/platform-specification/#global-predefined-properties boardProperties.Set("_id", boardID) board = platform.GetOrCreateBoard(boardID) board.Properties.Merge(boardProperties) @@ -406,7 +406,7 @@ func (pm *PackageManager) loadBoards(platform *cores.PlatformRelease) error { } if len(skippedBoards) > 0 { - return fmt.Errorf("skip loading of boards %s: malformed custom board options", strings.Join(skippedBoards, ", ")) + return fmt.Errorf("skipping loading of boards %s: malformed custom board options", strings.Join(skippedBoards, ", ")) } return nil diff --git a/test/test_core.py b/test/test_core.py index a778f940917..e01517589ec 100644 --- a/test/test_core.py +++ b/test/test_core.py @@ -709,6 +709,6 @@ def test_core_with_wrong_custom_board_options_is_loaded(run_command, data_dir): "Error initializing instance: " + "loading platform release arduino-beta-dev:platform_with_wrong_custom_board_options@4.2.0: " + "loading boards: " - + "skip loading of boards arduino-beta-dev:platform_with_wrong_custom_board_options:nessuno: " + + "skipping loading of boards arduino-beta-dev:platform_with_wrong_custom_board_options:nessuno: " + "malformed custom board options" ) in res.stderr From 2df7ce0d2fc245a64bc9c8db7ab82aba496281cc Mon Sep 17 00:00:00 2001 From: Silvano Cerza Date: Fri, 18 Jun 2021 10:22:58 +0200 Subject: [PATCH 5/7] Fix board skipping logic --- arduino/cores/packagemanager/loader.go | 12 ++++++++++-- .../boards.txt | 4 ++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/arduino/cores/packagemanager/loader.go b/arduino/cores/packagemanager/loader.go index 2e56cdfb10a..545604dc7d7 100644 --- a/arduino/cores/packagemanager/loader.go +++ b/arduino/cores/packagemanager/loader.go @@ -387,8 +387,16 @@ func (pm *PackageManager) loadBoards(platform *cores.PlatformRelease) error { skippedBoards := []string{} for boardID, boardProperties := range propertiesByBoard { var board *cores.Board - for _, key := range boardProperties.FirstLevelKeys() { - if key == "menu" && platform.Menus.Size() == 0 { + 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 diff --git a/test/testdata/platform_with_wrong_custom_board_options/boards.txt b/test/testdata/platform_with_wrong_custom_board_options/boards.txt index 87bbb3b86d8..aff4961424f 100644 --- a/test/testdata/platform_with_wrong_custom_board_options/boards.txt +++ b/test/testdata/platform_with_wrong_custom_board_options/boards.txt @@ -1,3 +1,5 @@ +menu.cpu=Processor + nessuno.name=Arduino Nessuno nessuno.vid.0=0x2341 nessuno.pid.0=0x0043 @@ -56,3 +58,5 @@ 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 From 61185c6b624a8fc179f5f2d4164afc0c3168cfbb Mon Sep 17 00:00:00 2001 From: Silvano Cerza Date: Fri, 18 Jun 2021 11:07:01 +0200 Subject: [PATCH 6/7] [skip changelog] Fix unit tests --- arduino/cores/packagemanager/package_manager_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/arduino/cores/packagemanager/package_manager_test.go b/arduino/cores/packagemanager/package_manager_test.go index 0c12edb506f..ff51923c35a 100644 --- a/arduino/cores/packagemanager/package_manager_test.go +++ b/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{ From e0feac8f02fa00963301972bce53a3dc4175093c Mon Sep 17 00:00:00 2001 From: Silvano Cerza Date: Fri, 18 Jun 2021 15:49:59 +0200 Subject: [PATCH 7/7] Fix panic if a board has a property that starts with menu --- arduino/cores/packagemanager/loader.go | 2 +- .../platform_with_wrong_custom_board_options/boards.txt | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/arduino/cores/packagemanager/loader.go b/arduino/cores/packagemanager/loader.go index 545604dc7d7..3b94fc3a76d 100644 --- a/arduino/cores/packagemanager/loader.go +++ b/arduino/cores/packagemanager/loader.go @@ -388,7 +388,7 @@ func (pm *PackageManager) loadBoards(platform *cores.PlatformRelease) error { for boardID, boardProperties := range propertiesByBoard { var board *cores.Board for key := range boardProperties.AsMap() { - if !strings.HasPrefix(key, "menu") { + if !strings.HasPrefix(key, "menu.") { continue } // Menu keys are formed like this: diff --git a/test/testdata/platform_with_wrong_custom_board_options/boards.txt b/test/testdata/platform_with_wrong_custom_board_options/boards.txt index aff4961424f..7eb84395f77 100644 --- a/test/testdata/platform_with_wrong_custom_board_options/boards.txt +++ b/test/testdata/platform_with_wrong_custom_board_options/boards.txt @@ -60,3 +60,4 @@ altra.build.core=arduino altra.build.variant=standard altra.menu.cpu.atmega328=ATmega328P altra.menu.cpu.atmega168=ATmega168 +altra.menufoo=bar