Update include directory install location, and other updates #2915

sloretz · 2022-08-04T21:33:42Z

My goal was to update the header locations to match the new recommendations for overriding packages, but I got a little carried away. There seemed to be a little bit of incorrect information, but most of these changes are me removing unnecessary information. I made updates after reviewing about up to the Windows section.

I'd recommend this for backport to Humble.

Signed-off-by: Shane Loretz <[email protected]>

sloretz

Comments for reviewers about each change

sloretz · 2022-08-04T21:34:17Z

source/How-To-Guides/Ament-CMake-Documentation.rst

@@ -29,17 +29,16 @@ The basic outline of the ``CMakeLists.txt`` of an ament package contains:

 .. code-block:: cmake

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


Changed because the minimum CMake version in REP 2000 for Humble is 3.14.4

Do you want to add .4 at the end to prevent someone using 3.14.3 for example?

Why not 3.16 since that is what Ubuntu 20.04 LTS ships with?

Because then it would exclude mac users on Humble.

Are there macOS users who aren't using CMake as provided by brew? Does Xcode ship with CMake 3.14.4?

Is there features in 3.16 you

Are there macOS users who aren't using CMake as provided by brew? Does Xcode ship with CMake 3.14.4?

I'm not a MAC person, but honestly don't think anyone will allow merge that goes against REP2000. If there is a strong reason to bump to 3.16, then it could be proposed. Taking a look at the release notes for 3.15 and 3.16, I don't see any features that are dealbreakers. Do you?

Every newer CMake release is better than the last so it's always worth increasing the minimum requirement if possible. Our default position should be to use the latest version we can get away with which is almost certainly 3.16.

I'm a macOS developer (for non-ROS things) who has Xcode installed and cannot find an installation of CMake other than what brew provides which is always the latest release, currently 3.24.3. I'm guessing brew shipped 3.14.4 whenever that REP was written. I can't think of any circumstance under which a macOS developer would be locked into a specific version, especially something as old as 3.14.

Reading through REP2000, it acknowledges how there is no single stable version of CMake on macOS and how Homebrew and Pip are what we should expect users are using to get CMake. That reinforces my opinion that no one using Humble on macOS is actually limited to 3.14 so that's not a requirement we should impose on ourselves.

I don't know how to propose a change to the ROS REP, but I'm in alignment with using the newest common denominator; that is what the CMake developers recommend too.

sloretz · 2022-08-04T21:36:29Z

source/How-To-Guides/Ament-CMake-Documentation.rst

   project(my_project)

   ament_package()

 The argument to ``project`` will be the package name and must be identical to the package name in the ``package.xml``.

 The project setup is done by ``ament_package()`` and this call must occur exactly once per package.
-``ament_package()`` installs the ``package.xml``, registers the package with the ament index, and installs config (and possibly target) files for CMake so that it can be found by other packages using ``find_package``.
-Since ``ament_package()`` gathers a lot of information from the ``CMakeLists.txt`` it should be the last call in your ``CMakeLists.txt``.
-Although it is possible to follow calls to ``ament_package()`` by calls to ``install`` functions copying files and directories, it is simpler to just keep ``ament_package()`` the last call.


Simplified - I think it's enough to tell a user to make ament_package() be the last call. There are some allowable exceptions, but there's no benefit that I know of to doing so.

sloretz · 2022-08-04T21:40:53Z

source/How-To-Guides/Ament-CMake-Documentation.rst

-
- files added by appending to ``${PROJECT_NAME}_CONFIG_EXTRAS_POST``
-
- files added by ``CONFIG_EXTRAS_POST``


The only places I found these features used is once in rviz_rendering and once in rosidl. I don't think they're helpful to explain to a new user.

sloretz · 2022-08-04T21:41:49Z

source/How-To-Guides/Ament-CMake-Documentation.rst

-The following best practice is proposed:
-
- if you are building a library, put all headers which should be usable by clients and therefore must be installed into a subdirectory of the ``include`` folder named like the package, while all other files (``.c/.cpp`` and header files which should not be exported) are inside the ``src`` folder.
+There are two main targets to build: libraries and executables.


These changes are subjective grammar fixes, plus links to CMake docs.

sloretz · 2022-08-04T21:45:54Z

source/How-To-Guides/Ament-CMake-Documentation.rst


 .. code-block:: cmake

    target_include_directories(my_target
      PUBLIC
        $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
-        $<INSTALL_INTERFACE:include>)
-
-This adds all files in the folder ``${CMAKE_CURRENT_SOURCE_DIR}/include`` to the public interface during build time and all files in the include folder (relative to ``${CMAKE_INSTALL_DIR}``) when being installed.


This is pretty standard CMake stuff. I think the linked target_include_directories() explanation is enough.

sloretz · 2022-08-04T21:49:15Z

source/How-To-Guides/Ament-CMake-Documentation.rst

-
- The ``ament_export_targets`` macro exports the targets for CMake.
-  This is necessary to allow your library's clients to use the ``target_link_libraries(client my_library::my_library)`` syntax.
-  ``ament_export_targets`` can take an arbitrary list of targets named as ``EXPORT`` in an install call and an additional option ``HAS_LIBRARY_TARGET``, which adds potential libraries to environment variables.


Confusingly ament_export_targets takes a list of export files to declare, not targets.

sloretz · 2022-08-04T21:50:48Z

source/How-To-Guides/Ament-CMake-Documentation.rst

@@ -146,76 +106,49 @@ When building a reusable library, some information needs to be exported for down
      LIBRARY DESTINATION lib
      ARCHIVE DESTINATION lib
      RUNTIME DESTINATION bin
-      INCLUDES DESTINATION include


I removed this because target_include_directories() already took care of associating this include with the target.

Should we add a recommendation to include GNUInstallDirs before calling install such that users will just use the defaults. There's a few options here that have some advantages.
https://youtu.be/m0DwB4OvDXk?t=1657

With cmake 3.14 minimum support that we have bumped for humble, a simple call to install(TARGETS my_library EXPORT my_libraryTargets) seems to suffice.

sloretz · 2022-08-04T21:51:35Z

source/How-To-Guides/Ament-CMake-Documentation.rst

-  It installs the CMake files for the ``my_library`` target.
-  It is named exactly like the argument in ``ament_export_targets`` and could be named like the library.
-  However, this will then prohibit using the ``ament_target_dependencies`` way of including your library.
-  To allow for full flexibility, it is advised to prepend the export target with something like ``<target>Targets``.


I don't think this prevents using ament_target_dependencies.

I've ran into situations where cmake gets mad about calling ament_target_dependencies and target_link_libraries with a mixed use of specifying PUBLIC/PRIVATE/etc or not specifying it. The recommendation by CMake maintainers is to always specify PUBLIC/PRIVATE, hence users in ROS2 should be using target_link_libraries. It might be worthwhile mentioning this as a reason that pacakge maintainers should phase out usage of ament_target_dependencies.

sloretz · 2022-08-04T21:52:43Z

source/How-To-Guides/Ament-CMake-Documentation.rst

-The second macro marks the location of the installed library (this is done by the ``HAS_LIBRARY_TARGET`` argument in the call to ``ament_export_targets``).
-
-Some of the macros can take different types of arguments for non-target exports, but since the recommended way for modern Make is to use targets, we will not cover them here.
-Documentation of these options can be found in the source code itself.


I removed the explanation of these macros because I wouldn't recommend using them in new packages.

sloretz · 2022-08-04T21:58:48Z

source/How-To-Guides/Ament-CMake-Documentation.rst

+    # If using C
+    target_compile_features(my_library PUBLIC c_std_99)
+    # If using C++
+    target_compile_features(my_library PUBLIC cxx_std_17)


This is a little subjective, but it is helpful to set the C++ standard this way. Just last week I encountered a package where it set it's C++ version to 14, but it failed to compile because it had a dependency on a package using a C++ 17 feature in its header. If it had used target_compile_features() then it wouldn't have encountered that problem.

Modern cmake guidelines also recommend target-based properties, rather than globally setting them.

clalancette

Sorry @sloretz , I may have raced with you about some of these changes and hence these may not take your comments into account. Feel free to ignore comments if you feel they've already been addressed.

clalancette · 2022-08-04T21:38:05Z

source/How-To-Guides/Ament-CMake-Documentation.rst


-This will also include necessary headers, libraries and their dependencies, but in contrast to ``ament_target_dependencies`` it might not correctly order the dependencies when using overlay workspaces.
+That will make sure your target can include ``rclcpp``'s headers and link against its libraries.



While I'm totally onboard with recommending target_link_libraries, I do think we should mention ament_target_dependencies. It is still heavily used inside of the ROS 2 core, in our tutorials, and in the wider ROS ecosystem.

One thing I would mention is to prefer the alias targets when they exist. Alias targets are not used commonly, but have clear advantages.

On this, one of the reasons ament_target_dependencies is convenient is that libraries have adopted a convention to match the project name with the target name, and only generate one target. If we want to support libraries not following that convention, or having multiple targets, I think it would be very helpful to add a note on how you determine what the target names exported by a library actually are. Without any notes, we may find uses resistant to switch off of ament_target_dependencies when they can't find the target name. Thoughts?

clalancette · 2022-08-04T21:39:08Z

source/How-To-Guides/Ament-CMake-Documentation.rst


+- The ``ament_export_targets`` makes sure packages that depend on yours can use your library with ``target_link_libraries(downstream_target my_package::my_library)``.


Suggested change

- The ``ament_export_targets`` makes sure packages that depend on yours can use your library with ``target_link_libraries(downstream_target my_package::my_library)``.

- The ``ament_export_targets`` makes sure packages that depend on yours can use your library with ``target_link_libraries(downstream_target my_package::my_library)`` or ``ament_target_dependencies(downstream_target my_package)``.

clalancette · 2022-08-04T21:40:50Z

source/How-To-Guides/Ament-CMake-Documentation.rst


 - The last large install command installs the library.
-  Archive and library files will be exported to the lib folder, runtime binaries will be installed to the bin folder and the path to installed headers is ``include``.
+  Archive and library files will be exported to the lib folder, runtime binaries will be installed to the bin folder.


Hm. While this is strictly true based on the snippet above, it is not what we recommend for things that want to be run with ros2 run. Should we change the wording and/or the code snippet?

I agree making executables compatible with ros2 run is desireable.

clalancette · 2022-08-04T21:42:05Z

source/How-To-Guides/Ament-CMake-Documentation.rst

-.. code-block:: cmake
-
-    ament_export_include_directories(include)
-    ament_export_libraries(my_library)


Like my comments on ament_target_dependencies, while I'm on board with not needing this by default, a lot of the ecosystem still uses it. So I think we should document it, even if we label it as "deprecated".

clalancette · 2022-08-04T21:42:21Z

source/How-To-Guides/Ament-CMake-Documentation.rst

-ROS 2 targets compilers which comply with the C++14 and C99 standard until at least ``Crystal Clemmys``.
-Newer versions might be targeted in the future and are referenced `here <https://www.ros.org/reps/rep-2000.html>`__.
-Therefore it is customary to set the corresponding CMake flags:
+ROS 2 releases have minimum C and C++ standards, which cna be found in `REP-2000 <https://www.ros.org/reps/rep-2000.html>`__.


Suggested change

ROS 2 releases have minimum C and C++ standards, which cna be found in `REP-2000 <https://www.ros.org/reps/rep-2000.html>`__.

ROS 2 releases have minimum C and C++ standards, which can be found in `REP-2000 <https://www.ros.org/reps/rep-2000.html>`__.

clalancette · 2022-08-04T21:43:06Z

source/How-To-Guides/Ament-CMake-Documentation.rst

-    if(NOT CMAKE_C_STANDARD)
-      set(CMAKE_C_STANDARD 99)
-    endif()
-    if(NOT CMAKE_CXX_STANDARD)
-      set(CMAKE_CXX_STANDARD 14)
-    endif()
+    # If using C
+    target_compile_features(my_library PUBLIC c_std_99)
+    # If using C++
+    target_compile_features(my_library PUBLIC cxx_std_17)


Same comment here. We don't actually do this in any packages that I know of, so while we can mention it we should also mention the old ways of doing things.

clalancette · 2022-08-04T21:43:53Z

source/How-To-Guides/Ament-CMake-Documentation.rst

-Since Linux, Mac and Windows are all officially supported platforms, to have maximum impact any package should also build on Windows.
+Windows is an officially supported platform, so we recommend making sure your packages can build on it.
+
 The Windows library format enforces symbol visibility:
 Every symbol which should be used from a client has to be explicitly exported by the library (and data symbols need to be implicitly imported).

-To keep this compatible with Clang and GCC builds, it is advised to use the logic in `the GCC wiki <https://gcc.gnu.org/wiki/Visibility>`__.
-To use it for a package called ``my_library``:
+Use the logic in `the GCC wiki <https://gcc.gnu.org/wiki/Visibility>`__ to keep this compatible with Clang and GCC builds.
+Say your package creates a library named ``my_library``:

 - Copy the logic in the link into a header file called ``visibility_control.hpp``.



Actually, I think we should remove this entire section and just leave in the link to the Windows_Symbol_Visibility document below.

Perhaps we instead refer to the GenerateExportHeader documentation instead of using users to copy-paste some preprocessor code from the web?

Perhaps we instead refer to the GenerateExportHeader documentation instead of using users to copy-paste some preprocessor code from the web?

@wjwwood and I have talked about GenerateExportHeader before. I used to be in favor of using it, but his argument was users who build ROS in other build systems will have to create that header manually. I just recently had to do this myself to build a ROS 2 package with bazel.

Does ROS2 officially support users who want to use alternative build systems? If not then we shouldn't make our projects more cumbersome to maintain because of them.

Does ROS2 officially support users who want to use alternative build systems? If not then we shouldn't make our projects more cumbersome to maintain because of them.

AFAIK it's not officially supported. That's a good thought about the maintenance burden, but our experience has been they're rarely touched: see rmw's whose last change was 5 years ago.

Yeah, I'm with @wjwwood; I think the status quo is fine. Once written, these files never change, so the maintenance burden is zero. And when people do want to use build systems other than the official ones, they don't need to do anything special.

At the end of the day both approaches are ok, so it's up to the maintainers. As a maintainer, I personally prefer the static file, but either way can work.

If we're talking about documentation and what we recommend, I think we can just refer to both as options. 🤷

RFRIEDM-Trimble · 2022-08-15T18:28:14Z

source/How-To-Guides/Ament-CMake-Documentation.rst

-At least until ``Crystal Clemmys`` target names are not supported in the ``ament_target_dependencies`` macro.
-Sometimes it will be necessary to call the ``target_link_libaries`` CMake function.
-In the example of Eigen3, the call should then look like
+The recommended way is to use `target_link_libraries() <https://cmake.org/cmake/help/v3.14/command/target_link_libraries.html>`__ and modern CMake targets.


If you have access to cmake 3.23, then file sets are recommended by Craig Scott in his latest book. Do you think it's worth mentioning that newer feature?

The thing is that most of our users won't be on CMake 3.23 yet; Ubuntu Jammy ships with CMake 3.22. So I wouldn't add a mention of it yet.

clalancette · 2023-08-07T13:59:45Z

Given the work we've done in #3812 , our documentation is now up-to-date with our implementation. There are definitely further improvements listed in here, and that we'd like to make to the core, but I think we should update the documentation as we update our best practices. So with that said, I'm going to close this out as no longer relevant.

@sloretz If you disagree, please do feel free to reopen. We'll then need to rebase this and figure out which parts we want to move forward with.

Update include directory install location, and other updates

c151107

Signed-off-by: Shane Loretz <[email protected]>

sloretz self-assigned this Aug 4, 2022

sloretz commented Aug 4, 2022

View reviewed changes

clalancette requested changes Aug 4, 2022

View reviewed changes

RFRIEDM-Trimble reviewed Aug 15, 2022

View reviewed changes

gavanderhoorn mentioned this pull request Aug 21, 2022

Wrong package arrangment ros-controls/ros2_control#800

Closed

This was referenced Nov 16, 2022

ROS2 docs still recommend usage of deprecated ament_export_include_directories, _definitions, _libraries, _link_flags #3191

Open

Docs for using ament results in broken libraries by default #3192

Closed

clalancette mentioned this pull request Dec 12, 2022

Use target_link_libraries instead of ament_target_dependencies ros2/rosbag2#1202

Merged

clalancette closed this Aug 7, 2023


		- files added by appending to ``${PROJECT_NAME}_CONFIG_EXTRAS_POST``

		- files added by ``CONFIG_EXTRAS_POST``


		This will also include necessary headers, libraries and their dependencies, but in contrast to ``ament_target_dependencies`` it might not correctly order the dependencies when using overlay workspaces.
		That will make sure your target can include ``rclcpp``'s headers and link against its libraries.


		- The ``ament_export_targets`` makes sure packages that depend on yours can use your library with ``target_link_libraries(downstream_target my_package::my_library)``.

	ROS 2 releases have minimum C and C++ standards, which cna be found in `REP-2000 <https://www.ros.org/reps/rep-2000.html>`__.
	ROS 2 releases have minimum C and C++ standards, which can be found in `REP-2000 <https://www.ros.org/reps/rep-2000.html>`__.

Update include directory install location, and other updates #2915

Update include directory install location, and other updates #2915

Conversation

sloretz commented Aug 4, 2022 • edited Loading

sloretz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sloretz Aug 4, 2022 • edited Loading

Choose a reason for hiding this comment

sloretz Aug 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RFRIEDM-Trimble Nov 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RFRIEDM-Trimble Aug 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clalancette left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RFRIEDM-Trimble Aug 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clalancette commented Aug 7, 2023

sloretz commented Aug 4, 2022 •

edited

Loading

sloretz Aug 4, 2022 •

edited

Loading

sloretz Aug 4, 2022 •

edited

Loading

RFRIEDM-Trimble Nov 16, 2022 •

edited

Loading

RFRIEDM-Trimble Aug 15, 2022 •

edited

Loading

RFRIEDM-Trimble Aug 15, 2022 •

edited

Loading