Update cpu features #631
Update cpu features #631
Conversation
- Update cpu_features to v0.9.0 - Added BUILD_EXECUTABLE for cpu_features to build list_cpu_features for github workflow tests - Added ENABLE_INSTALL to install cpu_feature for old behaviour - Renamed CpuFeature -> CpuFeatures to match new name Signed-off-by: Ashley Brighthope <[email protected]>
The else path can not be reached since the root CMake file aborts if CpuFeatures_FOUND if false Signed-off-by: Ashley Brighthope <[email protected]>
|
Related issue #629 |
CMakeLists.txt
Outdated
| @@ -144,6 +144,7 @@ if (VOLK_CPU_FEATURES) | |||
| FORCE) | |||
| set(BUILD_SHARED_LIBS_SAVED "${BUILD_SHARED_LIBS}") | |||
| set(BUILD_SHARED_LIBS OFF) | |||
| set(ENABLE_INSTALL ON) | |||
There was a problem hiding this comment.
I suggest to go for set(ENABLE_INSTALL OFF) because cpu_features should never be installed with VOLK. This causes issue #607 . Since we don't need the cpu_features library and don't expose any of its APIs, the NO install solution would be preferred.
There was a problem hiding this comment.
I did try leaving this off before I submit this PR, but it break's the static CI build. This make's sense since the finial stage link needs the cpu_feature library.
One solution to this would be to ENABLE_INSTALL for cpu_features if ENABLE_STATIC_LIBS is set
There was a problem hiding this comment.
I try to add a bit more context here. I suppose package maintainers, e.g. for Debian, explicitly add patches to make sure cpu_features is not installed together with with VOLK.
Furthermore, VOLK should not slip into a maintainer role for a dependency. Even if this implies to update the submodule pointer every once in a while.
As a consequence, we should try to:
- hide all dependencies as private and not expose them in our API (if possible)
- not install anything that is not directly VOLK.
Having written all of this, what would be a reasonable approach here?
It's "good news" that only the static build breaks. Right now, I see that the static test fails because one of the tests fails. I suggest to set ENABLE_INSTALL OFF and then we would be able to discuss the situation in more detail.
I would really prefer to not install anything related to cpu_features.
There was a problem hiding this comment.
All good, I've added the change.
As a follow up
It does the right thing when cpu_features is installed as a system package on Fedora distro., the volk shared library dynamical links to the the system cpu_features and it does not build the submodule version
There was a problem hiding this comment.
I asked in the gnuradio matrix chat. I received this hint from @marcusmueller
https://discourse.cmake.org/t/cmake-error-install-export-projecttargets-includes-target-xxx-which-requires-target-yyy-that-is-not-in-any-export-set/7344/3
There was a problem hiding this comment.
If I read this discussion correctly, this line:
target_link_libraries(volk_static PRIVATE CpuFeatures::cpu_features)though, I haven't had the time to fully understand it yet.
Signed-off-by: Ashley Brighthope <[email protected]>
ashley-b
force-pushed
the
fix/cpu_features_update
branch
from
ea3a5fd to
49c1581
Compare
This avoids issues in the static build since otherwise cmake will add cpu_features to the export set. Signed-off-by: Andrej Rode <[email protected]>
noc0lour
force-pushed
the
fix/cpu_features_update
branch
from
7fb4bf4 to
4925681
Compare
|
I hope you don't mind me adding another commit fixing the issues with the build. Though it would be useful to add a test building an "external" binary linking to the dynamic & static builds to see that everything works correctly with this linking. Maybe something for the issue tracker. |
Update cpu features
github workflow tests