osrf / docker_images

A repository to hold definitions of docker images maintained by OSRF
Apache License 2.0
579 stars 172 forks source link

ros_entrypoint.sh incorrectly forwards positional arguments (ROS1) #626

Closed h33p closed 2 years ago

h33p commented 2 years ago

Try the following:

docker run -it --rm ros:melodic bash -h

Output:

/tmp/setup.sh.fjjyyTaN60: line 1: usage:: command not found

It appears that the entire bash -h bit gets passed into /ros/melodic/setup.bash, since there are no arguments in the source line, and these arguments are then forwarded to _setup_util.py, and specifically the -h argument leads to the script to malfunction and output usage information. Editing ros_entrypoint.sh as follows seems to solve the issue:

#!/bin/bash
set -e

# setup ros environment
args="$@"
set --
source "/opt/ros/$ROS_DISTRO/setup.bash"
exec $args
ruffsl commented 2 years ago

Interesting find. My shell foo isn't familiar with all the ramifications of such a subtle but significant change to our entrypoint.

Perhaps I could get some second opinions? CC @tfoote @nuclearsandwich @mikaelarguedas @yosifkit @tianon

For comparison, here is the current default entrypoint:

https://github.com/osrf/docker_images/blob/c5d0709fd289e516a1c501c2120fcf1c08da6fd8/ros/rolling/ubuntu/jammy/ros-core/ros_entrypoint.sh#L1-L6

tianon commented 2 years ago

The problem with args="$@" ... exec $args is that space splitting will be affected (you can replicate this via docker run ... echo ' foo bar ' -- put even more spaces between those words for making it even more obvious; using $args like this is the same as dropping the 's there, which will just always print exactly foo bar no matter how many spaces you put in the command line :sweat_smile:).

I'd suggest one of the following as a workaround (depending on what setup.bash will tolerate - in my own personal order of preference):

#!/bin/bash
set -e

# setup ros2 environment
source "/opt/ros/$ROS_DISTRO/setup.bash" -- # adding an explicit (harmless) argument to reset "$@" for this script
exec "$@"

or: (using a Bash array to preserve $@ with spaces)

#!/bin/bash
set -e

# setup ros2 environment
args=( "$@" )
set -- # reset "$@" to avoid setup.bash using it
source "/opt/ros/$ROS_DISTRO/setup.bash"
exec "${args[@]}"

or: ([ab]using a function to reset $@ in a scoped way - slightly more "POSIX" compliant than using Bash specific features like my previous suggestion, but also a lot less readable/obvious IMO)

#!/bin/bash
set -e

# setup ros2 environment
_setup_ros() {
    source "/opt/ros/$ROS_DISTRO/setup.bash"
}
_setup_ros
exec "$@"
tfoote commented 2 years ago

Appending -- looks like a great way to resolve this.