-
Notifications
You must be signed in to change notification settings - Fork 96
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
Align CMake args in RPMs with debs #617
Conversation
The multiarch story has changed quite a bit since I set CMAKE_INSTALL_LIBDIR back in 2015. In particular, the ability to package plain CMake packages means that we've been forced to accept the presence of library directories other than 'lib' in ROS. This change removes the CMAKE_INSTALL_LIBDIR override and lets GNUInstallDirs instruct some packages to use 'lib64'. A typical ROS package will still install directly to 'lib'. One of the cases driving this removal is the vendor package pattern in ROS 2, where the CMAKE_INSTALL_LIBDIR override is not passed through from the vendor package to the external project it's wrapping, so override isn't working in all cases. Better to let the library directory differ and individually fix the packages that are misbehaving, like the one that originally motivated the change that this reverts. Besides the removal of CMAKE_INSTALL_LIBDIR, this change also sets AMENT_PREFIX_PATH for ament_cmake packages, which is already the case in the debian generator. Besides setting SETUPTOOLS_DEB_LAYOUT, the CMake arguments between the platforms should now be much better aligned. Reverts 84e67e2
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.
The changes to ament_cmake and cmake templates look good but I have one question about the catkin template.
@@ -41,7 +41,6 @@ mkdir -p obj-%{_target_platform} && cd obj-%{_target_platform} | |||
-USYSCONF_INSTALL_DIR \ | |||
-USHARE_INSTALL_PREFIX \ | |||
-ULIB_SUFFIX \ | |||
-DCMAKE_INSTALL_LIBDIR="lib" \ |
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.
If this change is also applied to catkin packages, won't it cause a regression for libccd (assuming it is still being vendored in ROS)?
Are there other ROS one packages that follow a third party vendor pattern even if they don't use the ROS 2 vendor conventions?
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.
My guess is that a similar approach to the one we're taking in ROS 2 was taken in ROS 1 to mitigate the issue that originally motivated that change, and I just wasn't aware of it because it was debian-specific.
I'd be more concerned if we were actually building ROS 1 packages from these spec files, but as of today, we're not.
tl;dr - if it's still a problem, we'll need to find a better solution. This just wasn't the right thing to do.
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.
Thanks for adding more context. I wanted to check my assumption that there is no other change which fixes the issue #372 and that has borne out.
The multiarch story has changed quite a bit since I set CMAKE_INSTALL_LIBDIR back in 2015. In particular, the ability to package plain CMake packages means that we've been forced to accept the presence of library directories other than 'lib' in ROS.
This change removes the CMAKE_INSTALL_LIBDIR override and lets GNUInstallDirs instruct some packages to use 'lib64'. A typical ROS package will still install directly to 'lib'.
One of the cases driving this removal is the vendor package pattern in ROS 2, where the CMAKE_INSTALL_LIBDIR override is not passed through from the vendor package to the external project it's wrapping, so override isn't working in all cases. Better to let the library directory differ and individually fix the packages that are misbehaving, like the one that originally motivated the change that this reverts.
Besides the removal of CMAKE_INSTALL_LIBDIR, this change also sets AMENT_PREFIX_PATH for ament_cmake packages, which is already the case in the debian generator. Besides setting SETUPTOOLS_DEB_LAYOUT, the CMake arguments between the platforms should now be much better aligned.
Reverts #372