drush make overrides modules folder with downloaded modules

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).

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.

Verified that the issue exists in both 5.0 and 5.1.

d7.make

core = 7.12
api = 2

projects[] = drupal

update_a.make

core = 7.x
api = 2

projects[] = google_analytics

update_b.make

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.

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.

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.

INSTALL.txt
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.

rdfviz.make

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!

Wha?

It didn't used to do that.

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).

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

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.

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

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.

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

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

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

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

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

Result:
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.

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.

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

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

Rock on. Thanks to all who contributed!

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