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

Add commandfile searchpath for global composer installers #1513

Closed
wants to merge 2 commits into from
Closed

Add commandfile searchpath for global composer installers #1513

wants to merge 2 commits into from

Conversation

markhalliwell
Copy link
Contributor

First step in fixing #572 & #1347

@greg-1-anderson
Copy link
Member

Overall, I think this is a good approach, although I think some little things that still need to be worked out.

Some considerations:

  1. Documentation will be needed to explain to folks what the expectations are for Drush extensions that use Composer libraries.

  2. Do we deprecate/obsolete drush_autoload(); in favor of requiring composer/installers in a Drush extension's composer.json file? Seems that we shouldn't support too many ways to do autoloading "right".

  3. If this is going to be required, do we want to insure that packagist.drupal.org injects composer/installers before continuing here?

  4. We should still support folks who want to install Drush via git clone followed by composer install. For example, if someone follows the Drush installation instructions, and ran git clone https://github.com/drush-ops/drush.git /usr/local/src/drush, then the Drush script would be at /usr/local/src/drush, and this PR would look for Drush extensions at /usr/local/src/drush/vendor/drush. This seems okay.

5) This PR should probably exclude the Drush directory itself when searching for commandfiles. For example, if someone has installed Drush via composer global require drush/drush, then the vendor directory will be at $HOME/.composer/vendor, and this patch would look for Drush extensions at $HOME/.composer/vendor/drush. However, Drush itself in this instance will be located at $HOME/.composer/vendor/drush/drush, and it seems undesirable that Drush should be included in its own searchpath. I know that we already exclude Drush's test directory, as some folks got into this same situation at times in the past, and we took some measures to prevent things from completely catching fire. Perhaps we should strengthen these tests, though? This might be as simple as adding 'drush' to drush_filename_blacklist(), but I'm not sure if this might hurt folks by sometimes excluding other directories named 'drush'.

@markhalliwell
Copy link
Contributor Author

For example, if someone has installed Drush via composer global require drush/drush, then the vendor directory will be at $HOME/.composer/vendor, and this patch would look for Drush extensions at $HOME/.composer/vendor/drush. However, Drush itself in this instance will be located at $HOME/.composer/vendor/drush/drush, and it seems undesirable that Drush should be included in its own searchpath.

No, this PR does not include Drush (core) in its own searchpath. I think you're missing the /../ bit that's in between:

$path = drush_get_context('DRUSH_VENDOR_PATH'); // ~/.composer/vendor

$path .= '/..'; // ~/.composer

$path .= '/drush'; // ~/.composer/drush

~/.composer/drush !== ~/.composer/vendor/drush/drush
It isn't the same directory where Drush core is installed, it's the Composer Installers path.

@greg-1-anderson
Copy link
Member

You're right, I slipped a directory in 5).

@markhalliwell
Copy link
Contributor Author

  1. Of course, documentation is paramount. However, I think this is the first step in actually achieving this goal so globally installed commands can be detected. I would likely hold off on any real documentation until this can be flushed out in more detail, this is just the first step in getting composer global install working from packagist.
  2. I'm not entirely sure about that one right now. But that also depends how it's being used (Drupal modules w/Drush support?) does the composer module leverage this at all? It may still be needed (for manual processes). If anything, I would imagine that just more documentation is added to the docblock saying something to the affect of "In most cases use of this function is not needed unless your commandfile has not been installed in a place where Composer has not generated autoloaders for you.".
  3. What is packagist.drupal.org? I don't think auto injecting any requirements is necessary or appropriate. I think documentation and examples of "how to create a global drush command" would be rather sufficient. I may be wrong.
  4. No, it would look for /usr/local/src/drush/drush, which likely wouldn't exist. That's fine though, because this is about supporting globally installed composer packages, so if they bypassed installing Drush with composer... I would imagine they would do the same for other packages?

Hmm... maybe the PR should instead be something along the lines of:

$composer = FALSE;
if (drush_shell_exec('which composer')) {
  $output = drush_shell_exec_output();
  $composer = array_pop($output);
}
$composer_dir = FALSE;
if ($composer && drush_shell_exec("$composer global config home")) {
  $output = drush_shell_exec_output();
  $composer_dir = array_pop($output);
}
$global_composer_drush_dir = $composer_dir ? $composer_dir . '/drush' : '';

@weitzman
Copy link
Member

I dont think we should put much energy into figuring out how to handle a globally installed drush and globally installed commandfiles. Composer's global support is pretty poor. But more than that, we have so many Composer dependencies going on with Drupal, Drush, and contributed extensions for each. I think our only prayer is to ask users to put everything in one composer.json at the root of their project.

I know that this is a big change from prior Drush usage, and may annoy some maintainers of global commandfiles. But those global commandfiles just have to be listed in each relevant project. If thats painful for some reason, you can always use --include to get your commandfile loaded. If you do that often, write a wrapper script that does it for you.

@markhalliwell
Copy link
Contributor Author

I dont think we should put much energy into figuring out how to handle a globally installed drush and globally installed commandfiles.

You didn't and won't have to.

I already I came up with a rather simple solution:
Only support global commandfiles via composer/installers with the type "drupal-drush"

It really is that simple. As it stands right now, there is no support whatsoever (i.e. Drush doesn't detect ~/.composer/drush).

I'm not suggesting that this is for all commands and that they would require composer/installers. No, this is only for global commandfile support for maintainers, like me, who wish to instruct users on how to install their global Drush command:

composer global install <vendor>/<project>:<version>

Composer's global support is pretty poor.

How so? If you mean in Drush, then sure... it's currently non-existent. To say that global installs of composer packages outside of Drush it isn't well supported, I have found (google searches) a lot of evidence to the contrary, especially when involving composer/installers.

But more than that, we have so many Composer dependencies going on with Drupal, Drush, and contributed extensions for each.

Again, this really isn't about Drupal installs. This is about global commandfiles that are system/utilitarian in nature and typically don't have much to do with a Drupal install (which would have it's own dependencies and whatnot). These global commandfiles just need to be discovered by Drush.

I think our only prayer is to ask users to put everything in one composer.json at the root of their project.

These projects already have individual composer.json files. This isn't about what it installs, but how and where.

I know that this is a big change from prior Drush usage, and may annoy some maintainers of global commandfiles. But those global commandfiles just have to be listed in each relevant project.

The fact that Drush moved to Composer at all is what is really the "big change". To say that a project is going to use something, then doesn't actually fully support all of it.... is such a Drupal-ism. I'm really tired of this mentality in the community.

I accepted the fact that Drush is using Composer. I have learned how Composer works because of Drush. I'm now trying to help make Drush work even better using Composer for the global commands that some of us maintain. How is this a bad thing? Why is there so much resistance and scrutiny for such a simple PR?

If thats painful for some reason, you can always use --include to get your commandfile loaded. If you do that often, write a wrapper script that does it for you.

Simple one-liner install commands is what people want. No one would use a Drush command if they had to constantly "include" it, or alias Drush to include it. Why even develop a command using Drush at that point?

Despite the fact that Drush is becoming more abstract (untethered from Drupal), the fact remains that there are common tasks and an ecosystem around Drush that global commandfiles would want to tie into.

To deny support for global commandfiles on the grounds that "composer globals are confusing" is ludicrous. This PR is evidence to the contrary.

Composer is the medium in which this project has moved forward and is installed. Its extensions should be supported in the same way, pure and simple.

@markhalliwell
Copy link
Contributor Author

I'm really not trying to be testy, I promise. I am simply trying to point out that there are indeed use cases where true global commands make sense. They're ones that are not necessarily tied to any one specific "project". There are two other drush commands that come to mind that could definitely take advantage of this change as well:

https://www.drupal.org/project/drush_iq
https://www.drupal.org/project/grn

@greg-1-anderson
Copy link
Member

One thing to clarify here--if the user is running with a site-local Drush, then the global locations (including the one added via this PR) are not considered or loaded. Not sure if this is clear to all or not.

I do agree that this PR is useful for global commands that do not need to bootstrap a Drupal site. There are only a few examples, such as the ones given above.

I think we could dispense with 2) - 4) in my initial post above; however, I do think that this PR should be considered together with the proposed documentation that goes with it. I think it is still worth moving forward here, but only if the use case is simple enough to easily explain in the documentation.

@greg-1-anderson
Copy link
Member

Actually, there's something that still needs to be worked out here before this PR would be usable. Currently, Drush identifies the Drupal site and determines if there is a site-local Drush there during preflight. As I mention above, with a site-local Drush, global locations are not considered at all. That means that this PR is only useful for commands that do not even target a Drupal site (including such things as iq and grn), as those that do must be copied into the composer.json of the Drupal site they are operating on. This PR might be useful for global commands that do not target a Drupal site (e.g. Drush Make, if it was a global command instead of part of Drush core).

I don't know if it makes any sense at all to have --local consider some global locations, or only certain kinds of global commands. Maybe, as Moshe says, the only thing that makes sense is for composer-managed Drush extensions to be part of the Drupal site they are used with.

@markhalliwell
Copy link
Contributor Author

One thing to clarify here--if the user is running with a site-local Drush, then the global locations (including the one added via this PR) are not considered or loaded. Not sure if this is clear to all or not.

Correct. That is why I added the search path inside the !drush_get_option('local') if statement. The same is true for commandfiles located in ~/.drush or $SHARE_PREFIX/share/drush/commands, they are ignored if the --local option is specified.

I do think that this PR should be considered together with the proposed documentation that goes with it.

I can add some documentation if y'all want, although not sure exactly where to put it. AFAICT though, there's no real documentation around the --local option to begin with anyway. Perhaps that should be a separate PR?

I don't know if it makes any sense at all to have --local consider some global locations, or only certain kinds of global commands.

The way I see it is this: if you have a Drupal install that has its own Composer install (i.e. a local Drush and whatnot), then it should be its own self-contained eco-system that manages its own packages and dependencies. It shouldn't load global commands of any kind. This is fine IMO. If I'm working on a site, then I would expect site specific Drush commands.

However, when I'm working on a module or theme independently with no site-install (or rather just a local dev instance), I would expect my global commands to show up (which are often utilitarian in nature for the development of said project).

FWIW, my recent involvement with Drush was sparked by https://www.drupal.org/node/2406661.

It was also sparked by https://www.drupal.org/node/1857042, in which I suggested creating a new Drush command for helping manage LWG exceptions (which I'm doing). This would be yet another "global tool" that isn't tied to any specific site install, but rather an individual module/theme project. It will help add, edit, list and show differences to assets in the exception list.

@greg-1-anderson
Copy link
Member

I guess I'm circling back to Moshe's comments it seems there are only a couple of examples of global commands that do not target any Drupal site. If a command targets a Drupal site (including something that only targets the directory root, and does not need an installed database et. al.), then it should go in the site's composer.json, or use existing global locations, or be selected via --include.

I think the workaround for this issue is to add $options['include'] = ... to your drushrc.php file, or ship with a wrapper script that uses --include. The main purpose for Drush is to bootstrap Drupal. You can use it for more than that, but I don't know that we should add extra infrastructure to support it.

@markhalliwell
Copy link
Contributor Author

or use existing global locations

I think y'all are really missing the point here: "existing global locations" isn't good enough.

composer global install DOES NOT install the package in ~/.drush. It installs it in ~/.composer/drush, if the package requires composer/installers with the type "drupal-drush".

If the package doesn't require composer/installers, it installs it into ~/.composer/vendor as normal. This obviously wouldn't work (#1347), which is why I'm saying that there's nothing wrong with requiring composer/installers for global commands (it adds specificity and a clear path on what is "supported" for global commands).

~/.drush is really just a legacy folder IMO and should be deprecated, plain and simple. Its not even being used for site-local, which means it's only being used in a global scope by Drush anyway. Since that's the case, why not just use Composer the way it's meant to be used?

As it stands now, if I were to run composer global update, it wouldn't update ~/.drush (which is a "composer local")

I would have to do cd ~/.drush && composer update. That's ridiculous.

What's even more ridiculous is if/when a command (like https://www.drupal.org/project/drush_iq) gives instructions like:

drush pm-download grn-6.x drush_iq
cd ~/.drush/drush_iq
composer self-update
composer install

That means if I want to update drush_iq, I now have to:

cd ~
drush pm-download drush_iq
cd ~/.drush/drush_iq
composer update

If drush_iq were a global composer install, then all I would have to do is:
composer global update

I really am failing to see why this is a recursive issue. Can we like actually talk this though in real-time or something?

@markhalliwell
Copy link
Contributor Author

Also, on a side note:

I just saw #1340 btw and this comment.

As stated above, I think the difference between global vs. site-local is being way over-thought or being made too complicated.

If people wanted global commands in their site-local Drush instance (which would normally be ignored, as it should), wouldn't they just add to the site-local Drupal composer.json the requirement for said "global" command so it gets installed alongside Drush itself? Why do we have to mix and match when they're included and not, that is just a recipe for disaster IMO.

To summarize scopes (I guess as a start for some documentation):

## Commandfile discovery process

Commandfiles are discovered in one of two different scopes:

### "global" scope
This scope is used when Drush does not discover a [site-local](#site-local-scope) install of Drush in
the current working directory. The following global paths are searched for any commandfiles:
  * `$COMPOSER_HOME/drush`
  * `$SHARE_PREFIX/share/drush/commands`
  * `~/.drush` (deprecated)

### "site-local" scope
This scope is used when Drush detects a Drupal installation that has it's own `site-local` installation of
Drush (or the `--site-local` option is specified). It will only discover commandfiles in the scope of the site-local project. In this case, no [global](#global-scope) commandfiles will be loaded. If a site-local
installation requires the use of "global" commands, then they should add the command as a requirement
to their project's `composer.json` file.

@markhalliwell
Copy link
Contributor Author

Btw, when I meant "deprecate" ~/.drush, I simply meant for the discovery of global commandfiles. I'm not suggesting the entire folder be remove. It obviously cannot be removed as it's used for many other internal Drush files (like project downloads, caching, etc.). However, that would also be a good thing so we're not traversing an internal folder for something as simple as trying to locate a commandfile.

The way I see this PR/issue is: this is really just the first step in transitioning into supporting "global" drush extensions/commands properly as composer packages (like it should be since its parent project, Drush, moved in this direction).

it seems there are only a couple of examples of global commands that do not target any Drupal site

The commands I have mentioned here are just the ones I use on, nearly, a daily basis. There are others (including personal/client global commands that aren't necessarily "public):
https://packagist.org/search/?tags=Drush
https://www.drupal.org/project/project_module?f[2]=im_vid_3%3A4654&solrsort=ds_project_latest_release+desc

@greg-1-anderson
Copy link
Member

While I acknowledge that there is still a problem with Drush + global commands in the Composer space, I don't think that this PR is the right way to go yet. It does not manage to get Composer to put the global Drush commands in the new location (through the recommended changes to the global command composer.json file), and the Drush global location that it adds is not one that Composer is already installing global commands to. Both are necessary, and then must be evangelized to global commandfile writers.

The reason we have to mix and match today is that global commandfiles that use Composer can cause a big problem for Drupal sites that also use Composer. I'd like to see this improved, but I don't think we're at the right solution yet. "Making a start" is not the right order to do things in, because we have to start telling people to do things one way or another. Let's figure out what the best final solution is first, then document it, then commit it, then evangelize it.

Until then, use an existing workaround

@markhalliwell
Copy link
Contributor Author

It does not manage to get Composer to put the global Drush commands in the new location (through the recommended changes to the global command composer.json file)

.... composer global install doesn't have to install them in ~/.drush.... nor should it really. That directory really should just be considered as an "internal" directory for Drush anymore. Drush should be able to pickup packages that were installed with composer global install....... it is that flipping simple....

The reason we have to mix and match today is that global commandfiles that use Composer can cause a big problem for Drupal sites that also use Composer.

Exactly... which is why global commands are not (nor should be) included in site-local instances. This fact has not changed whatsoever and has absolutely nothing to do with this PR/issue. It doesn't search ~/.composer/drush if it's a local instance..... at all.

"Making a start" is not the right order to do things in, because we have to start telling people to do things one way or another. Let's figure out what the best final solution is first, then document it, then commit it, then evangelize it.

Um... no y'all don't. I've already figured this out and I'm not even that hard-core of a developer. I just started reading how Composer works and expected that my global composer packages would be picked up by Drush... they're not... hence the PR.

This is literally just a matter of supporting global composer installs... that's it. This isn't "changing" anything whatsoever. It's purely an additional search path. You don't have to advertise this at all yet, if you truly think this so "horrible". Even if you did, it wouldn't be this catastrophic event y'all are making this out to be.... 😮

If anything, this would actually allow the standardization of how to build global Drush commands via Composer only.... it does not change any other method in how global Drush commands can be and are currently being built or installed.

This PR is about being able to switch from this method (which would still work btw):
drush dl drush_sql_sync_pipe --destination=$HOME/.drush && drush cc drush

To just be able to solely rely on Composer to install its global command. Composer is a package handler. It makes the project easier for people like myself to manage, install and provide updates to its users in a uniform way. This global composer support would purely be an opt-in process at this point... I'm not suggesting to make it permanent yet.
composer global require markcarver/drush-sql-sync-pipe && drush cc drush

Using Composer the way it's meant to be used deserves a trial run at least, jeez...

@greg-1-anderson
Copy link
Member

Deferring to Moshe's judgement here.

@markhalliwell
Copy link
Contributor Author

@greg-1-anderson, I'm really confused and frustrated. In various PR/Issues, you've said it's a good idea to support global composer installs... you just didn't know how Drush should detect them.

I come up with a real solid solution and then suddenly did a 180 and linked twice to "Moshe's judgement". Are you suggesting that I didn't read that comment? I did read it and then quickly responded/disputed it in the comment below it as well as the various comments made since.

I'm really not trying to be negative here. I'm just really frustrated that such a simple PR is being chalked up to "unnecessary" out of mere speculation and conjecture rather than actual cold-hard facts.

I'm also frustrated because I feel like I'm constantly having to re-explain and justify this PR when I really shouldn't have to, the code speaks for itself. It also doesn't help that I'm the only one defending it either.

It's baffling that this PR is getting the work around when this main issue (#572) has been rather indecisive for over a year now. It's like y'all are saying: "you should use Composer to install Drush globally, but everything else has to be done manually". This just doesn't make any sense whatsoever...

I have already given many valid reasons as to why this is a great PR, but I have yet to receive a single reason really based in any truth as to why it isn't.

@greg-1-anderson
Copy link
Member

Currently we support:

cd my-awesome-drupal-project
composer require drupal/some-cool-drush-command-that-works-on-any-site

We actively do not want to support:

composer global require drupal/some-cool-drush-command-that-works-on-any-site

This PR actively encourages this scenario, that we do not want to support, and doesn't even contain any documentation on proper usage, how to get your extension in ~/.composer/drush, etc. Moshe is the primary maintainer on Drush, and has already decided that he doesn't want this PR. That's why I linked to his comment (twice).

This situation makes an unfortunate casualty of:

composer global require drupal/some-cool-drush-command-that-does-not-bootstrap-drupal

I think that we might be in favor of that in theory, if we could do it in a way that does not actively promote the scenario we do not want to encourage. That means that a hard problem is standing in the way of your easy solution to a problem that is currently considered to be niche. Frustrating, yes, but restating the same reasons why you think this PR should go in won't make any forward progress here.

For now, you should use one of your workarounds -- perhaps add something to your documentation instructing people to add a search path including your extension to their drushrc.php file.

@greg-1-anderson
Copy link
Member

c.f. #572 (comment)

@ergonlogic
Copy link
Member

All of Provision (Aegir) is essentially made up of global Drush commands. Many don't bootstrap a hosted Drupal site, while others do. Now, we only need it to be "global" for a single user under normal circumstances. While I'd love to see Composer support for installing Provision, et. al. I'm particularly concerned when I read (#572):

Moving forward, it looks like we are going to completely disable support for any sort of global Drush commandfile in Drush 8, maybe with some way to opt-in for this behavior, maybe not.

@greg-1-anderson
Copy link
Member

I tried to straighten out the problems that Drush + Composer + Drupal has with global commands in #1520 -- although this PR does not magically allow Composer to do something that it cannot already do (robustly support global extensions), it only detects and warns the user of incompatible scenarios, and provides instructions on how to convert global commands to site-local commands as needed.

The problem is that this detection and reporting is complex, so it would be burdensome to maintain it. Doing nothing leaves the user open to really difficult-to-diagnose failure scenarios ("dependency hell"), though, which is what led to the conclusion that removing global commandfile support in Drush 8 was a lower-cost solution.

Maybe Aegir can stick with Drush 7 for a time, and ultimately replace Provision cli commands with either module-based Drush commands, or non-Drush cli commands? Short of that, the alternative is supporting something like #1520, but that would prove difficult. "Keep it in and let it fail" is not a good option; creative ideas that meet everyone's needs would be welcome.

@ergonlogic ergonlogic mentioned this pull request Aug 3, 2015
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