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

Draft: Use GNUInstallDirs for catkin packages #1185

Open
wants to merge 3 commits into
base: noetic-devel
Choose a base branch
from

Conversation

mikepurvis
Copy link
Member

@mikepurvis mikepurvis commented Mar 1, 2023

An initial piece of this much larger proposal.

I don't yet have a REP or an end-to-end demonstration of what is being achieved here, but the changes as they stand should be a no-op, as I believe we're just using standard variables whose default values are what was previously hard-coded. See:

https://cmake.org/cmake/help/v3.0/module/GNUInstallDirs.html

I'm testing with CMake 3.16.3 (Ubuntu Focal), but the documentation referenced is 3.0 as that's what catkin's minimum version is currently set at, and I don't want to bump that unless there's good reason to do so.


I've confirmed that catkin itself still builds, of course, and I've spot-checked a handful of small packages early in the dependency tree, for example:

$ BASE=$(pwd)/test-install
$ cmake ~/roscpp_core/cpp_common \
  -DCMAKE_INSTALL_PREFIX=/. \
  -DCMAKE_INSTALL_DATADIR=${BASE}/out/share \
  -DCMAKE_INSTALL_BINDIR=${BASE}/out/bin \
  -DCMAKE_INSTALL_LIBEXECDIR=${BASE}/out/lib \
  -DCMAKE_INSTALL_LIBDIR=${BASE}/out/lib \
  -DCMAKE_INSTALL_INCLUDEDIR=${BASE}/dev/include \
  -DCATKIN_INSTALL_INTO_PREFIX_ROOT=0
-- Using CATKIN_DEVEL_PREFIX: /home/mikepurvis/build/devel
-- Using CMAKE_PREFIX_PATH: /home/mikepurvis/catkin-install;/opt/ros/noetic
-- This workspace overlays: /home/mikepurvis/catkin-install;/opt/ros/noetic
[...]
-- Build files have been written to: /home/mikepurvis/build

$ make install
[100%] Built target cpp_common
Install the project...
-- Install configuration: ""
-- Installing: /home/mikepurvis/build/test-install/out/lib/pkgconfig/cpp_common.pc
-- Installing: /home/mikepurvis/build/test-install/out/share/cpp_common/cmake/cpp_commonConfig.cmake
-- Installing: /home/mikepurvis/build/test-install/out/share/cpp_common/cmake/cpp_commonConfig-version.cmake
-- Installing: /home/mikepurvis/build/test-install/out/share/cpp_common/package.xml
-- Installing: /home/mikepurvis/build/test-install/out/lib/libcpp_common.so
-- Installing: /home/mikepurvis/build/test-install/dev/include
-- Installing: /home/mikepurvis/build/test-install/dev/include/ros
-- Installing: /home/mikepurvis/build/test-install/dev/include/ros/exception.h
-- Installing: /home/mikepurvis/build/test-install/dev/include/ros/datatypes.h
-- Installing: /home/mikepurvis/build/test-install/dev/include/ros/platform.h
-- Installing: /home/mikepurvis/build/test-install/dev/include/ros/types.h
-- Installing: /home/mikepurvis/build/test-install/dev/include/ros/cpp_common_decl.h
-- Installing: /home/mikepurvis/build/test-install/dev/include/ros/header.h
-- Installing: /home/mikepurvis/build/test-install/dev/include/ros/macros.h
-- Installing: /home/mikepurvis/build/test-install/dev/include/ros/debug.h

$ grep test-install test-install/out/share/cpp_common/cmake/cpp_commonConfig.cmake
  set(cpp_common_INSTALL_PREFIX /home/mikepurvis/build/test-install)
  set(cpp_common_INSTALL_BINDIR /home/mikepurvis/build/test-install/out/bin)
  set(cpp_common_INSTALL_LIBDIR /home/mikepurvis/build/test-install/out/lib)
  set(cpp_common_INSTALL_DATADIR /home/mikepurvis/build/test-install/out/share)
  set(cpp_common_INSTALL_INCLUDEDIR /home/mikepurvis/build/test-install/dev/include)
if(NOT "/home/mikepurvis/build/test-install/dev/include;/usr/include " STREQUAL " ")
  set(_include_dirs "/home/mikepurvis/build/test-install/dev/include;/usr/include")
    elseif("${idir} " STREQUAL "/home/mikepurvis/build/test-install/dev/include ")
      get_filename_component(include "${cpp_common_DIR}/../../..//home/mikepurvis/build/test-install/dev/include" ABSOLUTE)
    foreach(path /home/mikepurvis/build/test-install/out/lib;/home/mikepurvis/catkin-install/lib;/opt/ros/noetic/lib)

In the Nix case (which is what I actually care about here), the CMake and pkgconfig files can be safely relocated after build to the dev output (equivalent of a debian libfoo-dev package) and they will still be able to find their libraries since they have been generated with absolute paths to them.

Remaining tasks before merging this:

  • Build at least to ros-base using catkin-tools, before and after this change, validate all generated CMake config changes are reasonable.
  • The same, but with the devel- rather than installspaces.
  • Decide if we like the <pkgname>_INSTALL_XXDIR naming (which is meant to ape what GNUInstallDirs looks like), or if it would make more sense to follow the <pkgname>_DIR convention already in use in a number of places (examples: ROS 1, ROS 2).
  • Decide if finding of package.xml files will be part of this change or a separate one (they have a similar issue to libraries in that they are no longer findable relative to the CMake config, since they're part of the runtime split).
  • Implement at least some of the companion changes in other packages to properly consume the new <pkg>_INSTALL_LIBDIR and <pkg>_INSTALL_DATADIR variables from their "extras" templates.

This is obviously a package that has to be modified pretty conservatively, so I'm open to suggestions for other means of validating this change and ensuring that it is safe.

@@ -286,7 +286,8 @@ function(_catkin_package)
set(DEVELSPACE TRUE)
set(INSTALLSPACE FALSE)

set(PROJECT_SPACE_DIR ${CATKIN_DEVEL_PREFIX})
set(PROJECT_SPACE_LIBDIR ${CATKIN_DEVEL_PREFIX}/lib)
set(PROJECT_SPACE_DATADIR ${CATKIN_DEVEL_PREFIX}/share)
Copy link
Member Author

Choose a reason for hiding this comment

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

These intentionally do not use the GNUInstallDirs vars, for two reasons:

  • The develspace is not a "real" FHS tree and has its own internal standard that shouldn't be impacted by whatever overrides come in from the CMAKE_INSTALL_XXXX variables.
  • If the GNUInstallDirs vars are set to absolute paths (as in the Nix build case), those absolute paths will be appended to the devel prefix, which is almost certainly not what is wanted here.

We could always check for absolute paths and only append in the case where they aren't, but then what to do when they are? I don't think adding branchiness here is worth whatever gains there might be, particularly when the develspace is dropped in ROS 2 anyway.

cmake/catkin_package.cmake Outdated Show resolved Hide resolved
@@ -76,6 +76,10 @@ else()
set(@PROJECT_NAME@_DEVEL_PREFIX "")
set(@PROJECT_NAME@_INSTALL_PREFIX @CMAKE_INSTALL_PREFIX@)
set(@PROJECT_NAME@_PREFIX ${@PROJECT_NAME@_INSTALL_PREFIX})
set(@PROJECT_NAME@_INSTALL_BINDIR @CMAKE_INSTALL_BINDIR@)
set(@PROJECT_NAME@_INSTALL_LIBDIR @CMAKE_INSTALL_LIBDIR@)
set(@PROJECT_NAME@_INSTALL_DATADIR @CMAKE_INSTALL_DATADIR@)
Copy link
Member Author

Choose a reason for hiding this comment

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

Could also be DATAROOTDIR, but nixpkgs seems to prefer DATADIR, so I went with that. They have the same value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant