introduce an options array in the root level of the makefile

This feature is required to allow the drupalorg_testing project's makefile to work again. See #1227910: Fix apachesolr.make for recursive use and #1227940: Module dependency fixes to drupalorg_testing.make. It would be great to have a version of make with this feature committed.

@greg: i'm a little concerned that module maintainers can add code to their make files that behave like command line switches to a packaging program. have the security implications of this been thought through? this is especially important as drush make is used on drupal.org.

@greg - drupal.org can strip parts of the make file for security. If they strip these options when they process makefiles (for downloading of full install profiles), will this still be a useful feature for folks who download the makefile only?

restoring status from cross post

There was an interesting post by Adrian a while back that explained that modules should not include makefiles named MODULE.make, because it is difficult to control the behavior adequately when the makefile is included recursively. I wish I could find a reference, because what we are doing right now is working through some of the limitations he pointed out. Google failed me.

Anyway, in the case of apachesolr, the provided makefile does not work unless you run it with --no-core, and there is no good way for a master makefile to include a sub-makefile in such a way as to make this possible.

I am reducing the priority of this from "critical" to "normal" because it seems that the lack of --no-core is only an error to drush_make-3.x; drush_make-2.x will let this go through, and not get in the way of installing drupalorg_testing.

As far as specific security vulnerabilities are concerned, it is my opinion that stripping options (in d.o.) is too fragile, and restricting the ability of make files to specify options is too restrictive. I agree that we would not want to make it possible for a sandbox install profile project to be able to write a make file that would cause drush make to install the sandbox code somewhere that d.o would run it. However, there is a demand for the ability to set options from drush_make, and these features are coming (c.f. #958844: PATCH: Allow [working-copy] in makefile as well as on commandline). The best way to address this, I think, would be to add a drush make cli option that enumerates the options that should be allowed to be set by makefile directives. This option could default to "all", and if "no-core" is all that d.o. needs to build correctly, then that would be the only one that it would need to enable.

Here is a more secure version of the same patch. This now allows the caller to specify --allow-overrides=a,b, where a and b are flags that can be overridden in makefiles. If --allow-overrides is not specified, then any option can be overridden. (I considered adding special checking for --allow-overrides=none, but ended up not putting it in. Since 'none' is not a drush_make flag, that idiom can be used to much the same effect without explicit support.)

Also, the original patch used drush_set_option to apply values set in a makefile; this could cause environment pollution, as an option set in one makefile would then also apply to all successive makefiles built after it. This patch now applies overridden options to drush's "custom" options context, which it saves and restores around each recursive invocation of drush_make to avoid this problem.

Wrong patch in #7. It works, but this one also supports projects[projectname][download][options][working-copy] = TRUE in addition to options[working-copy] = TRUE. Both respect the --allow-overrides option.

Argh, #8 is also the wrong patch! The feature you couldn't get to work isn't there. As to the patch being against 2.x, all I can say is $*@(#!. I was in too much of a hurry yesterday. I'll take a deep breath next week, re-roll the right code against 3.x, and go from there. It will probably work better once you have all of the code. ;) Sorry for the inconvenience.

[TMI: Actually, #8 was closer to what I intended than I thought; I was missing a minor edit or two from it. The odd thing is that git was reporting that I was on the branch 6.x-3.x, but when I did a reset --hard HEAD followed by a pull, it tried to merge 3.x changes into 2.x and in short, don't know what happened, but I killed the whole project and pulled it down again. :p]

I had a typo in my code comment that I copied into my description of #8. It should be projects[projectname][options][working-copy] = TRUE (take out the spurious '[download]' and it works).

The allow-override thing was a bug, fixed in this patch. I went back to special-casing 'all' and 'none' for clarity. The flag is now named --allow-override, like Apache, as intended.

This patch is also against 2.x, except this time on purpose. When I went back and re-rolled against drush make 3.x, I found that it was not working with drupalorg_testing today. I'm not going to change the version on this issue because this should be patched against 3.x, but for the time being, please enjoy the attached 2.x patch.

Sorry, I was actually testing no-core in #11, and assumed other options would work the same way. As you mention, some options might be applied before they are adjusted. :( This may take a little re-factoring; I'll look at it.

If [working copy] for instance is not working, are there going to be other problems with the approach in this thread?

I'd be happy to do a simple update of the working copy patch, but if we can find a better place for all of the code above that will always be called to populate the options array, that would be the best outcome.

I agree with #15 in concept; if this can be fixed in a generic way, that would be best. If it is just a matter of changing the way initialization is done for 'working-copy', though, then I think #14 is a sound approach.

I haven't had time to investigate, though; I need projects[projectname][options][working-copy] = TRUE to work soon for one of my projects, so I will be getting to this as soon as I can. I'm fine with doing the [working-copy] fix here or in a separate issue, as desired by the drush_make maintainers.

I was not able to understand nor rebase the patch against current 6.x-3.x code (the $context part of drush_make.project.inc has changed a lot)

Would anybody be interested in rolling this against Drush itself (in the 'make' branch), now that there's an initial port of Drush Make to Drush Core?

Also wondering if this patch would be able to set defaults, so, for example, one could specify "contrib" once for the sub directory, instead of project[foo][subdir] = "contrib" for each project.

#1310130: Put drush make in drush core

Cool. Moving to drush queue.

yeah ! I basically tested it and it works quite well.
I tried the sample makefile below (given you cloned --bare anything into /tmp/orig.git), with:

$ drush make --no-core test.make /tmp/test

core = 7.x
api = 2

options[working-copy] = TRUE

projects[drupal][type] = "core"
projects[drupal][download][type] = "git"

projects[drushtest][type] = "module"
projects[drushtest][download][type] = "git"
projects[drushtest][download][url]  = "/tmp/orig.git"

Documentation needs an update but there is also a "bug":
I added projects[drushtest][options][working-copy] = FALSE in an attempt to override the main value of TRUE but this didn't worked.
I also added options[allow-override] = "all" in the top part of the makefile but it didn't work either.

After a deeper look at the patch I noticed that both drush_set_option() and the preceding
array_merge() lines handle cases about submakefiles, but nothing seems to take into account the options of a subproject from the same Makefile.
So I was not able to put a default value (say [working-copy] = TRUE, then override it on a per-project basis, say projects[drushtest][options][working-copy] = FALSE)

did I missed a point ?

Setting working-copy to FALSE ideally would work; it probably isn't working due to a bug. As for setting allow overrides in a drushrc.php file, did you try $options['allow-override'] = 'all';?

It would be nice to incorporate the stub makefile from #22 (or something similar) into a test in tests/makeTest.php before we put this patch in. That would probably also help track down the potential bug in the working-copy option.

simply rerolled (does not address the above issues)
[ still have to test an override from drushrc.php but even if it works, it *should* work from the makefile itself isn't ? ]

Any test that is based on an expected md5 won't work with working-copy, since the .git directory will change if the remote repository has new commits (alternatively, I know dww has created a project on d.o. that intends to remain static, I just can't remember the project name offhand). Check out testInfoFileWritingGit() in makeTest.php for an example of more targeted tests that don't depend on an md5 hash.

http://drupal.org/project/update_test_module

However, I can't promise there will never be changes to the Git repo...

Just headed on over from issue #1454480.

I've taken a look at the current patch, and it doesn't seem to do anything with respect to per-project options? (Although I'm not too familiar with the drush codebase). Specifically, all the voodoo is happening in make_parse_info_file() which only gets called once, and only deals with "global" stuff. I'm happy to update my patch from #1454480 to match the [options][working-copy] format discussed here, and also to take into account the --allow-overrides stuff if that seems like a sensible approach?

Revised patch attached. This "worksforme", but not sure if it's the right approach?

1. The additional patch files added to tests/makefiles in my patches before didn't seem to be included here.

Good catch, will be included in future ones.

2. There was a whitespace warning on line 11

Fixed - although I'm curious - how did you catch this? [New-ish to drupal!]

3. When attempting to run the tests/makefiles/options-project.make it didn't seem to make context_admin a working copy.

I'll have a look at the problem you quoted, but as a note - I was unable to run the tests without adding to the array in makeTest.php as follows - is that right - or am I missing something?

--- a/tests/makeTest.php
+++ b/tests/makeTest.php
@@ -274,6 +314,18 @@ class makeMakefileCase extends Drush_CommandTestCase {
         // @todo This test often fails with concurrency set to more than one.
         'options'  => array('no-core' => NULL, 'concurrency' => 1),
       ),
+      'options-array' => array(
+        'name'     => 'Test global options array',
+        'makefile' => 'options-array.make',
+        'build'    => TRUE,
+        'options'  => array(),
+      ),
+      'options-project' => array(
+        'name'     => 'Test per-project options array',
+        'makefile' => 'options-project.make',
+        'build'    => TRUE,
+        'options'  => array(),
+      ),
     );
     return $tests[$key];
   }

If that's OK - I'll include it with my next patch.

This works for me with a real-life test scenario - thanks!

Only comment - should the options flags end up in $download['options'], rather than in $download direct, e.g.

$download['options']['working-copy']
rather than
$download['working-copy']

Not strictly necessary now, but may be better if other options come along?

Happy to update the patch if anyone thinks this is required - otherwise happy with the current patch.

Tested the makefile below (and the opposite boolean condition about working-copy), and it now works perfectly.

core = 7.x
api = 2

options[working-copy] = FALSE
options[allow-override] = "all"

projects[drupal][type] = "core"
projects[drupal][download][type] = "git"

projects[drushtest][type] = "module"
projects[drushtest][download][type] = "git"
projects[drushtest][download][url]  = "/tmp/orig.git"
projects[drushtest][download][working-copy] = TRUE

I also noticed that the --working-copy switch kindly overrides the the main section value, but the per-project values are still considered, that sounds good.
The only things I can neatpeak about are :
* the handling of the "allow-override" value. But it appears to be undocumented anyway.
* a bit about the per-project availability of working-copy in README.txt

thanks a lot !

I agree with #38 in that we need to document this new functionality in the commands/make/README.txt file.

I'm undecided about #37 with regards to moving working-copy into an options array under download. If new options do come along, we could always re-evaluate at that time.

The only reason we'd want downloads[options][foo] is if we think we're going to run into namespace conflicts with the things already in downloads[]. I don't see that happening, so I don't think we need the additional level of nesting here. And yeah, we can always revisit later if we're wrong. Seems like more of a WTF for now -- why is working-copy different from the other Git info? Internally in drush make, one is considered an "option" while the other is considered "input", but that distinction doesn't really make sense to end users. To them, it's all just info telling drush make how they want their Git clone/checkout to happen...

Thanks,
-Derek

Maybe we should use drush_get_option_list() in _make_is_override_allowed(). This would make testing for 'none' and 'all' a little less convenient, but it would allow --allow-override=a,b on the cli, which might be helpful for build scripts at some point.

Also, patch no longer applies; make_parse_info_file() moved to drush_drupal_parse_info_file() in drush.inc.

The attached includes Joe's re-roll, plus documentation of the working-copy flag in commands/make/README.txt.

I think that the docs need to warn about allow-overrides; otherwise, folks will be frustrated. It would also help reduce frustration if _make_is_override_allowed would warn whenever it returned false. 'working-copy' not allowed; use --allow-override=working-copy or --allow-override=all to permit, or something like that.

This patch with tests is looking good. It will need to be re-rolled though as this no longer applies (mainly because the README and EXAMPLE files have moved to the docs directory, but seeing other failures to apply as well).

Here's a reroll of #45, leaving at needs work as it currently has one of the new tests failing.

using options[allow-override] = "all" and makefile derivated from comment 22 :

projects[drushtest][options][working-copy] (only) : ok

options[working-copy] = FALSE
projects[drushtest][options][working-copy] = TRUE

: ok (.git preserved)

options[working-copy] = TRUE
projects[drushtest][options][working-copy] = FALSE

: not ok (existing .git)

[edit]using projects[drushtest][download][options][working-copy] instead of projects[drushtest][options][working-copy] works perfectly.

Should be noted that :
drush make --no-core --working-copy will makes use of working-copy for every projects even if they had specified projects[drushtest][download][options][working-copy] = FALSE, but it seems sane to me.

thank you !

Tests are passing now (although I'm curious what the problem was since tests shouldn't be dependent on one another).

I wonder if the global options array could use the new global defaults array (#1633050: Create defaults array in .make files for specifying attributes common to all projects) that was just committed? The only problem I can see with that is in the case of recursion.

I spent a bit of time trying to use the new defaults array, but couldn't figure an easy way to make that work on recursion, so I'm inclined to commit #49, then perhaps refactor to use the defaults array once that is working with recursive builds.

I think it is useful to expose --allow-overrides as a command option. Also mentioned by @greg.1.anderson in #41

Code review:

+++ b/commands/make/make.download.incundefined
@@ -471,10 +476,15 @@ function make_download_git($name, $download, $download_location) {
+  if ( _make_is_override_allowed('working-copy') && isset ( $download['working-copy'] ) ) {
+    $wc = $download['working-copy'];
+  } else {
+    $wc = drush_get_option('working-copy');

This snippet is reapeated for each vcs. It should be refactored out to other function.

Also } else { doesn't follow coding standards.

+++ b/commands/make/make.project.incundefined
@@ -443,15 +446,27 @@ class DrushMakeProject {
+      make_projects(TRUE, trim($build_path, '/'), $info, $this->build_path);
+      make_libraries(trim($build_path, '/'), $info, $this->build_path);

Use drush_trim_path() instead.

+++ b/commands/make/make.utilities.incundefined
@@ -11,13 +11,26 @@
+  $makefile_options = array_merge($makefile_options, empty($info['options']) ? array() : $info['options']);

This looks better to me:

if (!empty($info['options')) {
  $makefile_options = array_merge($makefile_options, $info['options']);
}

+++ b/commands/make/make.utilities.incundefined
@@ -492,3 +505,20 @@ if (!function_exists('sys_get_temp_dir')) {
+    $allow_override = explode(',', $allow_override);

We have a function for that: http://api.drush.org/api/drush/includes%21drush.inc/function/_convert_cs...

+++ b/tests/makefiles/options-project.makeundefined
@@ -0,0 +1,19 @@
+projects[caption_filter][download][revision] = "c9794cf"
+projects[caption_filter][download][working-copy] = TRUE

I'd prefer not to support [download][working-copy] if we also support [options][working-copy]. I think the latter is better (which probably means just going with the patch in #58).

There are quite a few coding standard issues in that patch though, which I'd like to see cleaned up prior to committing.

I've committed #62 to 6.x in 4ea7b82. Thanks all!

For those looking for more details on the use of the working-copy option (for preserving the Git repository), please check out the answers on this Drupal Answers question: Drush Make removes .git folders.