On drush 4.5 with drush_make 6.x-2.x-dev works fine downloads modules and leaves modules already in sites/modules there.

Drush 5.1 with drush_make in core overrides the modules folder with the downloaded packages.

I figure the problem is the version of drush make add to 5.1 is the 6.x-2.3 and not the dev version.

Please can someone have a look.

Thanks Z

Members fund testing for the Drupal project. Drupal Association Learn more


FluxSauce’s picture

I've been experiencing this exact issue, and consider it a blocker for upgrading from 4.5 to 5.1. At first I thought it was related to http://drupal.org/node/1134326 , that's more for adding something to a library, like themes for jQuery UI.

I'm going to investigate to see if it also exists in 5.0 and/or dev. If this is a "feature", then there should be an option to allow overwrites (upgrades).

zarexogre’s picture

Nice to know its not just me, I figure 5.1 is pretty blocked if you have custom coded modules. There is an override switch but its off by default.

FluxSauce’s picture

There is an override switch but its off by default.

Like projects[myproject][overwrite] = TRUE? I don't think there's a CLI switch yet, or at least not by that name.

zarexogre’s picture

I havent tried it that was the only thing I could find, I think the dev version of drush make should be the one included in drush 5.1 then the problem would be solved.

FluxSauce’s picture

Verified that the issue exists in both 5.0 and 5.1.


core = 7.12
api = 2

projects[] = drupal


core = 7.x
api = 2

projects[] = google_analytics


core = 7.x
api = 2

projects[] = page_title

How to replicate:

drush make d7.make test
cd test

drush 4.5 / drush_make 2.3 - this is the expected behavior

drush make --no-core ../update_a.make .
drush make --no-core ../update_b.make .
ls -la sites/all/modules
total 20
drwxr-xr-x  5 501 dialout 170 2012-04-19 19:30 .
drwxr-xr-x  5 501 dialout 170 2012-04-19 19:29 ..
drwxr-xr-x 12 501 dialout 408 2012-04-19 19:30 google_analytics
drwxr-xr-x 18 501 dialout 612 2012-04-19 19:30 page_title
-rw-r--r--  1 501 dialout 162 2012-04-19 19:29 README.txt

drush 5.1 and drush 5.0 - contents of sites/all/* is wiped - including libraries and themes.

drush make --no-core ../update_a.make .
drush make --no-core ../update_b.make .
ls -la sites/all/modules
total 12
drwxr-xr-x  3 501 dialout 102 2012-04-19 19:32 .
drwxr-xr-x  3 501 dialout 102 2012-04-19 19:32 ..
drwxr-xr-x 18 501 dialout 612 2012-04-19 19:32 page_title

Note that README.txt is also missing.

FluxSauce’s picture

Priority: Normal » Major

The old behavior in drush_make.drush.inc function drush_make_move_build was for every directory in the build path, recursively force copy from the build to the destination.

The new behavior in make.drush.inc function make_move_build calls drush_copy_dir with the overwrite flag, which does the following:

    if ($overwrite) {
      drush_op('drush_delete_dir', $dest, TRUE);

In both instances, the list of folders in __build__ consists of one directory; sites. With the old behavior, it just copied the contents of the temporary build directory into the destination, overwriting. Within 5.x, drush_copy_dir is destructive; it removes the destination directory, then copies to it.

There is a comment in the new version which indicates this is known.

        // To prevent the removal of top-level directories such as 'modules' or
        // 'themes', descend in a level if the file exists.
        // TODO: This only protects one level of directories from being removed.

With that said, the fact that it wipes sites is a pretty serious bug in my opinion.

In my opinion, the optimal solution would keep track of the relative directory of the built projects/themes/libraries/etc., then only remove those explicit directories from the target.

Also, the flag "overwrite" in drush_copy_dir isn't accurate as it's not overwriting.

zarexogre’s picture

easiest solution is to put the dev version of drush make into drush 5.1 insead of the stable version? as that has this fixed.

dman’s picture

This is really scary.
I have a few modules that like having things in sites/all/libraries

The easiest and most consistent way to help someone set this up until now has been to distribute a make file with my module that gets the and installs libraries for us. This also really helps my automated (aegir-based) deployments by making recursive dependencies modular based on requirements.

The easiest way to ensure you have the required dependencies is to run
drush make --no-core sites/all/modules/rdfvz/rdfvz.make
from your Drupal root. That fetches the required libraries from github etc for you.


I tried that today, and it got the library OK ... and deleted the entire contents of sites/all/modules in the process - like everything including my local development modules!


It didn't used to do that.

dman’s picture

I upgraded. Still exists in drush version 6.0-dev (todays git 'master')

I started thinking about this and started to patch in a third option to drush_copy_dir() to support 'merge' options.
This addresses my one test case, but I am not aware enough of all the other things going on to know if this is a good direction. When is it intentional for drush_make to delete/overwrite everything? I only ever use it to add things to my deployments.

Anyway, heres' a rough patch that suggests how to merge instead of deleting. Probably should convert the $overwrite flag into an ENUM instead of BOOL, but I'm just overloading it for now as that's easy and safe (and messy).

Steven Jones’s picture

Status: Active » Needs review

Yeah this is really annoying, and another regression of make functionality.
Tested patch in #9 and it seems to work quite nicely.

dman’s picture

Just a note - patch #9 is not intended as a release-quality solution. It's a dirty hack right now. Albeit a direction that *could* work if I took my comments out of it.
But to be clean, we should properly change the function signature from BOOL to an INT (enum) with three constants.

Steven Jones’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

We should add some tests for this functionality for sure, and clean up the patch in #9

jhedstrom’s picture

There's a rough test here: http://drupalcode.org/project/drush.git/blob/refs/heads/master:/tests/ma...

That just needs to be extended to add more directory depth.

Steven Jones’s picture

Well, we should probably add some tests for drush_copy_dir while we're here too.

jhedstrom’s picture

Here's a start at clean-up. Still need some unit tests for drush_copy_dir().

dman’s picture

Good job, that's the direction I was imagining also :-)

Looks good visually, I'll see if I can try out some actual tests.

zarexogre’s picture

I'd like to try and test this patch see if it solves the problem of clearing out the modules folder. I have a test site with drush which wasnt working ready to test, I've upgraded to drush 5.1 but not sure how to apply this patch? Any hints? I will let you know if this solves the problem. Thanks again for your help.

dman’s picture

Status: Needs work » Reviewed & tested by the community

Patch required -p1 to apply (I thought we are not supposed to need that anymore?)

  drush$ patch -p1 < drush-copy-overwrite-1539076-14.patch

Patch applies clean to todays drush/master

Confirmed problem is still current:

Before patch

Here is a simple makefile I put with a module for convenient library downloads:

6$ cat sites/all/modules/meta/meta_pjmt.make 
api = 2
core = 6.x

; Libraries
libraries[pjmt][download][type] = "get"
libraries[pjmt][download][url] = "http://www.ozhiker.com/electronics/pjmt/PHP_JPEG_Metadata_Toolkit_1.12.zip"
libraries[pjmt][directory_name] = "PHP_JPEG_Metadata_Toolkit"
libraries[pjmt][destination] = "libraries"

Intended way to use this makefile:

$ drush make --no-core -v sites/all/modules/meta/meta_pjmt.make

sites/all/libraries/PHP_JPEG_Metadata_Toolkit arrives,
sites/all/modules entire folder is deleted as a side-effect. !!

After patch

$ drush make --no-core -v sites/all/modules/meta/meta_pjmt.make
  Loading release_info engine.                                                                                                                                            [notice]
  Make new site in the current directory? (y/n): y
  Executing: which wget
  Executing: wget -q --timeout=30 -O /private/tmp/download_fileZ9Jp4F http://www.ozhiker.com/electronics/pjmt/PHP_JPEG_Metadata_Toolkit_1.12.zip
pjmt downloaded from http://www.ozhiker.com/electronics/pjmt/PHP_JPEG_Metadata_Toolkit_1.12.zip.                                                                     [ok]
  Executing: unzip /tmp/make_tmp_1337393482_4fb7014acdecc/PHP_JPEG_Metadata_Toolkit_1.12.zip -d /tmp/drush_tmp_1337393485_4fb7014da6949
  Merging existing ./sites/all directory                                                                                                                                  [notice]
  Command dispatch complete                                                                                                                                               [notice]

Yes, the library has arrived and did not destroy my entire platform at the same time!

Patch works as promised. YAY thanks.
Also, as above, code review is good. The solution (that I originally proposed) still feels a little clunky, but after investigating, I can't see a better way given the current API.

jhedstrom’s picture

Assigned: Unassigned » jhedstrom
Status: Reviewed & tested by the community » Needs work

I have some local work I'll post shortly that adds tests for drush_copy_dir() that should go in before this issue is closed.

jhedstrom’s picture

Version: 7.x-5.1 »
Status: Needs work » Fixed

I decided to commit #15 (see 458a05f) and opened #1590092: Add tests for drush_copy_dir() and drush_move_dir() for the tests.

FluxSauce’s picture

Yes, the library has arrived and did not destroy my entire platform at the same time!

Rock on. Thanks to all who contributed!

DamienMcKenna’s picture

Thanks everyone, I discovered the bug a few days ago and am glad someone fixed it!

Automatically closed -- issue fixed for 2 weeks with no activity.