-
Notifications
You must be signed in to change notification settings - Fork 342
CI: Separate header gen and sample stages #2441
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
base: main
Are you sure you want to change the base?
Conversation
asuessenbach
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just two questions:
| -D CMAKE_CXX_STANDARD=${{matrix.env.cxx_max}} \ | ||
| -D VULKAN_HPP_GENERATOR_BUILD=ON \ | ||
| -D VULKAN_HPP_RUN_GENERATOR=$RUN_GENERATOR \ | ||
| -D VULKAN_HPP_DISPATCH_LOADER_DYNAMIC=ON \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you just build the generator and generate the headers, VULKAN_HPP_DISPATCH_LOADER_DYNAMIC should not be needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may not be needed, but the Vulkan::Hpp that will always be created uses find_package(Vulkan) if we do not enable the dynamic dispatcher. This suppresses warnings (and potential issues) when it cannot be found (CI), so I left it there.
.github/workflows/build.yml
Outdated
| -D VULKAN_HPP_RUN_GENERATOR=OFF \ | ||
| -D VULKAN_HPP_SAMPLES_BUILD=ON \ | ||
| -D VULKAN_HPP_TESTS_BUILD=OFF \ | ||
| -D VULKAN_HPP_TESTS_CTEST=ON \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is VULKAN_HPP_TESTS_CTEST on here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has no effect, but I can explicitly disable it for the sake of consistency
I had previously merged them to be concise, but separating these has two distinct time advantages:
This pretty much covers all the ideas I had for reducing these runner times and will (hopefully) be the last PR about them for a while..