search

KhronosGroup / Vulkan-Docs

The Vulkan API Specification and related tools
Other
2.7k stars 452 forks source link

Add a "constants" type= attribute to <enums> tags #2366

Closed oddhack closed 1 month ago

oddhack commented 2 months ago

For more consistency with handling other enums type= attributes.

This is purely additive to the schema but it is conceivable that downstream scripts would look for this attribute not being present to indicate compile-time constants, so this should be checked against CTS / VVL / Vulkan-Hpp binding / Rust binding usage.

Closes #2359

oddhack commented 2 months ago

@MarijnS95 this changes seems to have triggered something bad in ash-rs - could you take a look at it? This PR is an experiment to see if the request in #2359 can be implemented without annoying downstream consequences.

spencer-lunarg commented 2 months ago

@oddhack confirmed that VVL is ok with this

oddhack commented 2 months ago

@oddhack confirmed that VVL is ok with this

Thanks! Do you know who I could ask to check CTS?

spencer-lunarg commented 2 months ago

Thanks! Do you know who I could ask to check CTS?

@rg3igalia

MarijnS95 commented 2 months ago

@oddhack it looks like it is skipping generation of all the constants. Unfortunately I just left the country and will only be back on Monday to investigate and address this, if that's not too long.

oddhack commented 1 month ago

@oddhack it looks like it is skipping generation of all the constants. Unfortunately I just left the country and will only be back on Monday to investigate and address this, if that's not too long.

Thanks. That's fine - this is not urgent.

oddhack commented 1 month ago

@rg3igalia would it be possible for you to check this branch to see if the XML change (adding a type="constants" to the API constants \ tag) causes any issues for CTS?

rg3igalia commented 1 month ago

@rg3igalia would it be possible for you to check this branch to see if the XML change (adding a type="constants" to the API constants tag) causes any issues for CTS?

I just checked and CTS code generation continues to work normally. The generated code also compiles fine.

MarijnS95 commented 1 month ago

@oddhack the ash change needed to get this working is in https://github.com/krolli/vk-parse/pull/32.

oddhack commented 1 month ago

I believe this is ready to merge. VVL and CTS have verified no impact, Vulkan-Hpp is causing no issues, and Ash has a fix queued up.

oddhack commented 1 month ago

@MarijnS95 thanks, re-running CI to see if that will fix the problem.

MarijnS95 commented 1 month ago

@oddhack looks like it did :tada:

In the future there's a "re-run CI" button, with some nice "Run number #x" UX.

oddhack commented 1 month ago

@oddhack looks like it did 🎉

In the future there's a "re-run CI" button, with some nice "Run number #x" UX.

I did initially re-run the failing job, which continued to fail, making me think the dependency had succeeded but generated incorrect code for the build-test job. Could probably have re-ran all jobs with equal effect to pushing the empty commit, true.

MarijnS95 commented 1 month ago

making me think the dependency had succeeded but generated incorrect code for the build-test job.

Correct, the generator succeeded but didn't find any constants by looking for <enums> without type attribute (since it's type="constants") now, so omitted generating them, resulting in compilation failures in the second step.