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

Create Go-syntax representation of the value #369

Closed
wants to merge 2 commits into from

Conversation

stonesbg
Copy link

The best I can tell the root of the issue is that the filepath for the script is converted to a single quote and this combined with the exec.Command causes the windows shell to not handle things correct.

This is the only simple change that I could come up with. It tested with windows and wsl (ubuntu) and both didn't have issues. The other alternatives that I could think of is rather than combining the args up front and then passing to downstream executioners instead keep the script path as an os.FileInfo type until it gets to the executioners (execute_windows.go & execute_unix).

The last option was to use exec.Command("cmd", "/c", ...) or exec.Command("powershell", "-c", ...) where it just passes n the raw built up command. Downside is with windows at least you might lose some functionality with cmd/pwsh/powershell based on the shell configuration.

@@ -222,7 +222,8 @@ func (r *Runner) runScript(script *config.Script, unquotedPath string, file os.F
args = strings.Split(script.Runner, " ")
}

quotedScriptPath := shellescape.Quote(unquotedPath)
quotedScriptPath := fmt.Sprintf("%#v", unquotedPath)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, so if I correctly understood, with Sprintf("%#v") the bug doesn't reproduce, right? Interesting 🤔
You know, a few releases before the path for scripts wasn't even quoted. I think this is a regression, and I added the quoting accidentally.

@stonesbg could you remove this line and rename the unquotedPath argument to be just path?

Good job with finding the reason for the buggy behavior! I appreciate it so much!

Copy link
Author

@stonesbg stonesbg Nov 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hopefully what I updated is what you meant. And yes once the Sprintf was changed things work in windows and wsl. I also tested with python script and a node script and both seemed to function correctly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not at all :) Let me help

@mrexox
Copy link
Member

mrexox commented Nov 17, 2022

Please, check v1.2.1. The issue should be fixed there. Thank you for contributing!

@mrexox mrexox closed this Nov 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants