In vagrant_drush_init there is a call to _drush_vagrant_help_text() that sets the context: 'vagrant_help_text'. This function call results in a call through to vagrant itself, which is expensive.

It seems like the 'vagrant_help_text' is only actually used in one place anyway, as the description of the 'vagrant' command in vagrant_drush_command().

The call through to vagrant happens on every Drush bootstrap, which is especially painful when doing a Drush make. Drush Vagrant will significantly slow down a Drush make!

I'm not 100% sure of the purpose of these calls to vagrant, are you trying to fetch the help text from Vagrant and provide it as Drush help text too?

CommentFileSizeAuthor
#20 issue-1779866-20.patch3.08 KBfranskuipers
#15 Don_t_call_vagrant_on_every_Drush_command-1779866-14.patch3.21 KBhelmo

Comments

helmo’s picture

helmo commented

Integrating help text from vagrant itself is indeed part of the intended behavior.
Also to know witch sub-commands are available for us to use.

We might be able to use drush_cache_set() to lessen the load...

steven jones’s picture

steven jones commented

Actually looking at the code in drush_vagrant_init I wonder if most of that could be moved into something that only runs on execution of a drush-vagrant Drush command. You could so something like this:


/**
 * Implements hook_drush_init().
 */
function vagrant_drush_init() {
  $command = drush_get_command();
  if (isset($command['requires_vagrant'])) {
    // Do checks for vagrant things and fetch help text.
  }
}

And then in the vagrant command definitions you could do something like:

/**
 * Implements hook_drush_command().
 */
function vagrant_drush_command() {
  $items = array();
  $items['vagrant-list'] = array(
    'description' => dt('List current Vagrant projects, VMs and statuses.'),
    'options' => array(
      'all' => dt('When in a project directory, list all projects instead of just the current one.'),
      'projects' => dt('Only list project names.'),
      'vms' => dt('Only list VM names.'),
      ),
    'strict-option-handling' => TRUE,  
    'aliases' => array('vls'),
    'bootstrap' => DRUSH_BOOTSTRAP_DRUSH,
    'requires_vagrant' => TRUE,
  );
  return $items;
}
ergonlogic’s picture

ergonlogic
he/him
Primary language English
Location Montréal, Québec 🇨🇦
commented
Status: Active » Needs review

Hmm... I'd thought by moving that code from hook_drush_load() to hook_drush_init() it would only run for drush-vagrant commands. I also wasn't aware you could extend the command array so easily.

I've implemented the suggestions above. Both only running vagrant-specific stuff on only drush-vagrant commands, and saving the results of the most common expensive operations to the drush cache.

steven jones’s picture

steven jones commented
Status: Needs review » Needs work

Getting there, but I think that the drush caches are global, so your cache keys need to be more specific, at least prefixed with 'drush_vagrant_', or possibly better, use the 'bin' argument to put them all in a 'drush-vagrant' bin.

ergonlogic’s picture

ergonlogic
he/him
Primary language English
Location Montréal, Québec 🇨🇦
commented
Status: Needs work » Needs review

Went with the latter option.

helmo’s picture

helmo commented

I'm not sure we'd want to cache the project root. Wouldn't that get messy when you work with multiple vagrant projects?

Anyway the code below looks like it could try to define PROJECT_PATH twice.

    if (isset($cache['project_root']->data) || isset($project_root)) {
      $project_root = isset($cache['project_root']->data) ? $cache['project_root']->data : $project_root;
      define("PROJECTS_PATH", $project_root);
    }
    define("PROJECT_PATH", drush_vagrant_get_project_path());

And related, when your not inside a project directory 'drush vbl' fails.
_drush_vagrant_get_project_path() through drush_vagrant_check_project sets an error.

ergonlogic’s picture

ergonlogic
he/him
Primary language English
Location Montréal, Québec 🇨🇦
commented

Yeah, that's confusing, but we're ony caching the PROJECTS_PATH. So, only '~/vagrant/projects' by default. I think it'd be worthwhile to change the name of the constant to PROJECTS_DIR, or something.

I'll check out the error with vbl.

ergonlogic’s picture

ergonlogic
he/him
Primary language English
Location Montréal, Québec 🇨🇦
commented

Hmm... It looks like I had an un-pushed commit, re-working how we now auto-load aliases, and climbs up the tree to determine our project's path. Sorry about that. 'drush vbl' works fine for me.

helmo’s picture

helmo commented

ok, after a git pull vbl is working again.

helmo’s picture

helmo commented

Then maybe the cache key should also have the extra S?
So cache['projects_root'] instead of $cache['project_root'] ...

helmo’s picture

helmo commented

The vagrant-blueprints command does not seem to need the 'requires_vagrant' flag.

Would it be usefull to have a similar flag indicate if a command needs a valid vagrant project directory?
This could be useful to generalize a check I'm proposing in #1790036: Issueing `drush vagrant delete project-name` behaves badly by promise to delete the pwd

ergonlogic’s picture

ergonlogic
he/him
Primary language English
Location Montréal, Québec 🇨🇦
commented

I think it's a good idea to have a 'requires_vagrant_project' flag, or similar. At what point do you suggest we check this?

helmo’s picture

helmo commented

I guess vagrant_drush_init() would be the place. Then we could generate an error there, through drush_vagrant_check_project() if the requirement is not met.

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
commented

Please do so. This would help #1790168: `vagrant aliases` doesn't validate a project directory forward too.

helmo’s picture

helmo commented
StatusFileSize
new Don_t_call_vagrant_on_every_Drush_command-1779866-14.patch3.21 KB

@ergonlogic: Do you agree with #10

The patch implements the 'requires_vagrant_project' flag.

edit: typo

ergonlogic’s picture

ergonlogic
he/him
Primary language English
Location Montréal, Québec 🇨🇦
commented

The change in #10 makes sense. But in this patch, I'm pretty sure that vagrant-list and vagrant-user should not require a vagrant project.

helmo’s picture

helmo commented

The patch from #15 does not add the flag for vagrant-list vagrant-user.

ergonlogic’s picture

ergonlogic
he/him
Primary language English
Location Montréal, Québec 🇨🇦
commented
Status: Needs review » Reviewed & tested by the community

Right, sorry. I parsed the patch incorrectly when reading it.

helmo’s picture

helmo commented
Status: Reviewed & tested by the community » Needs work

Committed #15 to 7.x-2.x

Now we can get back to #10

Then maybe the cache key should also have the extra S?
So cache['projects_root'] instead of $cache['project_root'] ...

and #11

The vagrant-blueprints command does not seem to need the 'requires_vagrant' flag.

Also committed to 7.x-2.x

franskuipers’s picture

franskuipers commented
StatusFileSize
new issue-1779866-20.patch3.08 KB

Patch for #10

Then maybe the cache key should also have the extra S?
So cache['projects_root'] instead of $cache['project_root'] ...

Changed also $user_settings['vagrant']['project_root'] to $user_settings['vagrant']['projects_root']
and $project_root to $projects_root

#11 is solved in this commit

franskuipers’s picture

franskuipers commented
Status: Needs work » Needs review
mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Patch in #20 saved me. It's hijacking and breaking other drush commands - like make_local. Drush Vagrant was hijacking the command only making it partially build.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Status: Reviewed & tested by the community » Needs review

False alarm, sorry.