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 drush launcher to bin, with drush 8 fallback. #1183

Merged
merged 11 commits into from
Sep 20, 2019

Conversation

simesy
Copy link
Contributor

@simesy simesy commented Aug 15, 2019

Checklist

  • Affected Issues have been mentioned in the Closing issues section
  • Documentation has been written/updated.
  • Changelog entry has been written

Drush recommends being installed as part of a site, so that a site's composer.json can specify which version of Drush is compatible. Drush launcher is the correct solution: a unified command that dynamically finds and calls Drush. It allows a fallback to the existing global drush.

This PR removes the ~/.composer/... directory from the PATH. It appears that it was only added there to allow drush to be executed and is redundant now.

The current docs only have a reference to Drush 9, no reference to Drush 8. Not sure exactly what the docs should have, so I just added a note about drush 8 in the drush9.md.

Changelog Entry

Drush launcher is now available. If you have drush available in a Drupal site, and you run drush from /app or the web root, Drush Launcher tries to run the local drush (usually vendor/bin/drush), before falling back to the global Drush. If you need to pin your Drush to the global drush, you should add /home/.composer/vendor/bin/drush to the start of PATH environment variable, or call this directly in your scripts.

Closes #1069

@simesy simesy changed the title Add drush launcher to bin, with drush 8 fallback, remove composer dir. Add drush launcher to bin, with drush 8 fallback. Aug 15, 2019
@Schnitzel
Copy link
Contributor

I like the approach here! Our current plan was that the global installed Drush8 already does the search for the site-specific Drush and if not it falls back to Drush8. But this solution is definitely more elegant and probably better for the future.

I'm not sure though if we can really remove the /home/.composer/vendor/bin from $PATH.
If anybody else that is using the Docker Images is installing something globally with composer, that person probably relies on the fact that the PATH contains the path to the executable that was installed globally. Unfortunately updating the $PATH would break functionality for these people and probably would be quite hard for them to figure out why this is.
So therefore I think we should leave the $PATH in there, even though the Image itself doesn't rely on it.

Also we just updated Drush versions: #1185, so we it's conflicting now, sorry.

@simesy
Copy link
Contributor Author

simesy commented Aug 15, 2019

Sounds good. There is one thing to change regarding the PATH, we'd need to move the composer reference to the end of PATH. Currently ~/.composer/vendor/bin is the first reference, so it has priority over /usr/local/bin. So I can modify the add_PATH script to put it at the end in cli/Dockerfile , or I can override it again completely cli-drupal/Dockerfile. What do you think? It is still a little problematic given most general advice on the internet suggests putting it at the start, eg export PATH=~/.composer/vendor/bin:$PATH

@simesy
Copy link
Contributor Author

simesy commented Aug 15, 2019

Alternatively we could install drush launcher to /usr/local/bin/drush-launcher, so that users can choose explicitly to use this (I think it would work fine).

@simesy
Copy link
Contributor Author

simesy commented Aug 16, 2019

There are a lot of approaches to making drush 9 (explicitly) and drush launcher available. Some people just do $DRUSH="/path/to/drush" and use $DRUSH everywhere. Acquia has drush8 and drush9. I'm happy to implement your preference that takes into account what "updating to drush 9" on Lagoon looks like.

@Schnitzel
Copy link
Contributor

Currently ~/.composer/vendor/bin is the first reference, so it has priority over /usr/local/bin. So I can modify the add_PATH script to put it at the end in cli/Dockerfile

Mhh are you sure? My understanding is that executables in the home directly should have preference over the global operating system level ones, or you could never overwrite the operation system level ones with local ones (like if you need another mysqldump version or so)

Some people just do $DRUSH="/path/to/drush" and use $DRUSH everywhere. Acquia has drush8 and drush9.

The problem is that we already started supporting drush and that's used by all the thousand of Lagoon Projects out there. I think we should do:

  1. Install the drush launcher at /usr/local/bin/drush
  2. Install Drush 8 not global anymore, but in another place inside the HOME directory
  3. point DRUSH_LAUNCHER_FALLBACK to wherever Drush 8 is installed
  4. Leave ~/.composer/vendor/bin in the path at the beginning, but nothing is existing in there anymore by default.

With this:

  • Developers don't need to change anything if they are okay with Drush8, plus projects that don't have a site specific drush installed still get a version of drush (the reason that we can't remove drush all together)
  • Developers that need a specific drush version just define it in composer.json and the drush launcher will find it.
  • We will upgrade the default drush8 to drush9 as soon as drush8 is EOL

@simesy
Copy link
Contributor Author

simesy commented Aug 17, 2019

Correct /home/.composer/vendor/bin is at the front of PATH, giving it priority over everything else. But that just defeats the point of drush launcher.

I like the plan you put forward I will follow up.

@Schnitzel Schnitzel added this to the v1.1.0 milestone Aug 28, 2019
@simesy
Copy link
Contributor Author

simesy commented Sep 5, 2019

I believe this is good to go. In the built image by default...

cli-drupal:/app$ drush --version
Drush Launcher Version: 0.6.0
 Drush Version   :  8.3.0

@simesy
Copy link
Contributor Author

simesy commented Sep 9, 2019

I've not been able to run the tests locally. But I take it I need to make this pass.

TASK [DRUSH - running drush -y sql-sync @drush-second @self on ci-drupal-drush-first@ssh on port 2020, searching for ''] ***

But if drush is still drush 8 by default i'm not sure why it would break - unless the Drupal site has drush 9 and this is running in the Drupal site ... so we'd expect the Drush 8 aliases to fail in a best practice situation and we need a Drush 9 alias equivalent in-place - whereever in-place is.

I see the tests eg testing the alias is there but I don't see a drush aliases file - would it be using the drush alias service?

@Schnitzel
Copy link
Contributor

so I think that error might has to do with an old drush version shipped inside the test drupal, I've updated all dependencies of that drupal, let's see

@Schnitzel Schnitzel merged commit 53d585f into uselagoon:master Sep 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Drush should not be installed globally, drush-launcher should be used.
2 participants