I discovered that both openpublic 7.x-1.0-alpha5 and openpublic 7.x-1.0-beta5 are trying to patch core. Currently, the release nodes even say it happened. However, if you download the tarballs and inspect them, the patches were never applied.

There's a very long story here, which I'll attempt to briefly summarize.

When the packaging script is building fully-packaged distro releases, it needs to do 3 distinct steps:

  1. Check the install profile directory out of Git from the appropriate tag and create a tarball of just that
  2. Build the drupal-org.make file (if it exists) in --no-core mode to get the profile and all dependencies without core
  3. Create a separate .make file to *just* build core and then move the directory tree from step #2 into place under the profiles directory

There can't possibly be a single .make file that handles step 3 for us, since there's no way to get .make to put the profile into place without the profile release already existing. And of course, we're in the middle of creating that release at this point. Chicken, meet egg. There's a secondary problem that once you have drush make building recursively, if you had a single .make file that defines core, the profile, and all the dependencies, there's no guarantee that the profile will get built first so that all the dependencies can install themselves as subdirs of the profile.

SO, we definitely need a separate .make file to build core, then to manually move the profile and all its dependencies into place to be able to package the full distro.

Currently, the packaging script is just spitting out a trivial little core-only .make file based on the core attribute in your drupal-org.make file and building that. Of course, that just silently ignores any special sauce distro maintainers put in drupal-org.make for the drupal project (checking out from a specific git revision, patching core, etc).

After extensive research and discussion with hunmonk, we came up with 2 possible solutions to this that will actually work. I'll list the unique pros to each approach. Also, since it's nice to have a .make file that can be built by developers or people deploying directly, I'll explain how you'd do that in each of the two approaches for comparison.

A) Add optional support for a separate drupal-org-core.make file if you need something more fancy than "core = 6.23" in your .make file.

The 'drupal' project would be disallowed in the main drupal-org.make file itself. So, if this file exists, the packaging script would build that when it needs to build core for your distro. Otherwise, it falls back to what it's doing right now.

Makefiles used

  1. drupal-org.make (all projects, libraries, patches, except core)
  2. drupal-org-core.make (just core, if you need something fancy)
  3. build-[profilename].make (bootstrap for local development builds) Something like this:
    api = 2
    core = 6.x
    includes[] = drupal-org-core.make
    projects[openpublish] = 1.0-beta5
     

    This includes the core specification directly, then points to a release (or git tag) of the profile which then builds the drupal-org.make file recursively.

Pros

  1. Requires no changes to drush itself -- could be fixed and deployed without intervention by drush maintainers.
  2. More simple and less duplication to get the build-[profilename].make file working properly
  3. Might be easier to not have core inside drupal-org.make for other reasons (febbraro had a gut feeling about this, but needed to clarify)

Cons

  1. Requires more documentation for distro maintainers.
  2. Possible gotcha that you have to define special sauce for core in a separate file.
  3. Will further complicate the drupalorg_drush plugin since the validation stuff is going to be more complex. We'll need to prevent 'drupal' project from being defined in drupal-org.make but *only* let you define that project (and its patches, etc) in drupal-org-core.make.

B) Keep everything, both core and all the profile dependencies, in drupal-org.make.

For this to work, we'd need to get #1452564: Allow making of specific projects/libraries committed to drush ASAP, and have the packaging script use that to build core directly out of your drupal-org.make file instead of creating a separate core-only .make file.

Makefiles used

  1. drupal-org.make (all projects, libraries, patches, and core)
  2. [profilename].make (everything from drupal-org.make *except* core)
  3. build-[profilename].make (replicate all the core stuff from drupal-org.make, then include a pointer to the profile to build that recursively, which will use [profilename].make)

Note that this requires a lot of duplication in the .make files (core lives in both drupal-org.make and build-[profilename].make, all the projects/libraries/patches have to be replicated between drupal-org.make and [profilename].make) although part of that could be mitigated if #1427752: Support drush make includes[] in drupal-org.make files was done.

Pros

  1. Easier to validate since everything is in a single .make file.
  2. Only one file to update when making a new release, instead of having to remember to update the core file, too.
  3. No changes needed to the drupalorg_drush plugin to change the validation logic.

Cons

  1. Requires a new feature in drush before we can deploy this and make it work.
  2. Complicates things for the "build" makefile to work properly (see above).

Now what?

After a long IRC chat with with febbraro, ezra-g and webchick we decided option A is better. However, that was before I realized the pros/cons associated with changing the validation logic in the drupalorg_drush plugin. ;) So, maybe B is better after all. Luckily, everyone agreed these were roughly equivalent solutions, and no one felt super strongly about either one.

So, if anyone has other thoughts, different assessment of the pros/cons, etc, please let me know ASAP.

Thanks!
-Derek

CommentFileSizeAuthor
#17 1455614-17.drupal-org-core.patch19.12 KBdww
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

febbraro’s picture

@dww: I confirmed that the recursive building will catch drupal-org.make if [profile].make does not exist.

In general I think Option A provides more of a DRY approach to distro building and still is my preferred approach. I have feelers out to the other folks on my team that do a lot of the distro development/releases to see if they have anything that would make A not our preferred option. Hope to see them comment here if there are any issues.

dww’s picture

If drupalorg_drush supported includes[] would that change your mind? As I weigh the work required for both options, getting includes[] working for B is probably about the same as all the validation changes needed for A, and folks want includes[] working for other things, too. In that case the only duplication would be the core definition (which would have to live in both drupal-org.make and the build.make. Or, you could put it in a separate openpublish-core.make and include it in both places... Just a thought.

tekante’s picture

Derek,

I have a question regarding your #3 con on option A as I'm not sure I understand the restriction. Here is how I currently typically work when I'm working on a distro:

Clone the distro repo, run drush make distro.make and then symlink the repo I already had checked out into profiles/distro. This enables me to test updates to the distro.make file and without needing to do an intermediate commit and rebuild at the expense of my contrib items being "incorrectly" located under sites/all/modules instead of profiles/distro/modules (and likewise for themes). In practice, this has never proven to be a problem for me.

I could easily switch to a drush make drupal-org.make strategy and keep this workflow but only if I can define core (which I'd prefer to do via an includes[] call) in this file. Your con #3 seems to indicate this won't be allowed but your comment #2 seems to indicate it might be. Not being intimately familiar with the build code I don't understand this potential restriction since I would think the 2nd build step you mention (build drupal-org.make with --no-core would mean it wouldn't matter).

For me, I think I would prefer the following 3 make files to exist

build-profile.make - set such that when a repo tag is checked out drush make build-profile.make would build the exact full build one would download from drupal.org. There is some circular dependency there (the build-profile.make file has to be committed to reference a tag that won't exist until that commit is tagged but it's not a big thing to handle manually). This one is only necessary to get things to land in the right directory locations and if there was a way to get rid of it completely I'd support that. This would use includes[] to include the core make file.

drupal-org.make - takes over the functions of the profile.make file, uses includes[] to include the core make file.

drupal-org-core.make - defines the drupal project core

So my questions are:
Can I define core in drupal-org.make?
Can I do so using includes[]?

tekante’s picture

Noticed the pointer to #1427752: Support drush make includes[] in drupal-org.make files so I think my questions now are "Can I define core in drupal-org.make" along with a vote for being able to do so via includes[].

febbraro’s picture

@tekante

I believe currently only drush on drupal.org itself does not support includes[] but if you are running drush make locally the includes[] in drupal-org.make would be supported.

@dww If you update drupalorg_drush to support includes in drupal-org.make files, would an included core literally fail validation, or just be ignored a la --no-core?

dww’s picture

@tekante: Sorry, I don't really understand what you're proposing. The two options could perhaps be more clearly stated as:

A) Define core in a separate file (drupal-org-core.make) if you need something more fancy than an official release of core.

vs.

B) Make it work to define the core project inside drupal-org.make.

You appear to be asking for both, which seems like the worst of both worlds. ;) If we're going to start supporting drupal-org-core.make files to define core, we want to ensure you do *not* define core in the drupal-org.make file itself, via includes[] or otherwise.

Whether or not we support includes[] is a more-or-less totally separate issue being addressed over at #1427752: Support drush make includes[] in drupal-org.make files -- however, if it turns out that supporting includes[] when running drush make on drupal.org was the easiest way to arrive at a sane solution to this bug, I'm prepared to bump that up in priority and make it work.

---

@febbraro: How the validation should work is still a bit TBD based on what we're doing here. However, from my POV, includes[] won't impact how validation works at all. AFAICT, it's a simple text replacement, as if you just cat'ed the files together before running drush make. I don't think we want to (and given how includes[] is implemented, I don't even think we can) have different validation rules for things defined in the main .make file vs. things pulled in via includes. So, I believe that regardless of includes[], we just need to decide how the validation should work.

Personally, I think it's better to have things fail validation than to be silently ignored. Given this bug and that it doesn't actually work as expected, I think it's a problem that we lifted the validation restrictions on defining the drupal project. If it's going to be silently ignored, I think it's safer to fail validation than to "pass" and then not do what you expect. But, I'm open to other perspectives. Why do you ask? Would silent ignore impact your opinion on how this should work? If so, please explain.

Thanks!
-Derek

tekante’s picture

I wasn't intending my post to read as a proposal. I don't know enough about the distro packaging process to understand the need for the core restriction and its presence is the only part of option A that I dislike. The rest of my post was information to explain why that restriction is problematic to me.

To your point in comment #2 about whether the perspective changes if includes[] is supported: at that point I believe option A and B are fairly equivalent to me as a developer as I would have to modify declarations in only one place in either case. Since I foresee option B using four make files to prevent declaration duplication rather than the three of option A I would give option A the slight edge purely from a distro developer's point of view.

Here is what I see as the option B make file setup I would use if includes[] are enabled to prevent declaration duplication

drupal-org.make - includes [profilename]-core.make and [profilename].make
[profilename]-core.make - core declarations
[profilename].make - everything but core
build-[profilename].make - includes profile-core.make and pointer to profile

None of the previous info takes into account the work to be done either to Drush or to the drupalorg_drush plugin. Without includes[] it seems like option A is the better choice. With includes[] it seems like option B would be better based on a naive reading of the impacts to the drupalorg_drush plugin that come with option A.

ergonlogic’s picture

My vote goes to (a), convention over configuration. Since it would require devs *not* to include core in their makefiles, it becomes much easier to re-use them. This is what we do in a couple sets of makefiles used in production on Aegir servers: http://drupal.org/project/kplatforms and http://drupal.org/project/openatria_makefiles. A more exotic example would be building a platform with multiple install profiles.

Except that, in (a), isn't drupal-org.make essentially the same thing as [profilename].make in (b)? If so, is there any way to use the latter naming convention? It would allow the makefile from the example above to be much more readable.

dww’s picture

Yes, A's drupal-org.make is basically exactly what profilename.make looks like in B. It's everything you need for the profile without core and without the profile itself.

However, no, I don't think we can get into the situation where the d.o packaging script is looking for profilename.make and using that to decide if it should attempt a fully-packaged distribution release or not. There might still be install profiles that can't be fully packaged by drupal.org, although they're going to still want to provide their own .make file. So, we really need full distribution packaging on d.o to be opt-in, and the way to do that is with this separately-named .make file.

Also, if we stick with the drupal-org.make and drupal-org-core.make naming convention, then drush verify-makefile could just look for both of those files in the current directory and do the right validation accordingly. That should simplify things for distro maintainers if we do A (although internally, the verification code is still going to be more complex that way).

ezra-g’s picture

After re-reviewing and reading #1427752: Support drush make includes[] in drupal-org.make files, my view is that option B provides a superior developer experience because pointers eliminate the need to maintain multiple make files with duplicate content, so that 90% of the time, distro developers only have to edit one make file.

As an example, here's a preview of the make files with option B for the Commons distro:

1) drupal-org.make (all projects, libraries, patches, and core)

includes[commons] = drupal.org/commons.make
includes[commons_core] = drupal.org/commons_core.make

2) commons.make

[edit] As a result, most of the time I'd be editing commons.make, and occasionally
[all of my contribs and their pathces, libraries]

3) build-[profilename].make (replicate all the core stuff from drupal-org.makePointers to the previous make files and include a pointer to the profile to build that recursively, which will use [profilename].make)

includes[commons_core] = drupal.org/commons_core.make
profiles[commons] = drupal.org/commons

4) commons_core.make
[grabs Drupal core]

[edit] So most of the time I'd be editing commons.make.

@dww, does that seem accurate :)?

dww’s picture

Yes, that's basically accurate. Can you explain why you think that's better DX? Just that the validation stuff is all in one place? Seems like if you're going to have commons_core.make anyway, it's pretty equivalent to just having drupal-org-core.make (or whatever). If we assume includes[] is going to work in either case, it seems the total # and basic point of the .make files is the same with the two approaches. So, I'm trying to figure out how to assess what's better or worse DX for distro maintainers...

Thanks!
-Derek

ezra-g’s picture

If we assume includes[] is going to work in either case, it seems the total # and basic point of the .make files is the same with the two approaches.

Thanks for that clarification. I missed that includes could/would be implemented as part of either approach but I see now that they fit into either. Supporting includes is the main DX improvement I was referring to, so this is great news :).

However, in option B, drupal-org.make still has the benefit of being a single make file that can be used to fully package the distro either locally, on Drupal.org or on an external packaging server somewhere. That consistency seems like a DX benefit to me because it reduces the possibility of having a bug introduced by (even a small) discrepancy between the make files or logic of the packaging servers (as described in the OP). For example, Acquia runs a set of automated tests against a packaged version of Commons each night and I might have greater confidence in the results of those tests if I knew that the drush make script used to package and the basic logic for the packaging servers were similar.

That being said, I'm thrilled at the clarification that we can use includes in either approach, so while I still learn towards option B, option A would still be acceptable if more folks prefer that option.

In general I think Option A provides more of a DRY approach to distro building and still is my preferred approach.

@febrbraro, given that includes are possible in either case, what about option B feels like it is more repetitive to distro developers than option A?

dww’s picture

@ezra: Re: "However, in option B, drupal-org.make still has the benefit of being a single make file that can be used to fully package the distro either locally, on Drupal.org or on an external packaging server somewhere."

That's not really true. drupal-org.make can't actually be used to build locally nor on an external packaging server somewhere. It can't refer to the profile itself, so it doesn't actually build you a working site. It can get you the version of core you want, complete with patches, and it can put all the profile's dependencies in sites/all/*, but it doesn't refer to the profile itself. The only way to convert a drupal-org.make file in this approach into a working site is to do the same magic that the packaging script does that I describe in the OP -- build just core out of this file, checkout the profile separately and move it into place, build drupal-org.make again with --no-core, and move everything into place under your core checkout. To have an automated local build, you need the build-profile.make described in both approaches. This is part of why I dislike B in a lot of ways. It's a .make file that doesn't actually work unless you do magic things with it. At least with A we know what we've got. We've got a separate file to define core so we can just build that, and another file to define all the profile's dependencies so we can just build those. It's always a two-stage process, and I believe A makes that more explicit.

In terms of includes[] -- that works just fine via drush make. The only thing is you can't currently put those into drupal-org.make itself. To avoid duplication, option A doesn't require includes[] to work inside drupal-org.make files. We only need to support includes[] inside drupal-org.make itself in option B to prevent major duplication. So, that's why I'm saying includes[] is really only necessary for option B. You can still always use includes[] inside profile.make, build-profile.make, etc, since those are never actually read by the d.o packaging scripts.

Therefore, I'm still leaning towards A. We don't have to get includes[] working in drupal-org.make (although we obviously still could). The .make files are more explicit. There's no (well, less) magic going on. There's no need for a separate profile-core.make file. To my eyes, the only downside is it complicates the validation a bit in the drupalorg_drush plugin, and it's something else to document. But in every other way, it seems like the cleaner + clearer approach.

I believe Frank already agrees. Ezra, do you find this argument compelling or are you still leaning towards B? ;)

Thanks!
-Derek

febbraro’s picture

@ezra That comment was only if includes[] were not possible in Option B. With includes it seems that the options are similar in terms of number of files, but I think I like the convention of A better.

@dww Given your recap I'm still in favor of Option A, things seem more explicit.

ezra-g’s picture

Sounds like consensus is towards option A. +1 from me.

dww’s picture

Since we seem to have converged on option A and a separate drupal-org-core.make file for distros that need it, interested parties are encouraged to check out #1469714: Fix validation to support drupal-org-core.make files and prevent defining the drupal project in drupal-org.make itself for a proposal on how the validation rules and such will work with drush make and drush verify-makefile. Please respond there if you have any thoughts/concerns with that proposal.

Thanks!
-Derek

dww’s picture

Status: Active » Needs review
FileSize
19.12 KB

Phew! Now that all of these are fixed:
#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
#1469714: Fix validation to support drupal-org-core.make files and prevent defining the drupal project in drupal-org.make itself
I could finally get down to fixing this one. ;)

Attached patch adds optional support for a profile-provided drupal-org-core.make file for building core. If it's not there, we fall back to what the packaging script always used to do (create one of these for you based on the 'core' attribute in your drupal-org.make file).

Tested fairly thoroughly:

http://distro-drupal.redesign.devdrupal.org/node/1402277 (without a drupal-org-core.make)
http://distro-drupal.redesign.devdrupal.org/node/1402278 (with a drupal-org-core.make that checks out from Git and applies some patches)

You can't actually download the tarballs since this dev box doesn't talk to ftp.d.o, but I verified that they're sane, the patches are actually applied, we really did check out from Git, etc. Hurray!

I doubt anyone's going to actually review this code, but I'm posting this here just in case. ;) It actually looks bigger than it is since the if() nesting changed and a bunch of lines are less nested than they used to be. Such is life. Anyway, I'll commit and deploy this tomorrow, if not sooner...

dww’s picture

Status: Needs review » Fixed

Of course, closely testing this uncovered another bug in drush make itself. ;)

#1474778: .info file re-writing is broken for core

Thankfully, that's now committed and deployed, so I felt safe moving this forward. Committed, pushed and deployed live. Huzzah!

Now I just need to update all the appropriate docs. ;)

dww’s picture

Note re docs. I added sections about this at:

That should hopefully be sufficient (if not excessive). ;)

dww’s picture

Also, I tried to document the best practices on distro .make files here: Managing Drush make files for an installation profile. Probably needs help, and webchick and I are reviewing all this stuff in IRC as I write this, but I wanted to post a link here for anyone following this issue to take a look.

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

  • Commit c943874 on 6.x-3.x, 7.x-3.x-dev by dww:
    [#1455614] by dww: Added support for drupal-org-core.make files.
    
    This...