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

Exclude the 'vendor' directory from consideration #1347

Closed
wants to merge 1 commit into from

Conversation

greg-1-anderson
Copy link
Member

Vendor directories have a large number of files in them, and should not be searched for commandfiles.

Currently, if you want to install a global Drush extension via Composer, you do this:

cd ~/.drush && composer require ....

This puts the commandfile and all of its dependencies in the ~/.drush directory, requiring Drush to search everything there for *.drush.inc files. This directory should be actively excluded by Drush.

Where should these commandfiles go, then? On further reflection (after creating this PR), I decided that global Drush extensions should be installed the same way as Drush is installed globally:

composer global require organization/project:dev-master

This is the only reasonable solution. It puts your global commandfiles in the same composer.json as the Global Drush. This insures that the dependencies for Drush and all of its extensions will be evaluated together at composer install time. Anything less than this can result in dependency hell. (See FAQ for composer_manager).

So, the big question is, how does Drush find commandfiles if we put them in ~/.composer/vendor/organization/project? This is no easier to search than ~/.drush/vendor, so we certainly should not simply add this location as a search path.

I see two options.

  1. Somehow find and register commandfiles in the vendor directory at composer install time.

  2. Alter ~/.composer/composer.json to include a custom installer, and redirect Drush extensions to ~/.drush/commands in its 'extras' section.

I will investigate these options, and see if I can come up with something workable.

@weitzman
Copy link
Member

 I decided that global Drush extensions should be installed the same way as Drush is installed globally:

Makes sense. Doesn't the PR make it hard to find such commandfiles since they live in user's home vendor dir?

@greg-1-anderson
Copy link
Member Author

Yeah, this PR breaks the usecase of composer global require for installing Drush global extensions, so it's not ready to go in yet.

I think that the right solution here is 1) above. If we made a custom composer installer, and require'd it from Drush, then we could write our own index of Drush commandfiles at composer install time. Then, we exclude 'vendor' directories from searching, per this PR, and find the commandfiles by including our own commandfile index.

We should just consider this PR postponed until that investigation is done.

@markhalliwell
Copy link
Contributor

I've been developing with the new Drush 7 code and have been fiddling around with a lot of different composer configurations. After a lot of trial and error with using drush_autoload autoloaders and running into dependency issues... I definitely agree that commands should be global and not in ~/.drush.

I don't think either solution proposed is very maintainable, neither for Drush nor a command maintainer. So far, the easiest way I have found to get a package picked up by Drush is use Composer Installers and add the following to my package's composer.json:

"type": "drupal-drush",
"require": {
  "composer/installers": "*"
}

Installing commandfile locally (~/.drush):

cd ~/.drush && composer require <vendor>/<package>:<version>

Installs package to: ~/.drush/drush/<package>
Modifies autoloader: ~/.drush/vendor/autoload.php

Aside from the aforementioned dependency issues, there is also the issue with having to include another autoloader, which may or may not be detected based on the current implementation of drush_autoload().

I also had to nest my commandfile under an internal commands directory so the ../../../vendor/autoload.php "candidate" would actually be the one that was ultimately detected. IMO this is really a nonstarter because project directory structures vary too greatly and will be a headache for Drush to ensure its including the correct autoload.php for that project.

Installing commandfile globally (~/.composer):

composer global require <vendor>/<package>:<version>

Installs package to: ~/.composer/drush/<package> (not ~/.composer/vendor/drush/<package>)
Modifies autoloader: ~/.composer/vendor/autoload.php

Installing a package globally actually solves a few issues:

  • By using Composer Installers and setting a package's type to drupal-drush installs it in a standalone directory that can be easily added to the searchpaths with something like:
drush_get_context('DRUSH_VENDOR_PATH') . '/../drush'
  • Modifies the autoloader Drush actually already uses. There would be no need to manually search for, and include, individual package autoloaders.
  • A tiny side bonus: would allow commands to actually start declaring which minimum version of Drush their command actually supports in their composer.json requirements.

Composer Installer Type

I saw the PR @RobLoach had summitted, composer/installers#65, but it's been sitting for quite a while.

Given that the PR is essentially adding a whole lot to validate a Drupal install, I think makes more sense if there were also a separate Composer Installer for Drush specifically: drush-command or drush-extension (since Drush commands are not always Drupal centric).

Perhaps even making the directory they install into drush-commands to help eliminate any confusion what it's for, maybe?

I'd be happy to submit a PR that lays this all out, I just figured I share my thoughts here first and get some feedback before proceeding.

@weitzman
Copy link
Member

@MarkCarver Have you considered adding drush to your project's composer.json? Thats the approach we are recommending at https://github.com/drupal-composer/drupal-project

@markhalliwell
Copy link
Contributor

Yes... it only works if you install globally though. Otherwise you run the risk of installing a duplicate Drush (and other packages) inside ~/.drush (same kind of dependency issues others were running into).

That isn't what my comment was about though.

The problem is: if you install globally, Drush does not find the command.

On Jul 28, 2015, at 12:17 PM, Moshe Weitzman notifications@github.com wrote:

@MarkCarver Have you considered adding drush to your project's composer.json? Thats the approach we are recommending at https://github.com/drupal-composer/drupal-project


Reply to this email directly or view it on GitHub.

@greg-1-anderson
Copy link
Member Author

Drush will find commands in ~/.drush/vendor/drush/drush. This is the current recommendation for global commands. (n.b. both of the examples in the comment above are global; local installation means with the Drupal site, in sites/all/drush).

This issue is, at the moment, just an optimization; if ~/.drush/vendor had a lot of large projects, Drush would search them all for commandfiles, which would be inefficient. We don't have any proposed implementations to improve the efficiency of this at the moment, though; this issue is pending waiting for such a proposal.

If Drush has a hard time finding a command that you install with composer install, try running drush cc drush.

@markhalliwell
Copy link
Contributor

The Local vs. Global I'm talking about is from Composer's perspective, not Drush's.... so no.... they're not both "global".

@greg-1-anderson
Copy link
Member Author

Yes, I did. Calling ~/.drush a "local" location vis-a-vis composer seems strange to me, because this is not the project root for Drush. But that's just semantics.

It is unclear to me, though, if you are working on the same use-case as this PR was originally meant to address, which is to say Drush extensions that are used in the absence of any Drupal site. Perhaps I was misdirected about this due to Moshe's comment in this respect.

If we are considering a Drush command that (a) has composer-based dependencies, and (b) runs on some Drupal site, then Moshe's answer is best. Drush, and the Drush command, should be included locally in the Drupal site's composer.json file. This will ensure that the autoload.php file is generated correctly, with full consideration for all libraries needed by the Drupal site, its modules, and any associated Drush extensions' libraries. But perhaps this was not the use-case you were considering.

I am kind of ambivalent about telling folks to require composer/installers in their Drush extensions. This did not feel right to me on first impression. I can't think of a better way to get the global extensions all in one place, though. Maybe if we changed drupal-composer/drupal-parse-composer to inject this to Drush extensions on drupal.org, it would be a good start. It would not hurt to also have this in Drush extensions that are included as requirements of some Drupal site, as custom installers are ignored when not in a top-level composer.json file. Perhaps this is one reason why it feels wrong for me to do this -- because I do not think of drush extensions as top-level components. But when used in a global (Drush) context, they effectively are (from a composer standpoint, anyway).

@markhalliwell
Copy link
Contributor

I definitely agree that commands should be global and not in ~/.drush

I really didn't say this right. I meant to say standalone commands here (the ones that are system/utilitarian in nature and not actually specific to any one particular Drupal install or even interacts with a Drupal install).

One command I maintain is https://www.drupal.org/project/drush_sql_sync_pipe, which really has nothing to do with Drupal itself, but rather Drush aliases, hosts and DBs. I would love to move this project to a composer based one, but to do so would mean the need of a way to globally install it.

So, the big question is, how does Drush find commandfiles if we put them in ~/.composer/vendor/organization/project? This is no easier to search than ~/.drush/vendor, so we certainly should not simply add this location as a search path.

I think I posted here because of this question in the first comment. I realize that my comments above may make more sense in regards to #572 (if that helps give you any context) and I perhaps had many tabs open. I apologize for convoluting this particular PR (which yes, is also still needed to help eliminate unnecessary vendor directory traversal).

I opened #1513, which I believe is the first step

I am kind of ambivalent about telling folks to require composer/installers in their Drush extensions.
Unfortunately, I think this is just the nature of Composer (at least from my perspective). The whole "local" vs "global" concept is rather difficult to grasp at first. As someone who didn't (at the time) know a lot about Composer. It's taken me a while, but after literally stepping through Drush's bootstrap process and viewing how Composer's autoload works I do understand how everything works a little bit better.

From my perspective, if someone had just said: "All you have to do is require composer/installers and then set the type of your package to "drupal-drush" and it'll just install like magic"... it would have made things a hell of a lot easier.

It's true that devs don't have to use the installers (especially if it's not a global type of Drush extension), but it is something that is (seemingly) quite common in Composer?

I would even go so far as to say that it is actually Composer's "official" way to deal with how wonky their "local" vs "global" installation confusion.

So Drush really doesn't have to re-invent their wheel here, nor should it. Let people use the installer and just add an additional search in the ~/.composer/drush directory. That way you don't have to deal with trying to filter out all the other vendor dirs/files.

@markhalliwell
Copy link
Contributor

Also to re-iterate the other huge pro to global composer package installs.

This requires the use of drush_autoload() in a commandfile's drush_COMMAND_init() (which may or may not work depending on the package's folder structure):
cd ~/.drush && composer require <vendor>/<package>:<version>

This does not:
composer global require <vendor>/<package>:<version>

@greg-1-anderson
Copy link
Member Author

Yes, it's hard for drush_autoload() to be completely bulletproof in all situations, and it is a drag to tell folks to call it. However, we can't get away from it completely unless everyone is using composer. If your Drush extension uses composer dependencies, and if you do not call drush_autoload(), then composer global require ... must be used with that extension. drush_autoload() was an attempt to bridge the gap between different potential operating environments for the same Drush extension. "Everyone should use Composer" is a more compelling story, but we're not there yet.

@markhalliwell
Copy link
Contributor

After typing up my responses below, I had this thought of one possible solution:

Upon Drush's installation or first run, maybe it should create a symlink from ~/.composer/drush to ~/.drush? This would allow global composer installer support.


Calling ~/.drush a "local" location vis-a-vis composer seems strange to me, because this is not the project root for Drush. But that's just semantics.

I think we're perhaps talking past each other, but basically the way I understand Composer is:

"local"
There is a ~/.drush/composer.json file and a ~/.drush/vendors directory. This technically has it's own autoloader and vendor packages that may or may not conflict with the ones installed globally (dependency issues mentioned). The same goes for Drupal installs that have their own composer.json file. These are all local, meaning they pertain to the directory in question.

"global"
This is whatever composer global config home says it is, which is typically ~/.composer. This is where Drush, its dependencies and its "global" autoloader is installed.

So because ~/.drush/composer.json !== ~/.composer/composer.json... it is technically a "local" Composer installation, despite the fact that this is considered a "global" Drush directory.

Yes, it's hard for drush_autoload() to be completely bulletproof in all situations, and it is a drag to tell folks to call it. However, we can't get away from it completely unless everyone is using composer.

Sure, which is why I didn't nix it in my PR :) I'm not suggesting that helper function be removed (yet anyway) and understand that function is/will be needed for other reasons.

If your Drush extension uses composer dependencies, and if you do not call drush_autoload(), then composer global require ... must be used with that extension.

Exactly. Except Drush doesn't actually find globally installed composer packages :) Which is what my initial comment was in response to: "how does Drush find [composer global] commandfiles".

"Everyone should use Composer" is a more compelling story, but we're not there yet.

I'm not suggesting that at all. However, in retrospect, that can't be the story at all if there isn't something to transition from in the first place. As it stands now, globally installed composer packages are not being picked up by Drush at all, they have to be installed in ~/.drush.

@greg-1-anderson
Copy link
Member Author

The thing about drush_autoload() is that I'm not sure how many extension writers ever got the message that it should be used, so adoption here might not be great anyway.

The problem with the "local" scenario above is that if ~/.drush/composer.json !== ~/.composer/composer.json, and Drush is installed in ~/.composer, then things won't work right. But I think we are agreed that this is a scenario that should be avoided.

@ghost
Copy link

ghost commented Nov 9, 2015

Can I create some kind of file in or above the node_modules(in my case) directory that would tell drush to skip scanning it?

@greg-1-anderson
Copy link
Member Author

Presently, no. Drush allows module developers to put Drush extensions anywhere they like, and at any depth inside their module. This is probably too flexible; I think it would be better to just make Drush more restrictive, rather than invent some sort of .drushignore file that we'd have to parse.

@mike-potter
Copy link

So what is the correct way to fix the drush performance problem when having a node_modules or bower_components directory in my theme. See https://www.drupal.org/node/2329453 core issue. That adds a patch to Drupal's file_scan_directory() function to not search these folders. Drush has the same problem with its drush_scan_directory() function.

When I have a theme that uses npm and bower (such as Radix) and I use any drush command from that directory, drush scans all of the files in node_modules and bower_components. When using dev environments such as docker where file performance is less than normal, this can be a significant performance drop. It adds 10-20 seconds on my local dev to any drush command, even simple "drush cc drush" commands because it's scanning all of these directories.

So like the Drupal patch, there needs to be some way to tell Drush to ignore these directories. I don't have the choice of installing globally. I might have several different themes with different gulp or npm configurations on my site. So getting rid of these directories within the theme folder is not an option.

Trying to bring this thread back on track to find a solution to this original issue. I'm not interested in a global vs local debate. I'm interested in knowing how to best improve drush to allow me to specify which folders I want it to skip via my drush config.

@ghost
Copy link

ghost commented Jan 12, 2016

.drushignore? :D

@mike-potter
Copy link

I thought the comment from greg above said they didn't want to use .drushignore. Did I read that wrong or is .drushignore the solution?

In my case, I have temporarily patched the drush_scan_directory() function like this:

$nomask = array_merge($nomask, array('node_modules', 'bower_components'));

but there needs to be a more general way.

@weitzman
Copy link
Member

For now, I think we can just add those 2 directories to the $no_mask. We need to examine usages of this function to see which ones would benefit from this change (probably all of them).

@greg-1-anderson
Copy link
Member Author

I think that ultimately, we should be more restrictive on where and how Drush searches for extensions in modules. I agree that @mike-potter 's workaround is the best interim solution.

…hing for commandfiles. vendor directories have a large number of files in them, and should not be searched for commandfiles.
@jhedstrom
Copy link
Member

Latest commit adds the 2 additional directories to the blacklist. It would be nice to get this fix in, and then make it more configurable in a follow-up.

@jhedstrom
Copy link
Member

Nevermind, I didn't realize that the vendor exclusion wasn't ready to go. I'll address the exclusion of node_modules and bower_components in a separate PR.

@weitzman
Copy link
Member

Noticed that WP-CLI has created a 'package' command which lists, installs, and uninstalls contibuted Composer packages. These are global commands, similar to what is being discussed here. The implementation is interesting. WP-CLI added a dependency on composer itself and then uses its internals to manage a composer.json at ~/.wp-cli (by default). At run time, the ~/.wp-cli/vendor/autoload.php is loaded so those commands can work.

Having said all that, global commands run a very real risk dependency hell and I'm wary of putting more time into supporting them.

@greg-1-anderson
Copy link
Member Author

Out of date.

@weitzman weitzman deleted the exclude-vendor branch September 28, 2020 02:51
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.

None yet

5 participants