Make $<INSTALL_INTERFACE... use INCLUDE_INSTALL_DIR, and install headers to include/${PROJECT_NAME}

[sloretz]

The first commit is a bug fix. The exported targets currently always say the include directory is include/ regardless of INCLUDE_INSTALL_DIR.

The second commit is an enhancement supporting ros2/ros2#1150. When using ROS 2 and Colcon it's possible to have two versions of a package installed: typically one from the debian packages (like ros-[rosdistro]-fastcdr) and another in a from-source workspace. In Colcon terms this is called "overriding a package", and the /opt/ros/[rosdistro] folder would be called the underlay while the from-source workspace would be called the overlay. A user would do this when they want to try a newer version of a package without building everything from source.

When two or more packages install their headers to the same directory in an underlay, it becomes possible for packages in the overlay to accidentally find the headers of an overridden package in the underlay. This can cause build failures or undefined behavior at runtime depending on the differences between the headers in the overridden and overriding packages. The exact conditions this happens are a little complicated, so I'll skip writing it out here. In ROS 2 we're solving the issue by making every package install its headers to a unique folder using ${PROJECT_NAME}. That's what the second commit does here.

[richiprosima]

Build status:

• Linux
• Linux aarch64
• Mac
• Windows

[sloretz]

I forgot a trailing slash, so I pushed a new commit fixing it.

[richiprosima]

Build status:

• Linux
• Linux aarch64
• Mac
• Windows

[EduPonz]

See this answer.

[richiprosima]

Build status:

• Linux
• Linux aarch64
• Mac
• Windows

[richiprosima]

Build status:

• Linux
• Linux aarch64
• Mac
• Windows

[MiguelBarro]

After reviewing the ros2 issue I understand why you require these changes and how they work, but I don't think this is the way to go.

In the issue I've proposed an alternative solution that only requires modifying colcon behavior to make it more CMake consistent and robust.

I'm open to whatever the ROS2 community decides on this subject, thus the pull request will remain open until
consensus is achieved.

[MiguelBarro]

We used to unset auxiliary variables like _include_dir when are no longer necessary.

[sloretz]

Unset _include_dir in 0e54e5d

[MiguelBarro]

The merit of these changes for ROS2 ecosystem have been justified on the associated issue.
LGTM

[richiprosima]

Build status:

• Linux
• Linux aarch64
• Mac
• Windows

[MiguelBarro]

Note that these changes are required also in dependencies solved via vendor packages.
For example, Fast-DDS depends on the package foonathan memory which is installed (if not available via) the auxiliary package foonathan memory vendor.
The memory package should be modified to accept the APPEND_PROJECT_NAME_TO_INCLUDEDIR option via:

• pull request agains the memory repo (with the corresponding backports)
• update of the CMakeLists.txt.patch in the vendor to introduce the change in the memory repo.
  The memory vendor package should be modified to accept the APPEND_PROJECT_NAME_TO_INCLUDEDIR option and pass it to the external project.

[sloretz]

Note that these changes are required also in dependencies solved via vendor packages.
For example, Fast-DDS depends on the package foonathan memory which is installed (if not available via) the auxiliary package foonathan memory vendor.

Good news, the foonathan/memory package already installs its headers to a unique include directory (see how it exports the include directories), so no changes need to be made to foonathan_memory_vendor 🎉

[MiguelBarro]

Good news, the foonathan/memory package already installs its headers to a unique include directory (see how it exports the include directories), so no changes need to be made to foonathan_memory_vendor 🎉

Right!!! 😎 Jonathan is no fool and has saved us some work 🎉