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.

* #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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

anarcat’s picture

Issue summary: View changes

summary

anarcat’s picture

Issue summary: View changes

add the backend stuff

anarcat’s picture

here's a failing test case for you.

anarcat’s picture

Actually, 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.

anarcat’s picture

Here's a test case that *really* fails. I actually had to change the mode on my tty to make it fail, but it fails!

anarcat’s picture

Status: Active » Needs review
FileSize
939 bytes

and here's the patch.

greg.1.anderson’s picture

I 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.

greg.1.anderson’s picture

anarcat’s picture

How can i test the interactive mode further? you seem to be saying sqlc works, what else?

greg.1.anderson’s picture

Search 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.

ergonlogic’s picture

Status: Needs review » Reviewed & tested by the community

I'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.

moshe weitzman’s picture

Assigned: anarcat » greg.1.anderson

Assigning 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.

greg.1.anderson’s picture

I 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.

moshe weitzman’s picture

Committed to 6.x. Thanks everyone.

ergonlogic’s picture

Version: 8.x-6.x-dev » 7.x-5.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Any chance we can get this backported to Drush 5?

moshe weitzman’s picture

I'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.

jonhattan’s picture

Tested 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.

greg.1.anderson’s picture

I'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.

anarcat’s picture

i 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.

greg.1.anderson’s picture

Let's try to get Drush-6beta2 and Drush-5.10 out, then backport this to Drush-7.x-5.x for Drush-5.11.

helmo’s picture

For a stable Aegir release this would mean we still have to depend on a patched drush 5.x...

greg.1.anderson’s picture

What if we did this for Drush-7.x-5.x?

  else {
    if (!drush_is_windows() && !empty($backend_options['interactive']) && !isset($backend_options['tty-is-evil'])) {
      $command .= ' > `tty`';
    }
  }

If we got that into 7.x-5.10, then Aegir could set 'tty-is-evil', and other code would be unaffected.

helmo’s picture

That sounds great!

Maybe we should include a drush_log() to a remaining user know about this feature being removed.

anarcat’s picture

I don't understand why backporting this patch into 5.10 is a problem, but okay.

greg.1.anderson’s picture

Status: Patch (to be ported) » Needs review
FileSize
888 bytes

It 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.

ergonlogic’s picture

Status: Needs review » Reviewed & tested by the community

The patch in #23 is identical to the one on #4. Either way, +1 for inclusion in 5.10

jonhattan’s picture

My 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.

greg.1.anderson’s picture

Assigned: greg.1.anderson » jonhattan

Assigning to @jonhattan to resolve in 5.x as desired.

greg.1.anderson’s picture

Version: 7.x-5.x-dev » 8.x-6.x-dev
Status: Reviewed & tested by the community » Closed (won't fix)
Issue tags: +Needs migration

This 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.

greg.1.anderson’s picture

Status: Closed (won't fix) » Closed (fixed)
greg.1.anderson’s picture

Issue summary: View changes

more related