Drush make allows for includes[] syntax.

Lets add this feature to drupalorg_drush.


dww’s picture

We'd have to really flesh out the implications for this. I don't know how this feature is implemented inside drush make. If it really just slurps in the bytes from some other source, stuffs that into the main .make file, and all the other validation stuff continues to happen, I don't see the problem. However, if it's invoking drush make separately on the other file or something, we'd have to make sure all the right arguments are getting propagated, etc.

But, if it's just a way to put more junk into your .make file before validation and execution, I think this should be a very trivial change that we can move forward with.

webchick’s picture

Priority:Normal» Major
Issue tags:+drupal.org distribution blockers

A real-world example of a distribution that uses this is http://drupal.org/project/openoutreach (currently the #2 most popular profile, so this seems like a reasonable thing to support):

Here's what it's doing:

includes[debut] = "http://drupalcode.org/project/debut.git/blob_plain/HEAD:/debut.make.inc"
includes[debut_article] = "http://drupalcode.org/project/debut_article.git/blob_plain/HEAD:/debut_article.make.inc"
includes[debut_blog] = "http://drupalcode.org/project/debut_blog.git/blob_plain/HEAD:/debut_blog.make.inc"
includes[debut_comment] = "http://drupalcode.org/project/debut_comment.git/blob_plain/HEAD:/debut_comment.make.inc"
includes[debut_event] = "http://drupalcode.org/project/debut_event.git/blob_plain/HEAD:/debut_event.make.inc"
includes[debut_media] = "http://drupalcode.org/project/debut_media.git/blob_plain/HEAD:/debut_media.make.inc"
includes[debut_section] = "http://drupalcode.org/project/debut_section.git/blob_plain/HEAD:/debut_section.make.inc"
includes[debut_social] = "http://drupalcode.org/project/debut_social.git/blob_plain/HEAD:/debut_social.make.inc"
includes[debut_wysiwyg] = "http://drupalcode.org/project/debut_wysiwyg.git/blob_plain/HEAD:/debut_wysiwyg.make.inc"

So it's all kosher; stuff hosted on Drupal[code].org.

Then an example of one of those .inc files (http://drupalcode.org/project/debut.git/blob_plain/HEAD:/debut.make.inc) would be:

; Drupal version
core = 7.x
api = 2

; Contrib modules
projects[debut][subdir] = contrib
projects[debut][version] = 1.0-beta2
projects[features][subdir] = contrib
projects[features][version] = 1.0-beta2
projects[features][patch][http://drupal.org/files/issues/features_views_fix-drush-make-1097560-53.patch] = http://drupal.org/files/issues/features_views_fix-drush-make-1097560-53.patch
dww’s picture

Title:Support drush make incldues[]» Support drush make includes[]

Reading #820992: Includes syntax for makefiles it sure seems like it's just a textual include before validation. It'd be nice to verify this before we start allowing this. Would probably be pretty easy to just add another drupalorg_drush test that includes existing test .make files that we expect to fail and make sure they still fail. Tests we expect to fail are quick and cheap since they just hit the validation phase and fail, instead of incurring the cost of actually downloading and assembling code. And the main thing we care about is that stuff included via these directives is still validated and we prevent badness from sneaking in via includes[].

dww’s picture

Note, now that these two are fixed and deployed:

#1433784: Fix how drupalorg_drush handles release history XML and propagates data to the packaging script
#1472744: Change packaging script to use the new JSON metadata from drush make for package contents

I believe this will be trivially easy to fix, since we no longer have to worry about the packaging script itself also honoring includes[]. Unless there's some other gotcha I'm not aware of, this should be a 12 byte change to drupalorg_drush to add 'includes' to the whitelist for top-level make attributes.

I just want the dust to settle on #1469714: Fix validation to support drupal-org-core.make files and prevent defining the drupal project in drupal-org.make itself and #1455614: Packaging script doesn't allow distributions to patch core or specify a git revision first...

dww’s picture

Status:Active» Needs work
Issue tags:+Needs tests
new760 bytes

This needs tests, and a more thorough review to make sure this isn't going to break anything. But, functionally, this should now be all we need for this to work. ;)

nedjo’s picture

Title:Support drush make includes[]» Support drush make includes[] in drupal-org.make files
Status:Needs work» Needs review
new3.26 KB

Patch with tests included. Two tests:

  • do-verify-makefile-fail-include: ensure including an invalid make file fails.
  • do-verify-makefile-succeed-include: ensure including a valid make file succeeds.

Not sure if more testing or analysis is needed?

I'm working this week on #1487926: Switch to fully packaging Open Outreach on drupal.org, which, it turns out, depends on a fix to this issue. I was impressed but not surprised to see webchick as usual a step ahead, identifying and helping address issues with my install profile before I was even aware of them myself. Thx webchick!

sylus’s picture

Hitting up against this issue as well. For the distribution I work with the build uses includes to reference a specific make file in each of the modules that reside in the same repo.

For instance:

includes[] = modules/custom/wetkit_wysiwyg/wetkit_wysiwyg.make
includes[] = modules/custom/wetkit_search/wetkit_search.make
includes[] = modules/custom/wetkit_language/wetkit_language.make
includes[] = modules/custom/wetkit_migrate/wetkit_migrate.make
includes[] = modules/custom/wetkit_bean/wetkit_bean.make
includes[] = modules/custom/wetkit_metatag/wetkit_metatag.make
includes[] = modules/custom/wetkit_wetboew/wetkit_wetboew.make
includes[] = modules/custom/wetkit_breadcrumbs/wetkit_breadcrumbs.make

This setup is beneficial as I can specifically place modules + patches + themes in their logical area and minimize a confusing and sometimes large drupal-org.make file. I really like when a module is self contained and has all the requirements (modules) that is needed for it to run. I guess I could just make a separate project for each of these modules and then use the projects[] spec but is less then ideal.

Hope we can get this in! ^_^

mgifford’s picture

@nedjo's patch from May of last year still applies fine. Some whitespace errors:

$ git apply drupalorg_make-includes-1427752-6.patch
drupalorg_make-includes-1427752-6.patch:62: new blank line at EOF.
drupalorg_make-includes-1427752-6.patch:72: new blank line at EOF.
drupalorg_make-includes-1427752-6.patch:83: new blank line at EOF.
drupalorg_make-includes-1427752-6.patch:91: new blank line at EOF.
warning: 4 lines add whitespace errors.

What else needs to be done to mark this RTBC, or more importantly to bring this onto Drupal.org. I think a lot of distros will benefit from it.

sylus’s picture

Seconding @mgiffords post, what else does need to be done?

jhedstrom’s picture

Status:Needs review» Reviewed & tested by the community

I committed #6. There are currently 5 failing tests, but they are unrelated to this patch. Leaving at RTBC for somebody to deploy to d.o.

dww’s picture

Status:Reviewed & tested by the community» Fixed
Issue tags:+needs drupal.org deployment

@jhedstrom: If I were to deploy this, would you be available to help deal with anything that might go wrong? :) I have my hands full with other things right now, so I can't really test/baby-sit this in case of trouble. It's probably fine, but I wanted to check if you've got bandwidth to help deal with a problem if it arises.

Also, this is the workflow we usually use for deploying to d.o -- once it's committed "upstream", the issue is fixed, but we tag it for deployment.


jhedstrom’s picture

@dww my bandwidth is good enough to cover this should something go wrong.

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

sylus’s picture

Any status on getting this deployed?

mgifford’s picture

Status:Closed (fixed)» Active

I'm setting this back to active for now until we know it's deployed.

beeradb’s picture

Status:Active» Fixed

Based on dww's comments in #11 I believe the correct status for this issue is fixed.

dww’s picture

I nearly missed that this now has tests. Thanks for adding those! Therefore, removing the "Needs tests" tag. ;)

See #2012054: PHPunit tests are currently failing in master about the current test failures in master. I'd really love to resolve that before deploying this or pushing any other commits.

Sorry I've been neglecting this queue!


dww’s picture

Status:Postponed» Fixed
dww’s picture

This is now deployed.

Automatically closed -- issue fixed for 2 weeks with no activity.