-
Notifications
You must be signed in to change notification settings - Fork 150
project: Replace pykwalify with jsonschema #904
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #904 +/- ##
==========================================
- Coverage 85.95% 85.75% -0.20%
==========================================
Files 11 11
Lines 3453 3440 -13
==========================================
- Hits 2968 2950 -18
- Misses 485 490 +5
|
4104153 to
b4de220
Compare
marc-hb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Can this catch quote strings that look like numbers in YAML files and force them to quote them?
Just curious because we just saw two instances of this back to back in Zephyr.
I didn't look at the PR yet sorry.
| -------------------------------------------------------------- | ||
| DO NOT CHANGE THIS FILE WITHOUT UPDATING THE DOCUMENTATION! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you update the documentation (since name is now required)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It "was" already required, as it would throw an error if users did not provide it.
But we probably should bump the manifest version (and docs), so that users could keep compatibility with versions that depend on pykwalify. Will update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But in order for users to target a version without the tooling change,
we need to bump the schema version.
I don't understand this sorry. Can you elaborate the use case?
If the schema is 100% compatible, why would the schema version be bumped?
MAINTAINERS.rst: Decide if west.manifest.SCHEMA_VERSION needs an update:
MAINTAINERS.rst-
MAINTAINERS.rst: - SCHEMA_VERSION should be updated to X.Y if release vX.Y will have manifest
MAINTAINERS.rst- syntax changes that earlier versions of west cannot parse.
MAINTAINERS.rst-
MAINTAINERS.rst: - SCHEMA_VERSION should *not* be changed for west vX.Y if the manifest
MAINTAINERS.rst- syntax is fully compatible with what west vX.(Y-1) can handle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But in order for users to target a version without the tooling change,
we need to bump the schema version.I don't understand this sorry. Can you elaborate the use case?
If the schema is 100% compatible, why would the schema version be bumped?
MAINTAINERS.rst: Decide if west.manifest.SCHEMA_VERSION needs an update: MAINTAINERS.rst- MAINTAINERS.rst: - SCHEMA_VERSION should be updated to X.Y if release vX.Y will have manifest MAINTAINERS.rst- syntax changes that earlier versions of west cannot parse. MAINTAINERS.rst- MAINTAINERS.rst: - SCHEMA_VERSION should *not* be changed for west vX.Y if the manifest MAINTAINERS.rst- syntax is fully compatible with what west vX.(Y-1) can handle.
The schema file has an entirely different syntax so... Yeah :)
It's the semantics that's meant to remain identical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The manifest syntax is the same though? I get Marc's point, but I think it would be beneficial to have different (minor) versions in this case? Not sure how to proceed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW in Zephyr I ended up picking a different name for the new manifest (one was .yml the other .yaml) - would doing something similar help?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did the same here :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I submitted some SCHEMA_VERSION clarifications in PR #909. I hope they explain why I think this should not be bumped.
FWIW in Zephyr I ended up picking a different name for the new manifest (one was .yml the other .yaml)
You meant the schema.y[a]ml, right? Not the west.yml manifest which should hopefully not be affected.
If I got this correctly, then I don't see how a mere rename of the schema file affects the SCHEMA_VERSION. It would be a very different story if pykwalify and jsonschema were not 100% compatible with each other! But I don't think the mere name of the schema file matters here in any case.
Not really, no. The version property, for example, currently allows both string and number, to support something like: version: 0.8Is this a problem? I think we would break a lot of users if we take this away? EDIT: Users could already be forced by setting the type to |
It depends where. I didn't mean "everywhere always", sorry for the confusion. It's not a problem at all where the code is ready and expects both. But it has recently been a problem in Zephyr in two places (zephyrproject-rtos/zephyr#101642 and zephyrproject-rtos/zephyr#101888 (comment)) where the code expects a string and the user (or file generator) entered a string that unfortunately looks like a valid number.
Again, it depends where. This new parser can also save users time by "failing fast" and giving them a clear error message that they must quote the strings that look like numbers, instead of some obscure stack trace much later. |
|
So let's pretend for instance that:
This is exactly what just happened in Zephyr and it could happen anywhere in west too. In that case, it would be a better experience that Can |
| { url = "https://files.pythonhosted.org/packages/15/18/b0e1fafe59051de9e79cdd431863b03593ecfa8341c110affad7c8121efc/ruamel.yaml.clib-0.2.14-cp314-cp314-musllinux_1_2_x86_64.whl", hash = "sha256:e7cb9ad1d525d40f7d87b6df7c0ff916a66bc52cb61b66ac1b2a16d0c1b07640", size = 764456, upload-time = "2025-09-22T19:51:11.736Z" }, | ||
| { url = "https://files.pythonhosted.org/packages/e7/cd/150fdb96b8fab27fe08d8a59fe67554568727981806e6bc2677a16081ec7/ruamel_yaml_clib-0.2.14-cp314-cp314-win32.whl", hash = "sha256:9b4104bf43ca0cd4e6f738cb86326a3b2f6eef00f417bd1e7efb7bdffe74c539", size = 102394, upload-time = "2025-11-14T21:57:36.703Z" }, | ||
| { url = "https://files.pythonhosted.org/packages/bd/e6/a3fa40084558c7e1dc9546385f22a93949c890a8b2e445b2ba43935f51da/ruamel_yaml_clib-0.2.14-cp314-cp314-win_amd64.whl", hash = "sha256:13997d7d354a9890ea1ec5937a219817464e5cc344805b37671562a401ca3008", size = 122673, upload-time = "2025-11-14T21:57:38.177Z" }, | ||
| { url = "https://files.pythonhosted.org/packages/06/0c/0c411a0ec64ccb6d104dcabe0e713e05e153a9a2c3c2bd2b32ce412166fe/rpds_py-0.30.0-cp310-cp310-macosx_10_12_x86_64.whl", hash = "sha256:679ae98e00c0e8d68a7fda324e16b90fd5260945b45d3b824c892cec9eea3288", size = 370490, upload-time = "2025-11-30T20:21:33.256Z" }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could wheels be upgraded in a separate, routine PR to make the dependency changes reviewable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's automated when packages are added/removed, where uv needs to resolve dependencies. I can't move to a separate PR, but I've split the commits as much as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's automated when packages are added/removed, where uv needs to resolve dependencies.
OK but how come switching from pkwalify to jsonschema changes the wheels version?
I can't move to a separate PR, but I've split the commits as much as possible.
Good enough.
Add the jsonschema package as a project dependency and update uv.lock. Signed-off-by: Pieter De Gendt <[email protected]>
Both The same can be said for the |
Thanks! A change about a particular property would be outside the scope of this issue, agreed. I was looking for examples to illustrate my point. I agree this PR should not make a schema change at the same time. On the other hand, gaining or losing that sort of validation capability in general would definitely be in the scope of this PR! That's what I've been wondering the whole time. I understand you tested that and made sure user feedback is (still) good when jsonschema catches and reports a string that looks like a (floating?!?) number? |
Which property are you referring to? If you refer to the |
Again, I'm just looking for examples, I'm not interested in any particular property. Apologies for the too deep digression on This is about testing
I wasn't focused on "version". I took a quick look at Also, I'm interested in the interactive feedback when validation failed. It's not realistic to expect automated testing to guarantee a "user experience" (We're lucky enough when there is any test coverage at all for error handling...) So, I just went ahead and tested this myself; before and after this PR. As you predicted, the experience is OK in both cases. Neither pkwalify nor jsonschema suggests quoting numbers but they are close enough: zephyr]$ git diff
diff --git a/west.yml b/west.yml
index a11238717de4..4a53b99fa445 100644
--- a/west.yml
+++ b/west.yml
@@ -16,10 +16,10 @@
manifest:
defaults:
- remote: upstream
+ remote: 12345
remotes:
- - name: upstream
+ - name: 12345
url-base: https://github.com/zephyrproject-rtosWith with As expected, Note |
- Convert pykwalify schema with jsonschema - Remove some manual testing in Python code with schema validation - Make name property required in west extension commends (would have raised a KeyError before if it was missing) Signed-off-by: Pieter De Gendt <[email protected]>
Remove the pykwalify package dependency from the project and update uv.lock. Signed-off-by: Pieter De Gendt <[email protected]>
We can update the check for empty group filters using jsonschema instead of Python code. Signed-off-by: Pieter De Gendt <[email protected]>
Move mutual exclusive remote/url and repo-path/url to jsonschema. Signed-off-by: Pieter De Gendt <[email protected]>
While the schema validation tooling has changed, the schema validation itself hasn't. But in order for users to target a version without the tooling change, we need to bump the schema version. Signed-off-by: Pieter De Gendt <[email protected]>
Updated the error message so it now provides more details: $ west update
FATAL ERROR: Malformed manifest file: /home/pdgendt/zephyrproject/zephyr/west.yml
Schema file: /home/pdgendt/west/src/west/manifest-schema.yaml
Hint: 12345 is not of type 'string'
Failed validating 'type' in schema['properties']['defaults']['properties']['remote']:
{'type': 'string'}
On instance['defaults']['remote']:
12345 |
marc-hb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have any alternative to suggest and the interwebs don't seem to have any either but I find it sad to discover that jsonscheme seems to introduce the very first, 3rd party, mandatory binary dependency for west, correct? This looks like a transparency and security regression to me. The 100+ lines of unreviewable checksums and git pollution in uv.lock is the icing on the cake :-(
| submodules: | ||
| oneOf: | ||
| - type: boolean | ||
| - type: array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice but that does not seem to match your "status quo" intention, does it?
Move to a separate commit?
| additionalProperties: false | ||
| properties: | ||
| path: | ||
| type: ["string", "null"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why "null"? Does this mean empty?
| - type: array | ||
| items: | ||
| type: string | ||
| - $ref: "#/$defs/import-object" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above, a bit cryptic, add a comment?
| # If present, a list of project groups to enable and disable. Prefix | ||
| # a group name with "-" to disable it; prefix with "+" to enable it. | ||
| group-filter: | ||
| $ref: "#/$defs/groups" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this just a textual replacement or something smarter? I find the syntax quite cryptic, can you add a one-line comment?
| items: | ||
| type: string | ||
|
|
||
| groups: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this have to be a the bottom? It was at the top with pkwalify and moving it makes the side-by-side (and: tedious) comparison significantly more time-consuming.
| userdata: {} | ||
|
|
||
| $defs: | ||
| import-object: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this in pkwalify? I could not find it.
Is this a deal-breaker? |
No. Do we have a choice anyway? As you found, pkwalify seems orphaned and there is no alternative, is there? |
|
Actually, could we make validation optional? Something like: try:
import jsonschema
try:
jsonschema.validate()
...
expect ImportError
self.wrn("f{manifest} not validated, UNSUPPORTED do not report bugs")I wouldn't suggest making this optional if west were used by Zephyr only. But |
As found in the review of zephyrproject-rtos#904 (migration from pkwalify to jsonschema), the schema version check can be confusing. - Add a couple sentences in MAINTAINERS.rst to clearly state what this version check actually performs and achieves and when (not) to bump the version. - Rename "min_version" to "manifest_version" and swap the perspective "min" and "max" are relative terms, ambiguous when losing track of the point of view. "min_version" stood for "minimum_west_version_needed_to_read_this_manifest". But this is the manifest perspective, which is confusing when reading west code which is the opposite point of view. A given _version_ of west code is never going to change when reading it or running it! So, flip the perspective and look at things from the west point of view when in the west code: rename the also vague _SCHEMA_VER to _MAX_SUPPORTED_SCHEMA_VER. Signed-off-by: Marc Herbert <[email protected]>
bjarki-andreasen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need this sooner than later, zephyr is currently failing to west packages pip --install because its missing jsonschema
The issue is larger, the |
I don't think this would have Anyway zephyrproject-rtos/zephyr#103671 made neither
... which now looks like a lot like zephyrproject-rtos/zephyr@a0a71dabf8401ab4d :-) Can we have the same here? |
KeyErrorbefore if it was missing)Fixes #807