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

Skip install when not building rosbag2_performance_benchmarking #1242

Merged
merged 1 commit into from Feb 3, 2023

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Feb 2, 2023

This fixes a bug where rosbag2_performance_benchmarking gets installed to the ament resource index when it's not built, and that causes rosdep to fail to find rosbag2_performance_benchmarking_msgs.

Example:

ERROR: the following packages/stacks could not have their rosdep keys resolved
to system dependencies (ROS distro is not set. Make sure `ROS_DISTRO` environment variable is set, or use `--rosdistro` option to specify the distro, e.g. `--rosdistro indigo`):
rosbag2_performance_benchmarking: Cannot locate rosdep definition for [rosbag2_performance_benchmarking_msgs]

The problem is the ament_package() call is still run when rosbag2_performance_benchmarking is not built, however it's dependency rosbag2_performance_benchmarking_msgs does not run ament_package(). This causes rosdep to think rosbag2_performance_benchmarking is installed and needs it's dependencies, but the dependency rosbag2_performance_benchmarking_msgs can't be found.

I fixed it by inverting the logic and putting it at the top of the file with a return() statement to make sure none of the logic is run when skipping this package.

…marking

Signed-off-by: Shane Loretz <sloretz@google.com>
@sloretz sloretz added the bug Something isn't working label Feb 2, 2023
@sloretz sloretz self-assigned this Feb 2, 2023
@sloretz sloretz requested a review from a team as a code owner February 2, 2023 22:05
@sloretz sloretz requested review from MichaelOrlov and jhdcs and removed request for a team February 2, 2023 22:05
sloretz added a commit to sloretz/drake-ros that referenced this pull request Feb 2, 2023
Copy link
Collaborator

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

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

No concerns here

@sloretz
Copy link
Contributor Author

sloretz commented Feb 3, 2023

CI (build: --packages-up-to rosbag2_performance_benchmarking test: --packages-select rosbag2_performance_benchmarking rosbag2_performance_benchmarking_msgs)

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

sloretz added a commit to sloretz/drake-ros that referenced this pull request Feb 3, 2023
@emersonknapp emersonknapp merged commit 8044dba into rolling Feb 3, 2023
@delete-merged-branch delete-merged-branch bot deleted the sloretz__skip_install_when_not_building branch February 3, 2023 04:25
carlossvg pushed a commit to carlossvg/rosbag2 that referenced this pull request Feb 13, 2023
…marking (ros2#1242)

Signed-off-by: Shane Loretz <sloretz@google.com>
Signed-off-by: Carlos San Vicente <carlos.sanvicente@apex.ai>
EricCousineau-TRI added a commit to RobotLocomotion/drake-ros that referenced this pull request Feb 16, 2023
The APIs we're using changed in ros2/rclcpp#2066, which
is released in rclcpp 18.0.0 in ROS 2 rolling

Additionally works around ros2/rosbag2#1242

Co-authored-by: Eric Cousineau <eric.cousineau@tri.global>
carlossvg pushed a commit to carlossvg/rosbag2 that referenced this pull request Feb 27, 2023
…marking (ros2#1242)

Signed-off-by: Shane Loretz <sloretz@google.com>
Signed-off-by: Carlos San Vicente <carlos.sanvicente@apex.ai>
MichaelOrlov added a commit that referenced this pull request Feb 28, 2023
…nchmarking (#1250)

* Add thread pool for benchmark_publishers execution

Signed-off-by: Carlos San Vicente <carlos.sanvicente@apex.ai>

* Default the number of threads to the number of publishers

Signed-off-by: Carlos San Vicente <carlos.sanvicente@apex.ai>

* Skip ament_package() call when not building rosbag2_performance_benchmarking (#1242)

Signed-off-by: Shane Loretz <sloretz@google.com>
Signed-off-by: Carlos San Vicente <carlos.sanvicente@apex.ai>

* Update rosbag2_performance/rosbag2_performance_benchmarking/include/rosbag2_performance_benchmarking/config_utils.hpp

Co-authored-by: Michael Orlov <morlovmr@gmail.com>
Signed-off-by: Carlos San Vicente <carlos.sanvicente@apex.ai>

* Update rosbag2_performance/rosbag2_performance_benchmarking/include/rosbag2_performance_benchmarking/thread_pool.hpp

Co-authored-by: Michael Orlov <morlovmr@gmail.com>
Signed-off-by: Carlos San Vicente <carlos.sanvicente@apex.ai>

* Update rosbag2_performance/rosbag2_performance_benchmarking/src/benchmark_publishers.cpp

Co-authored-by: Michael Orlov <morlovmr@gmail.com>
Signed-off-by: Carlos San Vicente <carlos.sanvicente@apex.ai>

* Update rosbag2_performance/rosbag2_performance_benchmarking/include/rosbag2_performance_benchmarking/thread_pool.hpp

Co-authored-by: Michael Orlov <morlovmr@gmail.com>
Signed-off-by: Carlos San Vicente <carlos.sanvicente@apex.ai>

* Update rosbag2_performance/rosbag2_performance_benchmarking/include/rosbag2_performance_benchmarking/thread_pool.hpp

Co-authored-by: Michael Orlov <morlovmr@gmail.com>
Signed-off-by: Carlos San Vicente <carlos.sanvicente@apex.ai>

* Address reviewer comments

Signed-off-by: Carlos San Vicente <carlos.sanvicente@apex.ai>

* Address reviewer comments

Signed-off-by: Carlos San Vicente <carlos.sanvicente@apex.ai>

* Address reviewer comments

Signed-off-by: Carlos San Vicente <carlos.sanvicente@apex.ai>

---------

Signed-off-by: Carlos San Vicente <carlos.sanvicente@apex.ai>
Signed-off-by: Shane Loretz <sloretz@google.com>
Co-authored-by: Shane Loretz <sloretz@google.com>
Co-authored-by: Michael Orlov <morlovmr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants