here's an initial version of a packaging script for the new release system, in particular, the conversion of releases into real nodes (http://drupal.org/node/83339).

this still doesn't handle periodic rebuilding from branches, nor translation reporting stuff, but it works just great for releases from tags, including updating the release node with the file, md5 hash, and date, and then publishing the node.

it's all CLI-drupal for now, though potentially we could split some of this up into a .inc file that was called directly from project_release.module when new tag-based releases are added...

Comments

dww’s picture

new version:

  • adds real support for rebuilding from branches, checking timestamps and only re-packaging if files have changed, etc
  • the arg on the command-line is now used to specify what you want packaged. the choices are currently either 'tag' (only package releases from fixed tags that haven't been packaged yet) or 'branch' (rebuild nightly snapshots where files have changed). the idea being we can call this with 'tag' every 5 minutes, since that's cheap (usually there's no work at all, or maybe 1-3 releases to package, which takes ~10 seconds on my laptop), whereas we'd still call it with 'branch' every 8 hours or so (currently, even just to verify that all ~2200 releases haven't been updated takes about 45 minutes on my highly loaded laptop -- hopefully it'll be faster on the d.o infrastructure).
  • added some better info in the watchdog messages
  • cleaned up some debugging cruft
  • generally cleaned up the code some more
  • path to rm is now a setting, instead of relying on the PATH
  • copied in some translation-related functions from the old script. still not being used

TODO:

  1. proper packaging of translations (there's .po file munging from the old script that needs to happen, which isn't in the code yet)
  2. better error propagation and robustness (e.g. if "cvs co" fails for some reason, etc). we should only update the DB with the file_path if we're sure it exists and was successfully created, etc.
  3. translation stats.

i agreed with killes that the best way to handle translation stats is a) store that info in the DB, not a flat file, and b) add a simple little translation_stats.module that displays the info from the DB into a human-readable table at some fixed menu location. however, it's not worth delaying the entire release system for this added complication, so translation stats will probably happen in a separate version of all this stuff, once the main system is live. probably on the order of a week after the initial release...

dww’s picture

StatusFileSize
new10.22 KB

...

dww’s picture

Status: Needs work » Needs review
StatusFileSize
new11.55 KB

new version that does all the msg* mojo to correctly package translations.
translation status stuff is still being postponed for now.

dww’s picture

Status: Needs review » Needs work

this needs work before it can go live:

  1. never call `rm` with a relative path
  2. rip out the debugging code that was left in the previous version. ;)
  3. fix $exclude vs. $to_exclude confusion
dww’s picture

just committed everything from this issue into the DRUPAL-4-7--2 branch of project/release (that seemed to make the most sense as the place to put this script, even though it depends on cvs.module, too).

also, i fixed #1 and #2 from the previous comment. so, the full directory path we're sending to rm -rf and better logging/error propagation is all that remains to be done.

dww’s picture

Status: Needs work » Needs review
StatusFileSize
new13.62 KB

- cleaned up all the customization variables
- all external tools have a setting for the full path so we never have to rely on PATH
- never invoke rm with relative path to the directory to destroy
- removed debugging code that was left in
- other fixes

RTBC?

(the only thing i could see as a minor safety measure is to validate that the $tmp_dir you specify has to be an absolute path)...

dww’s picture

the version from comment #6 is certainly a vast improvement over what was in CVS already, so i went ahead and committed it to the DRUPAL-4-7--2 branch as revision 1.1.2.5. kjartan installed it on scratch.d.o and it seems to be working fine.

however, i'd still like to get a thorough review from kjartan or killes before i mark this as fixed/closed.

Kjartan’s picture

Just some quick notes:
- Can we move the config to an include file? The settings we need on the d.o infrastructure are different than the default, including a lot of the external tool paths. Having it all in one file makes it easy to miss the path to tar being wrong when updating.
- Any reason to use cvs checkout? wouldn't export be better as we then can avoid excluding the CVS directories in all other commands.
- The chdir() on line 250 can fail when cvs co has nothing to checkout, would be a good point to include a little check as it can be rather hard to track down real errors in the output with a ton of minor warnings showing up. We probably have some junk in the various release tables causing this, so another option is to clear out the stuff that doesn't exist.
- There are a couple of Drupal coding standard issues (incorrect indentation, string concats, inconsistent use of t()), but not going to be too picky about that.
- The translation packaging seems to spew out a ton of "msgattrib: internationalized messages should not contain the `\r' escape sequence", these can probably be ignored safely but figured I would mention it in case someone has any ideas on how to avoid them.

Apart from that it looks good,

dww’s picture

Priority: Critical » Normal
Status: Needs review » Needs work

natrak and i agree none of this is critical enough to delay deployment of the new system. but, these are all good suggestions, so i'll work on them time permitting...

dww’s picture

Status: Needs work » Needs review
StatusFileSize
new7.4 KB

doesn't solve everything, but here's a start... fixes the stray whitespace and indentation issues i could find, along with the inconsistent use of t() (which also fixes a few *potential* watchdog XSS holes). ;) of course, we can't use t() before we successfully bootstrap ourselves, so the initial error checking and print() statements have to be untranslatable, but i don't see any problem with that...

Kjartan’s picture

Status: Needs review » Reviewed & tested by the community

Seems to fix up the worst of the offenses.

dww’s picture

Status: Reviewed & tested by the community » Needs work
StatusFileSize
new2.51 KB

committed patch from #10 to DRUPAL-4-7--2 as revision 1.1.2.7.

in terms of the config settings in a separate file, Kjartan and i decided in IRC it'll be a lot easier to just customize this locally by just putting all the config variables in a separate .inc file and adding a single line to the live script. this will make it much less likely to have conflicts when updating, etc. attached is an example of such a local config .inc file (notice the use of global everywhere to get this working)...

so, the only remaining TODO items from comment #8 are:

  1. cvs checkout vs. cvs export (good idea, should fix)
  2. chdir() failing when cvs checkout fails -- needs better error checking (good idea, should fix)
  3. "msgattrib: internationalized messages should not contain the `\r' escape sequence" (no idea about this)

therefore, i'm moving this back to "needs work" for now...

dww’s picture

Status: Needs work » Needs review
StatusFileSize
new7.83 KB

major cleanup of error checking, logging, and robustness. ;)

  1. use cvs export everywhere instead of cvs checkout since that avoids having to exclude the "CVS" directories all over the place.
  2. added drupal_exec() which is a wrapper for exec() that logs any errors (safely) to the watchdog. if the command failed , all output (including stderr) is logged...
  3. added drupal_chdir() which is a wrapper for chdir() that logs errors to the watchdog.
  4. all shell commands now use the appropriate drupal_* wrapper so we log any failures. plus, failures in the middle of packaging a given release (e.g. cvs export fails) cause that package attempt to abort, so we don't do unsafe things and keep going
  5. all errors are now logged to "release_error" watchdog type (where's this 16 char max coming from, anyway? :( boo, hiss...).
  6. we attempt to blow away the directory we're about to cvs export into, so that we don't get any errors about things being in the way...

i think that's everything i plan to fix here. ;) RTBC?

dww’s picture

Status: Needs review » Fixed

Kjartan in IRC said this was RTBC, so i committed to DRUPAL-4-7--2 branch as revision 1.1.2.11.

Anonymous’s picture

Status: Fixed » Closed (fixed)