Skip to content

register package in ament index#14

Merged
dirk-thomas merged 3 commits intoros2:masterfrom
mikaelarguedas:register_ament_index
Jan 9, 2020
Merged

register package in ament index#14
dirk-thomas merged 3 commits intoros2:masterfrom
mikaelarguedas:register_ament_index

Conversation

@mikaelarguedas
Copy link
Member

Signed-off-by: Mikael Arguedas mikael.arguedas@gmail.com

Installing a package.xml installed is not enough for rosdep to detect the package.
Making this an ament_package allows it to be registered in the index and recognized by rosdep. This permits to remove hacks like setting the ROS_PACKAGE_PATH manually for the packages to be detected.

Relates to ros-infrastructure/rosdep#724 and osrf/docker_images#328

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
@dirk-thomas
Copy link
Member

Installing a package.xml installed is not enough for rosdep to detect the package.

Correct, the install prefix must also be listed in the environment variable AMENT_PREFIX_PATH (similar to the ROS_PACKAGE_PATH in ROS 1).

Automatically extending environment variables from a non-ament package is certainly a problem. That is one major reason why ament exists in the first place.

Since ros-infrastructure/rosdep#699 rosdep also supports the ament index for faster package discovery. That might be the easier way to get this working (see next paragraph).

Making this an ament_package allows it to be registered in the index and recognized by rosdep.

That change seems to be overkill just to register the package in the ament index. That registration is nothing else than installing an empty file into a specific location in the file system. The CMake logic of this package doesn't need to be fundamentally changed but only something like this be added:

# create an empty file with the basename ${PROJECT_NAME}, e.g. with file(WRITE ...)
install(
  FILES somewhere/${PROJECT_NAME}
  DESTINATION share/ament_index/resource_index/packages
)

See the logic in ament_cmake for reference: https://github.com/ament/ament_cmake/blob/83e1b8147e2eda7d805e45c163aea60230a4d160/ament_cmake_core/cmake/index/ament_index_register_package.cmake#L29 and https://github.com/ament/ament_cmake/blob/83e1b8147e2eda7d805e45c163aea60230a4d160/ament_cmake_core/cmake/index/ament_index_register_resource.cmake#L88

@mikaelarguedas
Copy link
Member Author

That change seems to be overkill just to register the package in the ament index. That registration is nothing else than installing an empty file into a specific location in the file system. The CMake logic of this package doesn't need to be fundamentally changed but only something like this be added:
Since ros-infrastructure/rosdep#699 rosdep also supports the ament index for faster package discovery. That might be the easier way to get this working (see next paragraph).

That was my initial guess as well. But installing a marker file is not sufficient as the only folders considered for ament index resources are the ones listed on the AMENT_PREFIX_PATH (in get_search_paths function).
Which led me to the same statement as you:

Automatically extending environment variables from a non-ament package is certainly a problem. That is one major reason why ament exists in the first place.

So I ended up converting the package to be a real ament package rather than a non ament package that pretends to be one (installing package.xml, marker file in the index and modifying AMENT_PREFIX_PATH).

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
@dirk-thomas
Copy link
Member

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@dirk-thomas
Copy link
Member

The change breaks the naming of the CMake config file which results in downstream packages being unable to find this package anymore.

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
@mikaelarguedas
Copy link
Member Author

The change breaks the naming of the CMake config file which results in downstream packages being unable to find this package anymore.

Sorry about that, 4007472 should address this. The issue seems to be that the cmake Module provided by this package was not added to the CMake module path anymore.
I didnt notice a config file naming inconsistency, @dirk-thomas can you clarify which files you are referring to ?


In general, the problem I am trying to solve is for consumers of the nightly archive (and consumers of source builds in general) to be able to build overlay and use rosdep to install the dependencies of the archive and of their overlays.
It looks like the current situation is:

  • ament packages can be found by rosdep
  • cmake packages (mostly third-party or vendor package) need both a package.xml, be registered in the ament index and be on the AMENT_PREFIX_PATH to be found
    • no cmake package seem to modify the AMENT_PREFIX_PATH
    • most of them dont register themselves in the ament resource index

Goal: all packages are identified by rosdep with minimal changes to the third-party packages

Potential solutions:

  • The current workaround is to: provide bogus rosdep rules for third-party packages + modify the ROS_PACKAGE_PATH for non-ament packages to be detected as "catkin" packages
  • The approach taken in this PR: change vendor packages to be ament packages removing the need for a ROS_PACKAGE_PATH hack. Such logic can be extended to the other packages
    • con: harder to integrate third party packages as the change is not just installing a package.xml but changing the "package structure" by making it an ament package
  • Alternatives:
    • just register packages in the ament index without modifying the AMENT_PREFIX_PATH
      • package not identified by rosdep in isolated build cases
    • provide more bogus rosdep rules in the docker image
      • does not work for people using the nightly archives directly or working with source builds
    • ?

@dirk-thomas
Copy link
Member

dirk-thomas commented Jan 2, 2020

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@dirk-thomas
Copy link
Member

Thanks for the patch. CI finally passed. Merging...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants