gnuradio / volk

The Vector Optimized Library of Kernels
http://libvolk.org
GNU Lesser General Public License v3.0
557 stars 202 forks source link

volk CMakeLists.txt uses CMAKE_INSTALL_LIBDIR without including the 'GNUInstallDirs' module #723

Closed fventuri closed 10 months ago

fventuri commented 11 months ago

I am running Linux Fedora Core here, where for 64bit libraries LIBDIR is expected have the suffix '64' (i.e. /usr/local/lib64). I noticed today that instead volk installed the shared library and the cmake config files under /usr/local/lib.

I then looked at the volk configuration for cmake (CMakeLists.txt), and I saw that there's code there to decide between lib vs lib64 (https://github.com/gnuradio/volk/blob/main/CMakeLists.txt#L205-L210), however that codes uses the variable CMAKE_INSTALL_LIBDIR. According to the cmake documentation (https://cmake.org/cmake/help/latest/module/GNUInstallDirs.html) that variable is defined in the GNUInstallDirs module, but I don't see that cmake module included anywhere, and indeed when I added a message() statement to print the value of CMAKE_INSTALL_LIBDIR right before the check above, the value turned out to be empty.

Thanks, Franco

jdemel commented 11 months ago

I would expect that https://github.com/gnuradio/volk/blob/f2fb33c1c1f0998f4ea38c8e6a2d15219a2ad49c/CMakeLists.txt#L199-L201 needs an update here.

e.g. we could use something like:

include(GNUInstallDirs)

How does that affect custom install prefixes? e.g. a conda prefix might not differentiate between the different suffixes. What happens if this CMake module is used on Windows? I saw a comment regarding homebrew.

fventuri commented 11 months ago

@jdemel - instead of including the module GNUInstallDirs, another option could be to follow the approach used by GNU Radio, where it looks like they have a special case for the RedHat/Slackware family of distributions (which Fedora is part of): https://github.com/gnuradio/gnuradio/blob/main/cmake/Modules/GrPlatform.cmake#L38-L49

In that case you'll also need the code above that, where they set the variables 'REDHAT' and 'SLACKWARE'.

Franco

jdemel commented 10 months ago

I would really prefer to use GNUInstallDirs here. Trying to fix this like GR does, has the potential to open a can of worms.

How would this go along with a conda environment? conda envs seem to use suffix-less folders. How should CMake be configured in these cases?

fventuri commented 10 months ago

The other day I thought about asking @willcode about this issue, since I think he runs Fedora Core too, so he might already have figured out a way to deal with the 'lib64' suffix in Volk.

As per conda my go-to person is @ryanvolz ,since he knows it inside and out, and he might have a recommendation about using GNUInstallDirs.

Franco

ryanvolz commented 10 months ago

I've had to override LIB_SUFFIX for a long time for Conda to get the correct behavior, since all of that logic implicitly assumes that the installation environment is on the build machine. Without looking at the history, I assume that LIB_SUFFIX is from a time before GNUInstallDirs existed, and if that code is being updated it would be best to do away with LIB_SUFFIX entirely and use the CMAKE_INSTALL_LIBDIR that comes from including GNUInstallDirs. I still might have to override that in a conda recipe, but at least that's a standard CMake thing that's needed for a lot of projects and not a GR-and-friends thing.

So bottom line, include GNUInstallDirs to fix the immediate problem, and if you want to completely modernize then ditch LIB_SUFFIX entirely. Packagers will be able to adapt regardless (if they are aware of any changes).

jdemel commented 10 months ago

GNUInstallDirs seems to be the way to go. From the docs:

CMAKE_INSTALL_LIBDIR

object code libraries (lib or lib64)

On Debian, this may be lib/<multiarch-tuple> when [CMAKE_INSTALL_PREFIX](https://cmake.org/cmake/help/latest/variable/CMAKE_INSTALL_PREFIX.html#variable:CMAKE_INSTALL_PREFIX) is /usr.

I wanted to write about what to do in detail but decided a PR with the necessary changes is actually the faster way. Thus see #742