Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use catkin_pkg to parse manifests #137

Merged
merged 1 commit into from
Jun 4, 2018
Merged

use catkin_pkg to parse manifests #137

merged 1 commit into from
Jun 4, 2018

Conversation

dirk-thomas
Copy link
Contributor

Similar to ros2/ros2cli#94.

Build Status

The remaining usage of ament_package in this repo (as well as in the whole ROS 2 code base) only accesses the templates stored in that package:

from ament_package.templates import get_environment_hook_template_path
from ament_package.templates import get_package_level_template_names
from ament_package.templates import get_package_level_template_path
from ament_package.templates import get_prefix_level_template_names
from ament_package.templates import get_prefix_level_template_path

In a follow up ticket this can either handled either way:

  • the templates could be moved into ament_cmake_core
  • everything beside the templates could be removed from ament_package (at which point the name should maybe be reconsidered).

@dirk-thomas dirk-thomas added the in review Waiting for review (Kanban column) label Jun 1, 2018
@dirk-thomas dirk-thomas self-assigned this Jun 1, 2018
Copy link
Contributor

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm,
If we move the templates we will be able to not release ament_package at all in Bouncy.
If we keep them here, I agree that renaming could be valuable

@dirk-thomas
Copy link
Contributor Author

I will go ahead and merge this patch.

Any comment about the proposed follow up of ament_package would be appreciated.

@dirk-thomas dirk-thomas merged commit 4b17b7d into master Jun 4, 2018
@dirk-thomas dirk-thomas deleted the use_catkin_pkg branch June 4, 2018 16:33
@ghost
Copy link

ghost commented Jun 4, 2018

This merge just broke my ros2 installation script (based on https://github.com/ros2/ros2/wiki/Linux-Development-Setup).

Should I now be installing catkin_pkg as a dependency before building ros2?

@dirk-thomas
Copy link
Contributor Author

dirk-thomas commented Jun 4, 2018

@johnmarkwayve Yes, you will need python3-catkin-pkg-modules (from Debian) or catkin_pkg (from PIP). I updated the instructions: Linux, macOS, Windows.

@ghost
Copy link

ghost commented Jun 4, 2018

@dirk-thomas Thanks, worked perfectly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in review Waiting for review (Kanban column)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants