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

Speed up checking for vvv-hosts files #1182

Conversation

davecpage
Copy link
Contributor

The existing recursive blob causes massive slowdown of everything (provision, ssh etc) once I've got a few projects setup, especially if they have numerous directories.

This change limits the lookups to only look at the primary directory and a provision subfolder rather than unlimited depth. This matches what is documented, though other code such as provision-site.sh still looks 4 levels deep.

On my setup doing vagrant ssh returns in seconds rather than potentially minutes. Should be useful in situations such as https://jjj.blog/2016/05/slow-vagrant/

Related: #595

Only look 2 directories deep rather than unlimited.
@davecpage davecpage changed the title Speed up checking of vvv-hosts files Speed up checking for vvv-hosts files Apr 26, 2017
@LoreleiAurora
Copy link
Contributor

This could break site provisioners that rely on scanning for vvv-hosts multiple levels deep.
For example WordPress/meta-environment keeps its host files 3 levels deep.

Instead i think we should just restrict this to 4 levels deep bringing the hosts file on the hosts machine in sync with the hosts file on the VM.

To support other provisioners/setups such as WordPress/meta-environment
@davecpage
Copy link
Contributor Author

@LoreleiAurora I've rewritten the line to be more generic but restricted to 4 levels deep. Have checked locally using WordPress/meta-environment and it still picks up the vvv-hosts files but is still much faster in provisioning, vagrant ssh etc.

@LoreleiAurora
Copy link
Contributor

Looking at this again we need to come up with a different way to do the scan. the scan 4 levels deep in provision-site.sh is relative to a site dir where as this is relative to the www root.

@tomjn
Copy link
Member

tomjn commented Apr 27, 2017

This makes sense, I don't think more than 4 levels deep is necessary, as long as the docs are updated

@davecpage
Copy link
Contributor Author

Looking into it a bit more, the vvv-hosts code within Vagrantfile only looks within the www root hierarchy whereas the docs state that with custom paths the site local_dir path could be anywhere. With a custom local_dir the vvv-hosts file would only be used within provision-site.sh so any custom hosts would have to be duplicated in vvv-custom.yml otherwise nothing would be added to the hosts file on the hosts machine. At least that's how i'm reading it.

As the docs state that using vvv-custom.yml is the recommendation for setting hosts and a vvv-hosts file is only for backwards compatibility would a better solution be to see if hosts have been set in vvv-custom.yml and if not only then to look within that sites' local_dir to find a vvv-hosts file?

@LoreleiAurora
Copy link
Contributor

Personally i would go for the approach of

  1. Fix hosts files to scan site dirs
  2. Add a skip_vvv_hosts: true flag so the scan can be turned off globally and per site

This approach would prevent breaking bc.

@jeremyfelt @tomjn Thoughts?

Scan restricted to directories 4 levels deep
@davecpage
Copy link
Contributor Author

How about this? Covers point 1. Priority is hosts defined in vvv-custom.yml then on each site checks within the local_dir 4 directories deep for any vvv-hosts files to add those hosts.
Looking to add point 2 next.

Vagrantfile Outdated
@@ -71,6 +64,14 @@ vvv_config['sites'].each do |site, args|

vvv_config['sites'][site] = defaults.merge(args)

site_local_dir = vvv_config['sites'][site]['local_dir']
site_host_paths = Dir[File.join(site_local_dir, '*', 'vvv-hosts'), File.join(site_local_dir, '*', '*', 'vvv-hosts'), File.join(site_local_dir, '*', '*', '*', 'vvv-hosts'), File.join(site_local_dir, '*', '*', '*', '*', 'vvv-hosts')]
Copy link
Contributor

@LoreleiAurora LoreleiAurora Apr 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can shorten these 2 lines to

site_host_paths = Dir.glob(Array.new(4) {|i| vvv_config['sites'][site]['local_dir'] + '/*'*(i+1) + '/vvv-hosts'})

@LoreleiAurora
Copy link
Contributor

I have left a comment inline. Once this change is made this can be merged and a seperate ticket opened for skip_vvv_hosts: true

@davecpage
Copy link
Contributor Author

Have made that change, getting used to using Ruby so didn't realise that that was possible.

@LoreleiAurora LoreleiAurora modified the milestones: 2.x.x, 2.0.1 Apr 28, 2017
@LoreleiAurora LoreleiAurora merged commit 754f838 into Varying-Vagrant-Vagrants:develop May 4, 2017
@jeremyfelt jeremyfelt modified the milestone: 2.0.1 Sep 22, 2017
@jeremyfelt jeremyfelt modified the milestones: 2.0.1, 2.1 Nov 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants