Posted by langworthy on February 3, 2012 at 10:19pm
14 followers
| Project: | Drupal.org drush |
| Component: | Code |
| Category: | feature request |
| Priority: | major |
| Assigned: | Unassigned |
| Status: | active |
| Issue tags: | drupal.org distribution blockers, needs drupal.org deployment, Needs tests |
Issue Summary
Drush make allows for includes[] syntax.
Lets add this feature to drupalorg_drush.
Comments
#1
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.
#2
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
#3
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[].
#4
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...
#5
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. ;)
#6
Patch with tests included. Two tests:
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!
#7
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! ^_^
#8
@nedjo's patch from May of last year still applies fine. Some whitespace errors:
$ git apply drupalorg_make-includes-1427752-6.patchdrupalorg_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.
#9
Seconding @mgiffords post, what else does need to be done?
#10
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.
#11
@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.
Thanks!
-Derek
#12
@dww my bandwidth is good enough to cover this should something go wrong.
#13
Automatically closed -- issue fixed for 2 weeks with no activity.
#14
Any status on getting this deployed?
#15
I'm setting this back to active for now until we know it's deployed.