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

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

Closed
fventuri opened this issue Dec 13, 2023 · 6 comments · Fixed by #742
Closed
Labels
CMake Related to the CMake builder build scripts

Comments

@fventuri
Copy link

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
Copy link
Contributor

jdemel commented Dec 17, 2023

I would expect that

volk/CMakeLists.txt

Lines 199 to 201 in f2fb33c

if (${CMAKE_INSTALL_LIBDIR} MATCHES lib64)
set(LIB_SUFFIX 64)
endif()

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
Copy link
Author

@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
Copy link
Contributor

jdemel commented Jan 7, 2024

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
Copy link
Author

fventuri commented Jan 8, 2024

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
Copy link
Contributor

ryanvolz commented Jan 8, 2024

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).

@willcode willcode added the CMake Related to the CMake builder build scripts label Jan 8, 2024
@jdemel
Copy link
Contributor

jdemel commented Jan 12, 2024

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

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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants