fix headers destination installed by ament_auto_package

[HansRobo]

See ament#540. This is a copy of it.

[HansRobo]

Can you review this?
(I don't have the rights to assign reviewers)

@youtalk @xmfcx @VRichardJP

[youtalk]

Thank you!
Could you check the docker-build-and-push-main CI on autowarefoundation/autoware with this branch?

[HansRobo]

Running docker-build-and-push-main on my fork(no rights to running in awf repo)
https://github.com/HansRobo/autoware/actions/runs/9792120819

[youtalk]

https://github.com/HansRobo/autoware/actions/runs/9792120819/job/27037305038#step:8:1500

Error: buildx bake failed with: ERROR: failed to solve: failed to configure registry cache exporter: invalid reference format: repository name (HansRobo/autoware-buildcache) must be lowercase

It looks like the test failed by completely unrelated error. So I'll test it in my forked repository.
youtalk/autoware#63
https://github.com/youtalk/autoware/actions/runs/9802793025?pr=63

[youtalk]

The docker-build-and-push check using this change was failed.
@HansRobo We need to fix it to merge this PR.

/autoware/src/core/autoware_lanelet2_extension/autoware_lanelet2_extension/lib/query.cpp:31:10: fatal error: autoware_utils/autoware_utils.hpp: No such file or directory

https://github.com/youtalk/autoware/actions/runs/9802793025/job/27067969978?pr=63#step:5:15509

[HansRobo]

I have found that the current changes are insufficient, and am looking into ways to fix them.
The change in this PR also means that improper descriptions that did not previously cause errors will now cause errors, so some packages will need to be revised.

For example, previously ament_cmake_auto could be used even if it was missing in the build_tool_depend tag in package.xml, but once this PR is merged, it will no longer work properly without it.

[VRichardJP]

I am not sure I understand how this change solves the underlay/overlay problem, but I will assume it does.

The lanelet2_extension package compilation error is due to the include directory of autoware_utils package not being correctly exported:

  # export and install include directory of this package if it has one
  if(EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/include")
    ament_export_include_directories("include")
    install(DIRECTORY include/ DESTINATION include/${PROJECT_NAME})
  endif()

The files are installed (= copied) to include/${PROJECT_NAME}, but "exported" to install include directory. So what downstream packages include is $install_prefix/include (e.g. -isystem /home/sig/autoware/install/autoware_utils/include) instead of $install_prefix/include/$package_name (e.g. -isystem /home/sig/autoware/install/autoware_utils/include/autoware_utils). The fix is simple:

  # export and install include directory of this package if it has one
  if(EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/include")
    ament_export_include_directories("include/${PROJECT_NAME}")  # <- This!
    install(DIRECTORY include/ DESTINATION include/${PROJECT_NAME})
  endif()

[xmfcx]

@HansRobo Can you let me know if the following is somehow related to this?

https://autowarefoundation.github.io/autoware-documentation/main/contributing/coding-guidelines/ros-nodes/directory-structure/#exporting-headers

Here we are suggesting the developers to export their headers with the following structure in their include folder.

autoware_gnss_poser
└─ include
    └─ autoware
        └─ gnss_poser
            └─ exported_header.hpp

• autoware_gnss_poser/include folder should contain ONLY the autoware folder.
  • Rationale: When installing ROS debian packages, the headers are copied to the /opt/ros/$ROS_DISTRO/include/ directory. This structure is used to avoid conflicts with non-Autoware packages.
• autoware_gnss_poser/include/autoware folder should contain ONLY the gnss_poser folder.
  • Rationale: Similarly, this structure is used to avoid conflicts with other packages.

This is a manual suggestion and we are planning to enforce this structure for all packages within autoware directory.

Would this partially solve this issue? Or is it not related at all?

[HansRobo]

@xmfcx

This is a manual suggestion and we are planning to enforce this structure for all packages within autoware directory.
Would this partially solve this issue? Or is it not related at all?

The structure inside the include directory is not directly related to the issue.
I am only interested in the installation location of the include directory and the exported include paths.
In other words, the following parts are related and potentially dangerous:

Rationale: When installing ROS debian packages, the headers are copied to the /opt/ros/$ROS_DISTRO/include/ directory. This structure is used to avoid conflicts with non-Autoware packages.

If the sentense means the install results is like below, it is not appropriate.

/opt/ros/humble
└─ include
    └─ autoware
        └─ gnss_poser
            └─ exported_header.hpp

In conclusion, a secure install directory structure is as follows:

/opt/ros/humble
└─ include
    └─ autoware_gnss_poser
        └─ autoware
            └─ gnss_poser
               └─ exported_header.hpp

The reasons why the former is inappropriate and the latter is appropriate are explained in detail in ros2/ros2#1150.

Quickly, The most dangerous action here is to register /opt/ros/humble/include in the include path.
This include path can sometimes cause unexpected header file resolution.
For example, consider a situation where package A exists in both an underlay and an overlay, and you are working on package A in the overlay.
In this case, you would want your build to use package A from the overlay you worked on, rather than package A from the underlay installed with apt.
However, you register /opr/ros/humble/include in your include path, other packages that depend on package A may resolve header files by referencing the underlay package A instead of the overlay you are working on.
That's what I want to resolve in this pull-request.

P.S.

Your mention reminded me of this pull-request. Thank you.
I'll try to get back to it soon, although it's difficult to do so right away due to an upcoming team deadline.