-
Notifications
You must be signed in to change notification settings - Fork 280
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
base: noetic-devel
Are you sure you want to change the base?
Conversation
226589c
to
a134099
Compare
@@ -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) |
There was a problem hiding this comment.
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/templates/pkgConfig.cmake.in
Outdated
@@ -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@) |
There was a problem hiding this comment.
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.
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:
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 debianlibfoo-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:
<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).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).<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.