Skip to content

Do not treat custom menu options without label as 'malformed' #1882

New issue

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

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

Already on GitHub? # to your account

Merged
merged 2 commits into from
Sep 21, 2022

Conversation

cmaglie
Copy link
Member

@cmaglie cmaglie commented Sep 19, 2022

Please check if the PR fulfills these requirements

  • The PR has no duplicates (please search among the Pull Requests
    before creating one)
  • The PR follows
    our contributing guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • UPGRADING.md has been updated with a migration guide (for breaking changes)
  • What kind of change does this PR introduce?
    Do not report the board menu without a label as an error. It happens when the boards.txt does not specify a top-level menu.xxx directive, for example:
#menu.FlashSize=Flash Size    <--- removing this line will trigger the error described in this PR
[....]
m5stack-core-esp32.menu.FlashSize.4M=4MB (32Mb)
m5stack-core-esp32.menu.FlashSize.4M.build.flash_size=4MB
m5stack-core-esp32.menu.FlashSize.16M=16MB (128Mb)
m5stack-core-esp32.menu.FlashSize.16M.build.flash_size=16MB
  • What is the current behavior?
    If the menu directive is missing the CLI fails to load the whole platform with the message:
Error initializing instance: Error loading hardware platform: loading platform release xxxx@x.y.z: loading boards: skipping loading of boards xxxxxx: malformed custom board options
  • What is the new behavior?
    The CLI will silently accept the board option without the label.
  • Does this PR introduce a breaking change, and is titled accordingly?
    Not exactly a breaking change, but worth noting that the gRPC response to BoardDetails now may have a ConfigOption with a blank option_label field:
message ConfigOption {
  // ID of the configuration option. For identifying the option to machines.
  string option = 1;
  // Name of the configuration option for identifying the option to humans.
  string option_label = 2;
  // Possible values of the configuration option.
  repeated ConfigValue values = 3;
}

@cmaglie cmaglie self-assigned this Sep 19, 2022
@cmaglie cmaglie requested review from per1234 and ubidefeo September 19, 2022 13:27
@codecov
Copy link

codecov bot commented Sep 19, 2022

Codecov Report

Base: 36.68% // Head: 36.58% // Decreases project coverage by -0.10% ⚠️

Coverage data is based on head (ec5bbd5) compared to base (7d1916d).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1882      +/-   ##
==========================================
- Coverage   36.68%   36.58%   -0.11%     
==========================================
  Files         231      231              
  Lines       19680    19677       -3     
==========================================
- Hits         7219     7198      -21     
- Misses      11633    11648      +15     
- Partials      828      831       +3     
Flag Coverage Δ
unit 36.58% <ø> (-0.11%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
arduino/cores/packagemanager/loader.go 72.30% <ø> (-2.01%) ⬇️
internal/integrationtest/arduino-cli.go 79.85% <0.00%> (-2.73%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@cmaglie cmaglie force-pushed the fix_malformed_menu_platofrms branch from 8b74195 to ec5bbd5 Compare September 19, 2022 13:57
@cmaglie cmaglie added type: enhancement Proposed improvement priority: high Resolution is a high priority topic: code Related to content of the project itself criticality: high Of high impact labels Sep 20, 2022
@cmaglie cmaglie merged commit 5730e2e into arduino:master Sep 21, 2022
@cmaglie cmaglie deleted the fix_malformed_menu_platofrms branch September 21, 2022 12:08
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
criticality: high Of high impact priority: high Resolution is a high priority topic: code Related to content of the project itself type: enhancement Proposed improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants