Job hook provider now sets shell name for script handler #1826
Conversation
| @@ -82,18 +82,23 @@ internal static (string shellCommand, string shellArgs) ParseShellOptionString(s | |||
| } | |||
| } | |||
|
|
|||
| internal static string GetDefaultShellForScript(string path, Common.Tracing trace, string prependPath) | |||
| internal static string GetDefaultShellNameForScript(string path, Common.Tracing trace, string prependPath) | |||
There was a problem hiding this comment.
NOTE: This will cause conflicts for the container hook PR, but I'd rather solve them there
| if (string.IsNullOrEmpty(argFormat)) | ||
| // For these shells, we want to use system binaries | ||
| var systemShells = new string[] { "bash", "sh", "powershell", "pwsh" }; | ||
| if (!IsActionStep && systemShells.Contains(shell)) |
There was a problem hiding this comment.
Using !IsActionStep to explicitly limit the scope of this change to job hooks (and later container hooks - even if a container hook is a run-script-step or run-container-step, the ScriptHandler object executing the hook will not have an associated action)
| @@ -13,8 +13,8 @@ internal class ScriptHandlerHelpers | |||
| ["cmd"] = "/D /E:ON /V:OFF /S /C \"CALL \"{0}\"\"", | |||
| ["pwsh"] = "-command \". '{0}'\"", | |||
| ["powershell"] = "-command \". '{0}'\"", | |||
| ["bash"] = "--noprofile --norc -e -o pipefail {0}", | |||
| ["sh"] = "-e {0}", | |||
| ["bash"] = "--noprofile --norc -e -o pipefail \"{0}\"", | |||
There was a problem hiding this comment.
Relevant to 'Runner cannot execute a script with space in its filePath’ issue:
We added "" quotes around the {0} which will eventually become the filepath.
If we don’t quote it, a space in the middle would be a path termination and only a part of the path would be read.
A runner located at /home/user/space dir/runner/ cannot even run a step like run: echo hw, because it first flushes it to disc at /home/user/space dir/runner/_work/_temp/guid.sh, then gives it as a parameter to bash. It then becomes bash --noprofile --norc -e -o pipefail /home/user/space dir/runner/_work/_temp/guid.sh, meaning bash would try to execute a non-existent script at /home/user/space. Quotes solve this problem.
Possible problems:
- Do we perhaps ever substitute
{0}with a filePath that is already quoted?
|
|
Co-authored-by: Ferenc Hammerl <31069338+fhammerl@users.noreply.github.com>
Co-authored-by: Ferenc Hammerl <31069338+fhammerl@users.noreply.github.com>
There was a problem hiding this comment.
["bash"] = "--noprofile --norc -e -o pipefail '{0}'",
["sh"] = "-e '{0}'",
Added quotes to allow spaces in executable path.
For the above case, we elected to go with single quotes to be more opinionated, as single quotes don't support string interpolation.
Considering that the possible sources of the value substituting {0} are either a user set ENV (set to a path) or a runner generated path, a literal reading of the string is the simplest solution.
Closes #1825