Prevent makefile recursion through per-project properties or a command-line kill-switch [#1989174]

Problem/Motivation

Drush make recursion is a very powerful way to build a site, but it can also bring its own problems. If a makefile consists of many projects with makefiles (e.g. features) alongside normal modules, then the build process suddenly becomes a lot more complex:

Because _drush_backend_proc_open() uses a call to end() to find the next concurrent process, makefiles actually build from the bottom up; so overriding works in the opposite direction from how people expect (e.g. #1621030-11: Regression for recursive makefiles. .)
Getting the performance improvements from concurrency entails the behaviour that on average four lines of separation n the primary makefile, between a makefile-including feature and a module, are not sufficient to prevent race conditions and the wrong module being downloaded: in practice depending on e.g. network speed, anywhere between one and a dozen seems necessary (e.g. #1621030-2: Regression for recursive makefiles. and passim.)
If a makefile is re-created with generate-makefile, then every module in every feature will be downloaded by the main makefile anyway, leading to a lot of repetition (especially if the features are shared across many builds, and therefore specify a different version of those repeated module projects) (e.g. #1621030: Regression for recursive makefiles. - main issue regarding ctools.)
There's no great way to pick for certain what version of a given module the site ends up with: #1016924-10: Dealing with different versions of the same module in recursive makefiles is working on it, but as Linus Torvalds says in the context of automated merging algorithms, maybe it's best if there remains no algorithm, and the user has to make the decision!

Even if these specific problems were somehow resolved, there's clearly still an identifiable user need for preventing makefile recursion: while the default behaviour should clearly be as is currently the case, giving the drush user the option to prevent recursion will really help them make those decisions: after all, drush make offers the option to download without core; it doesn't merely assume that, as it's a straightforward job to pull the modules out of a Drupal core, that option isn't necessary.

Proposed resolution

Two parts:

Original patch from @joelcollinsdc on #1621030-5: Regression for recursive makefiles. to permit a projects[projectname][do_recursion] = 0 option which disables recursion for that one project
Rolled in extra code to add a command-line --no-recursion option to disable recursion across all projects

The first option permit quite granular control, if you personally are able to control the "master" makefile; otherwise, or just if the user prefers, the second option permits switching off recursion across all projects. In addition, a 'no-recursion' option sits very well alongside no-cache, no-clean, no-core etc. so there's precedent for that.

Patch to follow in first comment, so that its naming convention will be correct.

Remaining tasks

Adding to the recursion unit tests (hopefully to follow). Brief explanation of the --no-recursion parameter (and the [do_recursion] option) are included in the initial patch.

User interface changes

The --no-recursion option has been added to the drush make command. Also, any users maintaining a makefile have the extra makefile option of adding projects[projectname][do_recursion] = 0 to any projects they don't want to have made recursively by drush make.

API changes

See above. All API changes are backwards-compatible: the behaviour of existing makefiles, with existing command-line options, should be unchanged.

Also linked inline above:

Comment	File	Size	Author
#7	drush-prevent-recursion-1989174-7.patch	2.78 KB	jp.stacey
#3	drush-prevent-recursion-1989174-3.patch	2.78 KB	jp.stacey
#2	drush-disable-recursion-1989174-2.errors.txt	2.69 KB	jp.stacey
#1	drush-prevent-recursion-1989174-1.patch	1.93 KB	jp.stacey

Comments

Comment #1

jp.stacey commented 7 May 2013 at 15:55

Status:

Active

» Needs work

Status	File	Size
new	drush-prevent-recursion-1989174-1.patch	1.93 KB

Patch attached without tests. It does the following:

includes @joelcollinsdc's patch
changes object attribute name to $this->do_recursion to avoid possible confusion with $this->recurse() method
Also looks at the --no-recursion command-line option
Adds brief documentation

Looking at the tests now.

Comment #2

jp.stacey commented 7 May 2013 at 16:03

Status	File	Size
new	drush-disable-recursion-1989174-2.errors.txt	2.69 KB

Unfortunately the existing testMakeRecursion test is broken in 7.x-5.x: see attached. So it's hard at the moment to write a test. I'll see if I can patch against 7.x-5.9 and write a test on that basis for now.

Comment #3

jp.stacey commented 7 May 2013 at 16:23

Status:

Needs work

» Needs review

Status	File	Size
new	drush-prevent-recursion-1989174-3.patch	2.78 KB

OK, this patch contains both code changes and tests. It was written against a fork off 7.x-5.9, as the tests won't currently succeed on 7.x-5.x . But it should also successfully patch 7.x-5.x when the tests there start working, as the hunk offsets aren't much different.

Comment #4

jhedstrom

English

Portland, OR

commented 7 May 2013 at 16:31

re #2 tests in the 5.x branch should now work (there was an issue with the commit from #1454534: Invalid argument supplied for foreach() wget.inc:42 fixed last week that I didn't realize needed a port to 5.x.

Comment #5

jp.stacey commented 7 May 2013 at 16:39

@jhedstrom nice, thanks. I can confirm that the patch works against 5.x and that the two unit tests work too. I'd appreciate some more eyes on this!

Comment #6

jhedstrom

English

Portland, OR

commented 9 May 2013 at 00:35

Status:

Needs review

» Needs work

+++ commands/make/make.project.incundefined
@@ -455,6 +460,11 @@ class DrushMakeProject {
+    if (!$this->do_recursion || drush_get_option('no-recursion')) {
+      drush_log(dt("Preventing recursive makefile parsing for !project",
+                array("!project" => $this->name)), 'notice');
+      return FALSE;

I don't think this should return FALSE, since that will cause the code in the make() method to also return FALSE, which may have unintended consequences. It should either return NULL (which would require the code in the make() method be changed to account for that), or return TRUE

Aside from that, tests are passing, and it looks good to me.

Comment #7

jp.stacey commented 10 May 2013 at 13:19

Status:

Needs work

» Needs review

Status	File	Size
new	drush-prevent-recursion-1989174-7.patch	2.78 KB

On reflection I agree. Given the user or their makefile has stated what should happen, then there are no unexpected circumstances occurring in the function that we need to raise a warning about (like e.g. the makefile not validating, which does return FALSE.)

Also, all other successful completions of the function return TRUE. As there's no situation in which the current code ever returns NULL, I think TRUE is best in this case too.

Patch re-rolled and attached.