Skip to content

Quote mysql.exe calls on Windows correctly because IPC::S::S doesn't.#413

Closed
wchristian wants to merge 1 commit into
sqitchers:masterfrom
wchristian:win32_quoting_fix
Closed

Quote mysql.exe calls on Windows correctly because IPC::S::S doesn't.#413
wchristian wants to merge 1 commit into
sqitchers:masterfrom
wchristian:win32_quoting_fix

Conversation

@wchristian

Copy link
Copy Markdown

run() here calls IPC::S::S' runx(), which has two paths. On non-windows systems it calls system { $command } $command, @args which works just fine even with complex stuff in the args. On Windows however it calls _spawn_or_die($command, "$command @args"), which smashes all the arguments together separated by spaces without any additional quoting. This results in a garbage process call which mysql consequently rewards with RTFM.

This fix quotes the arguments on windows, and takes care to leave the executable name separate to allow proper naming of the created process object.

fixes #262

run() here calls IPC::S::S' runx(), which has two paths. On non-windows
systems it calls `system { $command } $command, @args` which works just
fine even with complex stuff in the args. On Windows however it calls
`_spawn_or_die($command, "$command @Args")`, which smashes all the
arguments together separated by spaces without *any* additional quoting.
This results in a garbage process call which mysql consequently rewards
with RTFM.

This fix quotes the arguments on windows, and takes care to leave the
executable name separate to allow proper naming of the created process
object.

fixes sqitchers#262
@theory

theory commented Dec 20, 2018

Copy link
Copy Markdown
Collaborator

I'm torn about this. I intentionally didn't make this change because I was optimistic that pjf/ipc-system-simple#29 would get merged and released. Perhaps too optimistic. :-( Perhaps we should try to figure proof it to only quote the words if the version is <= 1.25.

@wchristian

Copy link
Copy Markdown
Author

I have absolutely no preference for this at the moment. sqitch is simply broken on windows. Unusuable. No good. Any action is better than no action.

@theory

theory commented Dec 23, 2018

Copy link
Copy Markdown
Collaborator

Okay, done in 0148359.

@theory theory closed this Dec 23, 2018
@wchristian

Copy link
Copy Markdown
Author

Looks excellent! 👍

@wchristian wchristian deleted the win32_quoting_fix branch December 23, 2018 17:42
@theory

theory commented Jan 15, 2019

Copy link
Copy Markdown
Collaborator

FYI, 0148359 had a bug. I added windows testing and found it. Fixed in c697c9b.

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