properly escape $@
this ensures spaces in argv are not lost
i've used "$@"
, but some older shells may need to use ${1:+"$@"}
instead, worth to use maximum compatible?
Merge request reports
Activity
Thanks @glensc. Do you know what shells and versions might have trouble with
$@
offhand? It does need to work on CentOS 5... ;)For the changes to shells/bash.go, we also have https://gitlab.com/gitlab-org/gitlab-ci-multi-runner/issues/1550
To be honest, I don't see why we can't just use
sh "$@"
instead of this or the solution given in #1550. Thoughts @ayufan? Do we use any explicit bashisms? If so, falling back tosh
is inappropriate anyway, and we could just usebash "$@"
, right?@nick.thomas i do not know, but some very old shells, if you look at generated
configure
from any project you see they use${1:+"$@"}
. but autoconf configure supports very mystic things, i don't think it's relevant in this project.Thanks @glensc, I guess we don't need to worry about that - it'll be Solaris or AIX support or something!
The test failures do look genuine, but I've not done any digging into what's actually happening here yet - do you have time to look? Even if we end up compressing bashDetectScript to
sh "$@"
one day, it seems likely that would result in the same failure?Reassigned to @nick.thomas
@glensc I think the manipulation of bashDetectScript in BashShell.GetConfiguration() are why the tests are failing on this MR.
I'm experimenting a bit with a wholesale conversion to POSIX shell, it doesn't seem too unreasonable
mentioned in merge request !310 (closed)
Yeah, @ is completely replaced in the script, either with "--login" or "", so changing it to "@" is pointless.
I've added !310 (closed) which completely removes bashDetectScript.
Mentioned in merge request !310 (closed)