From bbf3a431fac3bb7247b6fcc2473a6204a7276981 Mon Sep 17 00:00:00 2001 From: david gauchard Date: Tue, 29 Dec 2020 22:24:44 +0100 Subject: [PATCH 1/8] check whether prevOpts is nil in WipeoutBuildPathIfBuildOptionsChanged's Run --- legacy/builder/wipeout_build_path_if_build_options_changed.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/legacy/builder/wipeout_build_path_if_build_options_changed.go b/legacy/builder/wipeout_build_path_if_build_options_changed.go index 57be59838d6..1d0e9fd7c82 100644 --- a/legacy/builder/wipeout_build_path_if_build_options_changed.go +++ b/legacy/builder/wipeout_build_path_if_build_options_changed.go @@ -45,13 +45,13 @@ func (s *WipeoutBuildPathIfBuildOptionsChanged) Run(ctx *types.Context) error { json.Unmarshal([]byte(previousBuildOptionsJson), &prevOpts) // If SketchLocation path is different but filename is the same, consider it equal - if filepath.Base(opts.Get("sketchLocation")) == filepath.Base(prevOpts.Get("sketchLocation")) { + if prevOpts != nil && filepath.Base(opts.Get("sketchLocation")) == filepath.Base(prevOpts.Get("sketchLocation")) { opts.Remove("sketchLocation") prevOpts.Remove("sketchLocation") } // If options are not changed check if core has - if opts.Equals(prevOpts) { + if prevOpts != nil && opts.Equals(prevOpts) { // check if any of the files contained in the core folders has changed // since the json was generated - like platform.txt or similar // if so, trigger a "safety" wipe From fa9f4f639647a65477966bda0938b809acb2c906 Mon Sep 17 00:00:00 2001 From: david gauchard Date: Wed, 30 Dec 2020 12:59:01 +0100 Subject: [PATCH 2/8] + debug message --- legacy/builder/constants/constants.go | 1 + .../wipeout_build_path_if_build_options_changed.go | 9 +++++++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/legacy/builder/constants/constants.go b/legacy/builder/constants/constants.go index 5773f4d770d..cdd23f7e14d 100644 --- a/legacy/builder/constants/constants.go +++ b/legacy/builder/constants/constants.go @@ -125,6 +125,7 @@ const MSG_USING_CACHED_INCLUDES = "Using cached library dependencies for file: { const MSG_WARNING_LIB_INVALID_CATEGORY = "WARNING: Category '{0}' in library {1} is not valid. Setting to '{2}'" const MSG_WARNING_PLATFORM_OLD_VALUES = "Warning: platform.txt from core '{0}' contains deprecated {1}, automatically converted to {2}. Consider upgrading this core." const MSG_WARNING_SPURIOUS_FILE_IN_LIB = "WARNING: Spurious {0} folder in '{1}' library" +const MSG_INVESTIGATE = " (unusual state to investigate)" const PACKAGE_NAME = "name" const PACKAGE_TOOLS = "tools" const PLATFORM_ARCHITECTURE = "architecture" diff --git a/legacy/builder/wipeout_build_path_if_build_options_changed.go b/legacy/builder/wipeout_build_path_if_build_options_changed.go index 1d0e9fd7c82..5b1c4209e69 100644 --- a/legacy/builder/wipeout_build_path_if_build_options_changed.go +++ b/legacy/builder/wipeout_build_path_if_build_options_changed.go @@ -44,14 +44,19 @@ func (s *WipeoutBuildPathIfBuildOptionsChanged) Run(ctx *types.Context) error { json.Unmarshal([]byte(buildOptionsJson), &opts) json.Unmarshal([]byte(previousBuildOptionsJson), &prevOpts) + if prevOpts == nil { + ctx.GetLogger().Println(constants.LOG_LEVEL_DEBUG, constants.MSG_BUILD_OPTIONS_CHANGED + constants.MSG_INVESTIGATE); + return doCleanup(ctx.BuildPath) + } + // If SketchLocation path is different but filename is the same, consider it equal - if prevOpts != nil && filepath.Base(opts.Get("sketchLocation")) == filepath.Base(prevOpts.Get("sketchLocation")) { + if filepath.Base(opts.Get("sketchLocation")) == filepath.Base(prevOpts.Get("sketchLocation")) { opts.Remove("sketchLocation") prevOpts.Remove("sketchLocation") } // If options are not changed check if core has - if prevOpts != nil && opts.Equals(prevOpts) { + if opts.Equals(prevOpts) { // check if any of the files contained in the core folders has changed // since the json was generated - like platform.txt or similar // if so, trigger a "safety" wipe From 82189d6ec06ee87dab6f215d78b552170bcf8c37 Mon Sep 17 00:00:00 2001 From: david gauchard Date: Wed, 30 Dec 2020 15:05:33 +0100 Subject: [PATCH 3/8] update from review: improve user message --- legacy/builder/constants/constants.go | 2 +- legacy/builder/wipeout_build_path_if_build_options_changed.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/legacy/builder/constants/constants.go b/legacy/builder/constants/constants.go index cdd23f7e14d..c8fea8684c8 100644 --- a/legacy/builder/constants/constants.go +++ b/legacy/builder/constants/constants.go @@ -87,6 +87,7 @@ const MSG_CORE_CACHE_UNAVAILABLE = "Unable to cache built core, please tell {0} const MSG_BOARD_UNKNOWN = "Board {0} (platform {1}, package {2}) is unknown" const MSG_BOOTLOADER_FILE_MISSING = "Bootloader file specified but missing: {0}" const MSG_BUILD_OPTIONS_CHANGED = "Build options changed, rebuilding all" +const MSG_BUILD_OPTIONS_INVALID = "{0} invalid, rebuilding all" const MSG_CANT_FIND_SKETCH_IN_PATH = "Unable to find {0} in {1}" const MSG_FQBN_INVALID = "{0} is not a valid fully qualified board name. Required format is targetPackageName:targetPlatformName:targetBoardName." const MSG_SKIP_PRECOMPILED_LIBRARY = "Skipping dependencies detection for precompiled library {0}" @@ -125,7 +126,6 @@ const MSG_USING_CACHED_INCLUDES = "Using cached library dependencies for file: { const MSG_WARNING_LIB_INVALID_CATEGORY = "WARNING: Category '{0}' in library {1} is not valid. Setting to '{2}'" const MSG_WARNING_PLATFORM_OLD_VALUES = "Warning: platform.txt from core '{0}' contains deprecated {1}, automatically converted to {2}. Consider upgrading this core." const MSG_WARNING_SPURIOUS_FILE_IN_LIB = "WARNING: Spurious {0} folder in '{1}' library" -const MSG_INVESTIGATE = " (unusual state to investigate)" const PACKAGE_NAME = "name" const PACKAGE_TOOLS = "tools" const PLATFORM_ARCHITECTURE = "architecture" diff --git a/legacy/builder/wipeout_build_path_if_build_options_changed.go b/legacy/builder/wipeout_build_path_if_build_options_changed.go index 5b1c4209e69..299c8c9e9ad 100644 --- a/legacy/builder/wipeout_build_path_if_build_options_changed.go +++ b/legacy/builder/wipeout_build_path_if_build_options_changed.go @@ -45,7 +45,7 @@ func (s *WipeoutBuildPathIfBuildOptionsChanged) Run(ctx *types.Context) error { json.Unmarshal([]byte(previousBuildOptionsJson), &prevOpts) if prevOpts == nil { - ctx.GetLogger().Println(constants.LOG_LEVEL_DEBUG, constants.MSG_BUILD_OPTIONS_CHANGED + constants.MSG_INVESTIGATE); + ctx.GetLogger().Println(constants.LOG_LEVEL_DEBUG, constants.MSG_BUILD_OPTIONS_INVALID, constants.BUILD_OPTIONS_FILE) return doCleanup(ctx.BuildPath) } From fbb8e61d5947d3049ce7d578ba521bad3f86d4fc Mon Sep 17 00:00:00 2001 From: david gauchard Date: Thu, 31 Dec 2020 23:00:12 +0100 Subject: [PATCH 4/8] fix per review --- .../builder/wipeout_build_path_if_build_options_changed.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/legacy/builder/wipeout_build_path_if_build_options_changed.go b/legacy/builder/wipeout_build_path_if_build_options_changed.go index 299c8c9e9ad..6a39d9c444e 100644 --- a/legacy/builder/wipeout_build_path_if_build_options_changed.go +++ b/legacy/builder/wipeout_build_path_if_build_options_changed.go @@ -40,11 +40,10 @@ func (s *WipeoutBuildPathIfBuildOptionsChanged) Run(ctx *types.Context) error { previousBuildOptionsJson := ctx.BuildOptionsJsonPrevious var opts *properties.Map - var prevOpts *properties.Map json.Unmarshal([]byte(buildOptionsJson), &opts) - json.Unmarshal([]byte(previousBuildOptionsJson), &prevOpts) - if prevOpts == nil { + var prevOpts *properties.Map + if err := json.Unmarshal([]byte(previousBuildOptionsJson), &prevOpts); err != nil || prevOpts == nil { ctx.GetLogger().Println(constants.LOG_LEVEL_DEBUG, constants.MSG_BUILD_OPTIONS_INVALID, constants.BUILD_OPTIONS_FILE) return doCleanup(ctx.BuildPath) } From 8d0038a613b43f73140b63d1aab98c22d429da15 Mon Sep 17 00:00:00 2001 From: david gauchard Date: Wed, 13 Jan 2021 10:17:05 +0100 Subject: [PATCH 5/8] add check to buildOptionsJson too --- legacy/builder/constants/constants.go | 5 +++-- .../wipeout_build_path_if_build_options_changed.go | 11 ++++++++--- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/legacy/builder/constants/constants.go b/legacy/builder/constants/constants.go index c8fea8684c8..7e35760adb3 100644 --- a/legacy/builder/constants/constants.go +++ b/legacy/builder/constants/constants.go @@ -86,8 +86,9 @@ const MSG_ERROR_ARCHIVING_CORE_CACHE = "Error archiving built core (caching) in const MSG_CORE_CACHE_UNAVAILABLE = "Unable to cache built core, please tell {0} maintainers to follow https://arduino.github.io/arduino-cli/latest/platform-specification/#recipes-to-build-the-corea-archive-file" const MSG_BOARD_UNKNOWN = "Board {0} (platform {1}, package {2}) is unknown" const MSG_BOOTLOADER_FILE_MISSING = "Bootloader file specified but missing: {0}" -const MSG_BUILD_OPTIONS_CHANGED = "Build options changed, rebuilding all" -const MSG_BUILD_OPTIONS_INVALID = "{0} invalid, rebuilding all" +const MSG_REBUILD_ALL = ", rebuilding all" +const MSG_BUILD_OPTIONS_CHANGED = "Build options changed" +const MSG_BUILD_OPTIONS_INVALID = "{0} invalid" const MSG_CANT_FIND_SKETCH_IN_PATH = "Unable to find {0} in {1}" const MSG_FQBN_INVALID = "{0} is not a valid fully qualified board name. Required format is targetPackageName:targetPlatformName:targetBoardName." const MSG_SKIP_PRECOMPILED_LIBRARY = "Skipping dependencies detection for precompiled library {0}" diff --git a/legacy/builder/wipeout_build_path_if_build_options_changed.go b/legacy/builder/wipeout_build_path_if_build_options_changed.go index 6a39d9c444e..e6560e1332e 100644 --- a/legacy/builder/wipeout_build_path_if_build_options_changed.go +++ b/legacy/builder/wipeout_build_path_if_build_options_changed.go @@ -18,7 +18,9 @@ package builder import ( "encoding/json" "path/filepath" + "os" + "github.com/arduino/arduino-cli/cli/errorcodes" "github.com/arduino/arduino-cli/legacy/builder/builder_utils" "github.com/arduino/arduino-cli/legacy/builder/constants" "github.com/arduino/arduino-cli/legacy/builder/types" @@ -40,11 +42,14 @@ func (s *WipeoutBuildPathIfBuildOptionsChanged) Run(ctx *types.Context) error { previousBuildOptionsJson := ctx.BuildOptionsJsonPrevious var opts *properties.Map - json.Unmarshal([]byte(buildOptionsJson), &opts) + if err := json.Unmarshal([]byte(buildOptionsJson), &opts); err != nil || opts == nil { + ctx.GetLogger().Println(constants.LOG_LEVEL_DEBUG, constants.MSG_BUILD_OPTIONS_INVALID, constants.BUILD_OPTIONS_FILE) + os.Exit(errorcodes.ErrGeneric) + } var prevOpts *properties.Map if err := json.Unmarshal([]byte(previousBuildOptionsJson), &prevOpts); err != nil || prevOpts == nil { - ctx.GetLogger().Println(constants.LOG_LEVEL_DEBUG, constants.MSG_BUILD_OPTIONS_INVALID, constants.BUILD_OPTIONS_FILE) + ctx.GetLogger().Println(constants.LOG_LEVEL_DEBUG, constants.MSG_BUILD_OPTIONS_INVALID + constants.MSG_REBUILD_ALL, constants.BUILD_OPTIONS_FILE) return doCleanup(ctx.BuildPath) } @@ -77,7 +82,7 @@ func (s *WipeoutBuildPathIfBuildOptionsChanged) Run(ctx *types.Context) error { func doCleanup(buildPath *paths.Path) error { // FIXME: this should go outside legacy and behind a `logrus` call so users can // control when this should be printed. - // logger.Println(constants.LOG_LEVEL_INFO, constants.MSG_BUILD_OPTIONS_CHANGED) + // logger.Println(constants.LOG_LEVEL_INFO, constants.MSG_BUILD_OPTIONS_CHANGED + MSG_REBUILD_ALL) if files, err := buildPath.ReadDir(); err != nil { return errors.WithMessage(err, "cleaning build path") From 023c9fc2966c260505ca61f673ae098fd51a7b8b Mon Sep 17 00:00:00 2001 From: david gauchard Date: Thu, 15 Apr 2021 23:02:00 +0200 Subject: [PATCH 6/8] panic'ing instead of keeping on with mess --- .../builder/wipeout_build_path_if_build_options_changed.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/legacy/builder/wipeout_build_path_if_build_options_changed.go b/legacy/builder/wipeout_build_path_if_build_options_changed.go index e6560e1332e..72f0ff89b2b 100644 --- a/legacy/builder/wipeout_build_path_if_build_options_changed.go +++ b/legacy/builder/wipeout_build_path_if_build_options_changed.go @@ -18,9 +18,7 @@ package builder import ( "encoding/json" "path/filepath" - "os" - "github.com/arduino/arduino-cli/cli/errorcodes" "github.com/arduino/arduino-cli/legacy/builder/builder_utils" "github.com/arduino/arduino-cli/legacy/builder/constants" "github.com/arduino/arduino-cli/legacy/builder/types" @@ -43,8 +41,7 @@ func (s *WipeoutBuildPathIfBuildOptionsChanged) Run(ctx *types.Context) error { var opts *properties.Map if err := json.Unmarshal([]byte(buildOptionsJson), &opts); err != nil || opts == nil { - ctx.GetLogger().Println(constants.LOG_LEVEL_DEBUG, constants.MSG_BUILD_OPTIONS_INVALID, constants.BUILD_OPTIONS_FILE) - os.Exit(errorcodes.ErrGeneric) + panic(constants.BUILD_OPTIONS_FILE + " is invalid") } var prevOpts *properties.Map From 7df8737aa59dedc3cd9d810fe879983129ea663f Mon Sep 17 00:00:00 2001 From: david gauchard Date: Fri, 22 Jan 2021 08:18:39 +0100 Subject: [PATCH 7/8] fix use of a constant Co-authored-by: per1234 --- legacy/builder/wipeout_build_path_if_build_options_changed.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/legacy/builder/wipeout_build_path_if_build_options_changed.go b/legacy/builder/wipeout_build_path_if_build_options_changed.go index 72f0ff89b2b..fbb621626ea 100644 --- a/legacy/builder/wipeout_build_path_if_build_options_changed.go +++ b/legacy/builder/wipeout_build_path_if_build_options_changed.go @@ -79,7 +79,7 @@ func (s *WipeoutBuildPathIfBuildOptionsChanged) Run(ctx *types.Context) error { func doCleanup(buildPath *paths.Path) error { // FIXME: this should go outside legacy and behind a `logrus` call so users can // control when this should be printed. - // logger.Println(constants.LOG_LEVEL_INFO, constants.MSG_BUILD_OPTIONS_CHANGED + MSG_REBUILD_ALL) + // logger.Println(constants.LOG_LEVEL_INFO, constants.MSG_BUILD_OPTIONS_CHANGED + constants.MSG_REBUILD_ALL) if files, err := buildPath.ReadDir(); err != nil { return errors.WithMessage(err, "cleaning build path") From 4d5a247ac3c2c1a8461b0a5ff64454972a10328d Mon Sep 17 00:00:00 2001 From: Silvano Cerza Date: Mon, 10 May 2021 12:18:39 +0200 Subject: [PATCH 8/8] [skip changelog] Add compile with invalid build.options.json test --- test/test_compile.py | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/test/test_compile.py b/test/test_compile.py index c1374aaf607..ffff1931092 100644 --- a/test/test_compile.py +++ b/test/test_compile.py @@ -1021,3 +1021,29 @@ def test_compile_with_conflicting_libraries_include(run_command, data_dir, copy_ assert 'Multiple libraries were found for "OneWire.h"' in lines assert f"Used: {one_wire_lib_path}" in lines assert f"Not used: {one_wire_ng_lib_path}" in lines + + +def test_compile_with_invalid_build_options_json(run_command, data_dir): + assert run_command("update") + + assert run_command("core install arduino:avr@1.8.3") + + sketch_name = "CompileInvalidBuildOptionsJson" + sketch_path = Path(data_dir, sketch_name) + fqbn = "arduino:avr:uno" + + # Create a test sketch + assert run_command(f"sketch new {sketch_path}") + + # Get the build directory + sketch_path_md5 = hashlib.md5(bytes(sketch_path)).hexdigest().upper() + build_dir = Path(tempfile.gettempdir(), f"arduino-sketch-{sketch_path_md5}") + + assert run_command(f"compile -b {fqbn} {sketch_path} --verbose") + + # Breaks the build.options.json file + build_options_json = build_dir / "build.options.json" + with open(build_options_json, "w") as f: + f.write("invalid json") + + assert run_command(f"compile -b {fqbn} {sketch_path} --verbose")