zeux/volk

1.3.275.0: missing checking VulkanHeaders and volk.c installation

Closed this issue · 22 comments

kloczek commented

Instead hardcoding pulling vulkan heders better would just add checking dependencies

--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -61,31 +61,7 @@
 # -----------------------------------------------------
 # Vulkan transitive dependency

-if(VOLK_PULL_IN_VULKAN)
-  # If CMake has the FindVulkan module and it works, use it.
-  find_package(Vulkan QUIET)
-
-  # Try an explicit CMake variable first, then any Vulkan paths
-  # discovered by FindVulkan.cmake, then the $VULKAN_SDK environment
-  # variable if nothing else works.
-  if(VULKAN_HEADERS_INSTALL_DIR)
-    message("volk: using VULKAN_HEADERS_INSTALL_DIR option")
-    set(VOLK_INCLUDES "${VULKAN_HEADERS_INSTALL_DIR}/include")
-  elseif(Vulkan_INCLUDE_DIRS)
-    message("volk: using Vulkan_INCLUDE_DIRS from FindVulkan module")
-    set(VOLK_INCLUDES "${Vulkan_INCLUDE_DIRS}")
-  elseif(DEFINED ENV{VULKAN_SDK})
-    message("volk: using VULKAN_SDK environment variable")
-    set(VOLK_INCLUDES "$ENV{VULKAN_SDK}/include")
-  endif()
-
-  if(VOLK_INCLUDES)
-    if(TARGET volk)
-      target_include_directories(volk PUBLIC "${VOLK_INCLUDES}")
-    endif()
-    target_include_directories(volk_headers INTERFACE "${VOLK_INCLUDES}")
-  endif()
-endif()
+find_package(VulkanHeaders)

 # -----------------------------------------------------
 # Installation

Please let me know if you want above as PR.

Other thing is installation of both files volk.h and volk.c.

volk/CMakeLists.txt

Line 99 in f2a16e3

install(FILES volk.h volk.c DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})

kloczek commented

Yet another issue: looks like tests/ content is n`ot integrated with ctest

+ /usr/bin/ctest --test-dir x86_64-redhat-linux-gnu --output-on-failure --force-new-ctest-process -j48
Internal ctest changing into directory: /home/tkloczko/rpmbuild/BUILD/volk-vulkan-sdk-1.3.275.0/x86_64-redhat-linux-gnu
Test project /home/tkloczko/rpmbuild/BUILD/volk-vulkan-sdk-1.3.275.0/x86_64-redhat-linux-gnu
No tests were found!!!
zeux commented

Please let me know if you want above as PR.

What if VulkanHeaders is not available as a CMake module? volk is explicitly designed to work without it.

zeux commented

Other thing is installation of both files volk.h and volk.c.

Intended to make header-only mode work I believe. See 51f8af5.

kloczek commented

What if VulkanHeaders is not available as a CMake module? volk is explicitly designed to work without it.

As usually cmake will exit with exit code and error message that this dependency is missing in build env.
Really please do not hardcode any network interactions because most of the distributions build envs are INTENTIONALLY cut off from access to public network.

zeux commented

What network interactions do you mean?

zeux commented

As usually cmake will exit with exit code and error message that this dependency is missing in build env.

The support for VULKAN_SDK env var in particular is valuable for Windows installations where CMake doesn't always have access to the modules I believe, or didn't in the past. It might make sense to replace the use of Vulkan module with VulkanHeaders but it still must be conditional and configurable externally. Additionally some of the "complexity" is due to Vulkan SDK build process where I think it wants to be able to supply VULKAN_HEADERS_INSTALL_DIR variable externally to override the default choice (see 22ce6a3). So there are three conditions in the snippet, and all three serve a specific purpose.

kloczek commented

Above patch is incorrect. Please ignore it.

Intended to make header-only mode work I believe. See 51f8af5.

Q: why by default cmake do not build and install, cmake module, static library and volk.h? 🤔

zeux commented

Yeah, it would probably make more sense for installation of volk.c to be conditional on VOLK_HEADERS_ONLY. cc @jeweg for opinion on this as the author of the original change.

kloczek commented

The support for VULKAN_SDK env var in particular is valuable for Windows installations where CMake doesn't always have access to the modules I believe

Vulkan-Headers installs Vulkan headers and VulkanHeaders cmake module which is possible to require in other projects like here
https://github.com/KhronosGroup/Vulkan-Utility-Libraries/blob/4cfc176e3242b4dbdfd3f6c5680c5d8f2cb7db45/CMakeLists.txt#L28

zeux commented

For FindVulkan specifically, I believe VULKAN_SDK env var will be considered from CMake 3.18 - but earlier versions will probably be safer doing the env check that volk is doing. This might not be very important for the project you linked and/or it might have been made after CMake 3.18. For that project the min. version is 3.17 but for volk it's 3.5.

kloczek commented

For example with below patch:

--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -61,31 +61,7 @@
 # -----------------------------------------------------
 # Vulkan transitive dependency

-if(VOLK_PULL_IN_VULKAN)
-  # If CMake has the FindVulkan module and it works, use it.
-  find_package(Vulkan QUIET)
-
-  # Try an explicit CMake variable first, then any Vulkan paths
-  # discovered by FindVulkan.cmake, then the $VULKAN_SDK environment
-  # variable if nothing else works.
-  if(VULKAN_HEADERS_INSTALL_DIR)
-    message("volk: using VULKAN_HEADERS_INSTALL_DIR option")
-    set(VOLK_INCLUDES "${VULKAN_HEADERS_INSTALL_DIR}/include")
-  elseif(Vulkan_INCLUDE_DIRS)
-    message("volk: using Vulkan_INCLUDE_DIRS from FindVulkan module")
-    set(VOLK_INCLUDES "${Vulkan_INCLUDE_DIRS}")
-  elseif(DEFINED ENV{VULKAN_SDK})
-    message("volk: using VULKAN_SDK environment variable")
-    set(VOLK_INCLUDES "$ENV{VULKAN_SDK}/include")
-  endif()
-
-  if(VOLK_INCLUDES)
-    if(TARGET volk)
-      target_include_directories(volk PUBLIC "${VOLK_INCLUDES}")
-    endif()
-    target_include_directories(volk_headers INTERFACE "${VOLK_INCLUDES}")
-  endif()
-endif()
+find_package(VulkanHeaders ${VERSION} REQUIRED)

 # -----------------------------------------------------
 # Installation

if headers are not isntalled

+ /usr/bin/cmake -B x86_64-redhat-linux-gnu -D BUILD_SHARED_LIBS=ON -D CMAKE_AR=/usr/bin/gcc-ar -D CMAKE_BUILD_TYPE=RelWithDebInfo -D CMAKE_C_FLAGS_RELEASE=-DNDEBUG -D CMAKE_CXX_FLAGS_RELEASE=-DNDEBUG -D CMAKE_Fortran_FLAGS_RELEASE=-DNDEBUG -D CMAKE_INSTALL_PREFIX=/usr -D CMAKE_NM=/usr/bin/gcc-nm -D CMAKE_RANLIB=/usr/bin/gcc-ranlib -D CMAKE_VERBOSE_MAKEFILE=ON -D INCLUDE_INSTALL_DIR=/usr/include -D LIB_INSTALL_DIR=/usr/lib64 -D LIB_SUFFIX=64 -D SHARE_INSTALL_PREFIX=/usr/share -D SYSCONF_INSTALL_DIR=/etc -S .
-- The C compiler identification is GNU 14.0.1
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/gcc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
CMake Error at CMakeLists.txt:64 (find_package):
  By not providing "FindVulkanHeaders.cmake" in CMAKE_MODULE_PATH this
  project has asked CMake to find a package configuration file provided by
  "VulkanHeaders", but CMake did not find one.

  Could not find a package configuration file provided by "VulkanHeaders"
  with any of the following names:

    VulkanHeadersConfig.cmake
    vulkanheaders-config.cmake

  Add the installation prefix of "VulkanHeaders" to CMAKE_PREFIX_PATH or set
  "VulkanHeaders_DIR" to a directory containing one of the above files.  If
  "VulkanHeaders" provides a separate development package or SDK, be sure it
  has been installed.


-- Configuring incomplete, errors occurred!
kloczek commented

For FindVulkan specifically, I believe VULKAN_SDK env var will be considered from CMake 3.18 - but earlier versions will probably be safer doing the env check that volk is doing.

Please do not use that cmake module.
https://github.com/KhronosGroup/Vulkan-Headers/ installs always headers and cmake VulkanHeaders module. That module i VERSIONED. What is in cmake FindVulkan has no version checking. I think that cmake should stop provide that module.

Yeah, it would probably make more sense for installation of volk.c to be conditional on VOLK_HEADERS_ONLY. cc @jeweg for opinion on this as the author of the original change.

But again .. what is the sense install volkc.c if that installed static library has been generated out of that file? 🤔

zeux commented

What is in cmake FindVulkan has no version checking

What version checking are you talking about? In other words, can you explain the issues that existing header discovery logic in volk is causing?

Let’s discuss the volk.c installation in a separate issue, feel free to open one.

kloczek commented
Here is proposition of the patch which I've added to my rpm build procedure: 
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -2,47 +2,35 @@
 cmake_policy(PUSH)
 cmake_policy(SET CMP0048 NEW) # project(... VERSION ...) support

-project(volk VERSION
-# VOLK_GENERATE_VERSION
-275
-# VOLK_GENERATE_VERSION
-  LANGUAGES C
+project(volk
+  VERSION      1.3.275.0
+  DESCRIPTION  "volk - meta-loader for Vulkan"
+  HOMEPAGE_URL https://github.com/zeux/volk/
+  LANGUAGES    C
 )

+include(GNUInstallDirs)
+
 # CMake 3.12 changes the default behaviour of option() to leave local variables
 # unchanged if they exist (which we want), but we must work with older CMake versions.
 if(NOT DEFINED VOLK_STATIC_DEFINES)
   option(VOLK_STATIC_DEFINES "Additional defines for building the volk static library, e.g. Vulkan platform defines" "")
 endif()
-if(NOT DEFINED VOLK_PULL_IN_VULKAN)
-  option(VOLK_PULL_IN_VULKAN "Vulkan as a transitive dependency" ON)
-endif()
-if(NOT DEFINED VOLK_INSTALL)
-  option(VOLK_INSTALL "Create installation targets" OFF)
-endif()
-if(NOT DEFINED VOLK_HEADERS_ONLY)
-  option(VOLK_HEADERS_ONLY "Add interface library only" OFF)
-endif()
-if(NOT DEFINED VULKAN_HEADERS_INSTALL_DIR)
-  option(VULKAN_HEADERS_INSTALL_DIR "Where to get the Vulkan headers" "")
-endif()

 # -----------------------------------------------------
 # Static library

-if(NOT VOLK_HEADERS_ONLY OR VOLK_INSTALL)
-  add_library(volk STATIC volk.h volk.c)
-  add_library(volk::volk ALIAS volk)
-  target_include_directories(volk PUBLIC
-    $<BUILD_INTERFACE:${CMAKE_CURRENT_LIST_DIR}>
-    $<INSTALL_INTERFACE:include>
-  )
-  if(VOLK_STATIC_DEFINES)
-    target_compile_definitions(volk PUBLIC ${VOLK_STATIC_DEFINES})
-  endif()
-  if (NOT WIN32)
-    target_link_libraries(volk PUBLIC dl)
-  endif()
+add_library(volk STATIC volk.h volk.c)
+add_library(volk::volk ALIAS volk)
+target_include_directories(volk PUBLIC
+  $<BUILD_INTERFACE:${CMAKE_CURRENT_LIST_DIR}>
+  $<INSTALL_INTERFACE:include>
+)
+if(VOLK_STATIC_DEFINES)
+  target_compile_definitions(volk PUBLIC ${VOLK_STATIC_DEFINES})
+endif()
+if (NOT WIN32)
+  target_link_libraries(volk PUBLIC dl)
 endif()

 # -----------------------------------------------------
@@ -61,77 +49,49 @@
 # -----------------------------------------------------
 # Vulkan transitive dependency

-if(VOLK_PULL_IN_VULKAN)
-  # If CMake has the FindVulkan module and it works, use it.
-  find_package(Vulkan QUIET)
-
-  # Try an explicit CMake variable first, then any Vulkan paths
-  # discovered by FindVulkan.cmake, then the $VULKAN_SDK environment
-  # variable if nothing else works.
-  if(VULKAN_HEADERS_INSTALL_DIR)
-    message("volk: using VULKAN_HEADERS_INSTALL_DIR option")
-    set(VOLK_INCLUDES "${VULKAN_HEADERS_INSTALL_DIR}/include")
-  elseif(Vulkan_INCLUDE_DIRS)
-    message("volk: using Vulkan_INCLUDE_DIRS from FindVulkan module")
-    set(VOLK_INCLUDES "${Vulkan_INCLUDE_DIRS}")
-  elseif(DEFINED ENV{VULKAN_SDK})
-    message("volk: using VULKAN_SDK environment variable")
-    set(VOLK_INCLUDES "$ENV{VULKAN_SDK}/include")
-  endif()
-
-  if(VOLK_INCLUDES)
-    if(TARGET volk)
-      target_include_directories(volk PUBLIC "${VOLK_INCLUDES}")
-    endif()
-    target_include_directories(volk_headers INTERFACE "${VOLK_INCLUDES}")
-  endif()
-endif()
+find_package(VulkanHeaders ${VERSION} REQUIRED)

 # -----------------------------------------------------
 # Installation

-if(VOLK_INSTALL)
+set(INSTALL_CONFIGDIR ${CMAKE_INSTALL_LIBDIR}/cmake/volk)

-  include(GNUInstallDirs)
-  set(INSTALL_CONFIGDIR ${CMAKE_INSTALL_LIBDIR}/cmake/volk)
+# Install files
+install(FILES volk.h DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})

-  # Install files
-  install(FILES volk.h volk.c DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
+# Install library target and add it and any dependencies to export set.
+install(TARGETS volk volk_headers
+  EXPORT volk-targets
+  LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
+  ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}
+  RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
+)

-  # Install library target and add it and any dependencies to export set.
-  install(TARGETS volk volk_headers
-    EXPORT volk-targets
-    LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
-    ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}
-    RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
-  )
-
-  # Actually write exported config w/ imported targets
-  install(EXPORT volk-targets
-    FILE volkTargets.cmake
-    NAMESPACE volk::
-    DESTINATION ${INSTALL_CONFIGDIR}
-  )
-
-  # Create a ConfigVersion.cmake file:
-  include(CMakePackageConfigHelpers)
-  write_basic_package_version_file(
-    ${CMAKE_CURRENT_BINARY_DIR}/volkConfigVersion.cmake
-    COMPATIBILITY AnyNewerVersion
-  )
-
-  # Configure config file
-  configure_package_config_file(${CMAKE_CURRENT_LIST_DIR}/cmake/volkConfig.cmake.in
-    ${CMAKE_CURRENT_BINARY_DIR}/volkConfig.cmake
-    INSTALL_DESTINATION ${INSTALL_CONFIGDIR}
-  )
-
-  # Install the fully generated config and configVersion files
-  install(FILES
-    ${CMAKE_CURRENT_BINARY_DIR}/volkConfig.cmake
-    ${CMAKE_CURRENT_BINARY_DIR}/volkConfigVersion.cmake
-    DESTINATION ${INSTALL_CONFIGDIR}
-  )
+# Actually write exported config w/ imported targets
+install(EXPORT volk-targets
+  FILE volkTargets.cmake
+  NAMESPACE volk::
+  DESTINATION ${INSTALL_CONFIGDIR}
+)
+
+# Create a ConfigVersion.cmake file:
+include(CMakePackageConfigHelpers)
+write_basic_package_version_file(
+  ${CMAKE_CURRENT_BINARY_DIR}/volkConfigVersion.cmake
+  COMPATIBILITY AnyNewerVersion
+)
+
+# Configure config file
+configure_package_config_file(${CMAKE_CURRENT_LIST_DIR}/cmake/volkConfig.cmake.in
+  ${CMAKE_CURRENT_BINARY_DIR}/volkConfig.cmake
+  INSTALL_DESTINATION ${INSTALL_CONFIGDIR}
+)
+
+# Install the fully generated config and configVersion files
+install(FILES
+  ${CMAKE_CURRENT_BINARY_DIR}/volkConfig.cmake
+  ${CMAKE_CURRENT_BINARY_DIR}/volkConfigVersion.cmake
+  DESTINATION ${INSTALL_CONFIGDIR}
+)

-endif()
 cmake_policy(POP)

That patch

  • removes all IMO useless stuff
  • adds checking for VulkanHeaders cmake module
  • fixes project version
  • adds more project() entries

I'm going now test is it will be possible to build vulkan-tools 1.3.275.0 which looks like unconditionally requires volk cmake module.

kloczek commented

What version checking are you talking about? In other words, can you explain the issues that existing header discovery logic in volk is causing?

Probably you don't want to build volk against to old VulkanHeaders 😋

zeux commented

I think this is going nowhere... Let's take this step by step.

  1. I can not accept the proposed patch. It removes a lot of variables that other people have added over the years without consulting these people. It breaks automatic version update by removing the annotations. It's probably changing other things, I'm not sure, I just won't take a change that "cleans" things up in CMake without a reason for the changes.
  2. The patch does not set up the include folder. It happens to work - as best as I can tell it only works if VulkanHeaders is installed into a globally visible location. In that case I'm not sure what the point of even checking the package is.
  3. Relying on VulkanHeaders unconditionally is simply a no-go. I just checked on my macOS installation where I had Vulkan SDK installed, and volk currently compiles perfectly fine out of the box - it's using FindVulkan CMake module. If I replace it with find_package(VulkanHeaders REQUIRED), the build fails because VulkanHeaders CMake module is not actually globally visible. I'm sure this can be "fixed" with adjusting CMAKE_MODULE_PATH or something of the sort - but that is exactly the point behind what volk is doing, to require no additional CMake configuration on user's behalf unless they so choose. That's why all this "useless stuff" is in the file.
  4. I do actually want to build volk against old, or new, VulkanHeaders. volk is designed to not care about the version of the vulkan.h that it's linking to - this makes sure that maximum compatibility is achieved. For Linux distributions that pre-package all carefully configured libraries this may seem redundant, but it's absolutely critical when volk that's bundled into a project, which is how it most often is used, is used against Android NDK or Vulkan SDK installed manually. Now, when volk is pre-built as a static library as part of Linux distribution, maybe you want to check some sort of compatibility, but it can not be there by default.
kloczek commented

I do actually want to build volk against old, or new, VulkanHeaders. volk is designed to not care about the version of the vulkan.h that it's linking to

Please check since when is provided VulkanHeaders. You can always remove check version.

To be honest I do not understand completely this project. It installs .c files on side with header file. That .c file is included in provided header. Literally NONE of other projects has such things in its public API interface.
I have more and more impression that volk still is not ready for production and it was kind of mistake to include use it in last Vulcan stack SDK release.

Looks like volk provides only statically linked bits used only by vulkan-tools.
Why in this case what is in volk is not part of the vulkan-tools especially that seems like it is not possible to build vulkan-tools without volk it is for me kind of mystery. 🤔
I have impression that this separation is artificial/has no real propose.

zeux commented

I think you don't understand what volk is, how it's used, and this ecosystem in general. I don't know what vulkan-tools uses volk for but that's not what the purpose here is. Your suggestion about volk not being production ready is outrageous, frankly, as it's been used by games in production for years.

I will close this issue because it has no real substance, and it just wasted hours of my time yesterday following up on commit history and testing suggestions to use CMake replacements that do not work reliably without extra configuration.

If your issue is the presence of volk.c in the install target, I agree it's questionable, but that needs separate followup as I think Vulkan SDK distributes volk.c in a similar fashion and I'm not sure if they use the INSTALL flow for packaging.

kloczek commented

I think you don't understand what volk is, how it's used, and this ecosystem in general.

That is what I've LITERALLY wrote so you don't need to suppose about that part.

I don't know what vulkan-tools uses volk for but that's not what the purpose here is.

You can see that in the code https://github.com/KhronosGroup/Vulkan-Tools/
KhronosGroup/Vulkan-Tools@e156996

I will close this issue because it has no real substance, and it just wasted hours of my time yesterday following up on commit history and testing suggestions to use CMake replacements that do not work reliably without extra configuration.

IMO this issue still is unresolved because:

  • by default volk install target do not install any files
  • IMO it should be just generated .h file without .c one.

If your issue is the presence of volk.c in the install target, I agree it's questionable, but that needs separate followup as I think Vulkan SDK distributes volk.c in a similar fashion and I'm not sure if they use the INSTALL flow for packaging.

Just checked all vulcan repos and only vulcan project which uses volk is Vulkan-Tools and it started using it only with last SDK release.

jeweg commented

Hey @zeux, thanks for the heads-up.
I don't have a strong opinion about FindVulkanHeaders.cmake. Requiring it would be silly since it doesn't come with CMake or the Vulkan SDK. I myself have it on my system exactly once, because I built the validation layers at one point. That is a rather obscure thing to do. We could still add it to the chain of conditions, but note that in the file itself it states "Vulkan applications should instead use the FindVulkan (or similar) find module". This part of volk seems fine and robust the way it is.
Regarding volk.c in the include path: I wanted a volk install to be usable in all supported ways: linking to the static lib or header-only, with the latter case requiring the .c file (rather than giving the generator a special case of stuffing everything into a header file). That impl file might look weird there, but what's the harm? Currently, VOLK_HEADERS_ONLY is a setting for project build-time, useful for when one adds volk to a project build tree. The proposed change would make it also a setting for volk build/install-time. That volk install would then only support the static lib way. I don't see an upside to limiting it like this.
In my view, volk has always made its decisions erring on the side of usage flexibility, and its CMake side tries to do the same. That's why it instead of being very opinionated supports all of: just adding the source files to a project, adding volk as a subdir and using its targets, or installing it and importing target configs.
Specifically, that's why the install targets are not just always created -- those would be added to the user project's installs if it includes volk with add_subdirectory, and that's usually not desired. It's debatable whether the default should be switched around, but at this point that'd probably break/inconvenience existing projects for close to no reason.

kloczek commented

I don't have a strong opinion about FindVulkanHeaders.cmake. Requiring it would be silly since it doesn't come with CMake or the Vulkan SDK.

FindVulkanHeaders cmake modle is OOTB interface to interact using cmake.

So again .. why volk cmake by default do not install any files? 🤔

FYI: in volk git tree there is no any comments about how it should be used.

jeweg commented

So again .. why volk cmake by default do not install any files? 🤔

I addressed that in the last paragraph above.

FYI: in volk git tree there is no any comments about how it should be used.

There is this section and the test directory that shows four different ways of using it with CMake. What do you feel is missing?