Skip to content

Comments

update for MediaPlugin#68

Merged
YoheiKakiuchi merged 3 commits intoIRSL-tut:mainfrom
YoheiKakiuchi:pr_up000
Nov 25, 2025
Merged

update for MediaPlugin#68
YoheiKakiuchi merged 3 commits intoIRSL-tut:mainfrom
YoheiKakiuchi:pr_up000

Conversation

@YoheiKakiuchi
Copy link
Member

No description provided.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds support for building the MediaPlugin by installing required multimedia dependencies and enabling the -DBUILD_MEDIA_PLUGIN=ON CMake flag. It also introduces two new Dockerfiles for SSH and Gazebo functionality.

  • Adds multimedia library dependencies (libpulse, libsndfile, gstreamer) required for MediaPlugin
  • Enables BUILD_MEDIA_PLUGIN flag in catkin build configuration for ROS 1 distributions
  • Introduces Dockerfile.add_ssh and Dockerfile.add_gazebo for additional container capabilities

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.

File Description
make_ws.sh Adds ROS2 branch selection logic, installs multimedia dependencies for MediaPlugin, and includes commented build commands
docker_files/Dockerfile.add_ssh New Dockerfile that adds SSH server support with configurable port
docker_files/Dockerfile.add_gazebo New Dockerfile that adds Gazebo simulation packages
Dockerfile.build_system.merged Installs multimedia dependencies and enables BUILD_MEDIA_PLUGIN flag in catkin config

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@@ -106,7 +107,7 @@ RUN apt update -q -qq && \
### Build using colcon
#RUN /bin/bash -c "source /opt/ros/${ROS_DISTRO}/setup.bash && colcon build --parallel-workers 1 --merge-install --event-handlers console_direct+ desktop_notification- log_command+ status- --packages-up-to irsl_choreonoid"
RUN if [ "${ROS_DISTRO}" == "noetic" -o "${ROS_DISTRO}" == "one" ]; then \
Copy link

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

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

The condition checks for "one" as a ROS distro, but there is no ROS distribution called "one". This appears to be a typo and should likely be removed or corrected to an actual ROS 1 distribution name like "melodic". This is inconsistent with standard ROS distribution naming.

Suggested change
RUN if [ "${ROS_DISTRO}" == "noetic" -o "${ROS_DISTRO}" == "one" ]; then \
RUN if [ "${ROS_DISTRO}" == "noetic" ]; then \

Copilot uses AI. Check for mistakes.

### install choreonoid
wget https://raw.githubusercontent.com/IRSL-tut/irsl_choreonoid/main/config/dot.rosinstall
if [ "$ROS_DISTRO" = "noetic" -o "$ROS_DISTRO" = "one" ]; then
Copy link

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

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

The condition checks for "one" as a ROS distro, but there is no ROS distribution called "one". This appears to be a typo. Did you mean to check for "melodic" or another specific ROS 1 distribution? The current code will treat "one" as a noetic-equivalent distribution.

Suggested change
if [ "$ROS_DISTRO" = "noetic" -o "$ROS_DISTRO" = "one" ]; then
if [ "$ROS_DISTRO" = "noetic" ]; then

Copilot uses AI. Check for mistakes.
else \
sudo apt install -q -qq -y python-catkin-tools libreadline-dev ; \
fi && \
sudo apt install -q -qq -y libpulse-dev libsndfile-dev gstreamer1.0-libav libgstreamer-plugins-base1.0-dev libgstreamer1.0-dev && \
Copy link

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

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

This line is missing proper continuation handling. The previous line ends with && \ but this line doesn't continue that pattern consistently with the surrounding code block structure. Consider adding sudo prefix to maintain consistency with the other apt install commands in the same RUN block.

Copilot uses AI. Check for mistakes.
rosdep install -y -q -r --ignore-src --from-path src/choreonoid_ros src/irsl_choreonoid_ros src/cnoid_cgal && \
apt clean && \
rm -rf /var/lib/apt/lists/
rosdep install -y -q -r --ignore-src --from-path src/choreonoid_ros src/irsl_choreonoid_ros src/cnoid_cgal
Copy link

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

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

The removal of the line continuation && \ at the end of this line breaks the multi-line command structure. The original code had apt clean and rm -rf /var/lib/apt/lists/ as part of the same command chain. These cleanup commands should either be restored or the script should explicitly handle the continuation.

Suggested change
rosdep install -y -q -r --ignore-src --from-path src/choreonoid_ros src/irsl_choreonoid_ros src/cnoid_cgal
rosdep install -y -q -r --ignore-src --from-path src/choreonoid_ros src/irsl_choreonoid_ros src/cnoid_cgal && \
sudo apt clean && sudo rm -rf /var/lib/apt/lists/*

Copilot uses AI. Check for mistakes.
@YoheiKakiuchi YoheiKakiuchi merged commit 5bfbf8c into IRSL-tut:main Nov 25, 2025
7 of 9 checks passed
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.

1 participant