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

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

Merged
merged 2 commits into from
May 22, 2024
Merged

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

merged 2 commits into from
May 22, 2024

Conversation

oddhack
Copy link
Contributor

@oddhack oddhack commented May 8, 2024

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

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
Copy link
Contributor Author

oddhack commented May 8, 2024

@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
Copy link

@oddhack confirmed that VVL is ok with this

@oddhack
Copy link
Contributor Author

oddhack commented May 8, 2024

@oddhack confirmed that VVL is ok with this

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

@spencer-lunarg
Copy link

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

@rg3igalia

@MarijnS95
Copy link
Contributor

@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
Copy link
Contributor Author

oddhack commented May 10, 2024

@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
Copy link
Contributor Author

oddhack commented May 10, 2024

@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 <enums> tag) causes any issues for CTS?

@rg3igalia
Copy link

@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
Copy link
Contributor

@oddhack the ash change needed to get this working is in krolli/vk-parse#32.

@oddhack
Copy link
Contributor Author

oddhack commented May 15, 2024

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
Copy link
Contributor Author

oddhack commented May 15, 2024

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

@MarijnS95
Copy link
Contributor

@oddhack looks like it did 🎉

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

@oddhack
Copy link
Contributor Author

oddhack commented May 16, 2024

@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
Copy link
Contributor

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.

@oddhack oddhack merged commit ce03761 into main May 22, 2024
21 checks passed
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.

Add type="constants" to the "API Constants" "enums" in vk.xml
4 participants