Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Please allow using external cpu_features package #556

Closed
yurivict opened this issue Jan 16, 2022 · 10 comments · Fixed by #558
Closed

Please allow using external cpu_features package #556

yurivict opened this issue Jan 16, 2022 · 10 comments · Fixed by #558
Labels
CMake Related to the CMake builder build scripts Dependencies Related to problems in dependencies or their detection

Comments

@yurivict
Copy link

Bundling and installing cpu_features causes conflicts with pre-installed package.

Version: 2.5.0

@marcusmueller
Copy link
Member

for reference: usually, debian is relatively strict about not including libraries:

Description: use system cpu_features package

Author: Shengjing Zhu <zhsj@debian.org>
Last-Update: 2020-12-26

--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -142,17 +142,7 @@
   option(VOLK_CPU_FEATURES "Volk uses cpu_features" OFF)
 endif()
 if (VOLK_CPU_FEATURES)
-  if(NOT EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/cpu_features/CMakeLists.txt" )
-    message(FATAL_ERROR "cpu_features/CMakeLists.txt not found. Did you forget to git clone recursively?\nFix with: git submodule update --init")
-  endif()
-  message(STATUS "Building Volk with cpu_features")
-  set(BUILD_PIC ON CACHE BOOL
-    "Build cpu_features with Position Independent Code (PIC)."
-    FORCE)
-  set(BUILD_SHARED_LIBS_SAVED "${BUILD_SHARED_LIBS}")
-  set(BUILD_SHARED_LIBS OFF)
-  add_subdirectory(cpu_features)
-  set(BUILD_SHARED_LIBS "${BUILD_SHARED_LIBS_SAVED}")
+  find_package(CpuFeatures)
 else()
   message(STATUS "Building Volk without cpu_features")
 endif()
--- a/lib/CMakeLists.txt
+++ b/lib/CMakeLists.txt
@@ -517,7 +517,7 @@
 if(VOLK_CPU_FEATURES)
   set_source_files_properties(volk_cpu.c PROPERTIES COMPILE_DEFINITIONS "VOLK_CPU_FEATURES=1")
   target_include_directories(volk_obj
-    PRIVATE $<TARGET_PROPERTY:cpu_features,INTERFACE_INCLUDE_DIRECTORIES>
+    PRIVATE $<TARGET_PROPERTY:CpuFeatures::cpu_features,INTERFACE_INCLUDE_DIRECTORIES>
 )
 endif()

@yurivict
Copy link
Author

cpu_features installs cmake files, so there should be no problem discovering it.

@marcusmueller
Copy link
Member

the thing is, knowing the maintainers, they would probably prefer to not have made their lives easier (by not using cpu_features as submodule) if there was not (hard to detect) broken cpu_features packages out there, but I'll have to defer to them.

@marcusmueller
Copy link
Member

In the meantime, @yurivict, do you think you could try the debian patch above and see whether it works for you (may I guess for FreeBSD, then?)

@marcusmueller marcusmueller added CMake Related to the CMake builder build scripts Dependencies Related to problems in dependencies or their detection labels Jan 16, 2022
@marcusmueller
Copy link
Member

@yurivict using FreeBSD-shipped cpu_features might be or have been blocked by: #484

@yurivict
Copy link
Author

In the meantime, @yurivict, do you think you could try the debian patch above and see whether it works for you (may I guess for FreeBSD, then?)

Could you create a pull request with this patch?

@jdemel
Copy link
Contributor

jdemel commented Jan 17, 2022

I know Debian ships VOLK and cpu_features separately. I'd be happy to make the submodule an optional dependency. This is probably an interesting feature for quite a lot of users now. @yurivict would you be able to provide a patch or point into the right direction? I'd be happy to add FreeBSD CI as well.

Having said that, why do we use cpu_features as a submodule?
The cpu_features package is fairly new on most systems. e.g. it got introduced after Ubuntu 20.04. With Debian 11 and soon Ubuntu 20.04 the situation improves quite significantly. I guess the Fedora situation is similar.

In case of FreeBSD: You'd probably want the latest development version https://github.com/google/cpu_features because cpu_features received FreeBSD support after the last release.

The argument is similar for MacOS. M1 support in cpu_features, and thus VOLK, got merged after the last release.

I hope we are able to unbundle cpu_features in the future. I hope we didn't miss any cpu_features includes that are public. So, cpu_features should be a private dependency as well.

@marcusmueller
Copy link
Member

marcusmueller commented Jan 17, 2022 via email

@jdemel
Copy link
Contributor

jdemel commented Jan 17, 2022

@marcusmueller Ah! I interpreted it as a suggestion. I'd like to merge smth similar. The Debian patch switches from submodule to package. I'd like to keep submodule usage optional. For now, I assume this is the best way forward.

jdemel added a commit to jdemel/volk that referenced this issue Jan 22, 2022
Since cpu_features is available as a package in recent distributions,
we'd like to use the distro provided package.

We try to find the CMake CpuFeatures package and build against it, if we
can't find one, we try to use the git submodule. If both options fail,
we emit an error message.

Fix gnuradio#556

Signed-off-by: Johannes Demel <demel@uni-bremen.de>
@jdemel
Copy link
Contributor

jdemel commented Jan 22, 2022

@yurivict does #558 work for you?

Alesha72003 pushed a commit to Alesha72003/volk that referenced this issue May 15, 2024
Since cpu_features is available as a package in recent distributions,
we'd like to use the distro provided package.

We try to find the CMake CpuFeatures package and build against it, if we
can't find one, we try to use the git submodule. If both options fail,
we emit an error message.

Fix gnuradio#556

Signed-off-by: Johannes Demel <demel@uni-bremen.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CMake Related to the CMake builder build scripts Dependencies Related to problems in dependencies or their detection
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants