Problem/Motivation
So we have stumbled upon a problem with the infamous > `tty` in aegir (while trying to fix our #1931000: Missing drush backend output in frontend log): when we call "tasks" (basically drush commands that call drush commands that... etc), we need them to be "#interactive" because otherwise some output is lost (thanks to #1982502: Reduce complexity of backend invoke; consider removing concurrency).
But then, we get this error message:
Calling proc_open(php /usr/share/drush/drush.php --invoke --verbose --debug --uri=aegir2.aegir2.local --root=/var/aegir/hostmaster-6.x-2.x hosting-task 212 --context_type=site --platform='@platform_hostmaster' --server='@server_master' --db_server='@server_localhost' --site_path=/var/aegir/hostmaster-6.x-2.x/sites/aegir2.aegir2.local --site_enabled --language=en --client_name=admin --redirection= --cron_key= --profile=hostmaster > `tty` 2>&1);
sh: cannot create /dev/pts/1: Permission denied
And the drush command just fails to be called there.
The actual bit of code to generate this is:
drush_invoke_process('@self', 'hosting-task', array($task->nid), array('invoke' => TRUE), array('interactive' => TRUE));
Note that fork = TRUE "works" (in that it doesn't crash and we get the output), but that's not what we want, we want to wait for the task to complete. We *could* wrap around another fork, but that would be really, really silly.
Really, > `tty` needs to go. It's non-sensical, and I would be very curious to hear the justification for using that thing. It was introduced in commit 84266bd8 within the work on #1037088: Improve upgrade procedure followed by site-upgrade to better conform to UPGRADE.txt, yet no explanation as to why this was introduced was given in the commitlog, or that issue. In fact, in commit 2048de5c we even introduced this comment:
// TODO: `tty` is not usable on Windows. Is this necessary at all, and if so, is there a better way to do it?
greg.1.anderson, mind you, is the one who introduced the `tty` thing *and* that comment, so my guess is no one knows why it's there or if it's even necessary.
This stuff is not used on windows (because they don't have ttys, kudos to them i guess), and drush seems to be happy to run there.
Proposed resolution
Rip that stuff out of drush already.
Remaining tasks
* write a failing test
* rip it out
User interface changes
Drush should be interactive again.
API changes
None.
Related Issues
* #1532672: Can't auto-download dependencies through drush en. (> `tty` problem)
* #1946018: drush site-upgrade error sh: /dev/pts/0: permission denied
* #1982502: Reduce complexity of backend invoke; consider removing concurrency
* #1931000: Missing drush backend output in frontend log
Comment | File | Size | Author |
---|---|---|---|
#23 | drush--tty-is-evil-and-2025527-23.patch | 888 bytes | greg.1.anderson |
#4 | 0002-don-t-use-dev-pts-1-it-breaks-when-drush-is-ran-thro.patch | 939 bytes | anarcat |
#3 | 0001-failing-test-for-interactive.patch | 2.04 KB | anarcat |
#1 | 0001-failing-test-for-interactive.patch | 1.32 KB | anarcat |
Comments
Comment #0.0
anarcat CreditAttribution: anarcat commentedsummary
Comment #0.1
anarcat CreditAttribution: anarcat commentedadd the backend stuff
Comment #1
anarcat CreditAttribution: anarcat commentedhere's a failing test case for you.
Comment #2
anarcat CreditAttribution: anarcat commentedActually, this test fails only when you don't own your terminal, which happens when you did sudo to another user, for example. I'll try to fix the test case to cover for that, but it's tricky.
Comment #3
anarcat CreditAttribution: anarcat commentedHere's a test case that *really* fails. I actually had to change the mode on my tty to make it fail, but it fails!
Comment #4
anarcat CreditAttribution: anarcat commentedand here's the patch.
Comment #5
greg.1.anderson CreditAttribution: greg.1.anderson commentedI was prepared to say that Adrian introduced ` > tty` in backend invoke, but that does not appear to be correct. It did not exist at all in Drush-2.0, and the only place that it exists in Drush-3.0 is in the implementation of the core-cli command. Still, I swear that I did not invent this construct, although I am not sure where it came from originally. Before taking it out completely, it should be confirmed that it is still possible to respond to prompts when using interactive backend invoke calls. It seems that site-ssh and sql-cli both get along quite nicely without using ' > `tty`', but perhaps pm-enable is using it when calling pm-download. Note that there are no unit tests that confirm that interactivity is working correctly.
If interactive commands work, then I strongly support removing ' > `tty`' from the code. I agree that it is evil.
Comment #6
greg.1.anderson CreditAttribution: greg.1.anderson commentedClosed #1946018: drush site-upgrade error sh: /dev/pts/0: permission denied as a duplicate of this issue.
Comment #7
anarcat CreditAttribution: anarcat commentedHow can i test the interactive mode further? you seem to be saying sqlc works, what else?
Comment #8
greg.1.anderson CreditAttribution: greg.1.anderson commentedSearch for commands that use 'interactive' mode, e.g. pm-enable, which calls pm-download and asks the user to answer a y/n prompt. I don't know if there are other places. If it doesn't work, compare implementation of sqlc and ssh (which are the same) with the implementation of backend invoke's 'interactive' mode -- which also uses proc_open now. Maybe things were fixed so that > tty isn't even needed any longer. I haven't had time to test it, though; otherwise I'd RTBC this.
Comment #9
ergonlogicI've tested pm-enable where a dependency download is prompted and successfully executed in both 7.x-5.x and 8.x-6.x branches.
Comment #10
moshe weitzman CreditAttribution: moshe weitzman commentedAssigning to Greg as I can't evaluate this patch properly. This could go into 6.1 if we release 6.0 before this is committed.
Comment #11
greg.1.anderson CreditAttribution: greg.1.anderson commentedI visually inspected this and think it looks good, save for the interactive issue I pointed out in #5. I don't have time to exhaustively test this myself right now. If #9 is in fact working, then I am guessing that everything else should work as well. I do wonder if remote commands work. In any event, based on #9, I think that we should perhaps commit this and release a 6.0-beta2, and see if it causes problems for anyone in a wider audience.
Comment #12
moshe weitzman CreditAttribution: moshe weitzman commentedCommitted to 6.x. Thanks everyone.
Comment #13
ergonlogicAny chance we can get this backported to Drush 5?
Comment #14
moshe weitzman CreditAttribution: moshe weitzman commentedI'm leaning towards no, but I'm open to input from Greg and jonhattan especially. My issue is that the fix is not widely tested and by the time it is 5.x will be quite old.
Comment #15
jonhattanTested succesfully pm-enable on a remote site with dependencies download. It seems safe to backport, but I'd prefer to do that after drush 5.10 is out.
Comment #16
greg.1.anderson CreditAttribution: greg.1.anderson commentedI'd like to see this in use in a Drush -6 beta for a while before we backport. Other than that, I think it would be safe to do. 5.x will be kind of old by then, true.
Comment #17
anarcat CreditAttribution: anarcat commentedi would like to see this in drush 5, as we would prefer not to force people to use drush 6 for the aegir 2.x branch.
Comment #18
greg.1.anderson CreditAttribution: greg.1.anderson commentedLet's try to get Drush-6beta2 and Drush-5.10 out, then backport this to Drush-7.x-5.x for Drush-5.11.
Comment #19
helmo CreditAttribution: helmo commentedFor a stable Aegir release this would mean we still have to depend on a patched drush 5.x...
Comment #20
greg.1.anderson CreditAttribution: greg.1.anderson commentedWhat if we did this for Drush-7.x-5.x?
If we got that into 7.x-5.10, then Aegir could set 'tty-is-evil', and other code would be unaffected.
Comment #21
helmo CreditAttribution: helmo commentedThat sounds great!
Maybe we should include a drush_log() to a remaining user know about this feature being removed.
Comment #22
anarcat CreditAttribution: anarcat commentedI don't understand why backporting this patch into 5.10 is a problem, but okay.
Comment #23
greg.1.anderson CreditAttribution: greg.1.anderson commentedIt depend on when 5.10 is going to come out. Since this patch was released in 8.x-6.x-rc2, perhaps we'll have had enough time with this patch running in the field that we can go straight to 5.10, and we don't need to worry about #20. Here's a patch.
Comment #24
ergonlogicThe patch in #23 is identical to the one on #4. Either way, +1 for inclusion in 5.10
Comment #25
jonhattanMy objections to this patch are only to be conservative and don't introduce a regression.
I'm fine either with #20 in 5.10 or #4 in after 5.10.
I was waiting for #1748228: Error in the PHP config setting of magic_quotes_gpc prevents diagnosis of the problem through drush core-status to release 5.10 (it is a regression in 5.9). I'm planning to release it in a week or so.
Comment #26
greg.1.anderson CreditAttribution: greg.1.anderson commentedAssigning to @jonhattan to resolve in 5.x as desired.
Comment #27
greg.1.anderson CreditAttribution: greg.1.anderson commentedThis issue was marked
closed (won't fix)
because Drush has moved to Github.If desired, you may copy this bug to our Github project and then post a link here to the new issue. Please also change the status of this issue to
closed (duplicate)
.Please ask support questions on Drupal Answers.
Comment #28
greg.1.anderson CreditAttribution: greg.1.anderson commentedSee https://github.com/drush-ops/drush/issues/71
Comment #28.0
greg.1.anderson CreditAttribution: greg.1.anderson commentedmore related