You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Chris Foote <cf...@v21.me.uk> on 2006/02/21 22:27:59 UTC
Re: [PATCH] v2 Add archive support to hot-backup
Thanks Dan for the review.
Here's an updated version of the patch and log.
Regards
Chris
[[[Log]]]
Add option to archive the backup using gz, bz2 or zip.
* tools/backup/hot-backup.py.in
Update copyright year.
(archive_map): New. Supported archive extentions.
(usage): New. Print out a usage message.
Add optional 3rd arg to the command line to specify the archive type.
(comparator): If archiving, add the archive extenstion to the regexp to
find archives as well.
Step 2, also find archives.
Step 4, archive backup.
Step 5, look for archives to remove as well.
----- Original Message -----
From: "Daniel Rall" <dl...@collab.net>
To: "Chris Foote" <cf...@v21.me.uk>
Cc: <de...@subversion.tigris.org>
Sent: Monday, February 20, 2006 2:49 AM
Subject: Re: [PATCH] Add archive support to hot-backup
> This patch seems pretty reasonable. A couple comments:
>
> 1) Changes to arguments processing aren't mentioned in the log
> message.
>
> 2) Description of the changes to comparator in the log message could
> be a little more clear.
>
> 3) Error handling for a failed import vs. failed execution should be
> more verbose, with the error string differentiating between the two
> cases (instead of using a single, generic "failed" message for both).
>
> - Dan
>
> On Sun, 19 Feb 2006, Chris Foote wrote:
>
> > This patch adds archive support to the hot-backup.py script using the python
> > modules. This is especially useful on Windows.
> >
> > Regards
> > Chris
> >
Re: [PATCH] v3 Add archive support to hot-backup
Posted by Daniel Rall <dl...@collab.net>.
Thanks Chris! I've committed your patch, with some very minor tweaks,
in r18874.
- Dan
On Sat, 04 Mar 2006, Chris Foote wrote:
> Hi Dan,
>
> Here's the latest update of the patch and log.
>
> Regards
> Chris
>
> [[[Log]]]
>
> Add option to archive the backup using gz, bz2 or zip.
>
> * tools/backup/hot-backup.py.in
> Update copyright year.
> (archive_map): New. Supported archive extentions.
> (usage): New. Print out a usage message.
> Redo command line processing using getopt and add option to specify the
> archive type.
> (comparator): If archiving, add the archive extenstion to the regexp to
> find archives as well.
> Step 2, also find archives.
> Step 3, print the error message to stderr.
> Step 4, archive backup.
> Step 5, look for archives to remove as well.
>
>
>
> ----- Original Message -----
> From: "Daniel Rall" <dl...@collab.net>
> To: "Chris Foote" <cf...@v21.me.uk>
> Cc: <de...@subversion.tigris.org>
> Sent: Tuesday, February 28, 2006 8:19 AM
> Subject: Re: [PATCH] v2 Add archive support to hot-backup
>
> On Tue, 21 Feb 2006, Chris Foote wrote:
>
> > Thanks Dan for the review.
> >
> > Here's an updated version of the patch and log.
>
> Chris, again this looks good. If you're up for it, I'd like a last
> few changes made before committing (related to UI and error handling).
> Comments inline below...
>
> [snip]
> Index: tools/backup/hot-backup.py.in
> ===================================================================
> --- tools/backup/hot-backup.py.in (revision 18714)
> +++ tools/backup/hot-backup.py.in (working copy)
> @@ -7,7 +7,7 @@
> # See http://subversion.tigris.org for more information.
> #
> # ====================================================================
> -# Copyright (c) 2000-2004 CollabNet. All rights reserved.
> +# Copyright (c) 2000-2006 CollabNet. All rights reserved.
> #
> # This software is licensed as described in the file COPYING, which
> # you should have received as part of this distribution. The terms
> @@ -22,7 +22,7 @@
>
> ######################################################################
>
> -import sys, os, shutil, string, re
> +import sys, os, getopt, shutil, string, re
>
> ######################################################################
> # Global Settings
> @@ -36,29 +36,85 @@
> # Number of backups to keep around (0 for "keep them all")
> num_backups = 64
>
> +# Archive types/extensions
> +archive_map = {
> + 'gz' : ".tar.gz",
> + 'bz2' : ".tar.bz2",
> + 'zip' : ".zip"
> + }
> +
> ######################################################################
> # Command line arguments
>
> +def usage(out = sys.stdout):
> + scriptname = os.path.basename(sys.argv[0])
> + out.write(
> +"""USAGE: %s [OPTIONS] REPOS_PATH BACKUP_PATH
>
> -if len(sys.argv) != 3:
> - print "Usage: ", os.path.basename(sys.argv[0]), " <repos_path> <backup_path>"
> - sys.exit(1)
> +Create a backup of the repository at REPOS_PATH in a subdirectory of
> +the BACKUP_PATH location, named after the youngest revision.
>
> +Options:
> + --archive-type=FMT Create an archive of the backup. FMT can be one of:
> + bz2 : Creates a bzip2 compressed tar file.
> + gz : Creates a gzip compressed tar file.
> + zip : Creates a compressed zip file.
> + --help -h Print this help message and exit.
> +
> +""" % (scriptname,))
> +
> +
> +try:
> + opts, args = getopt.gnu_getopt(sys.argv[1:], "h?", ["archive-type=", "help"])
> +except getopt.GetoptError, e:
> + sys.stderr.write("ERROR: " + str(e) + "\n\n")
> + usage(sys.stderr)
> + sys.exit(2)
> +
> +archive_type = None
> +
> +for o, a in opts:
> + if o == "--archive-type":
> + archive_type = a
> + elif o in ("-h", "--help", "-?"):
> + usage()
> + sys.exit()
> +
> +if len(args) != 2:
> + sys.stderr.write("ERROR: only two arguments allowed.\n\n")
> + usage(sys.stderr)
> + sys.exit(2)
> +
> # Path to repository
> -repo_dir = sys.argv[1]
> +repo_dir = args[0]
> repo = os.path.basename(os.path.abspath(repo_dir))
>
> # Where to store the repository backup. The backup will be placed in
> # a *subdirectory* of this location, named after the youngest
> # revision.
> -backup_dir = sys.argv[2]
> +backup_dir = args[1]
>
> +# Added to the filename regexp, set when using --archive-type.
> +ext_re = ""
> +
> +# Do we want to create an archive of the backup
> +if archive_type:
> + if archive_map.has_key(archive_type):
> + # Additionally find files with the archive extension.
> + ext_re = "(" + re.escape(archive_map[archive_type]) + ")?"
> + else:
> + sys.stderr.write("Unknown archive type '%s'.\n\n" % (archive_type,))
> + usage(sys.stderr)
> + sys.exit(2)
> +
> +
> ######################################################################
> # Helper functions
>
> def comparator(a, b):
> # We pass in filenames so there is never a case where they are equal.
> - regexp = re.compile("-(?P<revision>[0-9]+)(-(?P<increment>[0-9]+))?$")
> + regexp = re.compile("-(?P<revision>[0-9]+)(-(?P<increment>[0-9]+))?" +
> + ext_re + "$")
> matcha = regexp.search(a)
> matchb = regexp.search(b)
> reva = int(matcha.groupdict()['revision'])
> @@ -109,7 +165,8 @@
> # rather than start from 1 and increment because the starting
> # increments may have already been removed due to num_backups.
>
> -regexp = re.compile("^" + repo + "-" + youngest + "(-(?P<increment>[0-9]+))?$")
> +regexp = re.compile("^" + repo + "-" + youngest +
> + "(-(?P<increment>[0-9]+))?" + ext_re + "$")
> directory_list = os.listdir(backup_dir)
> young_list = filter(lambda x: regexp.search(x), directory_list)
> if young_list:
> @@ -127,23 +184,78 @@
> print "Backing up repository to '" + backup_subdir + "'..."
> err_code = os.spawnl(os.P_WAIT, svnadmin, "svnadmin", "hotcopy", repo_dir,
> backup_subdir, "--clean-logs")
> -if(err_code != 0):
> - print "Unable to backup the repository."
> +if err_code != 0:
> + sys.stderr.write("Unable to backup the repository.\n")
> sys.exit(err_code)
> else:
> print "Done."
>
>
> -### Step 4: finally, remove all repository backups other than the last
> +### Step 4: Make an archive of the backup if required.
> +if archive_type:
> + archive_path = backup_subdir + archive_map[archive_type]
> + err_msg = ""
> +
> + print "Archiving backup to '" + archive_path + "'..."
> + if archive_type == 'gz' or archive_type == 'bz2':
> + try:
> + import tarfile
> + tar = tarfile.open(archive_path, 'w:' + archive_type)
> + tar.add(backup_subdir, os.path.basename(backup_subdir))
> + tar.close()
> + except ImportError, e:
> + err_msg = "Import failed: " + str(e)
> + err_code = -2
> + except tarfile.TarError, e:
> + err_msg = "Tar failed: " + str(e)
> + err_code = -3
> +
> + elif archive_type == 'zip':
> + try:
> + import zipfile
> +
> + def add_to_zip(baton, dirname, names):
> + zp = baton[0]
> + root = os.path.join(baton[1], '')
> +
> + for file in names:
> + path = os.path.join(dirname, file)
> + if os.path.isfile(path):
> + zp.write(path, path[len(root):])
> + elif os.path.isdir(path) and os.path.islink(path):
> + os.path.walk(path, add_to_zip, (zp, path))
> +
> + zp = zipfile.ZipFile(archive_path, 'w', zipfile.ZIP_DEFLATED)
> + os.path.walk(backup_subdir, add_to_zip, (zp, backup_dir))
> + zp.close()
> + except ImportError, e:
> + err_msg = "Import failed: " + str(e)
> + err_code = -4
> + except zipfile.error, e:
> + err_msg = "Zip failed: " + str(e)
> + err_code = -5
> +
> +
> + if err_code != 0:
> + sys.stderr.write("Unable to create an archive for the backup.\n" + err_msg + "\n")
> + sys.exit(err_code)
> + else:
> + print "Archive created, removing backup '" + backup_subdir + "'..."
> + shutil.rmtree(backup_subdir)
> +
> +### Step 5: finally, remove all repository backups other than the last
> ### NUM_BACKUPS.
>
> if num_backups > 0:
> - regexp = re.compile("^" + repo + "-[0-9]+(-[0-9]+)?$")
> + regexp = re.compile("^" + repo + "-[0-9]+(-[0-9]+)?" + ext_re + "$")
> directory_list = os.listdir(backup_dir)
> old_list = filter(lambda x: regexp.search(x), directory_list)
> old_list.sort(comparator)
> del old_list[max(0,len(old_list)-num_backups):]
> for item in old_list:
> - old_backup_subdir = os.path.join(backup_dir, item)
> - print "Removing old backup: " + old_backup_subdir
> - shutil.rmtree(old_backup_subdir)
> + old_backup_item = os.path.join(backup_dir, item)
> + print "Removing old backup: " + old_backup_item
> + if os.path.isdir(old_backup_item):
> + shutil.rmtree(old_backup_item)
> + else:
> + os.remove(old_backup_item)
Re: [PATCH] v3 Add archive support to hot-backup
Posted by Chris Foote <cf...@v21.me.uk>.
Hi Dan,
Here's the latest update of the patch and log.
Regards
Chris
[[[Log]]]
Add option to archive the backup using gz, bz2 or zip.
* tools/backup/hot-backup.py.in
Update copyright year.
(archive_map): New. Supported archive extentions.
(usage): New. Print out a usage message.
Redo command line processing using getopt and add option to specify the
archive type.
(comparator): If archiving, add the archive extenstion to the regexp to
find archives as well.
Step 2, also find archives.
Step 3, print the error message to stderr.
Step 4, archive backup.
Step 5, look for archives to remove as well.
----- Original Message -----
From: "Daniel Rall" <dl...@collab.net>
To: "Chris Foote" <cf...@v21.me.uk>
Cc: <de...@subversion.tigris.org>
Sent: Tuesday, February 28, 2006 8:19 AM
Subject: Re: [PATCH] v2 Add archive support to hot-backup
On Tue, 21 Feb 2006, Chris Foote wrote:
> Thanks Dan for the review.
>
> Here's an updated version of the patch and log.
Chris, again this looks good. If you're up for it, I'd like a last
few changes made before committing (related to UI and error handling).
Comments inline below...
[snip]
Re: [PATCH] v2 Add archive support to hot-backup
Posted by Daniel Rall <dl...@collab.net>.
On Tue, 28 Feb 2006, Daniel Rall wrote:
...
> > + print "Archiving backup to '" + archive_path + "'..."
> > + if archive == 'gz' or archive == 'bz2':
> > + try:
> > + import tarfile
> > + tar = tarfile.open(archive_path, 'w:' + archive)
> > + tar.add(backup_subdir, os.path.basename(backup_subdir))
> > + tar.close()
> > + except ImportError, e:
> > + err_msg = "Import failed: " + str(e)
> > + err_code = -2
> > + except tarfile.TarError, e:
> > + err_msg = "Tar failed: " + str(e)
> > + err_code = -3
>
> There should be two separate try/except blocks here, the first around
> the "import tarfile" statement, and the second around the remainder of
> the existing "try" block.
Scratch that, tarfile.py embeds a ton of possible runtime
ImportErrors, long after the tarfile module has been imported.
--
Daniel Rall
Re: [PATCH] v2 Add archive support to hot-backup
Posted by Daniel Rall <dl...@collab.net>.
On Tue, 21 Feb 2006, Chris Foote wrote:
> Thanks Dan for the review.
>
> Here's an updated version of the patch and log.
Chris, again this looks good. If you're up for it, I'd like a last
few changes made before committing (related to UI and error handling).
Comments inline below...
> Add option to archive the backup using gz, bz2 or zip.
>
> * tools/backup/hot-backup.py.in
> Update copyright year.
> (archive_map): New. Supported archive extentions.
> (usage): New. Print out a usage message.
> Add optional 3rd arg to the command line to specify the archive type.
> (comparator): If archiving, add the archive extenstion to the regexp to
> find archives as well.
> Step 2, also find archives.
> Step 4, archive backup.
> Step 5, look for archives to remove as well.
Indent a couple more space for all those "Step N" lines.
> ----- Original Message -----
> From: "Daniel Rall" <dl...@collab.net>
> To: "Chris Foote" <cf...@v21.me.uk>
> Cc: <de...@subversion.tigris.org>
> Sent: Monday, February 20, 2006 2:49 AM
> Subject: Re: [PATCH] Add archive support to hot-backup
>
>
> > This patch seems pretty reasonable. A couple comments:
> >
> > 1) Changes to arguments processing aren't mentioned in the log
> > message.
> >
> > 2) Description of the changes to comparator in the log message could
> > be a little more clear.
> >
> > 3) Error handling for a failed import vs. failed execution should be
> > more verbose, with the error string differentiating between the two
> > cases (instead of using a single, generic "failed" message for both).
...
> Index: tools/backup/hot-backup.py.in
> ===================================================================
> --- tools/backup/hot-backup.py.in (revision 18545)
> +++ tools/backup/hot-backup.py.in (working copy)
...
> @@ -36,12 +36,34 @@
> # Number of backups to keep around (0 for "keep them all")
> num_backups = 64
>
> +# Archive types/extentions
> +archive_map = {
> + 'gz' : ".tar.gz",
> + 'bz2' : ".tar.bz2",
> + 'zip' : ".zip"
> + }
> +
> ######################################################################
> # Command line arguments
>
> +def usage():
> + scriptname = os.path.basename(sys.argv[0])
> + sys.stderr.write(
> +"""USAGE: %s REPOS_PATH BACKUP_PATH [ARCHIVE_TYPE]
An optional argument like ARCHIVE_TYPE would be better off as a flag,
rather than a positional arg, because it gives us the flexibility to
add more options in the future without worrying about command-line
interface changes. I suggest something like:
hot-backup.py [--archive-type=ARCHIVE_TYPE] REPOS_PATH BACKUP_PATH
> -if len(sys.argv) != 3:
> - print "Usage: ", os.path.basename(sys.argv[0]), " <repos_path> <backup_path>"
> +Create a backup of the repository at REPOS_PATH in a subdirectory of
> +the BACKUP_PATH location, named after the youngest revision.
> +
> +If specified, ARCHIVE_TYPE creates an archive of the backup instead;
> +can be one of:
> + bz2 : Creates a bzip2 compressed tar file.
> + gz : Creates a gzip compressed tar file.
> + zip : Creates a compressed zip file.
> +""" % (scriptname,))
> +
> +
> +if len(sys.argv) < 3 or len(sys.argv) > 4:
> + usage()
> sys.exit(1)
>
> # Path to repository
> @@ -53,12 +75,28 @@
> # revision.
> backup_dir = sys.argv[2]
>
> +# Do we want to create an archive of the backup
> +if len(sys.argv) < 4:
> + archive = None
> + ext_re = ""
> +elif not archive_map.has_key(sys.argv[3]):
> + sys.stderr.write(
> +"""Unknown archive type '%s'.
> +
> +""" % (sys.argv[3],))
> + usage()
> + sys.exit(1)
> +else:
> + archive = sys.argv[3]
> + ext_re = "(" + re.escape(archive_map[archive]) + ")?"
You could manage to stick with this positional argument parsing, but
more easily extensionable would be to switch to use of getopt.
(There's some good example of its usage all over our tools and contrib
directories.)
...
> -### Step 4: finally, remove all repository backups other than the last
> +### Step 4: Make an archive of the backup if required.
> +if archive:
> + archive_path = backup_subdir + archive_map[archive]
> + err_msg = ""
> +
> + old_cwd = os.getcwd()
> + try:
> + os.chdir(backup_dir)
> + if os.path.exists(archive_path):
> + os.remove(archive_path)
> +
> + print "Archiving backup to '" + archive_path + "'..."
> + if archive == 'gz' or archive == 'bz2':
> + try:
> + import tarfile
> + tar = tarfile.open(archive_path, 'w:' + archive)
> + tar.add(backup_subdir, os.path.basename(backup_subdir))
> + tar.close()
> + except ImportError, e:
> + err_msg = "Import failed: " + str(e)
> + err_code = -2
> + except tarfile.TarError, e:
> + err_msg = "Tar failed: " + str(e)
> + err_code = -3
There should be two separate try/except blocks here, the first around
the "import tarfile" statement, and the second around the remainder of
the existing "try" block.
TarError is a polymorphic base class, in this case probably
representing read, compress, or stream problems. (It can also
represent extraction, but there's none here.) Handle CompressionError
separately, so you can report a "Compression failed: " message.
> +
> + elif archive == 'zip':
> + try:
> + import zipfile
> +
> + def add_to_zip(baton, dirname, names):
> + zp = baton[0]
> + root = os.path.join(baton[1], '')
> +
> + for file in names:
> + path = os.path.join(dirname, file)
> + if os.path.isfile(path):
> + zp.write(path, path[len(root):])
> + elif os.path.isdir(path) and os.path.islink(path):
> + os.path.walk(path, add_to_zip, (zp, path))
> +
> + zp = zipfile.ZipFile(archive_path, 'w', zipfile.ZIP_DEFLATED)
> + os.path.walk(backup_subdir, add_to_zip, (zp, backup_subdir))
> + zp.close()
> + except ImportError, e:
> + err_msg = "Import failed: " + str(e)
> + err_code = -4
> + except zipfile.error, e:
> + err_msg = "Zip failed: " + str(e)
> + err_code = -5
Handle ImportError as above.
> + finally:
> + os.chdir(old_cwd)
> +
> + if err_code != 0:
> + print "Unable to archive the backup."
Error should go to stderr:
print >> sys.stderr, ...
For the message itself, how about something a little more verbose to
remove any ambiguity (e.g. "Unable to create an archive for the
backup")?
> + print err_msg
> + sys.exit(err_code)
> + else:
> + print "Archive created, removing backup '" + backup_subdir + "'..."
> + shutil.rmtree(backup_subdir)
> +
...