Installed Drush on Vista and successfully used dl and en. Ran up and after downloading three modules, it failed the updatedb phase, suggesting to run updatedb. Running updatedb gave the same results, specifically:
The command could not be executed successfully (returned: 's' is not a recognized as an internal or external command, operable program or batch file., code: 1.
Finished performing updates
An error occurred at function : drush_core_updatedb

I ran update.php manually, and it updated fine (reporting a problem with one of the modules -- no big deal). Drush updatedb now reports correctly that there is nothing to update.

I'm not sure if I have a configuration, installation, user error, or actual bug, so I tagged this support request.

Comments

greg.1.anderson’s picture

Status: Active » Postponed (maintainer needs more info)

I don't have Vista. Anyone have Vista?

@DanChadwick: If you can reproduce this problem, could you please attach the output of drush -d updatedb (in a failing configuration)? Thanks.

DanChadwick’s picture

Well, I can't reproduce it, but I still have the window open:

module recipe_pdf35> is incompatible with this release of Drupal, and will be disabled.
UPDATE {system} SET status = 0 WHERE name IN
<'recipe_pdf35#039;>
The following updates are pending:

custom_search module
6100 - Increase the module weight.

views module
6008 - Add the primary key to the views_display table.
6009 - description not available

Do you wish to run all pending updates?> (y.n): y
... then the error message reported in the original post.

greg.1.anderson’s picture

I'd like to see the output of the above with the -d flag included, if anyone reproduces this issue.

DanChadwick’s picture

A bit more info. I reverted Nice_Menus because I had a problem. I then re-ran drush up and got the same error, but in a different function: drush_pm_post_pm_update

I ran updatedb with the -d option, but this update doesn't require an updatedb, so it completed normally.

So I tried rerunning the drush -d up, and it does complain as follows:

C:\wamp\www\drupal-6.16>drush -d up
Drush bootstrap phase : _drush_bootstrap_drupal_root() [0.11 sec,    [bootstrap]
2.33 MB]
Initialized Drupal 6.16 root directory at c:/wamp/www/drupal-6.16       [notice]
[0.18 sec, 2.94 MB]
Drush bootstrap phase : _drush_bootstrap_drupal_site() [0.19 sec,    [bootstrap]
2.99 MB]
Initialized Drupal site kindredcocktails at sites/kindredcocktails      [notice]
[1.72 sec, 3.12 MB]
Drush bootstrap phase : _drush_bootstrap_drupal_configuration() [1.74[bootstrap]
sec, 3.2 MB]
Drush bootstrap phase : _drush_bootstrap_drupal_database() [1.77 sec,[bootstrap]
3.2 MB]
Successfully connected to the Drupal database. [1.77 sec, 3.2 MB]    [bootstrap]
Drush bootstrap phase : _drush_bootstrap_drupal_full() [1.8 sec, 3.44[bootstrap]
MB]
Drush bootstrap phase : _drush_bootstrap_drupal_login() [3.63 sec,   [bootstrap]
13.18 MB]
Found command: pm-update (commandfile=pm) [3.65 sec, 13.17 MB]       [bootstrap]
Including C:\wamp\tools\drush\commands/pm/updatecode.pm.inc [3.66    [bootstrap]
sec, 13.17 MB]
Refreshing update status information ...
WD update: Attempted to fetch information about all available new       [notice]
releases and updates. [10.62 sec, 14.04 MB]
Done.
Update information last refreshed: Fri, 04/09/2010 - 10:05am

Update status information on all installed and enabled Drupal projects:
 Name                      Installed version  Proposed version  Status
 Administration menu       6.x-1.5            6.x-1.5           Up to date
 Advanced help             6.x-1.2            6.x-1.2           Up to date
 Drupal                    6.16               6.16              Up to date
 Content Construction Kit  6.x-2.6            6.x-2.6           Up to date
 (CCK)
 Custom Search             6.x-1.2            6.x-1.2           Up to date
 Date                      6.x-2.4            6.x-2.4           Up to date
 Devel                     6.x-1.19           6.x-1.19          Up to date
 Nice Menus                6.x-2.1-alpha2     6.x-2.1-alpha2    Up to date
 Pathauto                  6.x-1.3            6.x-1.3           Up to date
 PHPMailer                 6.x-2.1            6.x-2.1           Up to date
 Recipe                    6.x-1.1            6.x-1.1           Up to date
 Token                     6.x-1.12           6.x-1.12          Up to date
 Views                     6.x-2.10           6.x-2.10          Up to date

No code updates available. [10.66 sec, 13.95 MB]                            [ok]
Running:   s  s  s --backend [10.93 sec, 15.61 MB]                     [command]
The command could not be executed successfully (returned: 's' is not     [error]
recognized as an internal or external command,
operable program or batch file.
, code: <em>1</em>) [11.5 sec, 19.74 MB]
An error occurred at function : drush_pm_post_pm_update [11.5 sec,       [error]
19.74 MB]
Command dispatch complete [11.5 sec, 19.73 MB]                          [notice]
 Timer  Cum (sec)  Count  Avg (msec)
 page   9.764      1      9764.33
Peak memory usage was 20.25 MB [11.5 sec, 19.73 MB]                     [memory]

C:\wamp\www\drupal-6.16>
greg.1.anderson’s picture

Status: Postponed (maintainer needs more info) » Needs review
FileSize
1.21 KB

Well that is bizarre. The problem seems to be coming from this line in includes/backend.inc:

$cmd = sprintf(escapeshellcmd(" %s %s %s --backend"), escapeshellcmd($drush_path), $option_str, $command);

I'm not sure why that would work most places, but not on your system. It's like the % symbols in each %s is being dropped. Try this patch, which replaces the above line with an equivalent construct that does not use sprintf.

DanChadwick’s picture

Status: Needs review » Active

Bingo. I applied (manually) the patch, and the patch resolved the problem.

Here's the definition of escapeshellcmd from the php manual (emphasis mine):

escapeshellcmd() escapes any characters in a string that might be used to trick a shell command into executing arbitrary commands. This function should be used to make sure that any data coming from user input is escaped before this data is passed to the exec() or system() functions, or to the backtick operator.

Following characters are preceded by a backslash: #&;`|*?~<>^()[]{}$\, \x0A and \xFF. ' and " are escaped only if they are not paired. In Windows, all these characters plus % are replaced by a space instead.

moshe weitzman’s picture

Status: Active » Reviewed & tested by the community

Thanks Dan. Once Greg is satisfied, he will will commit the fix.

greg.1.anderson’s picture

Status: Reviewed & tested by the community » Fixed

Ah brilliant. Learned something new today. Just removing escapeshellcmd from around the " %s %s %s --backend" would probably be a sufficient fix, but since I already tested #5, that's what I committed. Thanks for the testing and documentation reference.

jcmarco’s picture

Version: All-versions-3.0-rc3 »
Status: Fixed » Needs work

I had the reported problem with updb, but I thought that it was an issue related with last cygwin version.
This patch fix some part of the problem but now the issue I found is with the path:

$ drush updb -d
Drush bootstrap phase : _drush_bootstrap_drupal_root() [0,16 sec, 3,14 MB]                                   [bootstrap]
Initialized Drupal 6.16 root directory at d:/xampplite/htdocs/drupal6 [0,18 sec, 3,94 MB]                       [notice]
Drush bootstrap phase : _drush_bootstrap_drupal_site() [0,22 sec, 3,99 MB]                                   [bootstrap]
Initialized Drupal site default at sites/default [3,7 sec, 4,15 MB]                                             [notice]
Found command: updatedb (commandfile=core) [3,73 sec, 4,23 MB]                                               [bootstrap]
Drush bootstrap phase : _drush_bootstrap_drupal_configuration() [3,73 sec, 4,38 MB]                          [bootstrap]
Drush bootstrap phase : _drush_bootstrap_drupal_database() [4,08 sec, 8,85 MB]                               [bootstrap]
Successfully connected to the Drupal database. [4,09 sec, 8,85 MB]                                           [bootstrap]
Drush bootstrap phase : _drush_bootstrap_drupal_full() [4,09 sec, 9,06 MB]                                   [bootstrap]
Drush bootstrap phase : _drush_bootstrap_drupal_login() [6,63 sec, 59,28 MB]                                 [bootstrap]
The following updates are pending:

 webform module
 6316 - Remove the Webform Debug variable.

Do you wish to run all pending updates? (y/n): y
Updating module webform from schema version 6315 to schema version 6316 [77,89 sec, 69,32 MB]                   [notice]
Running:  /cygdrive/d/xampplite/php/php D: xampplite htdocs drush drush.php                                    [command]
--php=/cygdrive/d/xampplite/php/php  --root= d:/xampplite/htdocs/drupal6  --uri= http://default
updatedb-batch-process  54  --backend [77,91 sec, 69,36 MB]
  [error]nd could not be executed successfully (returned: The system can't find the specified path.
, code: <em>1</em>) [77,95 sec, 69,36 MB]
Finished performing updates. [77,95 sec, 69,31 MB]                                                           [ok]
An error occurred at function : drush_core_updatedb [77,95 sec, 69,31 MB]                                    [error]
Command dispatch complete [77,95 sec, 69,3 MB]                                                                  [notice]
 Timer  Cum (sec)  Count  Avg (msec)
 page   74,22      1      74220,2
Peak memory usage was 70,34 MB [77,96 sec, 69,29 MB]   

So debugging I found that with cygwin:
$drush_path: /cygdrive/d/xampplite/php/php D:\xampplite\htdocs\drush\drush.php --php=/cygdrive/d/xampplite/php/php
but at the end I get:
$cmd: /cygdrive/d/xampplite/php/php D: xampplite htdocs drush drush.php --php=/cygdrive/d/xampplite/php/php --root= d:/xampplite/htdocs/drupal6 --uri= http://default updatedb-batch-process 57 --backend
(see the \ characters in the path)

Using a dos command line, I get the same:
$drush_path: D:\xampplite\htdocs\drush\drush.php
$cmd: D: xampplite htdocs drush drush.php --root= D:/xampplite/htdocs/drupal6 --uri= http://default updatedb-batch-process 60 --backend
Finished performing updates.
(the same problems with \ characters)

greg.1.anderson’s picture

Assigned: Unassigned » greg.1.anderson

You're right, I accidentally double-escape-encoded the drush path. I'll fix the fix.

greg.1.anderson’s picture

Status: Needs work » Fixed
FileSize
862 bytes

Committed the enclosed patch. Please try again against HEAD and reopen if you're still having trouble.

jcmarco’s picture

Status: Fixed » Needs review

Hi Greg,
I am afraid that there is still problems with this in a Windows box:

With Cygwin:
drush_path : /cygdrive/d/xampplite/php/php D:\xampplite\htdocs\drush\drush.php --php=/cygdrive/d/xampplite/php/php
option_str : --root="d:/xampplite/htdocs/drupal6" --uri="http://default"
command : updatedb-batch-process "88"

Cmd : /cygdrive/d/xampplite/php/php D: xampplite htdocs drush drush.php --php=/cygdrive/d/xampplite/php/php --root="d:/xampplite/htdocs/drupal6" --uri="http://default" updatedb-batch-process "88" --backendThe command could not be executed

Win MS-Dos command:

drush_path : D:\xampplite\htdocs\drush\drush.php
option_str : --root="D:/xampplite/htdocs/drupal6" --uri="http://default"
command : updatedb-batch-process "89"
Cmd : D: xampplite htdocs drush drush.php --root="D:/xampplite/htdocs/drupal6" --uri="http://default" updatedb-batch-process "89" --backend

This is from _drush_backend_generate_command() in backend.inc just before and after running escapeshelcmd's

The problem is that the DRUSH_COMMAND environment variable obtained from drush_find_drush() gives an address commands with \ that is changed for spaces when using scapeshellcmd.

jcmarco’s picture

Testing in drush_find_drush() I found that the script executed by drush is,
for cygwin:
D:\xampplite\htdocs\drush\drush.php
and for windows command:
D:\xampplite\htdocs\drush\drush.php

In the case of cygwin it should be executed with the cygwin path format, this way it would avoid the problems with the \ and escapeshellcmd() .

For windows command it seems really hard to fix it unless the drush.php was executed from the drush directory and thus not using \,
if you run from the drush directory: php drush.php, the path returned by drush_find_drush() is drush.php and this would work with the escapeshellcmd(),
but it would need all the extra options to tell where is the original path using -r and the directory from where drush was called

greg.1.anderson’s picture

FileSize
829 bytes

Hm. After consulting the documentation on escapeshellcmd and escapeshellarg, it is clear that escapeshellcmd simply is not useful for our purposes on Windows, for the reason Dan quoted in #6.

In the case of backend invoke, drush is already using escapeshellarg rather than escapeshellcmd for all parameters except the path to drush. As there is no real concern in this context that the user might try to compose the path to drush to execute arbitrary commands (since arbitrary command execution is in theory already possible), I think it is appropriate even on Linux to use escapeshellarg rather than escapeshellcmd here. Should someone want to call drush from, say, a Drupal module, then it should be up to the caller to insure that the arguments passed cannot set the path to drush.

@jcmarco: Please try the enclosed patch and see if it works for you in cygwin and DOS. It works for me -- but then again, I always make sure that drush is on my path, and call it with "drush". This is best for remote command execution.

greg.1.anderson’s picture

Title: updatedb reports 's' is not an recognized command... » escaping the path to drush in backend invoke
greg.1.anderson’s picture

FileSize
1.69 KB

The above does not work in many common cases, such as when DRUSH_COMMAND is "/path/to/php /path/to/drush.php --php=/path/to/php".

This patch does things a little differently. It should work a lot better than before for both Linux and Windows. The one caveat is that if you set the drush path in an alias, for example, it is important that you escape the command correctly inside the alias file, as drush will no longer try to escape it. If drush did try to escape drush paths set in aliases, then it would not be possible to use the form shown above (with --php set), so I believe this is optimal behavior.

jcmarco’s picture

I realized that your patch still had problems and I started testing with configuration like yours.

The bad news is that it's still failing.

I am testing different configurations with/without "", using _drush_convert_path() to change directory format,
forcing the use of --php=, using escapeshellarg in other places..., but without luck yet.

greg.1.anderson’s picture

Status: Needs review » Needs work

Okay, thanks.

bob.hinrichs’s picture

The remote command works fine if I change the line (364 of backend.inc) to:
$cmd = "ssh " . $ssh_options . " " . escapeshellarg($username) . "@" . escapeshellarg($hostname) . " " . str_replace('= ','=',escapeshellarg($cmd));

I understand that this is not the correct solution, but it is elegant insofar as it targets only the space problem on windows (or any other case where these unwanted spaces could be generated by the function) without side-effects, unless there is a case where you need a space after an '=' in this string (seems completely unlikely?).

Possible fixes I can think of: come up with our own version of escapeshellarg (would probably be yucky); wrap it in something that corrects the output to keep the integrity of the command (as above); or change the way the $cmd is put together so that the whole thing does not need to be put through the escapeshellarg.

thanks greg and all...

greg.1.anderson’s picture

Status: Needs work » Postponed (maintainer needs more info)

Why is there a space after the = chars in escapeshellarg($cmd)? What is the value of $cmd with and without your change? On Linux, there are sequences that look like --uri='\''http://greenknowe.org'\'', but those do not have spaces. Is the behavior of escapeshellarg different on Windows, as escapeshellcmd is? The comments on the manual page (see link in #14, above) says that % is replaced with space on Windows, but quotes should not be...

Could you show the output of drush @someremotealias status -d on your system?

bob.hinrichs’s picture

Sure. The result is the same as that noted above.

C:\Windows\system32>drush @bap.staging status -d
 Begin redispatch via backend invoke [0.15 sec, 2.85 MB]                 [notice
]
Running: ssh -o PasswordAuthentication=no                              [command]

"www"@"/home/www/DRUSH/drush/drush  --d --root=
/var/www/html/mysite/pressflow  --uri= islco.com  status
--backend" [0.15 sec, 2.86 MB]
The external command could not be executed due to an application         [error]

error. [0.22 sec, 2.86 MB]
Undefined offset:  1 backend.inc:112 [0.22 sec, 2.86 MB]                [notice]

Undefined variable: output backend.inc:119 [0.22 sec, 2.86 MB]          [notice]

The command could not be executed successfully (returned:                [error]

Pseudo-terminal will not be allocated because stdin is not a
terminal.
ssh: Could not resolve hostname /home/www/DRUSH/drush/drush  --d
--root= /var/www/html/mysite/pressflow  --uri= islco.com  statu:
hostname nor servname provided, or not known
, code: 255) [0.22 sec, 2.86 MB]
Backend invoke is complete [0.22 sec, 2.86 MB]                          [notice]

If I place the str_replace to remove the spaces, the status returns normally.

The command being passed to escapeshellarg is:

/home/www/DRUSH/drush/drush  --d --root="/var/www/html/mysite/pressflow" --uri="islco.com" status --backend

It looks to me like escapeshellarg is indeed working differently on Windows than on linux. I don't know what else would account for the difference. I've looked this up and there is some evidence of quotes causing issues.

Just to confirm, I ran escapeshellarg on a string that had double quotes in it. On Windows, it converted the double quotes to spaces. The same code on linux returned the string with the double quotes intact.

greg.1.anderson’s picture

Status: Postponed (maintainer needs more info) » Needs work

Well drat. (Sorry I missed the -d output in the chain above; thanks for rerunning the test.)

It seems that escapeshellarg($cmd) is simply not appropriate in this context on Windows. I don't have time to fiddle with it right now, but I'm wondering if that last call to escapeshellargs can just be removed. Seems that this would probably work on Windows:

$cmd = "ssh " . $ssh_options . " " . escapeshellarg($username) . "@" . escapeshellarg($hostname) . " " . $cmd;

There might be some instances where this doesn't work right in Linux, though. This is going to take some more fiddling to find something that works in both places. If there's really no other way around it, I suppose I could reluctantly get behind #19, but right now I'm hoping there's a cleaner solution.

greg.1.anderson’s picture

Status: Needs work » Needs review

Just removing the escape as suggested in #22 breaks commands like drush @gklive sqlq "select * from users;". That example should also break #19 on Windows. addslashes($cmd) similarly does not work.

It seems that what we need is an equivalent to escapeshellarg that works under both Linux and Windows. This seems to work:

$cmd = "ssh " . $ssh_options . " " . escapeshellarg($username) . "@" . escapeshellarg($hostname) . " '" . str_replace("'","'\''",$cmd) . "'";

At least, it seems to work on Linux. Want to give it a try for me on Windows? Try other commands with other special characters and see if you can find examples that break it. Frankly, I'm disappointed that escapeshellarg doesn't work better on Windows.

bob.hinrichs’s picture

Hi there, sorry I'm late responding.

I tried the line from your last post #23 and so far it is working fine on Windows. Hopefully there is not a case where the command would already have escaped single quotes in it (in which case this would double-escape them). If that could ever happen, maybe a preg_replace could help, otherwise that's a phantom issue but thought I'd bring it up. Thanks much.

greg.1.anderson’s picture

Status: Needs review » Needs work

You're right; this needs to be updated with a new patch against HEAD that uses preg_replace to replace any ' not preceded by a \ with \'.

greg.1.anderson’s picture

See related issues #785048: Make drush's svnsync play nicely with windows and #477720: Commands do not escape spaces in directory names; same sort of issue, different locations. To really fix this issue, we'll need to escape all paths that are passed on to a shell. Ideally, the escape pattern should be invariant by platform; if we special-cased Windows paths, as suggested in #477720, then things could be confusing for paths that are sent via Backend invoke to a remote drush instance running on a different platform than the client. Any special-casing of paths would have to keep in mind the destination platform type, not the current platform. To that end, maybe #477720 is on to something, requiring the "D:" prefix to signify Windows paths.

We'll need some good testing from Windows users to make sure all of the different places work correctly.

wapnik’s picture

Component: Code » Core Commands
Status: Needs work » Needs review
FileSize
1.8 KB

Hope this is the right place for this code. I was working on a bash script which installs the site using Drush Site Install 6.x, which is by coincidence now freshly inside drush itself. I wanted it to run on windows. This is what it got to work on my win xp.

greg.1.anderson’s picture

Category: support » bug
Priority: Normal » Major
Status: Needs review » Needs work

This is a good start. I think it's a good idea to just break down and run different code for Windows and Linux. escapeshellarg works fine on Linux, and it seems unlikely that we'll come up with something that escapes correctly for both platforms. php_uname will never start with "Windows" on Linux, so that is good.

I am wondering if this works from both the DOS prompt and cygwin? If it only works in cygwin, then we should document the DOS prompt as unsupported. If it does not work in cygwin, that should be fixed.

There are about 20 places where drush calls escapeshellarg. If you could change these to drush_escapeshellarg and test on Windows, then I will confirm on Linux and commit.

It is my hope that we can confirm and include this before drush-4 stable (queue) goes out.

greg.1.anderson’s picture

Priority: Major » Minor

After further review, I do not think that this technique can be used effectively to solve this issue for drush-4. The problem with drush_escapeshellarg is that it produces different output depending on whether the machine it is running on is Windows or Linux. The problem with this is that sometimes, the output from escapeshellarg is sent to a remote machine which might very well be Linux.

Currently, drush does not have any good conventions for detecting or specifying whether a remote machine is Windows or Linux. Furthermore, there are a number of places where escapeshellarg is used in a context that could end up being executed locally or remotely, and for the remote case, the information about which machine the data will be sent to is not always available. Therefore, it is not possible to simply say "use the Windows algorithm for Windows, and the Linux algorithm for Linux", unless all of these places in the drush code are fixed. This would be hard.

The alternative would be to come up with a single implementation of drush_escapeshellarg that escapes arguments in a way that is usable on both Linux and Windows.

I think it might be possible to do this:

  1. Rename drush_escapeshellarg_windows to drush_escapeshellarg and change it to wrap with ' instead of " (to be like escapeshellarg)
  2. get rid of drush_is_windows
  3. use drush_escapeshellarg instead of escapeshellarg everywhere
  4. test
  5. test
  6. test some more

I'll work on this as time permits, but unless we get some serious help from Windows users, I think we might need to just punt on Windows support for drush-4.

greg.1.anderson’s picture

Priority: Minor » Normal

After further review, I do not think that this technique can be used effectively to solve this issue for drush-4. The problem with drush_escapeshellarg is that it produces different output depending on whether the machine it is running on is Windows or Linux. The problem with this is that sometimes, the output from escapeshellarg is sent to a remote machine which might very well be Linux.

Currently, drush does not have any good conventions for detecting or specifying whether a remote machine is Windows or Linux. Furthermore, there are a number of places where escapeshellarg is used in a context that could end up being executed locally or remotely, and for the remote case, the information about which machine the data will be sent to is not always available. Therefore, it is not possible to simply say "use the Windows algorithm for Windows, and the Linux algorithm for Linux", unless all of these places in the drush code are fixed. This would be hard.

The alternative would be to come up with a single implementation of drush_escapeshellarg that escapes arguments in a way that is usable on both Linux and Windows. In short, I should have re-read #26 before getting too interested in #27.

I think it might be possible to do this:

  1. Rename drush_escapeshellarg_windows to drush_escapeshellarg and change it to wrap with ' instead of " (to be like escapeshellarg)
  2. get rid of drush_is_windows
  3. use drush_escapeshellarg instead of escapeshellarg everywhere
  4. test
  5. test
  6. test some more

I'll work on this as time permits, but unless we get some serious help from Windows users, I think we might need to just punt on Windows support for drush-4.

wapnik’s picture

The main reason i began writing code for this issue is that i wanted to run `drush si` for drupal 6 on windows. Other commands (i'm using) are working for me well. Didn't come to any other issue. As you can see in the newly ported code from the Drush Site Install 6.x module, there are some php cli console calls to make the install happen. They didn't work on windows and actually that drush_escapeshellarg_windows() function is the right code to make them run. And drush continued working well for me on the other places i'm using it too, so i assumed this is the right implementation of escapeshellarg() for windows. As i realized one of the main problems for php cli on windows is the wrapping of php code in an argument into ' instead of ". As far as `drush si` would not be running on a remote machine we can maybe somehow separate this code only for php cli execution to a function, for example drush_shell_exec_php().

greg.1.anderson’s picture

@wapnik: If you want to help, submit a patch per #30. Thanks for your interest in getting drush running in Windows; all we really need to make that happen is some help with patches and testing.

greg.1.anderson’s picture

Priority: Normal » Major

I accidentally committed #27 as part of http://drupal.org/cvs?commit=458656. Although the committed code is innocuous in its current form (and according to #31 & c., can help Windows users in some instances), this issue still needs work.

I'm concerned per #31 that there may be some instances where drush_escapeshellarg_for_windows might not be suitable for use on Linux. If escapeshellarg were replaced with drush_escapeshellarg everywhere, then this would occasionally happen. If anyone has any concerns, let me know and I'll back out the patch.

Marking as 'major', as ideally we would like to see a resolution for drush-4. Not sure how feasible that is at this point; I think it is only possible if drush_escapeshellarg_for_windows does or can be made to work on both platforms.

wapnik’s picture

I fear that it's not quite possible have the same version of escapeshellarg for linux and windows. At least not for all cases.

C:\>cd 'WINDOWS'
The system cannot find the path specified.

C:\>cd "WINDOWS"

C:\WINDOWS>

The biggest problem seems to be with those quotes. I'll try to avoid them in cases where it's possible. And for cases where we don't know the platform of a remote host simply fallback to original escapeshellarg. Yeah and i'm testing, testing and testing more. :)

greg.1.anderson’s picture

Per #26 and #30, we must use the same drush_escapeshellarg for both platforms.

Per #34, in order to support the DOS prompt, we would therefore have to use " to escape on Linux. I don't think there's time to make that sort of change before drush-4 stable, with RC1 already out.

@wapnik: Could you test on Windows + cygwin? I think that using ' will work in that environment.

wapnik’s picture

Yes. Works. At least cd "WINDOWS", cd 'WINDOWS', cd 'Documents and Settings' and cd "Documents and Settings".

greg.1.anderson’s picture

@wapnik: Thank you for your tests and feedback. I am going to recommend that we mark Windows + DOS is deprecated / unsupported in the README, and attempt to fix this per #30 for Linux and Windows + cygwin using ' as the escape character. If you test the patch on Windows, then perhaps we can get it in.

moshe weitzman’s picture

I'm not too ken on deprecating DOS shell in favor of cygwin. We'll do it if we have to though. Anyone test on the new powershell that comes on windows?

greg.1.anderson’s picture

I don't think there's time to do the work that would be required to make DOS functional for all drush operations. I'll happily review any submitted patch that manages it, though.

greg.1.anderson’s picture

Edit: Deleted incorrect supposition, already disproved above.

greg.1.anderson’s picture

Assigned: greg.1.anderson » Unassigned

I was working on this last night, but did not have time to resolve the open issues. #16 will also need to be fixed before cygwin will work. Unfortunately, I am not going to have time to work on this issue for drush-4.

moshe weitzman’s picture

Status: Needs work » Needs review
FileSize
6.58 KB

I worked on Cygwin compatibility (gave up on naked Windows) for many hours today. It is SLOW going, working through the issues. Attached patch gets pm-download and site-install (d7) working for me. Next I will tackle backend invoke which looks like a tough assignment.

I'm interested in comments on my work so far.

moshe weitzman’s picture

I worked more today and could not fix backend_invoke. We need some Windows shell gurus in here. At this point, I don't think even Cygwin is enough.

A couple notes about the prior patch

  1. The site-install changes were just to use file redirection like sql-query so we don't have to struggle with those backticks that mysql likes
  2. The drush_convert_path() change calls for us to use `cygpath` to convert to Windows paths from cygwin paths. But instead we might want to shell out back to cygwin using the technique I stumbled on at http://us.php.net/manual/en/function.shell-exec.php#68647. Spawning a shell this way seems slow at first glance.
greg.1.anderson’s picture

Status: Needs review » Needs work

This change does not always work:

-exec $php "$SCRIPT_PATH" "$@" --php="$php"
+exec "$php" "$SCRIPT_PATH" "$@" --php="$php"

For example, if you have a php.ini in your $HOME/.drush folder, then you will get this:

exec: 88: /usr/bin/php --php-ini /home/ga/.drush/php.ini: not found

This is just one example of where drush combines commands and paths together and expects the shell to process them correctly. This is a broken practice, as it does not work if the command path has a space in it, for example. There is no way to fix it by correctly escaping the $php variable; the things that go into $php must be fixed.

Again, this is just one example. backend invoke is similar, but more complicated, but I think that the resolution is the same: it cannot be fixed by escaping alone. The way the code is called must be changed. Unfortunately, this is going to be very time consuming.

Other parts of the patch do look good, and some of this could probably be committed, but I did not have time to test any more of it yet.

moshe weitzman’s picture

I committed the README and site-install hunks from #42. There is lots more to do here. It is looking like drush4 will have lousy windows support unless we can get some bright volunteers here.

greg.1.anderson’s picture

Title: escaping the path to drush in backend invoke » Windows support for drush: escaping the path to drush in backend invoke and elsewhere
wapnik’s picture

drush_sql_build_exec() is falling on win xp if the path to php's temp folder contains spaces.

Calling system(mysql --database=information_schema --host=localhost --user=root --password= < C:\Documents and Settings\Owner\Local Settings\Temp\phpA38.tmp);

wapnik’s picture

Status: Needs work » Needs review
FileSize
1.64 KB

I've worked a bit on this and realized just like mentioned earlier in comment #34, there is no other way to escape a path containing whitespaces in Windows than enclosing it into double quotes, which breaks Linux. I used the same pattern as for drush_escapeshellarg() to have a different version of escape function on every platform. Is site_install running remotely too?

greg.1.anderson’s picture

Status: Needs review » Needs work

Thank you for your efforts; however, there are two things that are true about the current state of the drush code vis-a-vis this issue:

1. The current code may escape a string on Windows and then execute it remotely on Linux. At the point in the code where the escaping is done, you do not know what kind of a system you may eventually try to use it on.

2. The current code may escape a string that contains spaces via escapeshellcmd. It is impossible to fix the defects that arise in this scenario by escaping alone, because sometimes the spaces separate a command from its arguments, and sometimes the spaces are embedded in a path. You do not know at the point of escapeshellcmd what the intended purpose of the space is.

Problem #1 can be solved by quoting alone only if you can come up with an algorithm that quotes strings in a way that works equally well both on Linux and Windows. If you can't do that, then changes to the drush code that is doing the escaping is necessary to add context about where the string will be used. I think that a universal quoting algorithm wouldn't be too hard to build if we punt on DOS and require cygwin. I think everyone is willing to accept that; the alternative is to add a bunch of code that tracks the target platform for every string that is escaped (e.g in backend invoke), which would involve adding platform type items in alias records, more parameters in backend invoke, etc. (messy).

Problem #2 cannot be solved without reworking the calling code. Drush simply cannot arguments to be combined into variables that are intended to represent paths to executables, because when that happens, things break when the string is escaped.

jcmarco’s picture

A different approach could be trying to move everything to UNIX format.
Windows 7 and Windows Vista support both formats for directory separators, if you open a command shell you can run:
d:\xampplite\php\php.exe d:\xampplite\htdocs\drush\drush.php st
or
d:/xampplite/php/php.exe d:/xampplite/htdocs/drush/drush.php st
without problems.
(this example is with my xampplite installation and Drush location, using a .bat file with the right program locations is the same).

This is documented in a MSDN document where explains that last File API in NTFS convert formats before running, there is only a case that it doesn't work that is where you are using a \\?\ referrer, but I can't figure out how it could affect to the drush use ever.

I am patching the existing Drush code to add the _drush_convert_path() function anywhere there is any function reading the paths from the PHP file functions (that are are always returning the original windows format separators).

The other big caveat with the actual Drush/Windows integration is the drush_backend_invoke() that is using the _drush_proc_open() (backend.inc), this simply doesn't work in cygwin/windows. I am working with a simplified version of this function with a lean use of pipes that now works great but it needs much more work (it was not yet tested in Linux but as it works in Cygwin it should work as well in Linux).

At the same time, for testing all the UNIX command integration I am using the drush_make tests, and Cygwin in order to avoid installing the UNIX commands to the Windows shell. It is exactly the same than installing the GNUWin32 but you have all the Unix shell commands available and you can install almost any unix package.

For the drush_make (a different contrib module), I needed to fix some cygwin compatibility issues when running commands. I will send a different path to that module at the same time, but the drush_make seems to work fine as well.

So right now I am able to run any drush command, but the returning of batch processes.
For example, the updb is executed but it is not returning to the shell, now I am trying to fix that part.
Other nasty issue with the open_proc() in windows is that it opens the drush.php for editing everytime that this function is executed (this is reported in some PHP forums as well), but in cygwin it doesn't fail, probably a different handling of pipes.

I also consider that adding Windows compatibility to Drush it rather hard, but probably we should unify everything in a xNIX world format/style using Cygwin. Once I fix the issue with the return of batch processes in drush, and test everything in Linux I will upload this patch for testing, unless somebody would like to test the actual not yet finished patch.

wapnik’s picture

What about simply asking the user (waiting for user prompt) about the remote platform if we cannot determine it?

Edit: @jcmarco: Can you escape a whitespace in path in Windows 7 and Windows Vista like in xNIX (\ )?

jcmarco’s picture

@wapnik: if you are asking about the command shell, you need to use quotation marks when you have paths with spaces.
For example:
MS-DOS -> dir "c:/Program Files/WinRAR" -> this works fine.
Cygwin -> ls "c:/Program Files/WinRAR" -> it works too

About PHP: as you are using the same / than in UNIX then you don't have any problem.
The question is that any PHP filesystem function in Windows (in Cygwin you are running a windwos version PHP) returns paths with \, then in Drush if you pass this directories through the _drush_path_convert() then everything will be clean.

Since I changed the directories values with _drush_path_convert() I have not had any issue related with paths, but as I told you there are more problems than not just the directories separator.

About forcing the use of Windows Vista and Windows 7, they are the only MS supported OS's. Windows XP is over 10 years old and it is officially an unsupported OS.

wapnik’s picture

The directory separator is one issue. I'm using win xp and i can use both directory separators, don't know if i have some additions installed, maybe yes. So this is not a problem in my case. I can maybe find out how i did it if it's necessary.

Bigger issue is the issue with whitespaces. In Linux shells you simply quote them (with \) or you surround the whole path with single quotes. In Windows you have to surround it with double quotes. Therefore there cannot be a same one function for preparing paths for use in shell for Windows and Linux systems. And there are a few places in the code where paths are used in a shell call, like the one i addressed in my last patch. To properly interpret these paths you have to know the platform. So back to my questions :). What about asking the user when we need to know the platform? Is site_install invoked remotely?

Did you see the film? It was on television one of these last days of the previous year. Neverending story... At least it has a happy ending...

greg.1.anderson’s picture

@jcmarco: Thank you for all of your work in this area. At this point, I think that a patch that supported Vista and 7 but not XP would be welcome. I look forward to seeing your patch.

DanChadwick’s picture

I'm not familiar with Drush internals, but would it help detect the local OS and to assume that all remote commands are destined for a Linux system? And if it would help, would this be an acceptable assumption?

moshe weitzman’s picture

I do think that we are going to eventually need to support --os option commands that operate on remote hosts. Typically, one would set that up in the site alias.

agualog’s picture

Hi everyone,

Just started to work on this issue, and something I noticed : problems seems to come from Windows' escaping method in path with spaces. Am I right ?

One solution I didn't seen : why don't we transform the Windows path in a 8.3 format ? All spaces are wipped in this way, so no more problems? No more need to escape ?

It's just an advice at first glance, maybe I'm totally out of topic ?

MichaelCole’s picture

Hey, saw this. Check out the Quickstart Drupal project for a pre-made virtual machine that has drush already setup. It's a virtualbox VM, so it runs on anything.

http://drupal.org/project/quickstart

PierreJ’s picture

hi,

I was pointed to this issue by Daniel and Greg.

My suggestion was to actually either detect or ask the user which OS is used on the target host. The best being to define some config data so the user won't have to re define them on each use.

It is also important to note that using the escape functions is the easiest way, but they are platform specific. As in: php will escape the input according to the platform where it is executed.

In the long run, it would be nicer to use ssh/scp and/orthe windows equivalent (wmi APIs) instead. The ssh2 extension works like a charm and Curl supports ssh as well (on windows too). It could be done by defining a common interface for the needed operations and implement each target platform as backends. That's something I could help for the windows side (unix too but only in my free time :).

greg.1.anderson’s picture

I can support your work on Linux -- if you make it work on Windows, I will test and adjust so that it continues to work on the Linux side.

For your idea to work, drush must be modified so that the target platform is known. The low-level function to start at is backend_invoke_args in includes/backend.inc. The real work is done by _drush_backend_generate_command, in the same file. You will notice that it uses ssh to contact the target machine; if you knew the platform of the target, you could use a different executable than ssh on Windows if you wanted. There are four permutations: linux -> linux, windows -> windows, linux -> windows and windows -> linux. To keep things simple, we could handle just the first two initially; then you could just check the source platform in _drush_backend_generate_command, and you wouldn't need any additional parameters (at first).

To do the other cases, where the target platform does not equal the source platform, parameters will need to be added to backend invoke. This function is called from various places in drush; one example is drush_do_site_command in includes/drush.inc. (Aside: drush_do_site_command should be considered an internal API; it is called by drush_invoke_sitealias_args, which is the preferred public entrypoint.) You will see that drush_do_site_command uses something called an alias record to identify the target Drupal site to contact. An alias record looks something like this:

array(
  'remote-host' => 'host.machine.com',
  'remote-user' => 'wwwadmin',
  'root' => '/path/to/drupal',
  'uri' => 'http://website.uri.net',
  'platform' => 'Windows'
);

The 'platform' entry does not exist yet; I propose that if we add it to the alias record, then drush_do_site_command could pass it as a parameter to backend_invoke_args.

There are a slew of other minor Windows problems in drush, but if we fixed just those two things first, it would be a big step forward.

greg.1.anderson’s picture

I just looked at the code one more time, and would suggest the following refactoring:

backend_invoke_args could generate an alias record and call a new function backend_invoke_sitealias. Then, drush_do_site_command could just disappear; any function that formerly called it would call backend_invoke_sitealias instead.

That would make life easier, as we could add new parameters such as 'platform' to the alias record, and would not need to perturb function parameters so much. Here's a patch that moves in that direction; it does not quite get rid of drush_do_site_command, but moves things in the right direction, opening the way to adding parameters to the site alias record in a way that makes them accessible to the functions in backend.inc.

greg.1.anderson’s picture

#61 is working fine, so I committed it to help move things along in this issue.

greg.1.anderson’s picture

Now that we have a way to pass extra parameters into backend_invoke based on the target platform (from #61), I committed an additional adjustment that tracks the os of the remote machine independently of the os of the local machine.

See: http://drupal.org/cvs?commit=494632

Now _drush_escapeshellarg_windows should be called in place of escapeshellarg more or less as appropriate. Note that this is contingent on code correctly calling drush_escapeshellarg instead of escapeshellarg. When escaping for a remote machine, the correct pattern is drush_escapeshellarg($arg, drush_os($site_alias_record));.

jrsinclair’s picture

I don't know if this is the place to raise this, but I did some investigating, and there's a couple of additional issues to overcome before the _drush_backend_generate_command_sitealias() function will operate correctly under windows.

Both relate to line 556 of backend.inc:

  // @TODO: Implement proper multi platform / multi server support.
  $cmd = escapeshellcmd($drush_path) . " " . $option_str . " " . $command . (empty($data['#interactive']) ? " --backend" : "");

The first issue is that the escapeshellcmd() function will replace backslashes with a space, so if your Drush path is C:\Program Files\drush, for example, it will be translated to C: Program Files drush. This was pointed out by coryp_1 in #805998: drush up and drush updatedb not working on windows comment 6. In addition, paths with spaces (like C:\Program Files will also cause problems. I guess we need some kind of drush_escapeshellcmd() equivalent of drush_escapeshellarg().

The second, issue is that the DOS shell doesn't understand #! shell script designators. That is, under windows you can't run drush.php as an executable shell script. To get it to run, you have to run the PHP executable (i.e. php.exe) and pass the php file as the first argument.

Unfortunately, when I tried doing this, the command would hang. I'm assuming that the reason is because passing the drush path as first argument messes up the argument array in the drush script, but I ran out of time to investigate further and can't confirm this. It could just be a quirk of my particular setup.

In retrospect, I should have replaced $drush_path with the path to drush.bat as coryp_1 tried. That may have proved more helpful.

Apologies for just reporting issues rather than providing something more useful like a patch. Hopefully it will help point someone in the right direction though.

greg.1.anderson’s picture

@jrsinclair: Definitely good points. Additionally, there has been some discussion that ssh is not always available on Windows, and whether to require cygwin or use some other transport.

drush_escapeshellcmd is fraught with peril, as drush will sometimes include arguements in $drush_path -- that is, it might contain "/path/to/drush --php=/path/to/php". This will definitely cause problems if the spaces in the filename are escaped; however, spaces must be escaped if they appear in the path. The cheating answer would be to escape spaces up to " --" (might work, would be dirty); the right answer would be to fix drush such that the --php args are not included in $drush_path.

Definitely more work needed here.

MyXelf’s picture

Hi:

First of all I want to apologize if this is not the right place to post this. I've been looking around for 2 days and now I see that the source of what is happening could be related to this thread.

I'm using Linux (Kubuntu) with GNU bash, version 4.1.5(1)-release (x86_64-pc-linux-gnu). All the Drush commands related to drush_core_drupal_directory() in commands/core/core.drush.inc (at least in my case *dd* / *cdd* / *lsd* / *use*) will report a: "No such file or directory". This will happen even issuing the commands in "drush help dd" like:
cd `drush dd devel`

I was trying to find the culprit, looking the code in Drush (Awesome! but really complex to my knowledge), and found that the point is with escapeshellarg() in includes/exec.inc. But the real problem is with BASH. For some unknown reason to me I've been googling a lot, unsuccessfully.

When invoking:

  $ drush dd devel
  '/home/myxelf/d6site/sites/default/modules/devel'

So far, so good. Doing the following gives the error:

  $ cd `drush dd devel`
  bash: cd: '/home/myxelf/d6site/sites/default/modules/devel': No such file or directory

And here comes the tricky part. The problem is the combination of the backtick ` with the single quoted string ' (introduced in drush with the use of escapeshellarg(). At the shell level the problem looks like this:
$ cd `'/home/myxelf/d6site/sites/default/modules/devel'` (you can't run that directly). I would like some BASH guru to say something about what's going on here.

Trying to determine if the problem was only "my" BASH, I did the following:

  $ cat test.sh 
  #!/bin/bash
  echo "'/home/myxelf/d6site'"

  $ ls `./test.sh`
  ls: cannot access '/home/myxelf/d6site': No such file or directory

I also tested this on a Debian, with the same results.

Thanks in advance

MyXelf

PS: Feel free to ask for more information or tests. I'm just trying to transmit the idea and set an starting point.

greg.1.anderson’s picture

#66 should be a new issue. I am aware of this, but have not posted anything yet; maybe someone else already has.

So that extra level of quotes from dd is a problem. This is a confusing one, because:

$ drush dd devel
'/home/myxelf/d6site/sites/default/modules/devel'
$ cd '/home/myxelf/d6site/sites/default/modules/devel'
#WORKS!

As you pointed out above, though doing the dd in backquotes fails. The reason is that those backticks are discarded by the shell when typed on the command line, but are significant when output by dd. Ergo, when using dd in backticks, it is really equivalent to:

$ cd "'/home/myxelf/d6site/sites/default/modules/devel'"
# FAILS!

So, dd should not be outputting those extra quotes. It's a little tricky, because we do want the quotes if we are taking the same string and passing it to a function that will interpret it like the shell does (e.g. drush_shell_exec and family). So, to a certain extent, this issue is in fact closely related to what we are tracking here, which is that the way drush escapes text needs to be variable based on what the text will ultimately be used for.

jrsinclair’s picture

OK. I thought I had at least some understanding of what was going on in the backend.inc file, but after mucking around with this today, apparently I don't have a clue.

I have established two things though:

  1. _drush_proc_open() apparently does not work under Windows when there is a space in the filename. I suggest we add "Do not install drush in a path containing spaces" to the installation instructions. Try as I might, I can't seem to find any way to get around this. I'd also dearly love to know if this is a problem specific to windows or whether Linux users have this problem too.

  2. Even when the drush path does not have spaces, proc_open() hangs infinitely for some reason. I thought it may have something to do with http://www.php.net/manual/en/function.proc-open.php#97012, but making the suggested change did not help.

    Weirdly though, if I remove --backend from the command returned by _drush_backend_generate_command_sitealias(), like so:

      if ($os === 'Windows') {
        $cmd = preg_replace('/\s*--backend$/', '', $cmd);
      }
    

    Then, the update runs. The upshot being, that I can get updatedb to run under windows, even though it falsely reports an error.

So. I have a patch. Please forgive me if I made it wrong, this is my first time ever submitting a patch. It's not a good patch because it doesn't really fix any problems, but if you're desperate to get updb working under windows, then perhaps it will help you.

The patch will get updb working under the following circumstances:

  • You have added the drush path to your $PATH environment variable
  • You have edited your drushrc.php and set $options['remote-os'] = 'Windows';

If you do those things, and apply the patch, then updb should work. Maybe. Who knows with windows...

P.S. I also implemented the suggestion from #65, escaping spaces up to the first '--', but unfortunately proc_open() still breaks, so I ended up replacing the whole path with just 'drush.bat'. Hence the need for the $PATH variable.

jrsinclair’s picture

Finally, a useful patch.

The latest 5.0-dev HEAD version of Drush seems to have taken care of the escaping issue with the inclusion of _drush_backend_argument_string() and _drush_escape_option().

Unfortunately, on my Windows Server, Drush would still hang on any call that relied on backend invocation. After doing some more thorough debugging, it turns out that the reason Drush was hanging was a deadlock between drush and its child backend process.

It seems that under the Win/PHP version of pipes, the stream_get_contents() function (line 669 of backend.inc) doesn't receive an EOF until the pipe is closed at the other end. Unfortunately, in _drush_proc_open(), drush was waiting until the child process terminated before it would close the pipe.

So, the child process was waiting for the parent to close the pipe before it would finish, and the parent process was waiting for the child process to finish before it would close the pipe. Hence, deadlock.

Closing the pipe in _drush_proc_open() as soon as drush is finished writing to it fixes the issue. Patch attached.

P.S. I am sooooooooooooo glad that drupal.org is using Git now. Made this process so much easier. Many thanks to the git migration team.

greg.1.anderson’s picture

Code looks good & works well on Linux, so I committed #69 to master.

@jrsinclair: what environment are you using to do your remote command execution? cygwin?

jrsinclair’s picture

@greg.1.anderson Unfortunately I haven't found a good way to get remote command execution working. I currently have a very rough workaround where I map a network drive to the Drupal install I want to update, and run drush locally.

So long as the settings.php has an hostname or IP address for the SQL connector, then it works OK. If the database is set to connect to localhost, then it dies (or worse, ovewrites a local database), which isn't the best. It also relies on the DB server being able to accept external connections - also not always the best. But it works.

I did try installing copSSH/cygwin (ala http://www.timdavis.com.au/git/setting-up-a-msysgit-server-with-copssh-o...) but could not get git operating, so I abandoned it. Also, drush has always seemed a little shaky under cygwin.

If drush remote command execution is ever going to work on Windows though, then I'm guessing copSSH/cygwin is a reasonable place to start investigating. Cygwin seems like a much more reasonable dependency to get drush working than an entire VM running Ubuntu.

greg.1.anderson’s picture

Status: Needs work » Needs review
FileSize
9.58 KB

Here is a major step forward for this issue. The attached patch continues previous work, and advances the codebase to the point where it is possible to store drush in a location where the path contains a space. Paths are escaped by Windows-specific or Linux-specific escaping functions; escapeshellcmd is no longer used.

Works great on Linux; needs more testing on Windows.

greg.1.anderson’s picture

Status: Needs review » Needs work

Still not working right on Windows; the quoting is incorrect:

Backend invoke: "C:\Users\Greg Anderson\drush\drush.bat"  --all --root="c:\Users\Greg Anderson\Documents\My Web Sites\Drupal 7 Developer release\" --uri=default sql-conf --backend  2>&1 [0.11 sec, 4.31 MB]

The command could not be executed successfully (returned:  'C:\Users\Greg' is not recognized as an internal or external command, operable program or batch file., code: 1) [0.12 sec, 4.31 MB]

Behavior is the same for both Powershell and the DOS prompt.

greg.1.anderson’s picture

If you install cwrsync, then this patch will allow you to execute remote commands on a Linux server from a Windows machine. Windows remote commands are still not working, and sql-sync is not working yet.

greg.1.anderson’s picture

Here is the patch. Also, paths that start with "C:\..." confuse drush rsync.

babyboys’s picture

thanks for you share with us.

greg.1.anderson’s picture

This patch has the improvements from #1062962: drush dd on windows returns invalid path, plus additional fixes to allow rsync to work. On Windows, we assume that rsync is cwRsync, and translate paths from "C:\path" to "/cygdrive/c/path" (cwRsync requires the later and rejects the former).

Also includes some work-in-progress code to prevent --backend from freezing up on Windows. There is still some problem with proc_open on Windows; parameters are not being passed through from backend invoke.

greg.1.anderson’s picture

Issue tags: +Windows
FileSize
27.89 KB

I wanted to compare some environment differences between DOS/Powershell and MINGW, so I made some fixes to get MINGW closer to functional. I have discovered that the problems described above happen in about the same way on MINGW. There are still problems with the code in this patch; drush still works better under DOS/Powershell.

greg.1.anderson’s picture

Status: Needs work » Needs review
FileSize
51.7 KB

This latest patch pretty much clears up all of the escaping issues with DOS, Powershell and MINGW (msysgit, which even supports core-cli). Cygwin might work too; the adventurous might want to venture to try it.

There are still a few issues with Windows remaining: remote drush commands to Windows servers does not work, sql-sync does not fully work, etc.; however, I will be tackling those in separate issues. Please try out this patch and report any remaining issues related to escaping here.

Note that the attached patch was made with git format-patch, and must be applied with git am. It contains several commits, so it would be best to apply it on a branch before reviewing. If anyone wants to see a single unified patch for reviewing purposes, let me know and I'll post one.

moshe weitzman’s picture

I read through the code and it all looks good to me. I'd say that you can commit it as soon as you are comfortable. I'd like to move into release mode right afterwards. I'm hoping for Drush5 with Windows support by Drupalcon London.

I ran the unit tests on OSX and got only 1 failure which brings up an interesting problem in the tests.

1) backendCase::testOrigin
Expected ssh command was built
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-Simulating backend invoke: ssh -i mysite_dsa 'user'@'server' 'drush  --uri='\''sitename'\'' --root='\''/path/to/drupal'\'' --simulate version 2>&1'  > `tty` 2>&1
+Simulating backend invoke: ssh -i mysite_dsa user@server 'drush  --uri=sitename --root=/path/to/drupal --simulate version 2>&1' 2>&1

/Users/mw/c/h/drush/tests/backendTest.php:32

We are doing less quoting with this patch which helps readability. However, our quoting will depend on the OS that is running the tests. Not sure if we need to copy a few of our smart OS knowledgeble functions into the unit test suite so we can reuse them for assertions like this.

I'll try to run the test suite on windows and report back results.

Finally, it would be good to start thinking of new tests which would exercise any new code or brittle code that we don't want to break in the future.

mikeker’s picture

The patch in #79 doesn't apply:

$ git apply ~/Downloads/766080-escape-paths-for-windows.patch
warning: drush has type 100644, expected 100755
error: patch failed: drush:70
error: drush: patch does not apply
error: patch failed: includes/backend.inc:535
error: includes/backend.inc: patch does not apply
error: includes/exec.inc: No such file or directory
error: patch failed: includes/drush.inc:702
error: includes/drush.inc: patch does not apply
error: includes/filesystem.inc: No such file or directory
warning: drush has type 100644, expected 100755
error: patch failed: drush:55
error: drush: patch does not apply
error: patch failed: includes/drush.inc:700
error: includes/drush.inc: patch does not apply
error: includes/exec.inc: No such file or directory
error: includes/filesystem.inc: No such file or directory
error: commands/core/drupal/batch.inc: No such file or directory
error: patch failed: commands/sql/sql.drush.inc:375
error: commands/sql/sql.drush.inc: patch does not apply
error: patch failed: includes/backend.inc:542
error: includes/backend.inc: patch does not apply
error: patch failed: includes/drush.inc:699
error: includes/drush.inc: patch does not apply
error: includes/exec.inc: No such file or directory
error: includes/filesystem.inc: No such file or directory

Along those lines, is this a patch again the 5.x branch? If so, how does one git clone the 5.x branch. All I see are:

$ git branch -a
  7.x-4.x
* 766080
  remotes/origin/4.7.x-1.x
  remotes/origin/5.x-1.x
  remotes/origin/6.x-1.x
  remotes/origin/6.x-3.x
  remotes/origin/7.x-1.x
  remotes/origin/7.x-4.x
  remotes/origin/HEAD -> origin/master
  remotes/origin/debian
  remotes/origin/master
  remotes/origin/upstream

I'm hoping to follow up on the issues raised in #1103038: DIRECTORY_SEPARATOR causes errors on Windows running CygWin/Msys/Git shell, but I don't seem to Git it... Sorry, couldn't resist.

greg.1.anderson’s picture

Yes, the patch is against the 5.x branch, which is "master" in git. I'll commit it after kulov has a chance to run it through its paces on Windows.

The test cases need a little work before they will pass on Windows; I'll make some changes to that tomorrow.

I don't think that we can reliably use our smart quoting routines to generate the expected values; I think we'll instead need to use if (drush_is_windows()) { to select which of the hand-coded expected value to test against.

kulov’s picture

Patch in #79 can be applied successfull on 5.x branch. I went through most of the drush commands using local Win to Win env and they all work as expected. No issues in escaping or such. I think we are safe to go with commit.

greg.1.anderson’s picture

Status: Needs review » Fixed

Committed. Thanks to all who helped with this issue.

mikeker’s picture

Status: Fixed » Needs review
FileSize
36.65 KB

@greg: Thanks for the info. However, after applying the patch drush_mkdir() was failing because of forwardslashes in the path (shell is msysgit). I've attached a patch that adds

  // On Windows, the directory separator must be a backslash for creating directories.
  if (drush_is_windows()) {
    $path = str_replace('/', '\\', $path);
  } 

to drush_mkdir. But I noticed this patch, which should include everything from #79, seems to be much smaller than the one in #79...

While digging around I came across this comment which is says exec() command on Windows calls "cmd.exe /c my_command" (which is verified by looking at the PHP sources ~ line 533). That explains the need for backslashes on Windows. Perhaps a better place to check for this is in drush_shell_exec()?

The above comment also mentioned problems with PHP 5.2 vs 5.3 and whether you need to surround my_command in double-quotes. I quickly tried both 5.2.11 and 5.3 with drush upc and didn't have any problems.

mikeker’s picture

I'm hoping for Drush5 with Windows support by Drupalcon London

A big +1 for that!

greg.1.anderson’s picture

#79 is committed, so you should make all future patches off of HEAD of master. I think that I can fix this in drush_shell_exec based on your comments, though; thanks.

mikeker’s picture

FileSize
614 bytes

Sorry all, I obviously need to refresh my browser more often as I missed #83 and #84 while messing around with #85...

Anyhow, here's #85 against the latest and greatest from master.

msonnabaum’s picture

Assigned: Unassigned » greg.1.anderson
Priority: Major » Critical

Commit 8b8cc5a54bfb13554ef48829536c0f5e1bd431e9 is actually breaking all of drush on lucid, and most likely other versions of deb/ubuntu I can't test on because of this line:

  if [ "x${MSYSTEM:0:5}" = "xMINGW" ] ; then

Since /bin/sh is a symlink to /bin/dash on ubuntu, dash won't allow bash-style string manipulation like that, throwing a "Bad substitution" on every drush command. It's a bug which I think has been fixed since that works on most other OSes but we need to work around it here.

If we can rely on cut, this is probably the cleanest:

if [ "x$(echo ${MSYSTEM}| cut -c 1-4)" = "xMINGW" ]; then

or if we need to use sed (which we already reference in ./drush):

if [ "x$(echo ${MSYSTEM}| sed 's/^\(.\{4\}\).*/\1/')" = "xMINGW" ]; then

Marking as critical since its currently breaking all tests.

greg.1.anderson’s picture

Status: Needs review » Fixed

#89: Sorry about that, I already committed a fix. Although I forgot about the dash issue, I think that my fix is cleaner and still works acceptably un debian-based distros. This is what I did:

if [ ! -z "$MSYSTEM" ] && [ "x${MSYSTEM:0:5}" = "xMINGW" ] ; then

MSYSTEM is only defined on MINGW, and on MINGW, sh is bash. If MSYSTEM is not defined, then dash never sees that bash-only construct. (By this logic, maybe only the test for MSYSTEM is necessary...)

#88: I do not find this to be necessary. I can call:

drush ev "drush_mkdir('c:\users\greg anderson\newfolder');"

It works with no problem. I could fix up the slashes a bit harder -- and if you look in the code you will see that it used to even go so far as to replace c:\path with /c/path or /cygdrive/c/path per the platform, but again, this did not seem to be necessary, so I did not put it in.

Please re-open with more details if this is not working on your system. I just installed my MINGW this week; perhaps an upgrade would also fix the problem? I'm not opposed to supporting older versions of MINGW, but I want to understand why we're doing backslash replacement before blindly putting it in.

msonnabaum’s picture

Priority: Critical » Major

Ok, just saw 4873e84. Good solution.

I had tried if [[ -n "$MSYSTEM" && "x${MSYSTEM:0:5}" = "xMINGW" ]] ; then which didn't work, but what you did makes perfect sense.

msonnabaum’s picture

Status: Fixed » Needs review
FileSize
958 bytes

Also, it looks like commit c674b0b changed the output we expect on the backend test.

Patch should fix it, but I'd like Greg to review first.

moshe weitzman’s picture

Looks right at first glance. We'll need to give more attention to this when we get the test suite passing on Windows.

greg.1.anderson’s picture

I started working on the unit tests last night, and found it a bigger bother than expected -- just to install phpunit -- even on Linux. Seems it wasn't always this way, but apt-get install phpunit (on ubuntu 10.10) gives phpunit 3.4, and drush now requires 3.5. I was tempted to just back out all of the phpunit 3.5 constructs, but that seems like an uphill battle. Getting the latest phpunit via the pear install is a bit of a bother, but pulling from git isn't too bad. I think I'll make drush test install phpunit, fix up your php.ini (or tell you to do it if its not writable), adjust phpunit.xml and run the runner (skipping steps that are already done).

#92 looks good, except I notice that the tty thing is gone, and I thought I only took that out for Windows (where it blows up). It doesn't always work anyway; I'm getting "ambiguous redirect" on my remote ISP host with it for now. It is unnecessary locally, and unnecessary for non-interactive commands. Without it, sql-cli works, but you don't see the prompt, and core-cli does not work. Let's commit #1058874: drush_backend_invoke buffers output till end of command. Show progress. before deciding what to do with the tty redirect. I'm okay with committing #92 as-is in the meantime.

msonnabaum’s picture

We should probably note somewhere that you need to install phpunit from pear. It's not *too* bad, but you just have to get all the channels registered.

I wrote a chef recipe to install it on ci.drush.ws, which while it isn't a shell script exactly, it does show what needs to be done:

https://github.com/msonnabaum/drush-ci-chef/blob/master/cookbooks/phpuni...

Basically, you just need to register those three channels and it should work.

I'll commit 92 for now. That'll bring us back to all tests passing, so if tests start failing in jenkins as more windows patches go in, we should take them seriously.

greg.1.anderson’s picture

Status: Needs review » Fixed

For the record, this is what I did that worked:

    sudo apt-get install php5-curl
    sudo pear channel-discover pear.phpunit.de
    sudo pear channel-discover components.ez.no
    sudo pear channel-discover pear.symfony-project.com
    sudo pear upgrade --force PEAR
    sudo pear install --alldeps phpunit/PHPUnit

The "pear upgrade --force PEAR" was the tricky part that required some googling to get right. I also confused myself a bit the first time by omitting the install of php5-curl and misreading some error messages. On Windows, the install worked just with the channel-discover and install steps.

It seems like last time it was less of a bother to install; maybe I just used apt-get install then (which is no good now, since we need a newer PHPUnit). I added some instructions to the README.txt file in the drush/tests folder.

carole’s picture

Trying to get drush to work on windows and I'm not a sysadmin. Please advise if I need to worry about PEAR on WAMPServer 2, & exactly what to do if I should.

greg.1.anderson’s picture

The PEAR stuff is only necessary for phpunit, which you do not need to simply run drush. There will be a Drush installer for Windows available shortly; once that is available, it will be pretty easy to get all the components you need installed.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

mstrelan’s picture

I'm still having trouble getting this to work for features-update-all on Windows. Have posted an issue in the Features queue - #1177278: drush fu-all doesn't work on windows. I have set up an alias with os => 'windows', is this all that should be required? When it gets to the backend invoke it just flies straight through and the path to drush doesn't have any slashes in it, it has spaces. I am using 5.x-dev.