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.
| Comment | File | Size | Author |
|---|---|---|---|
| #92 | update_backendtest.patch | 958 bytes | msonnabaum |
| #88 | 766080-drush-mkdir.patch | 614 bytes | mikeker |
| #85 | 766080-backslash-mkdir.patch | 36.65 KB | mikeker |
| #79 | 766080-escape-paths-for-windows.patch | 51.7 KB | greg.1.anderson |
| #78 | 766080-mingw-fixes.patch | 27.89 KB | greg.1.anderson |
Comments
Comment #1
greg.1.anderson commentedI 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.Comment #2
danchadwick commentedWell, 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.
Comment #3
greg.1.anderson commentedI'd like to see the output of the above with the
-dflag included, if anyone reproduces this issue.Comment #4
danchadwick commentedA 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:
Comment #5
greg.1.anderson commentedWell 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.
Comment #6
danchadwick commentedBingo. I applied (manually) the patch, and the patch resolved the problem.
Here's the definition of escapeshellcmd from the php manual (emphasis mine):
Comment #7
moshe weitzman commentedThanks Dan. Once Greg is satisfied, he will will commit the fix.
Comment #8
greg.1.anderson commentedAh 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.
Comment #9
jcmarco commentedI 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:
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)
Comment #10
greg.1.anderson commentedYou're right, I accidentally double-escape-encoded the drush path. I'll fix the fix.
Comment #11
greg.1.anderson commentedCommitted the enclosed patch. Please try again against HEAD and reopen if you're still having trouble.
Comment #12
jcmarco commentedHi 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.
Comment #13
jcmarco commentedTesting 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
Comment #14
greg.1.anderson commentedHm. 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.
Comment #15
greg.1.anderson commentedComment #16
greg.1.anderson commentedThe 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.
Comment #17
jcmarco commentedI 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.
Comment #18
greg.1.anderson commentedOkay, thanks.
Comment #19
bob.hinrichs commentedThe 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...
Comment #20
greg.1.anderson commentedWhy 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 -don your system?Comment #21
bob.hinrichs commentedSure. The result is the same as that noted above.
If I place the str_replace to remove the spaces, the status returns normally.
The command being passed to escapeshellarg is:
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.
Comment #22
greg.1.anderson commentedWell 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.
Comment #23
greg.1.anderson commentedJust 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:
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.
Comment #24
bob.hinrichs commentedHi 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.
Comment #25
greg.1.anderson commentedYou'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\'.Comment #26
greg.1.anderson commentedSee 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.
Comment #27
wapnik commentedHope 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.
Comment #28
greg.1.anderson commentedThis 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.
Comment #29
greg.1.anderson commentedAfter 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:
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.
Comment #30
greg.1.anderson commentedAfter 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:
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.
Comment #31
wapnik commentedThe 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().
Comment #32
greg.1.anderson commented@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.
Comment #33
greg.1.anderson commentedI 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.
Comment #34
wapnik commentedI fear that it's not quite possible have the same version of escapeshellarg for linux and windows. At least not for all cases.
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. :)
Comment #35
greg.1.anderson commentedPer #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.
Comment #36
wapnik commentedYes. Works. At least
cd "WINDOWS",cd 'WINDOWS',cd 'Documents and Settings'andcd "Documents and Settings".Comment #37
greg.1.anderson commented@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.
Comment #38
moshe weitzman commentedI'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?
Comment #39
greg.1.anderson commentedI 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.
Comment #40
greg.1.anderson commentedEdit: Deleted incorrect supposition, already disproved above.
Comment #41
greg.1.anderson commentedI 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.
Comment #42
moshe weitzman commentedI 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.
Comment #43
moshe weitzman commentedI 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
Comment #44
greg.1.anderson commentedThis change does not always work:
For example, if you have a php.ini in your $HOME/.drush folder, then you will get this:
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.
Comment #45
moshe weitzman commentedI 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.
Comment #46
greg.1.anderson commentedClosing out some related issues:
#1000124: Run drush with virtualbox to avoid the many Windows bugs in drush-3 and drush-4.
#755294: Drush 3 not working in Vista
#805998: drush up and drush updatedb not working on windows
#655202: no destination directory using the destination argument on windows
#853708: Drush make PHP crash on Windows
#955768: drush.bat lacks sandwich making in Windows
#990544: drush @sites cron returns SSH not recognized error in Windows
#755294: Drush 3 not working in Vista
#921328: The drush command 'Files (x86)/php/php' could not be found.
If working on this issue, you may find useful info in those other incidents. However, I think that all of the above basically boil down to the escaping issues that are being addressed here.
Comment #47
wapnik commenteddrush_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);Comment #48
wapnik commentedI'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?
Comment #49
greg.1.anderson commentedThank 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.
Comment #50
jcmarco commentedA 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.
Comment #51
wapnik commentedWhat 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 (
\)?Comment #52
jcmarco commented@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.
Comment #53
wapnik commentedThe 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...
Comment #54
greg.1.anderson commented@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.
Comment #55
danchadwick commentedI'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?
Comment #56
moshe weitzman commentedI 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.
Comment #57
agualog commentedHi 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 ?
Comment #58
MichaelCole commentedHey, 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
Comment #59
PierreJ commentedhi,
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 :).
Comment #60
greg.1.anderson commentedI 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_argsin 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_commandin 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:The 'platform' entry does not exist yet; I propose that if we add it to the alias record, then
drush_do_site_commandcould pass it as a parameter tobackend_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.
Comment #61
greg.1.anderson commentedI just looked at the code one more time, and would suggest the following refactoring:
backend_invoke_argscould generate an alias record and call a new functionbackend_invoke_sitealias. Then,drush_do_site_commandcould just disappear; any function that formerly called it would callbackend_invoke_sitealiasinstead.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.Comment #62
greg.1.anderson commented#61 is working fine, so I committed it to help move things along in this issue.
Comment #63
greg.1.anderson commentedNow 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_windowsshould be called in place ofescapeshellargmore or less as appropriate. Note that this is contingent on code correctly callingdrush_escapeshellarginstead ofescapeshellarg. When escaping for a remote machine, the correct pattern isdrush_escapeshellarg($arg, drush_os($site_alias_record));.Comment #64
jrsinclair commentedI 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:
The first issue is that the
escapeshellcmd()function will replace backslashes with a space, so if your Drush path isC:\Program Files\drush, for example, it will be translated toC: 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 (likeC:\Program Fileswill also cause problems. I guess we need some kind ofdrush_escapeshellcmd()equivalent ofdrush_escapeshellarg().The second, issue is that the DOS shell doesn't understand
#!shell script designators. That is, under windows you can't rundrush.phpas 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_pathwith the path todrush.batas 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.
Comment #65
greg.1.anderson commented@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.
Comment #66
myxelf commentedHi:
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:
So far, so good. Doing the following gives the error:
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:
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.
Comment #67
greg.1.anderson commented#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:
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:
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.
Comment #68
jrsinclair commentedOK. I thought I had at least some understanding of what was going on in the
backend.incfile, but after mucking around with this today, apparently I don't have a clue.I have established two things though:
_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.Weirdly though, if I remove
--backendfrom the command returned by_drush_backend_generate_command_sitealias(), like so:Then, the update runs. The upshot being, that I can get
updatedbto 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
updbworking under windows, then perhaps it will help you.The patch will get
updbworking under the following circumstances:drushrc.phpand set$options['remote-os'] = 'Windows';If you do those things, and apply the patch, then
updbshould 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.
Comment #69
jrsinclair commentedFinally, 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 ofbackend.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.
Comment #70
greg.1.anderson commentedCode 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?
Comment #71
jrsinclair commented@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.phphas 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.
Comment #72
greg.1.anderson commentedHere 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.
Comment #73
greg.1.anderson commentedStill not working right on Windows; the quoting is incorrect:
Behavior is the same for both Powershell and the DOS prompt.
Comment #74
greg.1.anderson commentedIf 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.
Comment #75
greg.1.anderson commentedHere is the patch. Also, paths that start with "C:\..." confuse drush rsync.
Comment #76
babyboys commentedthanks for you share with us.
Comment #77
greg.1.anderson commentedThis 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.
Comment #78
greg.1.anderson commentedI 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.
Comment #79
greg.1.anderson commentedThis 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.
Comment #80
moshe weitzman commentedI 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.
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.
Comment #81
mikeker commentedThe patch in #79 doesn't apply:
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:
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.
Comment #82
greg.1.anderson commentedYes, 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.Comment #83
kulov commentedPatch 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.
Comment #84
greg.1.anderson commentedCommitted. Thanks to all who helped with this issue.
Comment #85
mikeker commented@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
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.
Comment #86
mikeker commentedA big +1 for that!
Comment #87
greg.1.anderson commented#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.
Comment #88
mikeker commentedSorry 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.
Comment #89
msonnabaum commentedCommit 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:
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:
or if we need to use sed (which we already reference in ./drush):
Marking as critical since its currently breaking all tests.
Comment #90
greg.1.anderson commented#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" ] ; thenMSYSTEM 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.
Comment #91
msonnabaum commentedOk, just saw 4873e84. Good solution.
I had tried
if [[ -n "$MSYSTEM" && "x${MSYSTEM:0:5}" = "xMINGW" ]] ; thenwhich didn't work, but what you did makes perfect sense.Comment #92
msonnabaum commentedAlso, 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.
Comment #93
moshe weitzman commentedLooks right at first glance. We'll need to give more attention to this when we get the test suite passing on Windows.
Comment #94
greg.1.anderson commentedI 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 makedrush testinstall 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.
Comment #95
msonnabaum commentedWe 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.
Comment #96
greg.1.anderson commentedFor the record, this is what I did that worked:
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.
Comment #97
carole commentedTrying 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.
Comment #98
greg.1.anderson commentedThe 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.
Comment #100
mstrelan commentedI'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.