Vulkan header mismatch failures with statically linked (non-standalone) nvrhi library

[SRSaunders]

Unless I disable precompiled headers in certain files (e.g. DeviceManager_VK.cpp), I experience Vulkan header mismatch failures when building RBDoom3BFG on linux. On Arch linux (manjaro) at least, Vulkan-Headers is installed by the package manager as a dependency for the Vulkan SDK. RBDoom3BFG pulls in the SDK which is ahead of the Vulkan-Headers version included with nvrhi. Up to this point I have been just living with this and disabling pch for files that require vulkan.hpp (i.e. DeviceManager_VK.cpp). However, today I decided to dive into the cmake processing to determine where things are going wrong. I found the following in nvrhi's main CMakeLists file:

if (NVRHI_WITH_VULKAN)
    add_subdirectory(thirdparty/Vulkan-Headers)
endif()

...

if (NVRHI_WITH_VULKAN)
    ...
    target_link_libraries(${nvrhi_vulkan_target} Vulkan-Headers)
endif()

This seems to be pulling in nvrhi's version which may conflict with the application's own use of Vulkan, especially if a newer Vulkan-Headers is installed elsewhere for the SDK. I am wondering whether you should only pull in Vulkan-Headers as a linked library if nvrhi is being built for standalone use vs. static linking, i.e. should the target_link_libraries line be guarded by NVRHI_BUILD_SHARED?

[apanteleev]

We usually just rely on NVRHI's Vulkan-Headers to build the entire project, but I can see how you might want to use the system one or just a different version. So, I don't think it's a good idea to just rely on NVRHI_BUILD_SHARED to decide that. I can suggest two options:

1. Change the first snippet in your post into

if (NVRHI_WITH_VULKAN AND NOT TARGET Vulkan-Headers)
    add_subdirectory(thirdparty/Vulkan-Headers)
endif()

This way, NVRHI won't use its headers when another version is available via a previously defined CMake target.

2. Add a cached variable like NVRHI_VULKAN_HEADER_DIR that will override the included copy of Vulkan-Headers with a custom path, when specified. Not sure if it should be processed through add_subdirectory hoping that the target will be defined there, or just by doing target_include_directories(${nvrhi_vulkan_target} PUBLIC "${NVRHI_VULKAN_HEADER_DIR").

Let me know what you think is better.
In the meantime, I just updated the included Vulkan-Headers submodule to the latest tagged version (1.3.277).

[SRSaunders]

Thanks for your thoughts on this. I would like to test out your ideas first and come back with some feedback - hopefully by tomorrow (done). The complexity is that environments are different on the three platforms: Windows and macOS only depend on the Vulkan SDK to be installed with no separate Vulkan-Headers install or target definition. On linux, the SDK itself can depend on system-installed Vulkan-Headers so things may be different there.

And the conflict is not arising from the add_subdirectory part, but instead the target_link_libraries part where the nvrhi version of vulkan.hpp is included in the build and causes problems. Here is the specific error (line 40 is where DeviceManager_VK.cpp includes <vulkan/vulkan.hpp>):

In file included from .../DeviceManager_VK.cpp:40:
.../nvrhi/thirdparty/Vulkan-Headers/include/vulkan/vulkan.hpp:118:34: error: static assertion failed: Wrong VK_HEADER_VERSION!
  118 | static_assert( VK_HEADER_VERSION ==  238, "Wrong VK_HEADER_VERSION!" );
      |                                  ^
.../nvrhi/thirdparty/Vulkan-Headers/include/vulkan/vulkan.hpp:118:34: note: the comparison reduces to ‘(274 == 238)’
ninja: build stopped: subcommand failed.

As I mentioned above, if I remove DeviceManager_VK.cpp from the PCH list, then nvrhi's Vulkan-Headers and its version of vulkan.hpp are excluded from the precompiled.gch file and I see no build errors. However, this seems like a fragile thing and the solution should be more robust. As an alternative experiment, if I comment out nvrhi's target_link_libraries line, the conflict is also removed and no build errors are observed.

Getting back to your solution options above:

1. NOT TARGET Vulkan-Headers will not work in my case since Vulkan-Headers is not defined as a target within RBDoom3BFG's build system
2. add_subdirectory(${NVRHI_VULKAN_HEADER_DIR}) would not work in my case since there is no target there, only the SDK's include directory or the system Vulkan install (e.g. /usr/include/vulkan)
3. Using target_include_directories(${nvrhi_vulkan_target} PUBLIC "${NVRHI_VULKAN_HEADER_DIR") instead of target_link_libraries(${nvrhi_vulkan_target} Vulkan-Headers) may work in my case, but I am not sure if there would be any build performance or other implications to this.
4. Would a simple config switch to enable or disable target_link_libraries be a viable option, or even simpler, add the PRIVATE keyword: target_link_libraries(${nvrhi_vulkan_target} PRIVATE Vulkan-Headers)? I have tested this and the PRIVATE keyword limits the scope of nvrhi's own copy of Vulkan-Headers and solves the issue in my case.

Clearly anything you do here has to be pretty general and handle various build setups. However, philosophically I think that libraries which have versioned header dependencies should not impose those same dependencies on the calling application. For instance, RBDoom3BFG has dependencies on MoltenVK which in turn depends on specific versions of Vulkan which are more recent than nvrhi's Vulkan-Headers copy. You may have just updated the Vulkan-Headers version, but that will always fall behind in time or if the consuming project freezes on a particular nvrhi version. So the notion that nvrhi should determine or conflict with the Vulkan version an application uses is quite limiting. That is why I am suggesting that the scope of nvrhi's Vulkan-Headers copy be limited to the nvrhi library itself, and not pass those upwards to the calling application. Does this make sense?

[apanteleev]

Thank you for the detailed explanation of options. I made the Vulkan-Headers dependency privately linked, as you suggested - see fe9e756; that doesn't seem to cause any issues with our usage of NVRHI, just need to add a dependency to Vulkan-Headers further up in the chain (donut_app specifically).
Please close the issue if it is resolved.

[SRSaunders]

Thanks very much for this. However, I just discovered something quite interesting - the solution works perfectly for Vulkan-Headers version 238, but fails for version 277 (independent of your fix) with:

CMake Error at extern/nvrhi/thirdparty/Vulkan-Headers/CMakeLists.txt:43 (add_library):
  add_library cannot create ALIAS target "Vulkan::Headers" because another
  target with the same name already exists.

which is caused by a conflict with this line (43) in Vulkan-Headers/CMakeLists.txt:

add_library(Vulkan::Headers ALIAS Vulkan-Headers)

After some digging and experimentation, it turns out this error is caused by a subtle change in cmake policy starting with cmake policy version 3.18, where you can't override an existing target with an alias. Vulkan-Headers v238 used an old policy (v3.10.2 - allow override), while Vulkan-Headers v277 uses a newer policy (up to v3.25 - disallow override). However, this message points to something more important - an imported target called Vulkan::Headers that is created when you use find_package(Vulkan) (via FindVulkan.cmake) to locate the Vulkan SDK. I was not aware of this imported target during our earlier discussion, as it is not explicitly used by RBDoom3BFG. However, this new info leads back to your solution, but with a slight modification:

if (NVRHI_WITH_VULKAN AND NOT TARGET Vulkan-Headers AND NOT TARGET Vulkan::Headers)
    add_subdirectory(thirdparty/Vulkan-Headers)
endif()

...

if (NVRHI_WITH_RTXMU)
    if (TARGET Vulkan-Headers)
        get_target_property(RTXMU_VULKAN_INCLUDE_DIR Vulkan-Headers INTERFACE_INCLUDE_DIRECTORIES)
    elseif (TARGET Vulkan::Headers)
        get_target_property(RTXMU_VULKAN_INCLUDE_DIR Vulkan::Headers INTERFACE_INCLUDE_DIRECTORIES)
    endif()

...

    if (TARGET Vulkan-Headers)
        target_link_libraries(${nvrhi_vulkan_target} PRIVATE Vulkan-Headers)
    elseif (TARGET Vulkan::Headers)
        target_link_libraries(${nvrhi_vulkan_target} PRIVATE Vulkan::Headers)
    endif()

Hopefully this solves the issue for good. It's debatable whether you now need the PRIVATE keyword vs. PUBLIC, but I believe PRIVATE may be a bit safer, e.g. handle the case when the app's CMakeLists.txt defines the location of Vulkan_SDK manually without using find_package(Vulkan) and both Vulkan-Headers and Vulkan::Headers targets are initially undefined.

UPDATE: I added the elseif (TARGET Vulkan::Headers) condition to be conservative, in case the app's CMakeLists.txt calls find_package(Vulkan) but never calls target_link_libraries() for Vulkan. I am not sure if this is a real or likely scenario but you never know.

Note I have taken my best guess regarding logic for get_target_property(RTXMU_VULKAN_INCLUDE_DIR ...) but I am less familiar with this option and don't have a test environment to check it out.

I have verified these changes, except for get_target_property(RTXMU_VULKAN_INCLUDE_DIR ...), on Windows, Linux, and macOS and they work correctly for RBDoom3BFG builds independent of the Vulkan-Headers version. Here is the patch file I used for my tests: nvrhi-cmakelists.patch

[apanteleev]

Hi @SRSaunders, thank you for investigating and providing a solution. I've implemented these changes in a2e059a

[SRSaunders]

Thank you @apanteleev for your flexibility in making these changes. Everything is working fine now!