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.

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

Please document JSON file format compatibility rules #248

Open
smcv opened this issue Sep 18, 2019 · 5 comments
Open

Please document JSON file format compatibility rules #248

smcv opened this issue Sep 18, 2019 · 5 comments
Labels
enhancement New feature or request
Milestone

Comments

@smcv
Copy link
Contributor

smcv commented Sep 18, 2019

Thanks for documenting the JSON file format specification - it's a useful resource, and I'm now trying to add a similar (but simpler) spec to GLVND for the EGL ICDs.

https://github.com/KhronosGroup/Vulkan-Loader/blob/master/loader/LoaderAndLayerInterface.md#icd-manifest-file-format documents that the current file format is 1.0.0, but does not document what formats a consumer can assume to be compatible with the current format.

Similar code in GLVND says this:

The minor version number will be incremented if we ever add an optional
value to the JSON format that libEGL has to pay attention to. That is,
an older vendor library will still work, but a vendor library with a
newer format than this library understands should fail.

https://github.com/NVIDIA/libglvnd/blob/master/src/EGL/libeglvendor.c#L279

If the intention is to have the same versioning policy as GLVND/EGL, perhaps something like this would be appropriate:

Versions 1.0.x are required to be compatible with this specification, in the sense that a consumer that only implements file format version 1.0.0 will load all version 1.0.x JSON files successfully (a new micro version may add new fields but will not break compatibility with existing loaders). Newer major and minor versions might require loader changes.

(I'm not sure what the intended distinction is between major and minor versions, if an increment to the minor version is allowed to include incompatible changes.)

@smcv
Copy link
Contributor Author

smcv commented Sep 18, 2019

Or, trying to rephrase that in a way that is maybe more suitable for both layers and ICDs:

It is safe for a consumer to load manifest files with a major and minor version that it understands, and a micro version that is newer that the consumer understands (micro versions will not introduce incompatible changes that require the loader to be changed). It is not safe for a consumer to load manifest files with a (major, minor) version pair that it does not understand (major or minor versions may introduce incompatible changes that require loader changes).

@lenny-lunarg
Copy link
Contributor

I think I see what you're saying. The statement:

There has only been one version of the ICD manifest files supported. This is version 1.0.0.

is a little strange given that the version is the same for the layer and ICD manifests (and they can be combined). We could change that language. I would think we should go with something along the lines of:

The ICD manifest format has not been modified by any version file_format_version. Any version that that is legal for a layer is legal for an ICD. Furthermore, the file_format_version will not affect the ICD.

Does that sound good to you?

@smcv
Copy link
Contributor Author

smcv commented Sep 19, 2019

For some context that I should probably have mentioned first, I'm working on some code to pick up Vulkan ICDs from the host system outside a container, and make them available inside the container. To do that, I need to be able to enumerate ICDs in the same way the loader would, and work out which shared library they refer to, without actually loading them (which is why I'm not using the reference loader itself) - and I need to be able to distinguish between JSON files that it is safe for me to load (those where I can assume that the format is compatible with 1.0.0), and JSON files that I can't assume I'm understanding correctly (because they were written to a future version of the specification).

The rule that's documented in GLVND/EGL is that a 1.0.0 parser can safely read any 1.0.x JSON file (where 1.0.x is allowed to add new non-critical fields, but only if a 1.0.0 parser can gracefully degrade by ignoring them), whereas a JSON file labelled as format 1.1.x or 2.0.x is allowed to contain incompatible changes (fields that would cause broken behaviour if ignored, or fields that changed their meaning) that would break a 1.0.0 consumer, so 1.0.0 consumers must consider 1.1.x or 2.0.x JSON files to be an error. What I was hoping for is a similar compatibility/incompatibility rule for Vulkan ICD JSON files.

I'm not currently looking at layers in any detail, but it would probably make sense to have a similar compatibility/incompatibility rule for layer JSON files, and it would probably make things simpler if it is literally the same rule.

Or when you say "Furthermore, the file_format_version will not affect the ICD", do you mean the rule is that for ICDs specifically, it's safe for a 1.0.0 parser to load any ICD JSON file, regardless of file_format_version? (That would mean that future format versions in ICD JSON can add new fields, as long as it would be valid for an existing consumer to fall back by ignoring them - but they cannot change the meaning of an existing field, or add new fields where it would be wrong for a consumer to ignore the new field.)

@KarenGhavam-lunarG KarenGhavam-lunarG added the enhancement New feature or request label Dec 2, 2019
@lenny-lunarg lenny-lunarg added this to the P2 milestone Apr 3, 2020
@charles-lunarg
Copy link
Collaborator

charles-lunarg commented Oct 11, 2022

Apologies for not responding.

Yes, the general pattern for ICD manifests is that 1.0.x means only optional fields are to be added to the manifest format. Currently, there are two optional fields, one for specifying if the driver is a 'portability driver' and one for the architecture of the driver (32 or 64 bit).

I'm trying to think of a resolution to the issue, would documentation that the stated intend of versioning is to follow the above pattern?

@charles-lunarg
Copy link
Collaborator

I'm looking into resolving this issue, and the fact that maintaining compatibility between versions has not been consistent makes it more difficult.

For example, Layer version 1.0.1 allows manifests to contain a node called "layers" containing an array of individual layers, rather than a single "layer" node. That means any consumer that only knows about 1.0.0 would be unable to use manifest files from 1.0.1 using the "layers" node. Incrementing the patch version certainly shouldn't cause such breakages but here it has. (Although, the 1.0.1 came out in 2016/2017, is easy enough to implement, so this only not a major concern).

Driver (ICD) manifests are much better in this regard. 1.0.0 and 1.0.1 are the only versions and the newer one adds optional components that can safely be ignored by 1.0.0 consumers.

A parallel effort to this is PR #1273 which adds schema files for each manifest version (both layer & driver). This greatly enhances the ability to define what is in each version, but it does not address whether consumers can use manifests from versions newer than what they were compiled with.

I'm thinking of using the following language to define the compatibility.

The versioning scheme used is Major.Minor.Patch. Breaking changes to the format must only occur when incrementing the Major version number. Breaking changes may add new fields which are required, remove fields, modify the format in a non-backwards compatible manner. Minor and Patch versions may add new optional fields. This scheme is only applicable for layer versions 1.0.1 and upwards, as 1.0.1 made a non-compatible change to the interface.

Criticism & comments are welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants