examples: fix shader-coverage demo portability#11623
Conversation
The two shader-coverage demos relied entirely on find_package(Vulkan) for both the loader and headers, and skipped themselves if either was missing. This left them unbuildable on machines that ship only the Vulkan runtime loader (no -dev/SDK package) and on older Vulkan headers. - CMakeLists: source headers from Slang's bundled Vulkan::Headers target, and fall back to locating the runtime loader (libvulkan.so.1) when find_package(Vulkan) is unavailable, instead of skipping the example. - vk_compute_demo.cpp: guard the VK_KHR_portability_enumeration references behind #if defined so headers older than 1.3.x still compile.
|
Caution Review failedAn error occurred during the review process. Please try again later. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughBoth ChangesVulkan Compatibility Hardening
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d96819e4-72bd-4c5b-b08a-e82fca89c54d
📒 Files selected for processing (4)
examples/shader-coverage-bvh-traversal/CMakeLists.txtexamples/shader-coverage-bvh-traversal/vk_compute_demo.cppexamples/shader-coverage-image-pipeline/CMakeLists.txtexamples/shader-coverage-image-pipeline/vk_compute_demo.cpp
jvepsalainen-nv
changed the title
jvepsalainen-nv
left a comment
jvepsalainen-nv
left a comment
There was a problem hiding this comment.
Claude Sonnet 4.6-authored clarity review:
This is a focused, well-described build-compatibility fix. The PR description matches the diff, the changes are symmetric across both examples, and the new comment blocks generally explain the approach well. Four clarity gaps remain in the newly added code:
Immediate fix: The image-pipeline CMakeLists still has a file-level comment saying "Requires Vulkan SDK at build time for the loader (libvulkan)." The PR's purpose is exactly the opposite—the SDK is now optional—so that line is now a direct contradiction of the new fallback logic.
Medium priority: The find_library NAMES list (vulkan vulkan-1 libvulkan.so.1) has a non-obvious entry: libvulkan.so.1 works as an exact-filename lookup in CMake (bypassing the normal lib/.so wrapping) to find the versioned soname on systems without a .so development symlink. Without a note, a future maintainer could mistake it for a typo or not understand why vulkan alone is insufficient. Similarly, _vulkan_loader can hold either an imported CMake target (with transitive include dirs) or a bare filesystem path (without), and the correctness of the fallback depends on Vulkan::Headers always being present in the link line—that invariant is implicit.
Lower priority: The portability-enumeration comment says "the runtime detection runs wherever the extension is available," but "available" is ambiguous: the detection loop is inside the #if guard, so it only runs when the header defines the macro, not wherever the GPU runtime supports the extension. On old headers + portability-capable GPU, portability enumeration is silently not requested. A few word changes would close that gap.
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
| // VK_KHR_portability_enumeration is only defined by Vulkan headers >= 1.3.x. | ||
| // Guard the references so older headers (e.g. on some Linux CI runners) still | ||
| // compile; the runtime detection runs wherever the extension is available. | ||
| #if defined(VK_KHR_PORTABILITY_ENUMERATION_EXTENSION_NAME) |
There was a problem hiding this comment.
Is the example still going to work/function without this extension?
If yes, why do we need to use the extension?
If no, should we stop the execution here when the extension is not available?
This code change looks like a temporary bandaid rather than a proper fix.
There was a problem hiding this comment.
The extensions are needed for macOS compatibility. The #if defined() guards were placed at the wrong layer. They fixed the compile error but silently stripped the runtime portability detection on old-header machines, which would have broken macOS/MoltenVK with an SDK < 1.3.x.
The #if defined(VK_KHR_PORTABILITY_ENUMERATION_EXTENSION_NAME) guards fixed the compile error on old Vulkan SDK headers but silently removed the runtime portability detection on those headers — breaking macOS / MoltenVK on any machine with an SDK older than 1.3.x. The correct fix is to define local fallback constants when the header doesn't provide them. Both values are fixed by the Vulkan spec, so hardcoding them as fallbacks is safe across all SDK versions. The runtime detection and flag assignment are now always compiled and always run; on old-header Linux the strcmp never matches at runtime (the extension isn't present there), so hasPortabilityEnum stays false and behaviour is unchanged. Also removes the (void)hasPortabilityEnum bandaid that the usage guards had introduced.
| if(TARGET Vulkan::Vulkan) | ||
| set(_vulkan_loader Vulkan::Vulkan) | ||
| else() | ||
| find_library(SLANG_VULKAN_LOADER NAMES vulkan vulkan-1 libvulkan.so.1) |
There was a problem hiding this comment.
The reported problem was a build failure.
I don't understand why we need to find VulkanSDK files to resolve the compile time errors.
Slang already ahs external/SPIRV-{headers,tools} directories to compile any spirv/vulkan related compilation.
We shouldn't need to find files from vulkan sdk for compilation error.
The two shader-coverage demos relied entirely on find_package(Vulkan) for both the loader and headers, and skipped themselves if either was missing. This left them unbuildable on machines that ship only the Vulkan runtime loader (no -dev/SDK package) and on older Vulkan headers. - CMakeLists: source headers from Slang's bundled Vulkan::Headers target, and fall back to locating the runtime loader (libvulkan.so.1) when find_package(Vulkan) is unavailable, instead of skipping the example. - vk_compute_demo.cpp: guard the VK_KHR_portability_enumeration references behind #if defined so headers older than 1.3.x still compile.
3 participants
The two shader-coverage demos relied entirely on find_package(Vulkan) for both the loader and headers, and skipped themselves if either was missing. This left them unbuildable on machines that ship only the Vulkan runtime loader (no -dev/SDK package) and on older Vulkan headers.