Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Hi,
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
Comment | File | Size | Author |
---|---|---|---|
#15 | drush-copy-overwrite-1539076-14.patch | 3.59 KB | jhedstrom |
#9 | drush_make-deletions-1539076-ROUGH.patch | 2.18 KB | dman |
Comments
Comment #1
FluxSauce CreditAttribution: FluxSauce commentedI'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).
Comment #2
zarexogre CreditAttribution: zarexogre commentedNice 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.
Comment #3
FluxSauce CreditAttribution: FluxSauce commentedLike
projects[myproject][overwrite] = TRUE
? I don't think there's a CLI switch yet, or at least not by that name.Comment #4
zarexogre CreditAttribution: zarexogre commentedI 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.
Comment #5
FluxSauce CreditAttribution: FluxSauce commentedVerified that the issue exists in both 5.0 and 5.1.
d7.make
update_a.make
update_b.make
How to replicate:
drush 4.5 / drush_make 2.3 - this is the expected behavior
drush 5.1 and drush 5.0 - contents of sites/all/* is wiped - including libraries and themes.
Note that README.txt is also missing.
Comment #6
FluxSauce CreditAttribution: FluxSauce commentedThe 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
callsdrush_copy_dir
with the overwrite flag, which does the following: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.
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.Comment #7
zarexogre CreditAttribution: zarexogre commentedeasiest solution is to put the dev version of drush make into drush 5.1 insead of the stable version? as that has this fixed.
Comment #8
dman CreditAttribution: dman commentedThis 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.
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.
Comment #9
dman CreditAttribution: dman commentedI 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).
Comment #10
Steven Jones CreditAttribution: Steven Jones commentedYeah this is really annoying, and another regression of make functionality.
Tested patch in #9 and it seems to work quite nicely.
Comment #11
dman CreditAttribution: dman commentedJust 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.
Comment #12
Steven Jones CreditAttribution: Steven Jones commentedWe should add some tests for this functionality for sure, and clean up the patch in #9
Comment #13
jhedstromThere'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.
Comment #14
Steven Jones CreditAttribution: Steven Jones commentedWell, we should probably add some tests for
drush_copy_dir
while we're here too.Comment #15
jhedstromHere's a start at clean-up. Still need some unit tests for drush_copy_dir().
Comment #16
dman CreditAttribution: dman commentedGood job, that's the direction I was imagining also :-)
Looks good visually, I'll see if I can try out some actual tests.
Comment #17
zarexogre CreditAttribution: zarexogre commentedI'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.
Comment #18
dman CreditAttribution: dman commentedPatch required -p1 to apply (I thought we are not supposed to need that anymore?)
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:
Intended way to use this makefile:
Result:
sites/all/libraries/PHP_JPEG_Metadata_Toolkit arrives,
sites/all/modules entire folder is deleted as a side-effect. !!
After patch
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.
Comment #19
jhedstromI have some local work I'll post shortly that adds tests for
drush_copy_dir()
that should go in before this issue is closed.Comment #20
jhedstromI decided to commit #15 (see 458a05f) and opened #1590092: Add tests for drush_copy_dir() and drush_move_dir() for the tests.
Comment #21
FluxSauce CreditAttribution: FluxSauce commentedRock on. Thanks to all who contributed!
Comment #22
DamienMcKennaThanks everyone, I discovered the bug a few days ago and am glad someone fixed it!