Fix running commands on Windows (#387) #422
Conversation
Cross-platform and Windows specific code should avoid use of UNIX specific PATH_SEPARATOR (colon) in favor of portable constant File::PATH_SEPARATOR
ShellWords used by WindowsCommandString for command line parsing is not tolerant to Windows path separator (backslash). As a result WindowsCommandString.to_a returns wrong values with path separator characters stripped out. This patch temporary replaces path separators with placeholders before feeding command line to ShellWords and then replaces those placeholders back in ShellWords output. While this is a hack rather than a clean solution, it fixes WindowsCommandString.to_a without introducing code or dependencies. As far as authors can tell, it also covers all possible scenarios and corner cases.
|
Hello, Have you had a chance to look at this yet? Thanks in advance, |
|
Ping? |
1 similar comment
|
Ping? |
|
I second this ping. This PR allowed me to run Aruba under Windows. It would be nice to be able to run a build from the main channel and not a custom patched version. |
|
For now, I pushed gem aruba-win-fix with this PR applied. |
| @@ -16,7 +16,8 @@ def initialize(cmd) | |||
|
|
|||
| # Convert to array | |||
| def to_a | |||
| Shellwords.split __getobj__ | |||
| Shellwords.split( __getobj__.gsub('\\', 'ARUBA_TMP_PATHSEPARATOR') ). | |||
There was a problem hiding this comment.
Does this mean, Shellwords does not support windows correctly?
There was a problem hiding this comment.
Yes, shellwords swallows Windows path separators:
irb(main):001:0> require 'shellwords'
=> true
irb(main):002:0> Shellwords.split('c:\\windows\\system32')
=> ["c:windowssystem32"]
The solution in this PR is not very elegant, but it looks safe in given context.
A few more interesting observations:
This does not work:
irb(main):003:0> Shellwords.split('c:\\windows\\system32'.gsub('\\', '\\\\'))
=> ["c:windowssystem32"]
While this does work:
irb(main):004:0> Shellwords.split('c:\\windows\\system32'.gsub('\\', '\\\\\\\\'))
=> ["c:\\windows\\system32"]
So it might be solution as well, however it looks too subtle...
There was a problem hiding this comment.
What about replacing \\ by / instead? Looks more obvious where the problem is. Or get rid of Shellwords.split for windows all together. Though I'm not sure, if this does not cause other problems.
__getobj__.split(/\s+/)
|
Sorry for the delay. I missed that PR. |
|
@maxmeyer, no problem, thanks for looking at it. |
|
Hi @maxmeyer , have you seen my comment regarding the proposed to_a implementation? |
|
Sorry for the delay, what do you think about my proposal? |
|
@maxmeyer, I would not recommend replacing Should I put a comment with explanations into the |
|
@maxmeyer, have you seen my reply? |
|
Thanks, @maxmeyer Would you mind releasing a new version? |
Summary
This pull request fixes #387 and makes aruba functional on Windows
Types of changes