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

generator: Demote vk-parse error-asserts to a warning message #960

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MarijnS95
Copy link
Collaborator

Fixes #959

When vk-parse encounters XML parsing errors (e.g. on unknown attributes or elements) it skips the element and emits an error in a list of errors that are returned to the caller. We were originally never checking these leading to hard-to-debug issues, but eventually started asserting on this list being empty or otherwise panicking in #930.

Since ash's generator is used in Khronos' upstream CI (in a fallible yet warning way), any new attribute would cause the generator to fail and subsequently require updates to vk-parse and ash before their CI is no longer flagged as "succeeded with warnings". "Solve" this by once again allowing errors to exist, while still at least printing them to stderr for insights (that were previously missing...) to anyone running the generator.

Note @oddhack that this once again allows the generator to fail further in the chain when expected elements are missing, in which case an update is required either way. If this happens often we can adjust vk-parse to be more lenient in still emitting errors but continuing to parse said element, if the new/unrecognized elements or attributes are considered harmless to the parsing process.

When `vk-parse` encounters XML parsing errors (e.g. on unknown attributes
or elements) it skips the element and emits an error in a list of errors
that are returned to the caller.  We were originally never checking
these leading to hard-to-debug issues, but eventually started asserting
on this list being empty or otherwise panicking in #930.

Since `ash`'s generator is used in Khronos' upstream CI (in a fallible
yet warning way), any new attribute would cause the generator to fail
and subsequently require updates to `vk-parse` and `ash` before their
CI is no longer flagged as "succeeded with warnings".  "Solve" this by
once again allowing errors to exist, while still at least printing them
to `stderr` for insights (that were previously missing...) to anyone
running the generator.

Note that this once again allows the generator to fail further in the
chain when expected elements are missing, in which case an update is
required either way.  If this happens often we can adjust `vk-parse` to
be more lenient in still emitting errors but continuing to parse said
element, if the new/unrecognized elements or attributes are considered
harmless to the parsing process.
@oddhack
Copy link

oddhack commented Nov 13, 2024

Thanks. It's very rare that new attributes are not harmless to existing uses, exactly because there are downstream consumers that can take a very long time to update their XML use to account for it. Mostly they are additional metainformation for our own generators used to create spec artifacts and bits of CTS and validation layer code. Sometimes there are new tags allowed as well, same thing - for example <feature> tags inside <require> blocks are a recent addition.

@MarijnS95
Copy link
Collaborator Author

Exactly, if I remember correctly that might have been one of the parser errors where a <feature> element inside a <require> element may have skipped the entire <require> element, leading to a broken generator further down the line.

If that happens we should make vk-parse more lenient by skipping the unknown <feature> element but still parsing the rest of the content.

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

Successfully merging this pull request may close these issues.

Ignoring unrecognized attributes (redux)
2 participants