-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
nlohmann::json instance and JSON Schema for MemorySourceAccessor
#14319
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: master
Are you sure you want to change the base?
Conversation
src/json-schema-checks/meson.build
Outdated
| '--map', | ||
| './hash-v1.yaml=' + schema_dir / 'hash-v1.yaml', |
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.
wasn't needed
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.
This seemed so simple, but bytes strike again.
| ../../src/libutil-tests/data/memory-source-accessor | ||
| ../../src/libutil-tests/data/hash |
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.
Maybe these should point the other way around?
Being a documentation example is more significant than being tested as part of some test suite.
Also it's helpful for the test suite to have an explicit reference to path that's in the manual, to distinguish it from test cases that only exist for the purpose of testing, i.e. corner cases that aren't relevant enough for the manual.
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 switched it from your version because of how _NIX_TEST_ACCEPT works, especially when making new files.
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.
Can it not write through a symlink? IIRC I had something like tests/data/examples point into the manual. Wouldn't it just write to data/examples/new-file if you passed examples/new-file as the stem?
Otherwise, I guess we could use a naming convention to highlight which examples are for docs.
| contents: | ||
| type: object | ||
| description: | | ||
| Map of names to nested file system objects (for type=directory) |
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.
There's probably semantics we want to document here, like we don't allow /?
Probably best to make a link, so that we don't duplicate all prose here.
I guess we could encode a no / restriction with draft 6 propertyNames if we want.
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.
Names may not be valid UTF-8 and therefore not be valid JSON (sequence of unicode characters, https://www.rfc-editor.org/rfc/rfc8259#section-1)
One possible solution, if this is something we've tested that it even works:
"It is commonly accepted that file names should be UTF-8, but not guaranteed.
Nix will forward invalid names, so if you wish to support all possible NARs in your application, you will need to use a parser that forwards invalid UTF-8 into your application, and allows you to produce invalid UTF-8 in the JSON you may have to output."
Alternatively, we could require base 64 here too.
Same applies to the symlink targets.
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.
Or we could have a LenientString type that's either a valid JSON string, or a { "base64": ... }, but that only helps for values, not field names. I guess we could steal syntax and have "/aGVsbG8=": be an equivalent, non-canonical field name to "hello":.
So LenientString would be a type for representing values that are typically valid strings, but may not always be, and base 64 would be an unwarranted hindrance in practice. (Not sure about the latter; maybe we should just use it for file contents too?)
| const: "symlink" | ||
| target: | ||
| type: string | ||
| description: Target path of the symlink. |
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.
Another lack of semantics. Should probably just be a link.
What is the meaning of an absolute path? Or a relative path? Can a relative path be allowed to ..-leave the FSO? I don't think the manual has good answers yet, but this should link to what we have, @docroot@/store/file-system-object.md
Progress on NixOS#13405, which asks for an explicit characterisation of the equivalence relation like the one given here. Also progress on NixOS#11895, because we're using the term "build trace entry" instead of "realisation". Mention NixOS#9259, a future work item. Co-authored-by: Robert Hensing <[email protected]>
Note, starting to make progress on NixOS#11895 by calling it this in the manual.
Progress on NixOS#13570. If we depend on the store dir, our JSON serializers/deserializers take extra arguements, and that interfaces with the likes of various frameworks for associating these with types (e.g. nlohmann in C++, Serde in Rust, and Aeson in Haskell).
This makes the proto serializer characterisation test data be accompanied by JSON data. This is useful because the JSON data is human-readable while the binary data is not.
As documented, this for file system objects themselves, since `MemorySourceAccessor` is an implementation detail.
9afb7e5 to
4d3d4b5
Compare
Motivation
As documented, this for file system objects themselves, since
MemorySourceAccessoris an implementation detail.Context
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.