If you have a makefile M containing:
projects[] = p_a
projects[] = p_b
And project p_a has a makefile N containing:
projects[] = p_b
Then you get a spurious "Directory not empty" error for p_b. Worse, if p_b is in a git repository, drush_make_download_git
fails horribly the second time round: drush make stops in its tracks when it tries to unpack p_b a second time (I think - it basically quits with "error in drush_drush_make_make" immediately after the second "p_b cloned from..." message.)
When you don't have control over the makefiles - if they're in contrib on d.o, or in someone else's module - then there's nothing you can do to prevent this from happening. Two project makefiles could justifiably bring in a CCK dependency, and you'll then get an error message that, it turns out, you can ignore; but if two makefiles try to bring in the same git repository, then you will never be able to build the site using drush make as it stands. This is probably going to become more and more of a problem as Drupal moves to using git.
Please find attached a patch which solves this. The patch does two major things:
1. DrushMakeProject::findDownloadLocation
keeps track of what it's been asked to download before. If it recognizes a package, it only reports a warning, not an error, on the directory not being empty.
2. drush_make_download_factory
checks for repeat projects, and produces a verbose warning when the second or future calls is attempted.
By still reporting [warning]s, that means that any potential problems can be tracked by looking at the reported drush log, but 1. means that the directory-not-empty isn't reported as a (spurious) error, and 2. means that a lower-level PHP error doesn't kill drush_drush_make_make
.
Comment | File | Size | Author |
---|---|---|---|
#36 | 947158-recursive.patch | 6.34 KB | boombatower |
#30 | 947158-recursive.patch | 6.33 KB | boombatower |
#23 | 947158-recursive.patch | 3.88 KB | boombatower |
#16 | 947158-recursive.patch | 3.65 KB | boombatower |
drush-make-recursive-makefiles-duplicates.patch.txt | 2.11 KB | jp.stacey |
Comments
Comment #1
middric CreditAttribution: middric commentedJust tested this, works here.
Comment #2
dmitrig01 CreditAttribution: dmitrig01 commentedI don't think this code belongs in the dowload factory. the download factory is meant to be something generalized and reusable beyond drush make itself, and if we hardcode projects in there, it becomes non-generalized. in the distant future of a 3.x branch this problem will be solved, but for now please make a new function that keeps a static variable of downloaded projects.
Comment #3
jp.stacey CreditAttribution: jp.stacey commentedI'm not clear what you want to be changed, and where.
There's two functions being changed in the existing patch:
* The factory needs changing somehow to prevent double downloading from different (or the same sources);
* findDownloadLocation needs changing somehow because it tries to create a directory for the download and throws a big red error, regardless of what the factory then finds out later on. But then this is in drush_make.project.inc, so maybe that's OK to be a bit more ad-hoc?
Basically: should I change
static $done
, orstatic $previous_locations
, or both? And do you want the patch's static arrays to just be replaced by function calls, or instead do you want the interception to happen the next level up?Comment #4
dmitrig01 CreditAttribution: dmitrig01 commentedthe $done - one level up. Ideally this patch should not touch .download.inc. Thanks!
Comment #5
Xen CreditAttribution: Xen commentedIt should also be more intelligent about it. If 2 makefiles pull in the same module, but specify different versions, it should error out, as there is probably a reason for the makefiles to be specific. Not specifying a version would be compatible with any other version.
It is a bit complicated to implement due to the recursive nature of drush make (say the master pulls in the lastest version of views and another module that has a makefile that specifies views in version 2.10).
Comment #6
dmitrig01 CreditAttribution: dmitrig01 commentedSolved by #919224-8: Allow use of subtrees or individual file from archives instead of the whole archive
Comment #7
freephile CreditAttribution: freephile commentedI too was experiencing spurious "Directory not empty" errors which would prevent me from downloading modules defined in my profile's make file.
The supplied patch did not apply for me cleanly, but when I manually edited drush_make.download.inc, it fixed the problem here. Thanks jp.stacey
I don't think that this issue should be marked 'closed' as a duplicate since the other issue does not (yet) have a solution and only speaks of a re-design of the workflow.
Comment #8
freephile CreditAttribution: freephile commentedI cloned the git source and patched it https://github.com/freephile/drush_make/commit/edbf21150d86e65e79a71693a... similar to jp.stacey's patch. The basic difference is that I did not change the signature of drush_make_download_factory(). I do setup static variables to keep track of what we've already seen.
This doesn't fix everything (it doesn't address the issue of whether first-in or last-in project specs are more important). But at least it won't stop you from building your profile.
Comment #9
dman CreditAttribution: dman commentedI get this also.
Given the situation described in the OP,
I'm trying to build a very large site out of a number or re-usable 'features'.
To allow these features to auto-install themselves, a number of them come with their own .make files.
Sometimes, more than one feature decides it requires the same module. Natural enough.
BUT if the same module is defined in more than one makefile, the project is fetched twice, the 'directory already exists' error is thrown, and a copy of the offending duplicate is dropped in the root of my drupal install.
This is different from git death, but has proven quite messy.
I've noticed this mainly with standard d.o project downloads (no args) though I also run with --working-copy and get it then also.
I'm trying patch #7, hopefully that will at least abort nicer than the current way it drops its eggs.
Comment #10
boombatower CreditAttribution: boombatower commentedI have exactly the same problem...just like #9 where a large site with a number of independent components depend on the same things.
The supplied patch definitely does not seem like the proper solution. Seems to make the end download correct, but still produces error messages along the way.
Comment #11
dman CreditAttribution: dman commentedI found the most horrible thing that was happening was if a *theme* was being downloaded a second time (edge case I know) then the contents of the theme directory, all the files individually, landed at drupalroot. This actually over-wrote or conflicted with the operation of the site altogether.
DrushMakeProject() will complain about "directory not empty" BUT the calling function doesn't pay attention to that error (a return of FALSE) and carrys on with the wrong parameters.
I needed to ensure that if directory existed ... then further processing should stop.
Comment #12
boombatower CreditAttribution: boombatower commentedWhat I ended up need since I have the module awssdk which also downloads a library of the same name was the following fix.
So we need to make sure that wherever this ends up being check that the project[type] is also considered.
Comment #13
boombatower CreditAttribution: boombatower commentedSeems like what is needed is a robust way of handling this. Perhaps the following ruleset:
1) download project (first means detected)
2) come across another entry for same project
3) ignore the download information, but attempt to apply patches or any other post processing, except use first locations directory location
or do we want to simply completely ignore duplicate references to a project?
Comment #14
boombatower CreditAttribution: boombatower commentedReading through code I'm not sure why things are as they are.
next line
which also returns the location and is ignored...
You run drush make --version and it attempts to build site...yikes
Comment #15
boombatower CreditAttribution: boombatower commentedI am actually a bit unnerved after reading through this code. It actually always "fails" to make EVERY SINGLE project since recurse never returns true...which also means it never prints an error even though the code feels that it failed...conditional statements assigning variables that never get used........
Comment #16
boombatower CreditAttribution: boombatower commentedThis cleans up only the necessary bits to get this to work.
I used the singleton pattern to keep track of project instances so we can easily determine if that have been "made" already. Something that can be done in the future would be to perform a sort of diff if the project has already been made...aka only apply patches or what not as described above.
Either way this fixes the problem, fixes the inconsistency of the recurse method, and the made property works as it should.
The notice that I print if the project has already been made never seems to display, but that seems like a general drush issue as it doesn't seem to display notices.
Comment #17
boombatower CreditAttribution: boombatower commentedComment #18
boombatower CreditAttribution: boombatower commentedComment #19
Xen CreditAttribution: Xen commentedSeems that my work in #1016924: Dealing with different versions of the same module in recursive makefiles is getting superceeded.
Like the singleton idea, but it needs to consider versions too. If 2 makefiles in a recursive makefile setup requires views in specific and differing versions, the make should fail. Different patch-sets should give a warning, as we'll assume that a patch doesn't change the patchee so much as to break it for other users, but the user might want to know.
We're trying to do the same thing as dman, and use recursive makefiles to allow subsystems to install their own dependencies, as some of the subsystems are optional.
Comment #20
boombatower CreditAttribution: boombatower commentedThat and more can be done with the singleton approach since we have all the instance data from the previous entry. So lets get this committed, and open up a followup issue to add a bunch of extra check for things like other versions and patches and what not.
Comment #21
boombatower CreditAttribution: boombatower commentedAnyone up for a review, lets get this in and then start making the improvements mentioned in the last few comments?
My openSUSE packages apply this patch against "upstream" if anyone wants easy management.
Comment #22
Xen CreditAttribution: Xen commentedI've tested it against the sandbox I used for testing my own patch. 6.x-2.x-dev fails when trying to fetch views again, with this patch it succeeds. With -v it prints "Attempt to build project views more then once prevented."
The patch looks tight to me.
Comment #23
boombatower CreditAttribution: boombatower commentedUpdated patch for offsets.
Comment #24
zzolo CreditAttribution: zzolo commentedThis patched worked for me. Thanks.
Comment #25
izmeez CreditAttribution: izmeez commentedsubscribing
Comment #26
attiks CreditAttribution: attiks commentedIt appears to be working for modules, but not for libraries.
test_947158.make:
output:
Comment #27
attiks CreditAttribution: attiks commentedverbose output
Comment #28
attiks CreditAttribution: attiks commentedI double checked with the following make file, only libraries declared twice, not the geophp lib and it's working
Comment #29
boombatower CreditAttribution: boombatower commentedYea, I'm patching...looks like since libraries have different defaults there was one other location that needed changing.
Comment #30
boombatower CreditAttribution: boombatower commentedThe following was needed:
I also went ahead and made the constructors private since they should never be called directly. Please double check and then RTBC.
Comment #31
boombatower CreditAttribution: boombatower commentedRan a grep to be sure I got them all.
Comment #32
attiks CreditAttribution: attiks commentedError: Call to private DrushMakeProject_Core::__construct() from context 'DrushMakeProject' in
/var/aegir/.drush/drush_make/drush_make.project.inc, line 43
Comment #33
attiks CreditAttribution: attiks commentedI changed them to protected and it's working
Comment #34
attiks CreditAttribution: attiks commentedRemark: In my test make file I used geoPHP as key, the geofield makefile uses geophp as key. The side effect is that it's downloaded twice.
Can we lowercase all keys? Or add this to the readme?
Comment #35
boombatower CreditAttribution: boombatower commentedWoops, yea they should be protected.
Comment #36
boombatower CreditAttribution: boombatower commentedComment #37
boombatower CreditAttribution: boombatower commented#34: That is a separate issue, as currently drush make is case sensitive as unix systems tend to be anyway. You can open a new issue for that if you want to pursue it.
Thanks for your reviews of this patch.
Comment #38
attiks CreditAttribution: attiks commentedWorking as expected, thanks
Comment #39
attiks CreditAttribution: attiks commented#37: issue created #1143102: Keys are case sensitive, but i think it's a bug
Comment #40
Leeteq CreditAttribution: Leeteq commentedSubscribing.
Comment #41
zoo33 CreditAttribution: zoo33 commentedYes, the last patch fixed the problem for me too.
Comment #42
mikl CreditAttribution: mikl commentedPlease merge this :)
Comment #43
jox CreditAttribution: jox commentedI was trying drush make the first time and ran into this problem. The patch in #36 fixed it for me.
Comment #45
dman CreditAttribution: dman commented#36 is good for me - fixed a recent new aegir deploy I had.
+1
Comment #46
boombatower CreditAttribution: boombatower commentedCommitted. (got access now)
Comment #48
igor.ro CreditAttribution: igor.ro commenteddo not work for me.
This is make file
projects[features][version] = "1.0-rc3"
projects[features][subdir] = "contrib"
projects[solution_core][type] = "profile"
projects[solution_core][download][type] = "git"
projects[solution_core][download][url] = "git://github.com/goruha/DrupalSolutionCore7.git"
projects[solution_core][download][branch] = "develop"
Comment #49
igor.ro CreditAttribution: igor.ro commentedComment #50
boombatower CreditAttribution: boombatower commentedAre you using drush 5.x which includes drush_make? If so please file a bug there and if necessary link to this.
Comment #51
Xen CreditAttribution: Xen commentedRecursion has general brokeness in 5.x: #1621030: Regression for recursive makefiles.