KhronosGroup / Vulkan-Headers

Vulkan header files and API registry
https://www.vulkan.org/
Other
841 stars 218 forks source link

cmake: Remove usage of configure_package_config_file #416

Closed juan-lunarg closed 1 year ago

juan-lunarg commented 1 year ago

Fixes issue shown here: https://github.com/KhronosGroup/Vulkan-Headers/pull/415

This also makes Vulkan-Headers more consistent with SPIRV-Headers

abouvier commented 1 year ago

I don't understand, why did you remove configure_package_config_file? Now there is no more config file in the current binary directory to be used by subsequent find_package.

juan-lunarg commented 1 year ago

There is still a config file. We actively test find_package support as part of CI.

juan-lunarg commented 1 year ago

configure_package_config_file isn't needed to generate a config file.

abouvier commented 1 year ago

Yes there is a config file but only after installation. The goal was to have a config file in the the build tree, which can be used by subprojects linking with VulkanHeaders:

add_subdirectory(Vulkan-Headers)
set(VulkanHeaders_DIR "${CMAKE_CURRENT_BINARY_DIR}/Vulkan-Headers") # previous location of the VulkanHeadersConfig generated by configure_package_config_file
add_subdirectory(ProjectUsingVulkanHeaders) # will invoke find_package(VulkanHeaders) and find the VulkanHeadersConfig in $VulkanHeaders_DIR

By the way configure_package_config_file and write_basic_package_version_file only write files to the build tree and shouldn't be guarded by VULKAN_HEADERS_INSTALL. Only the install commands should be guarded.

juan-lunarg commented 1 year ago

The goal was to have a config file in the the build tree, which can be used by subprojects linking with VulkanHeaders:

The CMake code you are showing is full of various issues and is not a supported use case.

You are mixing add_subdirectory and find_package of the same dependency in order to work around another dependency.

juan-lunarg commented 1 year ago

It's clear your issue stems from the CMake code in vulkan memory allocator. I've made a PR to address the various CMake issues in VulkanMemoryAllocator.

juan-lunarg commented 1 year ago

https://github.com/GPUOpen-LibrariesAndSDKs/VulkanMemoryAllocator/pull/349

abouvier commented 1 year ago

The CMake code you are showing is full of various issues and is not a supported use case.

You are mixing add_subdirectory and find_package of the same dependency in order to work around another dependency.

It's the equivalent of this code but for old CMake versions:

include(FetchContent)
FetchContent_Declare(
    VulkanHeaders
    SOURCE_DIR "${CMAKE_CURRENT_LIST_DIR}/Vulkan-Headers"
    OVERRIDE_FIND_PACKAGE # introduced in CMake 3.24
)
add_subdirectory(ProjectUsingVulkanHeaders)
juan-lunarg commented 1 year ago

My proposed changes to VMA (https://github.com/GPUOpen-LibrariesAndSDKs/VulkanMemoryAllocator/pull/349) will allow the following use cases to work without any issue.

add_subdirectory(Vulkan-Headers)
add_subdirectory(VulkanMemoryAllocator)
find_package(Vulkan-Headers)
add_subdirectory(VulkanMemoryAllocator)
add_subdirectory(Vulkan-Headers)
find_package(VulkanMemoryAllocator)
find_package(Vulkan-Headers)
find_package(VulkanMemoryAllocator)
abouvier commented 1 year ago

My proposed changes to VMA (GPUOpen-LibrariesAndSDKs/VulkanMemoryAllocator#349) will allow the following use cases to work without any issue.

Of course, there is no more find_package(Vulkan) and/or linking with Vulkan::Headers by default in your PR :p But the problem still persists for projects linking with Vulkan::Headers. There is no way to hijack their find_package(VulkanHeaders) to use a local/custom Vulkan::Headers added via add_subdirectory(Vulkan-Headers).

juan-lunarg commented 1 year ago

There is no way to hijack their find_package(VulkanHeaders) to use a local/custom Vulkan::Headers added via add_subdirectory(Vulkan-Headers).

https://discourse.cmake.org/t/idiomatic-way-to-handle-packages-and-add-subdirectory/8400

This isn't the responsibility of Vulkan-Headers. According to the CMake maintainers it's the responsibility of the repo in question.

abouvier commented 1 year ago

Look what I found in your Professional CMake book written by a CMake co-maintainer 😱 Screenshot_20230708_214103

juan-lunarg commented 1 year ago

Yes I was already aware of that. I looked at the config file generated by this approach and saw no downside since it had parity with the existing solution. CI testing for find_package didn't show any difference.