Skip to content

Conversation

@alycm
Copy link
Contributor

@alycm alycm commented Sep 7, 2022

Ensure cl.xml passes validation, and add validation to CI to avoid regressions.

Some issues were fixed by changing cl.xml, some by changing registry.rnc. I tested that XML changes didn't break anything by ensuring the generated spec doesn't change, and that the re-generated extension headers didn't change.

I tested the CI change by leaving a validation in the XML at first and ensuring that the build failed.

Fixes #830

@alycm alycm marked this pull request as draft September 7, 2022 22:29
@alycm alycm force-pushed the 830-validate-xml branch 2 times, most recently from 0631802 to 3b5f91b Compare September 7, 2022 23:05
Enum |
element command {
Name ,
attribute requires { text } ? ,
Copy link
Contributor Author

@alycm alycm Sep 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I flip-flopped on whether this should be a schema change or an XML change. I landed on schema change because the single use of this pattern does seem valid.

It is for clEnqueueMigrateMemINTEL within cl_intel_unified_shared_memory which requires="CL_VERSION_1_2". In other cases it's the entire extension that requires an OpenCL version, but this instance is specified at a finer granularity.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I remember discussing various options for clEnqueueMigrateMemINTEL and settling on this as the best option, though I can't find the discussion in an issue so it may have been in a PR comment. I agree this could be a useful pattern going forward for other extensions, as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't see a discussion on this specifically, but here you say you settled on this to get things un-stuck.
And the requires="CL_VERSION_1_2" didn't exist on clEnqueueMigrateMemINTEL before #290 - checked with git blame.

@alycm alycm marked this pull request as ready for review September 7, 2022 23:24
@bashbaug bashbaug merged commit bc20c1e into KhronosGroup:main Sep 8, 2022
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.

cl.xml does not validate with registry.rnc

3 participants