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

rosrun: Allow spaces in command names and search paths. #94

Merged
merged 1 commit into from Oct 13, 2015

Conversation

Projects
None yet
2 participants
@de-vri-es
Copy link
Contributor

commented Oct 8, 2015

This PR allows command names and search paths for rosrun to contain spaces. It mainly adds quotes around a bunch of variables and sets IFS=$'\n' on two places to split only on newlines.

In the case of $catkin_package_libexec_dr, I turned it into an array (also by splitting the output with IFS=$'\n', and renamed it to be plural) so the individual elements can contain spaces when passing it to find.

I tested with a test packages containing one script with a space in the name:
rosrun test_package "test script"

Without the PR, rosrun errors out on a find command. WIth the PR it works as expected. The same goes for workspace paths containing spaces, previously they would have caused find to error, but with the PR it works.

Does not fix #85 but it makes sense to do this first ;)

@@ -16,13 +16,13 @@ function debug() {

while [ $args == 1 ]
do
case $1 in
case "$1" in

This comment has been minimized.

Copy link
@dirk-thomas

dirk-thomas Oct 8, 2015

Member

The first argument is the package name which can by definition not contain spaces.

This comment has been minimized.

Copy link
@de-vri-es

de-vri-es Oct 8, 2015

Author Contributor

Fair enough, but it can't hurt either.

This comment has been minimized.

Copy link
@de-vri-es

de-vri-es Oct 9, 2015

Author Contributor

Actually, at this point the arguments may also be options (--help or --prefix). Not that those contain spaces, but the user could still type them. Even on invalid input the program should behave nice and display the proper error message, I think.

Though as it turns out bash doesn't do word splitting on the argument to case so it still isn't necessary here. But it also can't hurt either.

@@ -48,12 +48,14 @@ case $2 in
esac

if [[ -n $CMAKE_PREFIX_PATH ]]; then
catkin_package_libexec_dir=`catkin_find --without-underlays --libexec --share $1 2> /dev/null`
debug "Looking in catkin libexec dir: $catkin_package_libexec_dir"
IFS=$'\n'

This comment has been minimized.

Copy link
@dirk-thomas

dirk-thomas Oct 8, 2015

Member

What is the $ for? Shouldn't it be just IFS='\n'.

Same below.

This comment has been minimized.

Copy link
@de-vri-es

de-vri-es Oct 8, 2015

Author Contributor

$'\n' is the easiest way to get a literal newline in bash. You can try it yourself:

$ foo='\n'
$ bar="\n"
$ echo $foo$bar

prints: \n\n

This comment has been minimized.

Copy link
@dirk-thomas

dirk-thomas Oct 8, 2015

Member

Thank for clarifying. Its good as it is then. I usually use:

IFS="
"
@@ -66,15 +68,18 @@ if [[ ! $2 == */* ]]; then
_perm="/111"
fi
debug "Searching for $2 with permissions $_perm"
exepathlist=(`find -L $catkin_package_libexec_dir $pkgdir -name $2 -type f -perm $_perm ! -regex ".*$pkgdir\/build\/.*" | uniq`)
exepathlist="`find -L "${catkin_package_libexec_dirs[@]}" "$pkgdir" -name "$2" -type f -perm "$_perm" ! -regex ".*$pkgdir\/build\/.*" | uniq`"

This comment has been minimized.

Copy link
@dirk-thomas

dirk-thomas Oct 13, 2015

Member

Since ${catkin_package_libexec_dirs[@]} might result in multiple paths find -L "/path/foo /path/bar" ... will fail.

This comment has been minimized.

Copy link
@dirk-thomas

dirk-thomas Oct 13, 2015

Member

Running the find command manually fails. Within the rosrun script is seems to deal with the fact that two paths are quotes as one...

This comment has been minimized.

Copy link
@de-vri-es

de-vri-es Oct 13, 2015

Author Contributor

find -L means follow symbolic links. It doesn't take any arguments. Any non-option arguments before predicates are search paths for find.

Additionaly "${array[@]}" (quotes are important) expands to one word (argument) per element. The previous version also resulted in multiple arguments, but split on any whitespace character instead of one argument per array element. So in the case of paths containing spaces the old version actually resulted (incorrectly) in more arguments.

This comment has been minimized.

Copy link
@de-vri-es

de-vri-es Oct 13, 2015

Author Contributor

Ah, I think there is a slight misconception here how arrays are expanded.

Suppose you have the array array=("aap noot mies" "wim zus jet").
Then "${array[@]}" expands to "aap noot mies" "wim zus jet" (one word per array element) and not to "aap noot mies wim zus jet".

That's the entire reason I switched $catkin_package_libexec_dirs to an array.

This comment has been minimized.

Copy link
@dirk-thomas

dirk-thomas Oct 13, 2015

Member

Thank you for clarifying it. It took me a bit how the quoting works (https://gist.github.com/dirk-thomas/b7c9c77a996518c2fe5e).

@dirk-thomas

This comment has been minimized.

Copy link
Member

commented Oct 13, 2015

Thank you for the patch.

dirk-thomas added a commit that referenced this pull request Oct 13, 2015

Merge pull request #94 from de-vri-es/rosrun-spaces
rosrun: Allow spaces in command names and search paths.

@dirk-thomas dirk-thomas merged commit 01a5354 into ros:jade-devel Oct 13, 2015

@dirk-thomas

This comment has been minimized.

Copy link
Member

commented Oct 13, 2015

Also cherry-picked to the indigo-devel branch: 6cef817

@de-vri-es

This comment has been minimized.

Copy link
Contributor Author

commented Oct 13, 2015

Thanks for the merge :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.