8000 check whether prevOpts is nil in WipeoutBuildPathIfBuildOptionsChanged's Run by d-a-v · Pull Request #1118 · arduino/arduino-cli · GitHub
[go: up one dir, main page]
Skip to content

check whether prevOpts is nil in WipeoutBuildPathIfBuildOptionsChanged's Run #1118

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
May 10, 2021
Merged

check whether prevOpts is nil in WipeoutBuildPathIfBuildOptionsChanged's Run #1118

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
bbf3a43
check whether prevOpts is nil in WipeoutBuildPathIfBuildOptionsChange…
d-a-v Dec 29, 2020
fa9f4f6
+ debug message
d-a-v Dec 30, 2020
82189d6
update from review: improve user message
d-a-v Dec 30, 2020
fbb8e61
fix per review
d-a-v Dec 31, 2020
8d0038a
add check to buildOptionsJson too
d-a-v Jan 13, 2021
023c9fc
panic'ing instead of keeping on with mess
d-a-v Apr 15, 2021
7df8737
fix use of a constant
d-a-v Jan 22, 2021
4d5a247
[skip changelog] Add compile with invalid build.options.json test
silvanocerza May 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
add check to buildOptionsJson too
  • Loading branch information
@d-a-v @silvanocerza
d-a-v authored and silvanocerza committed May 10, 2021
commit 8d0038a613b43f73140b63d1aab98c22d429da15
5 changes: 3 additions & 2 deletions legacy/builder/constants/constants.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}"
Expand Down
11 changes: 8 additions & 3 deletions legacy/builder/wipeout_build_path_if_build_options_changed.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this message is confusing:

build.options.json invalid

Because it's not the build.options.json file that's invalid, but rather the internal build options data.

Since this isn't something that should occur under any normal circumstances, I would lean toward just doing a panic(err). Probably @silvanocerza would be able to tell us what is the preferred approach though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine like this, it seems to me that ctx.BuildOptionsJson is populated with data from the build.options.json file only.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The build.options.json file is created from ctx.BuildOptionsJson:

arduino-cli/legacy/builder/store_build_options_map.go

Line 26 in 079bb6c

ctx.BuildPath.Join(constants.BUILD_OPTIONS_FILE).WriteFile([]byte(ctx.BuildOptionsJson))

It's ctx.BuildOptionsJsonPrevious that is populated with data from the build.options.json file:

arduino-cli/legacy/builder/load_previous_build_options.go

Line 38 in 079bb6c

ctx.BuildOptionsJsonPrevious = string(bytes)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I leave it as it currently is,
or change the message,
or replace it by panic(constants.BUILD_OPTIONS_FILE + " is invalid") ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to get @silvanocerza's response to my previous reply just to make sure everyone is on the same page here. After that, I'd recommend going with whatever Silvano think is best.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, forgot about this.
@per1234 is right, probably a message like build options are invalid might be less confusing, it's not exactly transparent like this either but at least it won't confuse the user if the build.options.json file doesn't exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@silvanocerza @per1234 I just proceeded with your suggestion.

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)
}

Expand Down Expand Up @@ -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")
Expand Down
0