Merge lp:~xnox/apt-btrfs-snapshot/py3 into lp:apt-btrfs-snapshot

Proposed by Dimitri John Ledkov
Status: Merged
Merged at revision: 39
Proposed branch: lp:~xnox/apt-btrfs-snapshot/py3
Merge into: lp:apt-btrfs-snapshot
Diff against target: 288 lines (+93/-33)
6 files modified
apt-btrfs-snapshot (+6/-6)
apt_btrfs_snapshot.py (+20/-17)
debian/changelog (+26/-2)
debian/control (+8/-2)
debian/rules (+23/-1)
test/test_apt_btrfs_snapshot.py (+10/-5)
To merge this branch: bzr merge lp:~xnox/apt-btrfs-snapshot/py3
Reviewer Review Type Date Requested Status
Colin Watson (community) Approve
Michael Vogt Pending
Review via email: mp+109632@code.launchpad.net

Description of the change

python3 port and other fixes

To post a comment you must log in.
Revision history for this message
Colin Watson (cjwatson) wrote :

Looks good to me, though I'd like Michael to review this and merge if he's happy.

review: Approve
Revision history for this message
Barry Warsaw (barry) wrote :
Download full text (3.9 KiB)

On Jun 11, 2012, at 01:20 PM, Dmitrijs Ledkovs wrote:

>Dmitrijs Ledkovs has proposed merging lp:~dmitrij.ledkov/apt-btrfs-snapshot/py3 into lp:apt-btrfs-snapshot.
>
>Requested reviews:
> Canonical Foundations Team (canonical-foundations)
> Michael Vogt (mvo)
>
>For more details, see:
>https://code.launchpad.net/~dmitrij.ledkov/apt-btrfs-snapshot/py3/+merge/109632
>
>python3 port and other fixes

=== modified file 'apt-btrfs-snapshot'
--- apt-btrfs-snapshot 2011-07-19 11:42:45 +0000
+++ apt-btrfs-snapshot 2012-06-11 13:19:19 +0000
> @@ -1,4 +1,4 @@
> -#!/usr/bin/python
> +#!/usr/bin/python3
> # Copyright (C) 2011 Canonical
> #
> # Author:
> @@ -17,6 +17,8 @@
> # this program; if not, write to the Free Software Foundation, Inc.,
> # 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>
> +from __future__ import print_function

This import got me thinking. If apt-btrfs-snapshot (the script) will only run
in Python 3, then clearly we don't need this. If it could run in Python 2
(e.g. explicitly invoked ignoring the #! line, or if you want to hold out the
possibility of reverting to Python 2), then the import is fine, but possibly
incomplete.

(Aside: print_function was introduced in Python 2.6.0a2:
http://docs.python.org/py3k/library/__future__.html)

It might be incompletely because you may also need to import unicode_literals
(FWIW, just as a blanket rule, I always import those two plus
absolute_import). The other thing you'll need to check is that the _()
function is defined properly for cross-py2/py3 compatibility. It's not clear
from the diff if this is the case.

Here's the thing: you always want _() to return unicode strings, and you want
the input to be unicode strings. In Python 2, gettext.gettext() is *not* the
right thing to use, you need gettext.ugettext(). In Python 3,
gettext.gettext() does operate on and return unicodes.

You can do something like this to make it work right (this is pulled from the
c-j code in u-m).

def setup_gettext():
    domain = 'foo'
    localedir = os.environ.get('LOCPATH')
    t = gettext.translation(domain, localedir=localedir, fallback=True)
    try:
        return t.ugettext
    except AttributeError:
        return t.gettext

Then...

_ = setup_gettext()

OTOH, if the script will always run under Python 3, most of that won't be
necessary.

=== modified file 'apt_btrfs_snapshot.py'
--- apt_btrfs_snapshot.py 2011-10-17 08:05:22 +0000
+++ apt_btrfs_snapshot.py 2012-06-11 13:19:19 +0000
> @@ -16,9 +16,10 @@
> # this program; if not, write to the Free Software Foundation, Inc.,
> # 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>
> +from __future__ import print_function

Even though you're not using gettext, I'd still consider including
unicode_literals here. The problem is that if in Python 2, you're operating
on 8-bit strings, they'll magically become unicodes in Python 3, and then
you're just asking for trouble. If the literals really should be 8-bit
strings (a.k.a. bytes) in both versions, then import unicode_literals and
prefix your byte literals with b''. That way, the intentions are clear, and
things will work the same in both versions.

> @@ -57,14 +58,16 @@
...

Read more...

lp:~xnox/apt-btrfs-snapshot/py3 updated
43. By Dimitri John Ledkov

Address barry's comments

Revision history for this message
Michael Vogt (mvo) wrote :

Thanks for your work on this branch!

it looks good. One comment below:

On Mon, Jun 11, 2012 at 01:20:25PM -0000, Dmitrijs Ledkovs wrote:
> - def test_fstab_detect_snapshot(self):
> + @mock.patch('os.path.exists', side_effect=lambda f: f in ('/sbin/btrfs'))

This seems this requires python-mock 0.8, it would be nice if that
would still work on 12.04 for now if its not too much hassle to make
it easy for contributors to hack on it without the need to upgrade to
quantal just yet.

We should probably add a generic pyflakes and pep8 test to the tests
dir, I will do that in a bit.

Thanks,
 Michael

Revision history for this message
Dimitri John Ledkov (xnox) wrote :

Pushed upto -r44, which uses python-mock 0.7 style api/syntax. Works in precise pbuilder.
Regards,
Dmitrijs.

lp:~xnox/apt-btrfs-snapshot/py3 updated
44. By Dimitri John Ledkov

use python-mock 0.7 API/syntax

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'apt-btrfs-snapshot'
--- apt-btrfs-snapshot 2011-07-19 11:42:45 +0000
+++ apt-btrfs-snapshot 2012-06-11 17:29:49 +0000
@@ -1,4 +1,4 @@
1#!/usr/bin/python1#!/usr/bin/python3
2# Copyright (C) 2011 Canonical2# Copyright (C) 2011 Canonical
3#3#
4# Author:4# Author:
@@ -84,21 +84,21 @@
84 logging.basicConfig(level=logging.INFO)84 logging.basicConfig(level=logging.INFO)
8585
86 if os.getuid() != 0:86 if os.getuid() != 0:
87 print _("Sorry, you need to be root to run this program")87 print(_("Sorry, you need to be root to run this program"))
88 sys.exit(1)88 sys.exit(1)
8989
90 apt_btrfs = AptBtrfsSnapshot()90 apt_btrfs = AptBtrfsSnapshot()
91 if not apt_btrfs.snapshots_supported():91 if not apt_btrfs.snapshots_supported():
92 print _("Sorry, your system lacks support for the snapshot feature")92 print(_("Sorry, your system lacks support for the snapshot feature"))
93 sys.exit(1)93 sys.exit(1)
9494
95 res = False95 res = False
96 if args.command == "supported":96 if args.command == "supported":
97 res = apt_btrfs.snapshots_supported()97 res = apt_btrfs.snapshots_supported()
98 if res:98 if res:
99 print _("Supported")99 print(_("Supported"))
100 else:100 else:
101 print _("Not supported")101 print(_("Not supported"))
102 elif args.command == "snapshot":102 elif args.command == "snapshot":
103 res = apt_btrfs.create_btrfs_root_snapshot()103 res = apt_btrfs.create_btrfs_root_snapshot()
104 elif args.command == "list":104 elif args.command == "list":
@@ -112,7 +112,7 @@
112 elif args.command == "delete-older-than":112 elif args.command == "delete-older-than":
113 res = apt_btrfs.clean_btrfs_root_snapshots_older_than(args.time)113 res = apt_btrfs.clean_btrfs_root_snapshots_older_than(args.time)
114 else:114 else:
115 print _("ERROR: Unhandled command: '%s'") % args.command115 print(_("ERROR: Unhandled command: '%s'") % args.command)
116116
117 # return the right exit code117 # return the right exit code
118 if res:118 if res:
119119
=== modified file 'apt_btrfs_snapshot.py'
--- apt_btrfs_snapshot.py 2011-10-17 08:05:22 +0000
+++ apt_btrfs_snapshot.py 2012-06-11 17:29:49 +0000
@@ -16,9 +16,10 @@
16# this program; if not, write to the Free Software Foundation, Inc.,16# this program; if not, write to the Free Software Foundation, Inc.,
17# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA17# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
1818
19from __future__ import print_function, unicode_literals
20
19import datetime21import datetime
20import os22import os
21import string
22import subprocess23import subprocess
23import sys24import sys
24import time25import time
@@ -57,14 +58,16 @@
57 """ a list of FstabEntry items """58 """ a list of FstabEntry items """
58 def __init__(self, fstab="/etc/fstab"):59 def __init__(self, fstab="/etc/fstab"):
59 super(Fstab, self).__init__()60 super(Fstab, self).__init__()
60 for line in map(string.strip, open(fstab)):61
61 if line == "" or line.startswith("#"):62 with open(fstab) as fstab_file:
62 continue63 for line in (l.strip() for l in fstab_file):
63 try:64 if line == "" or line.startswith("#"):
64 entry = FstabEntry.from_line(line)65 continue
65 except ValueError:66 try:
66 continue67 entry = FstabEntry.from_line(line)
67 self.append(entry)68 except ValueError:
69 continue
70 self.append(entry)
6871
69class LowLevelCommands(object):72class LowLevelCommands(object):
70 """ lowlevel commands invoked to perform various tasks like73 """ lowlevel commands invoked to perform various tasks like
@@ -133,7 +136,7 @@
133 self._btrfs_root_mountpoint = None136 self._btrfs_root_mountpoint = None
134 return res137 return res
135 def _get_now_str(self):138 def _get_now_str(self):
136 return datetime.datetime.now().replace(microsecond=0).isoformat("_")139 return datetime.datetime.now().replace(microsecond=0).isoformat(str('_'))
137 def create_btrfs_root_snapshot(self, additional_prefix=""):140 def create_btrfs_root_snapshot(self, additional_prefix=""):
138 mp = self.mount_btrfs_root_volume()141 mp = self.mount_btrfs_root_volume()
139 snap_id = self._get_now_str()142 snap_id = self._get_now_str()
@@ -153,7 +156,7 @@
153 if older_than != 0:156 if older_than != 0:
154 entry = self._get_supported_btrfs_root_fstab_entry()157 entry = self._get_supported_btrfs_root_fstab_entry()
155 if not entry:158 if not entry:
156 raise AptBtrfsSnapshotNotSupportedError()159 raise AptBtrfsNotSupportedError()
157 if "noatime" in entry.options:160 if "noatime" in entry.options:
158 raise AptBtrfsRootWithNoatimeError()161 raise AptBtrfsRootWithNoatimeError()
159 # if there is no older than, interpret that as "now"162 # if there is no older than, interpret that as "now"
@@ -170,8 +173,8 @@
170 self.umount_btrfs_root_volume()173 self.umount_btrfs_root_volume()
171 return l174 return l
172 def print_btrfs_root_snapshots(self):175 def print_btrfs_root_snapshots(self):
173 print "Available snapshots:"176 print("Available snapshots:")
174 print " \n".join(self.get_btrfs_root_snapshots_list())177 print(" \n".join(self.get_btrfs_root_snapshots_list()))
175 return True178 return True
176 def _parse_older_than_to_unixtime(self, timefmt):179 def _parse_older_than_to_unixtime(self, timefmt):
177 now = time.time()180 now = time.time()
@@ -182,9 +185,9 @@
182 def print_btrfs_root_snapshots_older_than(self, timefmt):185 def print_btrfs_root_snapshots_older_than(self, timefmt):
183 older_than_unixtime = self._parse_older_than_to_unixtime(timefmt)186 older_than_unixtime = self._parse_older_than_to_unixtime(timefmt)
184 try:187 try:
185 print "Available snapshots older than '%s':" % timefmt188 print("Available snapshots older than '%s':" % timefmt)
186 print " \n".join(self.get_btrfs_root_snapshots_list(189 print(" \n".join(self.get_btrfs_root_snapshots_list(
187 older_than=older_than_unixtime))190 older_than=older_than_unixtime)))
188 except AptBtrfsRootWithNoatimeError:191 except AptBtrfsRootWithNoatimeError:
189 sys.stderr.write("Error: fstab option 'noatime' incompatible with option")192 sys.stderr.write("Error: fstab option 'noatime' incompatible with option")
190 return False193 return False
@@ -202,7 +205,7 @@
202 return res205 return res
203 def command_set_default(self, snapshot_name):206 def command_set_default(self, snapshot_name):
204 res = self.set_default(snapshot_name)207 res = self.set_default(snapshot_name)
205 print "Please reboot"208 print("Please reboot")
206 return res209 return res
207 def set_default(self, snapshot_name, backup=True):210 def set_default(self, snapshot_name, backup=True):
208 """ set new default """211 """ set new default """
209212
=== modified file 'debian/changelog'
--- debian/changelog 2011-10-17 08:05:22 +0000
+++ debian/changelog 2012-06-11 17:29:49 +0000
@@ -1,5 +1,6 @@
1apt-btrfs-snapshot (0.2.2) UNRELEASEDoneiric-proposed; urgency=low1apt-btrfs-snapshot (0.2.2) UNRELEASED; urgency=low
22
3 [ Michael Vogt ]
3 * apt_btrfs_snapshot.py, test/test_apt_btrfs_snapshot.py:4 * apt_btrfs_snapshot.py, test/test_apt_btrfs_snapshot.py:
4 - disable "delete-older-than" command if noatime is in use on 5 - disable "delete-older-than" command if noatime is in use on
5 the snapshot fs (LP: #833980)6 the snapshot fs (LP: #833980)
@@ -8,7 +9,30 @@
8 - be even more robust against invalid fstab entries 9 - be even more robust against invalid fstab entries
9 (LP: #873411 comment #7)10 (LP: #873411 comment #7)
1011
11 -- Michael Vogt <michael.vogt@ubuntu.com> Tue, 11 Oct 2011 10:20:45 +020012 [ Dmitrijs Ledkovs ]
13 * apt_btrfs_snapshot.py, apt-btrfs-snapshot:
14 - port to python3
15 * apt_btrfs_snapshot.py:
16 - fix pyflakes warning use 'AptBtrfsNotSupportedError' instead of
17 undefined name 'AptBtrfsSnapshotNotSupportedError'
18 * test/test_apt_btrfs_snapshot.py:
19 - remove unused imports 'LowLevelCommands' and 'apt_btrfs_snapshot'
20 - mock sys.stdout and sys.stderr to avoid confusing and spurious output
21 - mock /sbin/btrfs to allow running unit-tests without btrfs-tools
22 * debian/rules:
23 - build with supported versions of python2 and python3
24 - run unittests at build time, honoring nocheck
25 * debian/control:
26 - adjust dependencies for python3 support
27 - bump standards version to 3.9.3
28
29 -- Dmitrijs Ledkovs <dmitrij.ledkov@ubuntu.com> Mon, 11 Jun 2012 14:07:41 +0100
30
31apt-btrfs-snapshot (0.2.1build1) precise; urgency=low
32
33 * Rebuild to drop python2.6 dependencies.
34
35 -- Matthias Klose <doko@ubuntu.com> Sat, 31 Dec 2011 02:00:37 +0000
1236
13apt-btrfs-snapshot (0.2.1) oneiric; urgency=low37apt-btrfs-snapshot (0.2.1) oneiric; urgency=low
1438
1539
=== modified file 'debian/control'
--- debian/control 2011-10-10 21:21:46 +0000
+++ debian/control 2012-06-11 17:29:49 +0000
@@ -2,16 +2,22 @@
2Section: admin2Section: admin
3Priority: optional3Priority: optional
4Maintainer: Michael Vogt <michael.vogt@ubuntu.com>4Maintainer: Michael Vogt <michael.vogt@ubuntu.com>
5Build-Depends: debhelper (>= 7),5Build-Depends: debhelper (>= 7.0.50~),
6 python3,
7 python3-distutils-extra,
8 python3-mock,
6 python (>= 2.6.5-2~),9 python (>= 2.6.5-2~),
7 python-distutils-extra,10 python-distutils-extra,
11 python-mock,
8X-Python-Version: all12X-Python-Version: all
9Standards-Version: 3.9.113X-Python3-Version: >= 3.2
14Standards-Version: 3.9.3
10Vcs-Bzr: https://code.launchpad.net/apt-btrfs-snapshot15Vcs-Bzr: https://code.launchpad.net/apt-btrfs-snapshot
1116
12Package: apt-btrfs-snapshot17Package: apt-btrfs-snapshot
13Architecture: all18Architecture: all
14Depends: ${python:Depends},19Depends: ${python:Depends},
20 ${python3:Depends},
15 ${misc:Depends},21 ${misc:Depends},
16 btrfs-tools22 btrfs-tools
17Description: Automatically create snapshot on apt operations23Description: Automatically create snapshot on apt operations
1824
=== modified file 'debian/rules'
--- debian/rules 2011-02-16 11:56:59 +0000
+++ debian/rules 2012-06-11 17:29:49 +0000
@@ -1,5 +1,27 @@
1#!/usr/bin/make -f1#!/usr/bin/make -f
2PYTHON2:=$(shell pyversions -r)
3PYTHON3:=$(shell py3versions -r)
4py3sdo=set -e; $(foreach py, $(PYTHON3), $(py) $(1);)
5pyalldo=set -e; $(foreach py, $(PYTHON2) $(PYTHON3), $(py) $(1);)
26
3%:7%:
4 dh $@ --with python28 dh $@ --with python2,python3
9
10ifeq (,$(filter nocheck,$(DEB_BUILD_OPTIONS)))
11override_dh_auto_test:
12 $(call pyalldo, -m unittest discover -vv test/)
13endif
14
15override_dh_auto_build:
16 dh_auto_build
17 $(call py3sdo, setup.py build)
18
19override_dh_auto_install:
20 dh_auto_install
21 $(call py3sdo, setup.py install --root=$(CURDIR)/debian/apt-btrfs-snapshot --install-layout=deb)
22
23override_dh_auto_clean:
24 dh_auto_clean
25 rm -rf build
26 rm -rf *.egg-info
527
628
=== modified file 'test/test_apt_btrfs_snapshot.py'
--- test/test_apt_btrfs_snapshot.py 2011-10-17 08:05:22 +0000
+++ test/test_apt_btrfs_snapshot.py 2012-06-11 17:29:49 +0000
@@ -1,5 +1,9 @@
1#!/usr/bin/python1#!/usr/bin/python
22
3try:
4 from StringIO import StringIO
5except ImportError:
6 from io import StringIO
3import mock7import mock
4import os8import os
5import sys9import sys
@@ -8,17 +12,16 @@
812
9sys.path.insert(0, "..")13sys.path.insert(0, "..")
10sys.path.insert(0, ".")14sys.path.insert(0, ".")
11import apt_btrfs_snapshot
12from apt_btrfs_snapshot import (AptBtrfsSnapshot,15from apt_btrfs_snapshot import (AptBtrfsSnapshot,
13 AptBtrfsRootWithNoatimeError,16 AptBtrfsRootWithNoatimeError)
14 LowLevelCommands)
1517
16class TestFstab(unittest.TestCase):18class TestFstab(unittest.TestCase):
1719
18 def setUp(self):20 def setUp(self):
19 self.testdir = os.path.dirname(os.path.abspath(__file__))21 self.testdir = os.path.dirname(os.path.abspath(__file__))
2022
21 def test_fstab_detect_snapshot(self):23 @mock.patch('os.path.exists', side_effect=lambda f: f in ('/sbin/btrfs'))
24 def test_fstab_detect_snapshot(self, mock_commands):
22 apt_btrfs = AptBtrfsSnapshot(25 apt_btrfs = AptBtrfsSnapshot(
23 fstab=os.path.join(self.testdir, "data", "fstab"))26 fstab=os.path.join(self.testdir, "data", "fstab"))
24 self.assertTrue(apt_btrfs.snapshots_supported())27 self.assertTrue(apt_btrfs.snapshots_supported())
@@ -38,7 +41,9 @@
38 self.assertEqual(apt_btrfs._uuid_for_mountpoint("/"),41 self.assertEqual(apt_btrfs._uuid_for_mountpoint("/"),
39 "UUID=fe63f598-1906-478e-acc7-f74740e78d1f")42 "UUID=fe63f598-1906-478e-acc7-f74740e78d1f")
4043
41 def test_fstab_noatime(self):44 @mock.patch('sys.stdout', new_callable=StringIO)
45 @mock.patch('sys.stderr', new_callable=StringIO)
46 def test_fstab_noatime(self, mock_stdout, mock_stderr):
42 apt_btrfs = AptBtrfsSnapshot(47 apt_btrfs = AptBtrfsSnapshot(
43 fstab=os.path.join(self.testdir, "data", "fstab.bug833980"))48 fstab=os.path.join(self.testdir, "data", "fstab.bug833980"))
44 # ensure our test is right49 # ensure our test is right

Subscribers

People subscribed via source and target branches