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

Fix ROS2 launch file install rule not installing launch subfolder #243

Merged
merged 3 commits into from
Jul 12, 2023
Merged

Fix ROS2 launch file install rule not installing launch subfolder #243

merged 3 commits into from
Jul 12, 2023

Conversation

memtech3
Copy link
Contributor

@memtech3 memtech3 commented Jul 7, 2023

Public-Facing Changes

Fix ROS2 launch file install rule not installing launch subfolder

Description

ROS2 launch file snippet in README.md had incorrect path to foxglove bridge launch file <include file="$(find-pkg-share foxglove_bridge)/launch/foxglove_bridge_launch.xml">. Path should be <include file="$(find-pkg-share foxglove_bridge)/foxglove_bridge_launch.xml">

Fixes #242
Fixes FG-4181

ROS2 launch file snippet had incorrect path to foxglove bridge launch file (../../launch/foxglove_bridge_launch instead of ../../foxglove_bridge_launch)
@CLAassistant
Copy link

CLAassistant commented Jul 7, 2023

CLA assistant check
All committers have signed the CLA.

@memtech3
Copy link
Contributor Author

memtech3 commented Jul 7, 2023

Fixes #242

@memtech3
Copy link
Contributor Author

memtech3 commented Jul 7, 2023

ROS Melodic Test fails during step make melodic-test with message

ERROR: the following packages/stacks could not have their rosdep keys resolved
to system dependencies:
foxglove_bridge: Cannot locate rosdep definition for [ros_babel_fish]
The command '/bin/sh -c . /opt/ros/$ROS_DISTRO/setup.sh &&     apt-get update && rosdep update && rosdep install -y       --from-paths         src       --ignore-src     && rm -rf /var/lib/apt/lists/*' returned a non-zero code: 1
make: *** [Makefile:13: melodic] Error 1

Error: Process completed with exit code 2.

Is this a foxglove_bridge bug?

Copy link
Collaborator

@achim-k achim-k left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

While this change is technically correct, I think it would be better to fix the install rule by removing the trailing slash on line 258. The launch file should be installed in the launch directory, but the current install rules installed it in the package share root directory.

install(DIRECTORY ros2_foxglove_bridge/launch/
DESTINATION share/${PROJECT_NAME}/
)

@achim-k
Copy link
Collaborator

achim-k commented Jul 7, 2023

ROS Melodic Test fails during step make melodic-test with message

ERROR: the following packages/stacks could not have their rosdep keys resolved
to system dependencies:
foxglove_bridge: Cannot locate rosdep definition for [ros_babel_fish]
The command '/bin/sh -c . /opt/ros/$ROS_DISTRO/setup.sh &&     apt-get update && rosdep update && rosdep install -y       --from-paths         src       --ignore-src     && rm -rf /var/lib/apt/lists/*' returned a non-zero code: 1
make: *** [Makefile:13: melodic] Error 1

Error: Process completed with exit code 2.

Is this a foxglove_bridge bug?

No, this is due to melodic being EOL. I have pushed a commit that adds --include-eol-distros to rosdep update but it's not merged yet.

Edit: Merged a PR that fixes melodic CI builds

achim-k added a commit that referenced this pull request Jul 7, 2023
### Public-Facing Changes

Use `--include-eol-distros` for `rosdep` to fix melodic builds

### Description
Fixes melodic CI builds since melodic is EOL

See also
#243 (comment)
@memtech3
Copy link
Contributor Author

Thanks for the contribution!

While this change is technically correct, I think it would be better to fix the install rule by removing the trailing slash on line 258. The launch file should be installed in the launch directory, but the current install rules installed it in the package share root directory.

install(DIRECTORY ros2_foxglove_bridge/launch/
DESTINATION share/${PROJECT_NAME}/
)

I've updated my branch with that change

@memtech3
Copy link
Contributor Author

Change might have made the CI tests failed, investigating

CMakeLists.txt Outdated
@@ -256,7 +256,7 @@ elseif(ROS_BUILD_TYPE STREQUAL "ament_cmake")
RUNTIME DESTINATION bin
)
install(DIRECTORY ros2_foxglove_bridge/launch/
DESTINATION share/${PROJECT_NAME}/
DESTINATION share/${PROJECT_NAME}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you have to remove the trailing slash one line above (L258)

@achim-k achim-k changed the title Update README.md ROS2 launch file snippet Fix ROS2 launch file install rule not installing launch subfolder Jul 12, 2023
@achim-k achim-k enabled auto-merge (squash) July 12, 2023 13:40
@achim-k
Copy link
Collaborator

achim-k commented Jul 12, 2023

I have added the required change. Thanks for your contribution @memtech3!

@achim-k achim-k merged commit a0ee79d into foxglove:main Jul 12, 2023
11 checks passed
@memtech3 memtech3 deleted the patch-1 branch July 12, 2023 15:28
@memtech3
Copy link
Contributor Author

I have added the required change. Thanks for your contribution @memtech3!

Thanks! Can you explain the cmake behavior? I'm a little new to C++ and ROS2

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

Successfully merging this pull request may close these issues.

ROS2 Launch File Snippet Incorrect
3 participants