Skip to content

Commit

Permalink
Cancel the force default libdir value.
Browse files Browse the repository at this point in the history 
On 64bits systems, this forces the installation path
to be lib/ rather than x86_64-linux-gnu/lib
Also, when passing in the if condition, the lib install
path becomes in the .pc file
${CMAKE_INSTALL_PREFIX}/${CMAKE_INSTALL_PREFIX}/lib
which will cause some issues later when using those informations.
  • Loading branch information
Francois Keith committed Sep 13, 2013
1 parent 53d898a commit fba6aef
Showing 1 changed file with 2 additions and 2 deletions.
4 changes: 2 additions & 2 deletions base.cmake
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,10 +113,10 @@ ENDMACRO(_CONCATENATE_ARGUMENTS OUTPUT)
# file).
#
MACRO(SETUP_PROJECT)
INCLUDE(cmake/GNUInstallDirs.cmake)
IF(NOT DEFINED CMAKE_INSTALL_LIBDIR)
SET(CMAKE_INSTALL_LIBDIR ${CMAKE_INSTALL_PREFIX}/lib)
SET(CMAKE_INSTALL_LIBDIR lib)
ENDIF()
INCLUDE(cmake/GNUInstallDirs.cmake)
SET(CMAKE_INSTALL_FULL_PKGLIBDIR ${CMAKE_INSTALL_FULL_LIBDIR}/${PROJECT_NAME})
SET(CMAKE_INSTALL_PKGLIBDIR ${CMAKE_INSTALL_LIBDIR}/${PROJECT_NAME})

Expand Down

10 comments on commit fba6aef

@olivier-stasse
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some issue with this commit.
I understand that ${CMAKE_INSTALL_PREFIX} before lib is wrong.
But if you put GNUInstallDirs.cmake in front then CMAKE_INSTALL_LIBDIR
it is already testing if CMAKE_INSTALL_LIBDIR exists
https://github.com/jrl-umi3218/jrl-cmakemodules/blob/master/GNUInstallDirs.cmake#L72

Then what is the point of lines 116-118 ?

Moreover lines 83-87 are implementing the "broken default value" from
https://github.com/jrl-umi3218/jrl-cmakemodules/blob/master/GNUInstallDirs.cmake.

Either we acknowledge that this should be kept for Debian packaging and then CMAKE_INSTALL_LIBDIR is set by the user,
or we acknowledge that the Debian package people should be the one fixing their CMAKE_INSTALL_LIBDIR and fix
our GNUInstallDirs.

@thomas-moulard
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate on the error you are encountering?

Yes, prefixing lib by the prefix was an error.

The other change (lib vs lib/ on Debian) is debatable.

My opinion is that the CMake is once more trying to be smarter than necessary and once more fail to do it properly.
I.e. only Debian packages have to care about multiarch because everything has to be in /usr. This is a huge assumption nobody cares about except package maintainers. So yes we need a libdir variable for package maintenance but most of the users, even on Debian does not care about multiarch. I.e. if you use RobotPkg on Ubuntu you certainly do not want to have lib/ as your libdir. If you want to cross-compile then you will bootstrap robotpkg once more somewhere else and this is done. No need for multiarch.

But once again, the goal is not fix all the issues in CMake so for me I am not against partially reverting this commit if it can help.

@francois-keith what was the issue when you wrote this one?

@olivier-stasse
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fully for keeping CMAKE_INSTALL_LIBDIR and the fix about having two times CMAKE_INSTALL_PREFIX prefixed is a good one.
This is just the behavior to set the variable when none is defined that I am against.
I think Debian maintainer have automatic tools to set this variable.
From the developer point of view this would help a bit.

@thomas-moulard
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think moving the include around is a mistake.

@olivier-stasse
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So just putting back the include should fix the issue.

@francois-keith
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thomas-moulard There were two problems with the commit:
1\ the installation path for the lib was automatically set to INSTALLATION_PATH/lib, without any regard to the architecture.
(aka, the libs were not longer installed in x86_64-linux-gnu/lib for my system, but rather lib).
2\ if CMAKE_INSTALL_LIBDIR is not defined, the install path becomes: CMAKE_INSTALL_LIBDIR / CMAKE_INSTALL_LIBDIR /lib (which did not make sense).

@thomas-moulard
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. This is a feature, not a bug ;) Cf my previous comment.
  2. This is a bug.

@AdamWill
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the record, this forcing of lib is also wrong for Fedora (and for Mageia and openSUSE and other similar distros, I believe) where shared libraries should be installed to lib64 on x86_64 (/usr/lib64 for a package build or /usr/local/lib64 for a sysadmin compile, but always the arch-specific final component). See roboptim/roboptim-core#101 (comment) .

@bchretien
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I updated GNUInstallDirs.cmake 5 months ago from upstream (cf. the differences). Some of the changes are directly related to multi-arch (cf. the commit message I pasted when updating it), so this may help solve the problem.

@bchretien
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: the highlight of the changes to GNUInstallDirs.cmake:

This patch fixes this issue by using "lib/" only if the user is installing the software in /usr, and uses "lib" in all the other cases.

Unless I missed something, this would solve the problem here, except if people on multi-arch distributions want to install the libraries to /usr/lib specifically for some reason.

Please sign in to comment.