Add some limited JSON Patch support for board description files. #423
Conversation
Boards can now be provided as .patch files that are JSON documents containing two nodes: - `base` is the board that this is based on, which can be another .patch file, a full path (note: relative paths are not yet supported, but they should be for out-of-tree BSPs). - `patch` is a JSON Patch. The patch is a subset of RFC 6902. Current limitations: - It doesn't handle escapes in keys or any weird keys. - It assumes that numbers in the JSON Pointer path are array indexes and will do the wrong thing if you try to replace or add an object field with a number as a key. - It doesn't implement the copy, move, or test operations, only add, remove, and replace. This is sufficient to be useful. The Arty A7 and Sonata Simulator JSON files are now replaced with patches.
davidchisnall
force-pushed
the
json-patch
branch
from
73939c0 to
72f5f87
Compare
|
|
||
| -- If a string value is a number, return it as number, otherwise return it | ||
| -- in its original form. | ||
| function asNumberIfNumber(value) |
There was a problem hiding this comment.
Hrm. This function isn't just tonumber(value) or value (numeric 0 is true in Lua, and tonumber won't return false) because we want to keep things Lua can't exactly represent as strings? I don't think we're likely to notice the difference in practice, but defense in depth is probably a good idea.
There was a problem hiding this comment.
I’m not sure I understand. JSON keys are strings for objects or numbers for dictionaries. Will this do the wrong thing if a key is called “true”?
There was a problem hiding this comment.
Lua's a strange beast. tonumber("true") or "true" is the string "true", not the boolean true, because nil or X evaluates to X unchanged, and X or Y with X neither nil or false evaluates to X.
There was a problem hiding this comment.
That is the desired behaviour. Booleans do not want this kind of special casing, only numbers (technically, only integers). But given the ability to accurately tell if a JSON thing is an array, I don’t think this function is needed.
| -- build so this doesn't really matter. | ||
| function isarray(t) | ||
| local i = 1 | ||
| for k, v in pairs(t) do |
There was a problem hiding this comment.
The manual sayeth "The order in which the indices are enumerated is not specified, even for numeric indices" (emphasis theirs). I think you want to test for the existence of a "border" at each k, and check that you've only seen one by the end of the loop. Of course, a non-numeric k means that this isn't a table backing a JSON array, even if Lua doesn't consider non-numeric keys to make a table a non-sequence.
xmake's JSON goo in particular exports an is_marked_as_array method that might be useful instead?
There was a problem hiding this comment.
Aha! It’s not documented, I expected something like that to exist. With that, we can just do the coercion of keys to numbers when the thing is an array, so that simplifies it a lot.
There was a problem hiding this comment.
It seemed like such a good idea. Unfortunately, it doesn't seem to work on my xmake build (I'm guessing it's using the built-in JSON parser rather than json-c and there's a bug).
There was a problem hiding this comment.
I rewrote it to use this function if it works and use a fallback if not. The fallback is correct, though potentially O(n) in terms of the underlying tree, rather than just the top-level array: it will convert the entire tree to JSON and then see if the first char is '['. If that function works, it's O(1).
Even with the fallback, execution time is negligible with the current examples in the tree, so I don't care too much.
| -- currently use those for anything. It also assumes that we really do | ||
| -- mean array indexes when we say numbers. If we have an object with | ||
| -- "3" as the key and try to replace 3, it will currently not do the | ||
| -- right thing. |
There was a problem hiding this comment.
The bit about numeric path components might be somewhat easy to solve with is_marked_as_array? Might not be worth doing yet, but just a comment with the name so we don't forget, perhaps.
| local boarddir, boardfile = board_file_for_target(target); | ||
| local board = load_board_file(json, boardfile) | ||
| print("Board file saved as ", target:targetfile()..".board.json") | ||
| json.savefile(target:targetfile()..".board.json", board) |
There was a problem hiding this comment.
Oh thank you. It's so obnoxious when build systems hide their computed state. :)
There was a problem hiding this comment.
It’s mostly there because I didn’t want to rewrite all of this logic for cheriot-audit (though, at least, the JSON library that it uses has JSON Patch support). This also means that we can use any JSON extensions that xmake supports and not worry because other tools can just use the output (which is plain.simple JSON).
There was a problem hiding this comment.
This looks right to me.
Boards can now be provided as .patch files that are JSON documents containing two nodes:
baseis the board that this is based on, which can be another .patch file, a full path (note: relative paths are not yet supported, but they should be for out-of-tree BSPs).patchis a JSON Patch.The patch is a subset of RFC 6902. Current limitations:
This is sufficient to be useful. The Arty A7 and Sonata Simulator JSON files are now replaced with patches.