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.

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

Add JSON schema based validation to mbed config script #5022

Merged
merged 6 commits into from Feb 1, 2018

Conversation

Projects
None yet
8 participants
@scartmell-arm
Contributor

scartmell-arm commented Sep 5, 2017

Description

The current configuration system allows a user to specify various compile time configuration options for various targets within JSON files in the projects directory mbed_app.json for applications, and mbed_lib.json for libraries. The parameters will be converted to a collection of macros that are defined when compiling the code. The format of the JSON file is defined here.

Currently, the process of the checking the validity of the config file is done through a series of manual checks in tools/config/__init__.py. This PR will replace the manual checks with a solution that uses JSON schema to validate the configuration files.

Status

IN DEVELOPMENT

Todos

  • Define the JSON schema for app and lib library files
  • Remove remaining manual checks

Steps to test or reproduce

Test the current validation test cases still all pass:

Linux: $ PYTHONPATH=. python -m pytest tools/test/config/config_test.py

Windows: $ set PYTHONPATH=. && python -m pytest tools/test/config/config_test.py

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 5, 2017

@scartmell-arm scartmell-arm force-pushed the scartmell-arm:mbed-config-json-schema branch Sep 5, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Sep 5, 2017

@scartmell-arm Wow thanks!

@theotherjimmy

Looks great so far! In it's current form, it's a huge improvement over the old validation mechanism. Be aware that #4984 may create a conflict with the test changes.

tools/config/schema_app.json Outdated
"pattern": "^([A-Za-z0-9_]+|[A-Za-z0-9_]+=[0-9]+|[A-Za-z0-9_]+=\\\".*\\\")$"
}
},
"__config_path": {

This comment has been minimized.

@theotherjimmy

theotherjimmy Sep 5, 2017

Contributor

Is this key something that we should allow in the configuration file? or is it something that the tools could add after validation?

This comment has been minimized.

@scartmell-arm

scartmell-arm Sep 5, 2017

Contributor

I wasn't sure when I added it, it was in the allowable keys list, but isn't mentioned anywhere in the specification so it's probably fine to remove.

This comment has been minimized.

@theotherjimmy

theotherjimmy Sep 5, 2017

Contributor

It's added here

This comment has been minimized.

@theotherjimmy

theotherjimmy Sep 5, 2017

Contributor

You could do the validation before adding that key.

This comment has been minimized.

@scartmell-arm

scartmell-arm Sep 5, 2017

Contributor

That seems sensible, I'll do that.

tools/config/schema_lib.json Outdated
"description": "Configuration file for an mbed library",
"type": "object",
"$id": "http://example.com/root.json",
"definitions": {

This comment has been minimized.

@theotherjimmy

theotherjimmy Sep 5, 2017

Contributor

These definitions look almost the same. Could they be moved to another file, and imported?

This comment has been minimized.

@scartmell-arm

scartmell-arm Sep 5, 2017

Contributor

Yeah, that should be possible. I'll have a look at separating out the common parts of them, I don't think there are any differences between app and lib config parameter handling that needs to be handled in the validation.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Sep 5, 2017

It looks like travis failed because it does not have the jsonschema package installed. Could you add that to the requirements.txt?

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Sep 5, 2017

@scartmell-arm Looks like there's something missing from the schema: defining macros as evaluating to other macros. Do we need to add this to the documentation?

ConfigException: 'UVISOR_MODE=UVISOR_DISABLED' does not match '^([A-Za-z0-9_]+|[A-Za-z0-9_]+=[0-9]+|[A-Za-z0-9_]+=\\".*\\")$'
'MEM_FREE=free' does not match '^([A-Za-z0-9_]+|[A-Za-z0-9_]+=[0-9]+|[A-Za-z0-9_]+=\\".*\\")$','MEM_ALLOC=malloc' does not match '^([A-Za-z0-9_]+|[A-Za-z0-9_]+=[0-9]+|[A-Za-z0-9_]+=\\".*\\")$'
@scartmell-arm

This comment has been minimized.

Contributor

scartmell-arm commented Sep 6, 2017

Looks like there's something missing from the schema: defining macros as evaluating to other macros. Do we need to add this to the documentation?

@theotherjimmy Yeah, that isn't currently included in the documentation. I have a feeling it's going to be a futile endeavour writing a regex pattern to cover the various valid macro forms so I'll also change that to something reasonable.

Also, wrt the documentation, it states:

However, if target_overrides is defined, config must also be defined.

A number of the tests, and potentially some of the actual application configurations don't follow this so I haven't currently added it to the schema. Whilst it might be correct, I can't see any harm in defining target_overrides without config being set.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Sep 6, 2017

Yeah, that isn't currently included in the documentation.

I'll make a note to include that capability in the documentation.

I have a feeling it's going to be a futile endeavour writing a regex pattern to cover the various valid macro forms so I'll also change that to something reasonable.

Something reasonable like [A-Z0-9_]+(=.*)?? I think that's the best we have.

A number of the tests, and potentially some of the actual application configurations don't follow this so I haven't currently added it to the schema.

Good. That line is just wrong. Adding to the list of documentation updates.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Sep 7, 2017

@scartmell-arm It looks like #4984 changed the way those test worked out from underneath you. Could you rebase to resolve those conflicts?

@theotherjimmy

Was this an omission?

tools/config/schema_lib.json Outdated
"title": "Mbed Library Schema",
"description": "Configuration file for an mbed library",
"type": "object",
"$id": "http://example.com/root.json",

This comment has been minimized.

@theotherjimmy

theotherjimmy Sep 7, 2017

Contributor

I don't think this is quite right.

tools/config/schema_app.json Outdated
"title": "Mbed Library Schema",
"description": "Configuration file for an mbed library",
"type": "object",
"$id": "http://example.com/root.json",

This comment has been minimized.

@theotherjimmy

theotherjimmy Sep 7, 2017

Contributor

This looks off too.

@scartmell-arm scartmell-arm force-pushed the scartmell-arm:mbed-config-json-schema branch Sep 12, 2017

@scartmell-arm scartmell-arm force-pushed the scartmell-arm:mbed-config-json-schema branch 2 times, most recently Sep 28, 2017

@scartmell-arm scartmell-arm force-pushed the scartmell-arm:mbed-config-json-schema branch 2 times, most recently Sep 28, 2017

@scartmell-arm scartmell-arm force-pushed the scartmell-arm:mbed-config-json-schema branch 2 times, most recently Oct 10, 2017

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 10, 2017

@scartmell-arm scartmell-arm force-pushed the scartmell-arm:mbed-config-json-schema branch Oct 10, 2017

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 10, 2017

@adbridge

This comment has been minimized.

Contributor

adbridge commented Oct 16, 2017

/morph build

@adbridge

This comment has been minimized.

Contributor

adbridge commented Oct 16, 2017

@theotherjimmy looks like this needs a re-review

@adbridge adbridge added needs: review and removed needs: work labels Oct 16, 2017

@scartmell-arm

This comment has been minimized.

Contributor

scartmell-arm commented Oct 16, 2017

It's currently breaking in CI because some examples have mbed-app.json that apparently aren't handled by the schema.

Specifically, mbed-cloud-client-example-sources-internal broke during the CI build because it's configuration contains "mbed-trace.enable": null which is being rejected as invalid, though I'm not entirely sure why.

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 removed the needs: work label Feb 1, 2018

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Feb 1, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Feb 1, 2018

This is now ready for integration

@cmonr

This comment has been minimized.

Contributor

cmonr commented Feb 1, 2018

@scartmell-arm Could you rebase this PR one last time? Looks like a conflict was recently introduced.
I'll keep an eye to push it through CI once the rebase is completed.

scartmell-arm added some commits Sep 4, 2017

Add JSON schema based validation to mbed config script
- Added app and lib JSON schema definition files which specify the valid
  keys and values that can be used in mbed library and application
  configuration files. The primary different between the app and lib
  schema is that the lib config requires a name key.

- Modified the expected error code in some of the test cases. The error
  message is now issued by the JSON validator.

- Added some validation code to the config script which checks the
  validity of the mbed_app.json file when it is initially loaded.

- Added some validation code to config script which checks each of the
  mbed_lib.json scripts when they are loaded.

- Removed manual checks for allowable config keys from within the mbed_app
  and mbed_lib json files.

- Removed the check_dict_types() function which was no longer being
  called.
Remove config_path setting and amend regex in mbed config JSON schema
- Removed the config_path setting from schema and moved the addition of the
config_path value after the validation is done.

- Altered the macro validation regex to be more lenient. Now verifies that
  if '=' is used in the macro definition that something comes after it.
Refactor common JSON schema definitions into a seperate file
- Moved all common definitions from schema_app and schema_lib into a
  separate definitions file.

- Changed the calling code to resolve multiple schema files correctly.
Fix test errors produced by JSON schema after rebase
- Modified the expected error code in some of the test cases. The error
  message is now issued by the JSON validator.

- Stop validation code being called if no application configuration is
  given to validate.

- Change test mock code to check for specific calls, instead of number of
  calls.

- Derive absolute paths for JSON schema files before loading them, which
  seems to work better with the URI path resolution of the schema parser.

@scartmell-arm scartmell-arm force-pushed the scartmell-arm:mbed-config-json-schema branch from 1af6aaa to b1ea388 Feb 1, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Feb 1, 2018

/morph build

@cmonr cmonr added needs: CI and removed ready for merge labels Feb 1, 2018

@mbed-ci

This comment has been minimized.

mbed-ci commented Feb 1, 2018

Build : SUCCESS

Build number : 1043
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5022/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@theotherjimmy

I think the schema can be more restrictive, and therefore more helpful at pointing out errors.

"type": "object",
"patternProperties": {
"\\*": {
"type": "object",

This comment has been minimized.

@theotherjimmy

theotherjimmy Feb 1, 2018

Contributor

This should be the same as the pattern property below, "$ref": "#/target_override_entry"

"target_override_entry": {
"type": "object",
"patternProperties": {
"^\\S+$": {}

This comment has been minimized.

@theotherjimmy

theotherjimmy Feb 1, 2018

Contributor

This should contain "$ref": "#/config_parameter_short", I think

@theotherjimmy

Let's get this in, and I'll submit another PR to make it more strict. @cmonr If you could do the honors

@cmonr cmonr merged commit 860b66a into ARMmbed:master Feb 1, 2018

19 checks passed

ARM mbed CI Verification build successful.
Details
AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Local events testing has passed
Details
travis-ci/littlefs Local littlefs testing has passed
Details
travis-ci/mbed2-ATMEL Local mbed2-ATMEL testing has passed
Details
travis-ci/mbed2-MAXIM Local mbed2-MAXIM testing has passed
Details
travis-ci/mbed2-NORDIC Local mbed2-NORDIC testing has passed
Details
travis-ci/mbed2-NUVOTON Local mbed2-NUVOTON testing has passed
Details
travis-ci/mbed2-NXP Local mbed2-NXP testing has passed
Details
travis-ci/mbed2-RENESAS Local mbed2-RENESAS testing has passed
Details
travis-ci/mbed2-SILICON_LABS Local mbed2-SILICON_LABS testing has passed
Details
travis-ci/mbed2-STM Local mbed2-STM testing has passed
Details
travis-ci/tools Local tools testing has passed
Details

@cmonr cmonr removed the needs: CI label Feb 1, 2018

@scartmell-arm scartmell-arm deleted the scartmell-arm:mbed-config-json-schema branch Dec 5, 2018

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