Skip to content
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

Jump to bottom

scripts: west: genboard: use JSON schema #18891

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

scripts: west: genboard: use JSON schema #18891

wants to merge 1 commit into from
+135 −75

Conversation

gmarull
Copy link
Member

@gmarull gmarull commented Nov 14, 2024

Use JSON-schema for input/output. This eases tool integration with other tooling as we have a standardized interface.

@gmarull gmarull requested review from a team as code owners November 14, 2024 11:40
@github-actions github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Nov 14, 2024
@NordicBuilder
Copy link
Contributor

NordicBuilder commented Nov 14, 2024 •
edited
Loading

CI Information

To view the history of this post, clich the 'edited' button above
Build number: 3

Inputs:

Sources:

sdk-nrf: PR head: 6cac861b57b16c94da5557801f822ef69ea4c0ca

more details

sdk-nrf:

PR head: 6cac861b57b16c94da5557801f822ef69ea4c0ca
merge base: 0a95881598725ccf32be2ec642f4805231f058e1
target head (main): e6443ca70a94483de5ac34318c0028f3e95741e2
Diff

Github labels

Enabled Name Description
ci-disabled Disable the ci execution
ci-all-test Run all of ci, no test spec filtering will be done
ci-force-downstream Force execution of downstream even if twister fails
ci-run-twister Force run twister
ci-run-zephyr-twister Force run zephyr twister
List of changed files detected by CI (4) 
scripts
│  ├── requirements-base.txt
│  ├── west_commands
│  │  ├── genboard
│  │  │  ├── ncs_genboard.py
│  │  │  ├── schema.json
│  │  │  │ west-ncs-genboard-test.sh

Outputs:

Toolchain

Version: f51bdba1d9
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:f51bdba1d9_912848a074

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

  • ◻️ Toolchain - Skipped: existing toolchain is used
  • ✅ Build twister
    • sdk-nrf test count: 42
  • ✅ Integration tests
Disabled integration tests
    • desktop52_verification
    • doc-internal
    • test_ble_nrf_config
    • test-fw-nrfconnect-apps
    • test-fw-nrfconnect-ble_mesh
    • test-fw-nrfconnect-ble_samples
    • test-fw-nrfconnect-boot
    • test-fw-nrfconnect-chip
    • test-fw-nrfconnect-fem
    • test-fw-nrfconnect-nfc
    • test-fw-nrfconnect-nrf-iot_libmodem-nrf
    • test-fw-nrfconnect-nrf-iot_lwm2m
    • test-fw-nrfconnect-nrf-iot_mosh
    • test-fw-nrfconnect-nrf-iot_nrf_provisioning
    • test-fw-nrfconnect-nrf-iot_positioning
    • test-fw-nrfconnect-nrf-iot_samples
    • test-fw-nrfconnect-nrf-iot_serial_lte_modem
    • test-fw-nrfconnect-nrf-iot_thingy91
    • test-fw-nrfconnect-nrf-iot_zephyr_lwm2m
    • test-fw-nrfconnect-nrf_crypto
    • test-fw-nrfconnect-ps
    • test-fw-nrfconnect-rpc
    • test-fw-nrfconnect-rs
    • test-fw-nrfconnect-tfm
    • test-fw-nrfconnect-thread
    • test-fw-nrfconnect-zigbee
    • test-low-level
    • test-sdk-audio
    • test-sdk-dfu
    • test-sdk-find-my
    • test-sdk-mcuboot
    • test-sdk-pmic-samples
    • test-sdk-sidewalk
    • test-sdk-wifi

Note: This message is automatically posted and updated by the CI

FilipZajdel
FilipZajdel requested changes Nov 14, 2024
View reviewed changes
scripts/west_commands/genboard/ncs_genboard.py Show resolved Hide resolved
scripts/west_commands/genboard/ncs_genboard.py
@@ -119,7 +122,7 @@ def do_run(self, args, unknown_args):
break

if not targets:
log.err(f"Invalid/unsupported variant: {args.variant}")
log.err(f"Invalid/unsupported variant: {req_variant}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to validate soc and variant after the validation in line 62. passed? All supported combinations of socs and variants can be listed in the schema.

Copy link
Member Author

Choose a reason for hiding this comment

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

this means the internal config file is not aligned, should really be catched in tests...

scripts/west_commands/genboard/ncs_genboard.py
Copy link
Contributor

Choose a reason for hiding this comment

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

Can I ask to add a logging messages to stdout that unambiguously inform about the successful generation of the board at the end of the process, please. And other one that informs about the errors?

We are going to integrate with this tool on windows, linux, and macOs which makes logging through the stdout the simplest way to interface through.

For example "The board has been successfully generated" and "Failed to generate board - schema validation error" and "Failed to generate board - (...) whatever other recognizable failure occurred".

Copy link
Member Author

Choose a reason for hiding this comment

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

added some messages, pls check

@gmarull
scripts: west: genboard: use JSON schema
6cac861 
Use JSON-schema for input/output. This eases tool integration with
other tooling as we have a standardized interface.

Signed-off-by: Gerard Marull-Paretas <[email protected]>
FilipZajdel
FilipZajdel approved these changes Nov 15, 2024
View reviewed changes
Copy link
Contributor

@FilipZajdel FilipZajdel left a comment

Choose a reason for hiding this comment

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

Thank you for implementing requested changes. Looks good for me now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FilipZajdel FilipZajdel FilipZajdel approved these changes

@kylebonnici kylebonnici Awaiting requested review from kylebonnici

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gmarull @NordicBuilder @FilipZajdel