-
Notifications
You must be signed in to change notification settings - Fork 54
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
run.sh: Check for spaces in the path to the current directory. #1079
Conversation
The shell wrapper script for the ODK is assuming that the directory containing the repository does not contain spaces in its path. This is required for the logic that binds the repository to the /work directory inside the container to work, and we may not change that without breaking the requirement that the wrapper script shall be POSIX compliant (supporting space-containing paths would require using array-typed variables, which are not specified by POSIX). This commit adds an explicit check at the beginning of the wrapper script to ensure that the current working directory does not contain any spaces, and immediately abort if it does. closes #1078
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thank you! Looks good!
The fact that the repository must not be at the end of a space-containing path should be documented somewhere. Since |
Good idea. Feel free to do it yourself, if you dont have time, just drop an issue and assign to me! Thanks a lot! |
Will do that next week. My work machine is off until Monday. 🌴 |
@@ -16,6 +16,14 @@ | |||
|
|||
set -e | |||
|
|||
# Check for spaces in the current directory path | |||
for item in $PWD ; do | |||
if [ "$item" != "$PWD" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get how this fixes the issue, but I trust that it does and I'm just misunderstanding something. Thanks for addressing #1078!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a way of checking whether the current directory (as stored in the PWD
variable) contains spaces.
To illustrate how it works, let’s first suppose that PWD
does not contain any space (for example, let’s say it contains /home/alice/myont
). The first line will be expanded as for item in /home/alice/myont
. Because /home/alice/myont
cannot be split by the shell (since it contains no spaces), the for
loop will be executed only once, where the loop variable item
will be set to /home/alice/myont
.
Inside the loop, the "$item" != "$PWD"
condition will be evaluated to false (since item
contains the same value as PWD
), so we’ll leave the loop without having done anything.
But suppose that PWD
contains (at least) one space (for example /home/alice/my ont
). The first line will be expanded as for item in /home/alice/my ont
. The shell will split /home/alice/my ont
in two (notice that $PWD
is not enclosed in quotes), so the for
loop will (in principle) be executed twice: first with item
set to /home/alice/my
, and then with item
set to ont
.
During the first turn in the loop, the "$item" != "$PWD"
condition will now be evaluated to true, so the script will print the error message and terminate.
That’s a slightly convoluted way of doing if echo $PWD | grep ' ' ; then echo "There are spaces in the current directory path ; fi
, with the advantage that we don’t need to spawn a grep
process. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, thanks for taking the time on the explanation!
I only needed to read up to:
The first line will be expanded as
for item in /home/alice/myont
.
I thought that $PWD
would get interpolated as a string, so I thought it would be iterating over characters in that string.
The shell wrapper script for the ODK is assuming that the directory containing the repository does not contain spaces in its path. This is required for the logic that binds the repository to the
/work
directory inside the container to work, and we may not change that without breaking the requirement that the wrapper script shall be POSIX compliant (supporting space-containing paths would require using array-typed variables, which are not specified by POSIX).This commit adds an explicit check at the beginning of the wrapper script to ensure that the current working directory does not contain any spaces, and immediately abort if it does.
closes #1078