Fix numerous inconsistencies in ament-cmake-docs w.r.t installation of linkage, and target installation by Ryanf55 · Pull Request #3805 · ros2/ros2_documentation

Ryanf55 · 2023-07-27T21:20:28Z

Nodes need a special RUNTIME directory to be recognized by ROS 2 CLI
The order of creating the export set and using it was not intuitive
target_link_libraries was missing linkage type which is not recommended
It did not show to install nodes and libraries at the same time
Header install location did not match colcon's recommendations for supporting overlays
HAS_LIBRARY_TARGET wasn't explained that it's only needed if you are installing libraries

It would be a good idea to make these changes to an autogenerated package and check the includes are still working as expected. I understand the other tutorials and ros2 pkg create could also use an update to match, and can do that work once we're happy with it.

This work was contributed on behalf of AeroVironment, Inc.

Ryanf55 · 2023-07-27T21:21:05Z

 .. code-block:: cmake

-   cmake_minimum_required(VERSION 3.5)
+   cmake_minimum_required(VERSION 3.14)


Let's require cmake 3.14 so we can take advantage of the default ARCHIVE install directory that GNUInstallDirs provides in CMake >=3.14. This can safely be backported to humble.

Ryanf55 · 2023-07-27T21:22:59Z

      PUBLIC
-        $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
-        $<INSTALL_INTERFACE:include>)
+        "$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>")


This matches what colcon recommends for the build interface: https://colcon.readthedocs.io/en/released/user/overriding-packages.html#install-headers-to-a-unique-include-directory

The install interface is set during installation instead, which is cleaner because you only need to type it once.With target_include_directories, if you have multiple libraries, the previous approach created a lot of code duplication.

I'm fairly certain we also need the INSTALL_INTERFACE generator, updated to be include/${PROJECT_NAME}. I can't remember the specifics, but I'm pretty sure you need both to handle all situations, and all of our core code currently does this. @sloretz do you remember why we need both the BUILD_INTERFACE and the INSTALL_INTERFACE generator here?

This comes from Craig Scott's book Professional CMake, 15th ed, section 27.10 recommended practices, page 455, which has recommendations on installing.

Removed the changes from this PR in b0fce5c, now it matches exactly what the colcon docs show for both setting the include directories for the target as well as installing the header files.

@sloretz do you remember why we need both the BUILD_INTERFACE and the INSTALL_INTERFACE generator here?

target_include_directories(foolib ... $<BUILD_INTERFACE:...> makes sure other targets in the same package are able to include headers from the current package's source space by just depending on foolib. $<INSTALL_INTERFACE:...> makes sure targets in downstream packages are able to include this package's headers from the install space by just depending on foolib.

Based on the docs it looks like we could safely replace $<INSTALL_INTERFACE:...> with install(TARGET ... INCLUDES DESTINATION ...). Both are adding the paths to the headers in the install space to INTERFACE_INCLUDE_DIRECTORIES on the installed target.

https://cmake.org/cmake/help/v3.14/command/target_include_directories.html

https://cmake.org/cmake/help/v3.14/command/install.html

All right, that's fair.

That said, I do think we should stick for now with making this PR update to what we are actually doing in our sources, which is to use the INSTALL generator. As I said in my overall comment, we can think about switching away from that, but that should be done in the code first, then the documentation can follow.

Ok, the PR reflects the current state of the system best, as-is.

Yep, it's just a matter of when/how you set the INTERFACE_INCLUDE_DIRECTORIES property.

Ryanf55 · 2023-07-27T21:24:43Z

+      TARGETS my_library my_node
      EXPORT my_libraryTargets
-      LIBRARY DESTINATION lib
-      ARCHIVE DESTINATION lib


CMake 3.14 minimum allows us to stop specifying LIBRARY and ARCHIVE dirs since they default to this when using GNUInstallDirs. Now, we only recommend changing values that MUST be overridden from defaults.

Ryanf55 · 2023-07-27T21:32:53Z

-      RUNTIME DESTINATION bin
-      INCLUDES DESTINATION include
+      RUNTIME DESTINATION lib/${PROJECT_NAME}
+      INCLUDES DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/${PROJECT_NAME}


Instead of hard coding include, I use the one from GNUInstallDirs. This could give more flexibility for packaging maintainers, but should not change current behavior.

This has been changed back due potential risks for GNUInstallDirs. Now, the only modification is the addition of ${PROJECT_NAME} which fixes the inconsistency with colcon

Ryanf55 · 2023-07-27T21:35:38Z

+Take special care to override the default RUNTIME directory for nodes. This allows the nodes to be started with ``ros2 run``.
+Additionally, the include directory is custom to support overlays in ``colcon``.
+
+Add all the nodes and libraries for your project to the ``TARGETS`` argument.


I feel like this is important to add and demonstrate, otherwise you get people calling install multiple times, once for each target, not realizing you can add multiple targets to the export set.

clalancette

Thanks for opening this! We definitely need this update.

I've added a couple of comments below.

clalancette · 2023-07-27T21:27:01Z

      PUBLIC
-        $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
-        $<INSTALL_INTERFACE:include>)
+        "$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>")


I'm fairly certain we also need the INSTALL_INTERFACE generator, updated to be include/${PROJECT_NAME}. I can't remember the specifics, but I'm pretty sure you need both to handle all situations, and all of our core code currently does this. @sloretz do you remember why we need both the BUILD_INTERFACE and the INSTALL_INTERFACE generator here?

clalancette · 2023-07-28T14:03:06Z

+
+.. code-block:: cmake
+
+    include(GNUInstallDirs)


I think we shouldn't do this, at least not yet.

That is, what I think we should do first is to update this document to reflect our current best practices. Since we don't use GNUInstallDirs in most places in the core sources, I would suggest not doing that here. Also, my understanding is that GNUInstallDirs will have some knock-on effects, like on RHEL and Windows (see ros2/rosidl#754). Because of those things, let's stick with the regular install stanza for now.

If we want to explore using GNUInstallDirs, we can definitely do that, but I think we'd want to have that conversation elsewhere, probably in a issue on https://github.com/ros2/ros2

Can do, I'd rather get the glaring issues fixed with header and executable installs not aligning with the needs of colcon overlays and ros2 CLI node search path, then we can cover GNUInstallDirs later. Thanks for the cross-reference.

Removed usage of GNUInstallDirs in b0fce5c

* Nodes need a special RUNTIME directory to be recognized by ROS 2 CLI * The order of creating the export set and using it was not intuitive * target_link_libraries was missing linkage type which is not recommended * It did not show to install nodes and libraries at the same time * The installation of headers was overly complex since the install directory is the same for all headers * HAS_LIBRARY_TARGET wasn't explained that it's only needed if you are installing libraries Signed-off-by: Ryan Friedman <ryanfriedman5410+github@gmail.com>

Signed-off-by: Ryan Friedman <ryanfriedman5410+github@gmail.com>

Signed-off-by: Chris Lalancette <clalancette@gmail.com>

clalancette

@Ryanf55 FYI, I just pushed a commit here which somewhat redoes what you did.

In particular:

I put us back to cmake minimum of 3.8, since that is what our ros2 pkg create generator creates right now.
I've removed most of the discussion about installing executables and nodes. While I do think we should add that in, I also think that it should be in an entirely new section explicitly discussing that. Once we get this in, I'll go ahead and open a new PR with those changes.
I ended up changing the RUNTIME DESTINATION back to bin for the library section. On Windows, it is actually imperative that libraries go into bin, otherwise they aren't considered DLL artifacts. This is the main reason why we need a separate section for executables, since they go into a different place.

With those changes, I'm happy with this, so I'll approve. But I'd like to hear your opinion on this PR as-is before I go ahead and merge.

Ryanf55 · 2023-07-31T18:32:31Z

@Ryanf55 FYI, I just pushed a commit here which somewhat redoes what you did.

In particular:

I put us back to cmake minimum of 3.8, since that is what our ros2 pkg create generator creates right now.

I've removed most of the discussion about installing executables and nodes. While I do think we should add that in, I also think that it should be in an entirely new section explicitly discussing that. Once we get this in, I'll go ahead and open a new PR with those changes.

I ended up changing the RUNTIME DESTINATION back to bin for the library section. On Windows, it is actually imperative that libraries go into bin, otherwise they aren't considered DLL artifacts. This is the main reason why we need a separate section for executables, since they go into a different place.

With those changes, I'm happy with this, so I'll approve. But I'd like to hear your opinion on this PR as-is before I go ahead and merge.

LGTM, sounds like a plan. Your changes look good.

clalancette · 2023-07-31T18:33:19Z

LGTM, sounds like a plan. Your changes look good.

Thanks for the quick review! I'll go ahead and merge this (and backport to the stable distributions), and then I'll open up a new PR in a little while here.

…f linkage, and target installation (#3805) * Fix numerous inconsistencies in ament-cmake-docs * Nodes need a special RUNTIME directory to be recognized by ROS 2 CLI * The order of creating the export set and using it was not intuitive * target_link_libraries was missing linkage type which is not recommended * It did not show to install nodes and libraries at the same time * The installation of headers was overly complex since the install directory is the same for all headers * HAS_LIBRARY_TARGET wasn't explained that it's only needed if you are installing libraries * Updates from review. Signed-off-by: Ryan Friedman <ryanfriedman5410+github@gmail.com> Signed-off-by: Chris Lalancette <clalancette@gmail.com> (cherry picked from commit 42ccd72)

…f linkage, and target installation (#3805) (#3810) * Fix numerous inconsistencies in ament-cmake-docs * Nodes need a special RUNTIME directory to be recognized by ROS 2 CLI * The order of creating the export set and using it was not intuitive * target_link_libraries was missing linkage type which is not recommended * It did not show to install nodes and libraries at the same time * The installation of headers was overly complex since the install directory is the same for all headers * HAS_LIBRARY_TARGET wasn't explained that it's only needed if you are installing libraries * Updates from review. Signed-off-by: Ryan Friedman <ryanfriedman5410+github@gmail.com> Signed-off-by: Chris Lalancette <clalancette@gmail.com> (cherry picked from commit 42ccd72) Co-authored-by: Ryan <ryanfriedman5410+github@gmail.com>

…f linkage, and target installation (#3805) (#3811) * Fix numerous inconsistencies in ament-cmake-docs * Nodes need a special RUNTIME directory to be recognized by ROS 2 CLI * The order of creating the export set and using it was not intuitive * target_link_libraries was missing linkage type which is not recommended * It did not show to install nodes and libraries at the same time * The installation of headers was overly complex since the install directory is the same for all headers * HAS_LIBRARY_TARGET wasn't explained that it's only needed if you are installing libraries * Updates from review. Signed-off-by: Ryan Friedman <ryanfriedman5410+github@gmail.com> Signed-off-by: Chris Lalancette <clalancette@gmail.com> (cherry picked from commit 42ccd72) Co-authored-by: Ryan <ryanfriedman5410+github@gmail.com>

Ryanf55 requested review from audrow and clalancette as code owners July 27, 2023 21:20

Ryanf55 commented Jul 27, 2023

View reviewed changes

clalancette requested changes Jul 28, 2023

View reviewed changes

Ryanf55 mentioned this pull request Jul 28, 2023

Proposal - Use GNUInstallDirs for CMake Installation ros2/ros2#1465

Open

Ryanf55 and others added 3 commits July 31, 2023 10:08

Remove usage of GNUInstallDirs due to potential RHEL/windows effects

fbcb6a4

Signed-off-by: Ryan Friedman <ryanfriedman5410+github@gmail.com>

Updates from review.

6bf7149

Signed-off-by: Chris Lalancette <clalancette@gmail.com>

clalancette force-pushed the fix-linkage-and-binary-library-ament-install branch from b0fce5c to 6bf7149 Compare July 31, 2023 18:19

clalancette approved these changes Jul 31, 2023

View reviewed changes

clalancette added the backport-all backport at reviewers discretion; from rolling to all versions label Jul 31, 2023

clalancette merged commit 42ccd72 into ros2:rolling Jul 31, 2023

Uh oh!

Conversation

Ryanf55 commented Jul 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ryanf55 Jul 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ryanf55 Jul 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ryanf55 Jul 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ryanf55 Jul 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ryanf55 Jul 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ryanf55 Jul 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clalancette left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ryanf55 Jul 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clalancette left a comment

Choose a reason for hiding this comment

Uh oh!

Ryanf55 commented Jul 31, 2023

Uh oh!

clalancette commented Jul 31, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Ryanf55 commented Jul 27, 2023 •

edited

Loading

Ryanf55 Jul 27, 2023 •

edited

Loading

Ryanf55 Jul 28, 2023 •

edited

Loading

Ryanf55 Jul 27, 2023 •

edited

Loading

Ryanf55 Jul 27, 2023 •

edited

Loading

Ryanf55 Jul 28, 2023 •

edited

Loading

Ryanf55 Jul 27, 2023 •

edited

Loading

Ryanf55 Jul 28, 2023 •

edited

Loading