Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drush doesn't pick up current terminal width when run through a pipe #3843

Closed
joachim-n opened this issue Jan 4, 2019 · 17 comments
Closed

Comments

@joachim-n
Copy link
Contributor

joachim-n commented Jan 4, 2019

Describe the bug

When I run a drush command on a remote site, the output is wrapped even though my local machine's terminal might not need it.

E.g. if I run 'drush pml' on the local site, where I've set my terminal super-wide to prevent wrapping, I might get:

  Purge                             Purge Tokens (purge_tokens)                                                         Disabled   8.x-3.0-beta8
  Purge                             Purge UI (purge_ui)                                                                 Disabled   8.x-3.0-beta8

But on a remote site:

Voting        Voting API (votingapi)           Disabled   8.x-3.0-beta1
votingapi     votingapi_widgets                Disabled   8.x-1.0-alpha3
              (votingapi_widgets)
Webform       Webform (webform)                Enabled    8.x-5.0-rc16
Webform       Webform Bootstrap                Disabled   8.x-5.0-rc16
              (webform_bootstrap)
Webform       Webform Demo:                    Disabled   8.x-5.0-rc16

Expected behavior

It would be nice if Drush used the local terminal window's size for output from the remote site.

For instance, with the 'pml' command, it's useful to pipe this through a filtering tool (grep/ack/ag), and if lines are wrapped, then the results aren't as good, as the module machine name is not always on the same line as its status and version number.

Workaround

None afaik.

System Configuration

Q A
Drush version? 9.3.1
Drupal version? 8.x
PHP version 7.1
OS? Mac
@greg-1-anderson
Copy link
Member

I can't remember if we changed this code recently, but it would be helpful if you reported issues using the most recent version of Drush. We happen to be re-working the process code in the latest Drush 9.6.0 betas (stable release coming shortly).

@joachim-n
Copy link
Contributor Author

Yup, I need to upgrade Drush. Unfortunately, I've got things that need updating all over the place in my project's codebase!

Do feel free to consider this dormant until I report back with findings on the latest Drush :)

@greg-1-anderson
Copy link
Member

I'll see if I can do some of my own investigation while I have the hood open on the process code.

@weitzman
Copy link
Member

weitzman commented Jan 4, 2019

We could send COLUMNS environment variable to the subprocess. We already do some of that in the postgres db driver -

$process = Drush::process($sql->connect(), null, $sql->getEnv());
.

I've seen some oddness when you do that around inheriting the environment variables of the parent process. Not sure if that would matter here. There is a Process method for forcing that inheritance.

@joachim-n
Copy link
Contributor Author

joachim-n commented Jan 7, 2019

UPDATE: NOPE!

Just a quick note to say I've temporarily hacked around this in environment.inc's drush_build_drush_command():

  else {
    // Set environment variables to propogate config to redispatched calls.
    if (drush_has_bash($os)) {
      if ($php) {
        $environment_variables['DRUSH_PHP'] = $php;
      }
      if ($php_options_alias = drush_get_option('php-options', NULL, 'alias')) {
        $environment_variables['PHP_OPTIONS'] = $php_options_alias;
      }
      $columns = drush_get_context('DRUSH_COLUMNS');
      if (($columns) && ($columns != 80)) {
        $environment_variables['COLUMNS'] = 250; // $columns; <-- HERE!
      }
    }
  }

@joachim-n
Copy link
Contributor Author

joachim-n commented Jan 7, 2019

Ok, that hack didn't do anything at all.

The problem is actually more complicated than originally stated, it turns out!

drush @foo pml works fine with my wide terminal.

The problem actually is when I use xargs to run a command on multiple sites.

I'm doing this to check which live sites are running a particular module:

drush sa --format=list | ag live | xargs -t -I % drush % pml | ag MODULE

Removing the prettifying filters from that to strip it back to basics, the command is:

 drush sa --format=list | xargs -t -I % drush % pml

This doesn't pick up the columns. I imagine it's something to do with the piping?

@joachim-n
Copy link
Contributor Author

Yup, the problem is in Environment::calculateColumns():

        // Trying to export the columns using stty.
        exec('stty size 2>&1', $columns_output, $columns_status);
        dump($columns_output);

debug output is:

array:1 [
  0 => "stty: stdin isn't a terminal"
]

and so that method falls back to 80 further down.

@joachim-n joachim-n changed the title forcing drush output from remote sites to be the width of the current terminal drush doesn't pick up current terminal width when run through a pipe Jan 7, 2019
@joachim-n
Copy link
Contributor Author

Tweaked version of hack in drush_build_drush_command() which does work:

    // Set environment variables to propogate config to redispatched calls.
    if (drush_has_bash($os)) {
      if ($php) {
        $environment_variables['DRUSH_PHP'] = $php;
      }
      if ($php_options_alias = drush_get_option('php-options', NULL, 'alias')) {
        $environment_variables['PHP_OPTIONS'] = $php_options_alias;
      }
      $columns = drush_get_context('DRUSH_COLUMNS');
      if (($columns) && ($columns != 80)) {
        $environment_variables['COLUMNS'] = $columns;
      }

      $environment_variables['COLUMNS'] = 200; <-- HERE
    }

@greg-1-anderson
Copy link
Member

In my brief experiments, it seemed that setting COLUMNS worked for the current call, but when Drush made a call to the remote system, it would pass a new COLUMNS that contained the current terminal width (or 80 if stty didn't work) instead of the COLUMNS environment variable that was passed to it.

@weitzman
Copy link
Member

weitzman commented Jan 9, 2019

In my brief experiments, it seemed that setting COLUMNS worked for the current call, but when Drush made a call to the remote system, it would pass a new COLUMNS that contained the current terminal width (or 80 if stty didn't work) instead of the COLUMNS environment variable that was passed to it.

@greg-1-anderson Is that with Drush8? Does Drush9 work the same?

@greg-1-anderson
Copy link
Member

That was Drush 9. I believe Drush 8 works correctly, but haven't tried it recently.

@weitzman weitzman added the drush9 label Jan 9, 2019
@joachim-n
Copy link
Contributor Author

joachim-n commented Jan 18, 2019

Still seeing this on Drush 9.5.2.

$ drush @foo.bar pml
 --------------------------------- -------------------------------------------------- ---------- --------------------
  Package                           Name                                               Status     Version
 --------------------------------- -------------------------------------------------- ---------- --------------------
  Core                              Actions (action)                                   Disabled   8.6.5
  Core                              Aggregator (aggregator)                            Disabled   8.6.5
$ drush sa --format=list | ag live | xargs -t -I % drush % pml
drush @foo.bar pml
 ------------- -------------------------------- ---------- --------------------
  Package       Name                             Status     Version
 ------------- -------------------------------- ---------- --------------------
  Acquia        Acquia connector                 Disabled   8.x-1.14
                (acquia_connector)
  Purge -       Acquia Purge (acquia_purge)      Disabled   8.x-1.0-beta3
  reverse
  proxies &
  CDNs
  Acquia        Acquia search (acquia_search)    Disabled   8.x-1.14

@wimleers
Copy link
Contributor

Seeing this too, another >2 years later 😞

I'm seeing this as part of writing test coverage for a custom drush command, even though I am using \Drush\TestTraits\DrushTestTrait::drush():

  use DrushTestTrait;

  public function testCustomDrushCommand(): void {
    $this->drush('custom');
    var_dump($this->getOutput());
  }

… this insists on using 80-width terminal, I could not figure out a work-around in half an hour of digging.

Because it is using an 80-width terminal, it causes https://github.com/consolidation/output-formatters to fail to compute a sensible column width (my custom Drush command is returning a \Consolidation\OutputFormatters\StructuredData\RowsOfFields).

Even though the column width for the table header is okay, the contents within each column only get a single character width … which obviously causes a ridiculous amount of wrapping. So neither consolidation/output-formatters#55 nor consolidation/output-formatters#56 have fixed it for me.

This is why I wanted to just force a wider terminal … but it seems impossible without hacking drush. I even got as far as \Drush\Config\Environment::exportConfigData()'s

            // These values are available as global options, and
            // will be passed in to the FormatterOptions et. al.
            'options' => [
                'width' => $this->calculateColumns(),
            ],

… but I can't figure out how to pass this "width" "option" to Drush.

Eventually I found https://www.drush.org/latest/cron/#setting-columns, and so now I have my alternative drush() helper method with the necessary work-around hardcoded:

  /**
   * Invoke drush via execute(), with a wider-than-default terminal.
   *
   * @param string $command
   *   The drush command to execute.
   *
   * @see \Drush\TestTraits\DrushTestTrait::drush
   */
  protected function drush(string $command) : void {
    $this->execute(
      'COLUMNS=160 ' . $this->getPathToDrush() . ' --no-interaction ' . $command,
      0,
      NULL,
      [
        'HTTP_USER_AGENT' => drupal_generate_test_ua($this->databasePrefix),
      ]
    );
  }

I hope this contains helpful pointers for the maintainers, and hopefully also helpful work-arounds for other users 🤓 😊

@weitzman
Copy link
Member

@wimleers If you have time, I propose a fix at #4688. You could hack your drush by hand if you dont want to deal with Composer. All changes are in one method.

@weitzman
Copy link
Member

@joachim-n - perhaps you can tets the #4688 PR as well? You can also hack the changes in by hand instead of using my branch in your Composer.

@joachim-n
Copy link
Contributor Author

I tried this with xargs | drush pml to just run a single command. It doesn't seem to terminate, but it gives output.

With the diff from #4688, I still get the same output. Adding

      dump( (new Terminal())->getWidth());

to the method before the return shows '80', unfortunately.

I'm on Drush 10.3.6.

@weitzman
Copy link
Member

Hopefully fixed by #4688. At least it wont be our fault if its wrong :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants