zeux / volk

Meta loader for Vulkan API
MIT License
1.35k stars 118 forks source link

Name clash with volk from https://www.libvolk.org/ #66

Closed daissi closed 3 years ago

daissi commented 3 years ago

Hi, While working on the Debian package of this volk library, I found there was already an unrelated volk library in Debian (https://www.libvolk.org/). I don't know yet how this could be problematic, but at least the discussion is open.

Best, Dylan

zeux commented 3 years ago

While unfortunate, I don't think there's anything actionable here. If the issue is in package name collision for Debian, I'd recommend using a name like volk-loader, assuming the package names can contain dashes.

jexposit commented 10 months ago

Hi!

Sorry for commenting in a closed issue, but we are having the same problem in Fedora.

As it doesn't look like renaming this library is an option, I'm considering patching it downstream to avoid conflicts with the volk (GNU Radio) library. Installed files would look like: https://pagure.io/packaging-committee/issue/1324#comment-889653

It'd be nice if you could provide feedback.

Also, if we could agree on a similar solution for Debian and Fedora we'd avoid software using volk (Vulkan SDK) compiling on Fedora but not compiling on Debian or vice versa.

zeux commented 10 months ago

Debian is packaging without a prefix I believe, see https://packages.debian.org/bookworm/all/libvulkan-volk-dev/filelist. In general for users of packaged libraries, from my experience, having locations vary by distro is inconvenient, but maybe if pkg-config/CMake module discovery is used it's not a problem. I'd prefer that Fedora follows Debian here, but if that's not an option I'm happy to accept patches that add configuration options to CMakeLists.txt that make retargeting installation et al easier. And yes, distributions are free to rename the packages (eg see ninja), but I'm not open to renaming the library - it existed for years and is widely used and distributed in Vulkan SDK.

jexposit commented 10 months ago

Thanks for the quick feedback.

Debian is packaging without a prefix I believe, see https://packages.debian.org/bookworm/all/libvulkan-volk-dev/filelist.

Yes, Debian is not adding a prefix, however, they are removing the .a file and the .cmake files. From the rules file:

override_dh_auto_install:
    dh_auto_install
    # Removing files related to the static library until the name clash is resolved
    # https://github.com/zeux/volk/issues/66
    find . -name '*.a' -delete
    find . -name '*.cmake' -delete
    find . -type d -empty -delete

In order to upgrade to vulkan-tools v1.3.272 Debian would need to install the .cmake files, having the same problem we have in Fedora.

I'm happy to accept patches that add configuration options to CMakeLists.txt that make retargeting installation et al easier

I was planning to do something like this:

Patch ```diff diff --git a/CMakeLists.txt b/CMakeLists.txt index 761ac9a..2c52f59 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -31,17 +31,17 @@ 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 + add_library(vulkan_volk STATIC volk.h volk.c) + add_library(volk::vulkan_volk ALIAS vulkan_volk) + target_include_directories(vulkan_volk PUBLIC $ $ ) if(VOLK_STATIC_DEFINES) - target_compile_definitions(volk PUBLIC ${VOLK_STATIC_DEFINES}) + target_compile_definitions(vulkan_volk PUBLIC ${VOLK_STATIC_DEFINES}) endif() if (NOT WIN32) - target_link_libraries(volk PUBLIC dl) + target_link_libraries(vulkan_volk PUBLIC dl) endif() endif() @@ -52,7 +52,7 @@ add_library(volk_headers INTERFACE) add_library(volk::volk_headers ALIAS volk_headers) target_include_directories(volk_headers INTERFACE $ - $ + $ ) if (NOT WIN32) target_link_libraries(volk_headers INTERFACE dl) @@ -81,7 +81,7 @@ if(VOLK_PULL_IN_VULKAN) if(VOLK_INCLUDES) if(TARGET volk) - target_include_directories(volk PUBLIC "${VOLK_INCLUDES}") + target_include_directories(vulkan_volk PUBLIC "${VOLK_INCLUDES}") endif() target_include_directories(volk_headers INTERFACE "${VOLK_INCLUDES}") endif() @@ -93,13 +93,13 @@ endif() if(VOLK_INSTALL) include(GNUInstallDirs) - set(INSTALL_CONFIGDIR ${CMAKE_INSTALL_LIBDIR}/cmake/volk) + set(INSTALL_CONFIGDIR ${CMAKE_INSTALL_LIBDIR}/cmake/vulkan_volk) # Install files - install(FILES volk.h volk.c DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}) + install(FILES volk.h volk.c DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/vulkan_volk) # Install library target and add it and any dependencies to export set. - install(TARGETS volk volk_headers + install(TARGETS vulkan_volk volk_headers EXPORT volk-targets LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR} ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR} diff --git a/cmake/volkConfig.cmake.in b/cmake/volkConfig.cmake.in index 6e45c37..5a2358c 100644 --- a/cmake/volkConfig.cmake.in +++ b/cmake/volkConfig.cmake.in @@ -1,6 +1,6 @@ get_filename_component(volk_CMAKE_DIR "${CMAKE_CURRENT_LIST_FILE}" PATH) -if(NOT TARGET volk::volk) +if(NOT TARGET volk::vulkan_volk) include("${volk_CMAKE_DIR}/volkTargets.cmake") endif() @@ -12,10 +12,10 @@ endif() if(VOLK_PULL_IN_VULKAN) find_package(Vulkan QUIET) if(TARGET Vulkan::Vulkan) - add_dependencies(volk::volk Vulkan::Vulkan) + add_dependencies(volk::vulkan_volk Vulkan::Vulkan) add_dependencies(volk::volk_headers Vulkan::Vulkan) elseif(DEFINED ENV{VULKAN_SDK}) - target_include_directories(volk::volk INTERFACE "$ENV{VULKAN_SDK}/include") + target_include_directories(volk::vulkan_volk INTERFACE "$ENV{VULKAN_SDK}/include") target_include_directories(volk::volk_headers INTERFACE "$ENV{VULKAN_SDK}/include") endif() endif() ```

Which would install these files:

/usr/include/vulkan_volk/volk.h
/usr/include/vulkan_volk/volk.c
/usr/lib64/libvulkan_volk.a
/usr/lib64/cmake/vulkan_volk/volkTargets.cmake
/usr/lib64/cmake/vulkan_volk/volkTargets-release.cmake
/usr/lib64/cmake/vulkan_volk/volkConfig.cmake
/usr/lib64/cmake/vulkan_volk/volkConfigVersion.cmake

By keeping the volk CMake namespace the only change required in vulkan-tools (and other apps using volk) is:

- find_package(volk QUIET CONFIG)
+ find_package(volk QUIET CONFIG NAMES volk vulkan_volk)

Which is far from perfect, but it minimizes the amount of changes needed.

Honestly, I might be paranoid about a possible conflict with volk (GNU Radio) adding cmake support in the future and creating a conflict. Pinging @jdemel to see if he can share some input about future plans of the project on CMake and if leaving /usr/lib64/cmake/volk/*.cmake without a prefix could result in a conflict or not.

In general for users of packaged libraries, from my experience, having locations vary by distro is inconvenient, but maybe if pkg-config/CMake module discovery is used it's not a problem.

As you mentioned, the change proposed is inconvenient, as it requires changes in existing applications and breaks compilation between distros.

I'm trying to find a good solution for everyone... Or at least a common solution for Debian, Fedora and other distros.

jexposit commented 9 months ago

JFYI, I started the packaging process for Fedora: https://bugzilla.redhat.com/show_bug.cgi?id=2257275

The package will be available soon. If you find any issues with the package or want to made any change in the future, feel free to ping me and I'll do my best to help you.

daissi commented 9 months ago

@jexposit if I understand correctly, so far you are following the Debian way? by just marking both packages in conflicts? you have not applied your patch, right?

I'm all for making things uniform across distros and to avoid the conflict with gnuradio/volk.

jexposit commented 9 months ago

Yes, I marked the packages as conflicting. No patches applied, so programs should compile in the same manner on Debian and Fedora.