Merge lp:~vorlon/ddeb-retriever/python3 into lp:ddeb-retriever

[Steve Langasek]

Since germanium is running 16.04 already, we should use python3 rather than python2, so move this over in one go. Also, use urllib.request.urlopen() and getcode to avoid publishing error pages as ddebs.

[Brian Murray (brian-murray)]

Is there a reason the shebang in ddeb_retriever.py wasn't changed to python3 also?

I also have one nitpicky comment which you can find in-line.

review: Needs Information

[Steve Langasek (vorlon)]

didn't notice ddeb_retriever.py had a shebang since it wasn't executable. thanks, fixed. also fixed the whitespace consistency issue.

[lp:~vorlon/ddeb-retriever/python3]

175. By Steve Langasek on 2017-05-23

also fix shebang in ddeb_retriever.py (though unused)

176. By Steve Langasek on 2017-05-23

spacing consistency

[Brian Murray (brian-murray)]

review: Approve

[Colin Watson (cjwatson)]

review: Approve

[lp:~vorlon/ddeb-retriever/python3]

177. By Steve Langasek on 2017-05-23

better python3ing of hash sorting

178. By Steve Langasek on 2017-05-23

with gzip.open

179. By Steve Langasek on 2017-07-10

Write to a temporary file, to avoid truncated downloads on interrupt

[Brian Murray (brian-murray)]

I approve the new changes to this branch.

review: Approve

=== modified file 'archive_tools.py'
--- archive_tools.py	2017-04-22 23:07:59 +0000
+++ archive_tools.py	2017-07-10 17:23:38 +0000
@@ -11,7 +11,6 @@
 '''
 
 import apt_pkg
-import urllib2
 import urllib
 import os
 import gzip
@@ -25,7 +24,7 @@
 import subprocess
 import glob
 import logging
-from cStringIO import StringIO
+from io import StringIO
 
 apt_pkg.init_system()
 
@@ -42,13 +41,13 @@
     if url.startswith('http://') or url.startswith('ftp://') or \
             url.startswith('https://') or url.startswith('file://'):
         data = tempfile.NamedTemporaryFile()
-        src = urllib2.urlopen(url, timeout=60)
+        src = urllib.request.urlopen(url, timeout=60)
         data.write(src.read())
         src.close()
         data.flush()
         data.seek(0)
     else:
-        data = open(url)
+        data = open(url, mode='rb')
 
     tagfile = tempfile.TemporaryFile()
     if url.endswith('.gz'):
@@ -158,8 +157,7 @@
         if cmp(max.get(parser.section[group_by], ('', ''))[0], parser.section[sort_key]) < 0:
             max[parser.section[group_by]] = (parser.section[sort_key], str(parser.section))
 
-    ks = max.keys()
-    ks.sort()
+    ks = sorted(max)
     first = True
     for k in ks:
         if first:
@@ -169,7 +167,7 @@
         outfile.write(max[k][1])
 
 
-def install_pool(archive_root, file_url, component, source):
+def install_pool(archive_root, file_path, component, source):
     '''Install given file into the pool, with the given component and source
     package name.
 
@@ -181,13 +179,14 @@
         hash = source[0]
 
     destdir = os.path.join(archive_root, 'pool', component, hash, source)
-    destfile = os.path.join(destdir, os.path.basename(file_url))
+    destfile = os.path.join(destdir, os.path.basename(file_path))
     if os.path.exists(destfile):
         return False
     if not os.path.isdir(destdir):
         os.makedirs(destdir)
 
-    urllib.urlretrieve(file_url, destfile)
+    with open(file_path, 'rb') as infile, open(destfile, 'wb') as outfile:
+        outfile.write(infile.read())
 
     return True
 
@@ -213,8 +212,8 @@
         raise NotImplementedError('create_index() does not support source packages with a reference_archive')
 
     # create configuration file
-    conf = tempfile.NamedTemporaryFile()
-    print >> conf, '''Dir {
+    conf = tempfile.NamedTemporaryFile(mode='w+')
+    print('''Dir {
         ArchiveDir ".";
         CacheDir ".cache";
         FileListDir "./lists";
@@ -237,7 +236,7 @@
 Tree "dists/%s" {
         Sections "%s";
         Architectures "%s";
-}''' % (ext, pocket, ' '.join(components), ' '.join(archs))
+}''' % (ext, pocket, ' '.join(components), ' '.join(archs)), file=conf)
     conf.flush()
 
     old_cwd = os.getcwd()
@@ -307,9 +306,8 @@
             stdout=subprocess.PIPE, stderr=stderr)
         out = apt_ftparchive.communicate()[0]
         assert apt_ftparchive.returncode == 0
-        r = open('Release', 'w')
-        r.write(out)
-        r.close()
+        with open('Release', 'wb') as r:
+            r.write(out)
         if gpg_sign:
             try:
                 os.unlink('Release.gpg')
@@ -330,18 +328,21 @@
 def _packages_files(path, files):
     '''Add all files from given Packages path to the 'files' set.'''
 
-    parser = apt_pkg.TagFile(open(path))
+    with open(path) as f:
+        parser = apt_pkg.TagFile(f)
     while parser.step():
         f = parser.section['Filename']
         if f.startswith('./'):
             f = f[2:]
         files.add(f)
+    parser.close()
 
 
 def _sources_files(path, files):
     '''Add all files from given Sources path to the 'files' set.'''
 
-    parser = apt_pkg.TagFile(open(path))
+    with open(path) as f:
+        parser = apt_pkg.TagFile(f)
     while parser.step():
         d = parser.section.get('Directory')
         for l in parser.section['Files'].splitlines():
@@ -351,6 +352,7 @@
                 files.add(os.path.join(d, fields[2]))
             else:
                 files.add(fields[2])
+    parser.close()
 
 
 def archive_cleanup(archive_root, keep_days=0):
@@ -428,7 +430,7 @@
         z = gzip.open(dest + '.new', 'w', 9)
     elif dest.endswith('.bz2'):
         z = bz2.BZ2File(dest + '.new', 'w')
-    f = open(src)
+    f = open(src, 'rb')
     while True:
         block = f.read(10240)
         if not block:
@@ -454,7 +456,8 @@
     for root, dirs, files in os.walk(distroot):
         if 'Packages' in files:
             path = os.path.join(root, 'Packages')
-            debcontrol_dominate(open(path), open(path + '.new', 'w'))
+            with open(path) as old, open(path + '.new', 'w') as new:
+                debcontrol_dominate(old, new)
 
             # regenerate compressed files
             if 'Packages.gz' in files:
@@ -468,7 +471,8 @@
 
         if 'Sources' in files:
             path = os.path.join(root, 'Sources')
-            debcontrol_dominate(open(path), open(path + '.new', 'w'))
+            with open(path) as old, open(path + '.new', 'w') as new:
+                debcontrol_dominate(old, new)
 
             # regenerate compressed files
             if 'Sources.gz' in files:
@@ -504,7 +508,8 @@
 
         d = tempfile.mkdtemp()
         os.mkdir(os.path.join(d, 'DEBIAN'))
-        open(os.path.join(d, 'DEBIAN', 'control'), 'w').write('''Package: %s
+        with open(os.path.join(d, 'DEBIAN', 'control'), 'w') as file:
+            file.write('''Package: %s
 Version: %s
 Priority: optional
 Section: devel
@@ -534,10 +539,9 @@
             d = os.path.join(self.workdir, repo, 'dists', pocket, component, 'binary-' + arch)
             if not os.path.isdir(d):
                 os.makedirs(d)
-            f = gzip.GzipFile(os.path.join(d, 'Packages.gz'), mode='w')
-            for (name, version) in packages:
-                f.write('Package: %s\nVersion: %s\n\n' % (name, version))
-            f.close()
+            with gzip.open(os.path.join(d, 'Packages.gz'), mode='wt') as f:
+                for (name, version) in packages:
+                    f.write('Package: %s\nVersion: %s\n\n' % (name, version))
 
 
 #
@@ -555,8 +559,8 @@
 
         self.tmpfiles = set(os.listdir('/tmp'))
 
-        self.sources = tempfile.NamedTemporaryFile()
-        print >> self.sources, '''Package: aalib
+        self.sources = tempfile.NamedTemporaryFile(mode='w')
+        print('''Package: aalib
 Binary: libaa-bin, libaa1-dev, libaa1
 Version: 1.4p5-30
 Directory: pool/main/a/aalib
@@ -566,11 +570,11 @@
 
 Package: abiword
 Binary: abiword-plugins, abiword-plugins-gnome, abiword-help, abiword, abiword-common, abiword-gnome
-Version: 2.4.4-0ubuntu5'''
+Version: 2.4.4-0ubuntu5''', file=self.sources)
         self.sources.flush()
 
-        self.packages = tempfile.NamedTemporaryFile()
-        print >> self.packages, '''Package: abiword
+        self.packages = tempfile.NamedTemporaryFile(mode='w')
+        print('''Package: abiword
 Architecture: amd64
 Version: 2.4.4-0ubuntu5
 Description: Short
@@ -579,7 +583,7 @@
 
 Package: abiword-common
 Source: abiword
-Version: 2.4.4-0ubuntu5'''
+Version: 2.4.4-0ubuntu5''', file=self.packages)
         self.packages.flush()
 
     def tearDown(self):
@@ -710,8 +714,7 @@
     '''Return string with space-separated sorted list of keys of a
     dictionary.'''
 
-    k = map.keys()
-    k.sort()
+    k = sorted(map)
     return ' '.join(k)
 
 
@@ -749,22 +752,26 @@
         os.chdir(d)
 
         os.makedirs(os.path.join(package, 'debian'))
-        open(os.path.join(package, 'README'), 'w').write('hello\n')
-        open(os.path.join(package, 'debian', 'control'), 'w').write('''\
+        with open(os.path.join(package, 'README'), 'w') as f:
+            f.write('hello\n')
+        with open(os.path.join(package, 'debian', 'control'), 'w') as f:
+            f.write('''\
 Source: %s
 Maintainer: Joe <[e-mail]>
 
 Package: %s
 Architecture: all
 ''' % (package, package))
-        open(os.path.join(package, 'debian', 'changelog'), 'w').write('''\
+        with open(os.path.join(package, 'debian', 'changelog'), 'w') as f:
+            f.write('''\
 %s (%s) dummy; urgency=low
 
   * Dummy package.
 
  -- Joe <[e-mail]>  Fri, 22 Sep 2006 13:07:40 +0200
 ''' % (package, version))
-        open(os.path.join(package, 'debian', 'rules'), 'w')
+        with open(os.path.join(package, 'debian', 'rules'), 'w') as f:
+            pass
 
         if not native:
             upstreamver = '-'.join(version.split('-')[:-1])
@@ -811,14 +818,14 @@
     def test_install_pool(self):
         '''install_pool()'''
 
-        f = tempfile.NamedTemporaryFile()
-        print >> f, 'hello'
+        f = tempfile.NamedTemporaryFile(mode='w')
+        print('hello', file=f)
         f.flush()
 
         self.assertTrue(install_pool(self.archivedir, f.name, 'main', 'src1'))
         self.assertTrue(install_pool(self.archivedir, f.name, 'main', 'src2'))
         self.assertTrue(install_pool(self.archivedir, f.name, 'main', 'libsrc'))
-        self.assertTrue(install_pool(self.archivedir, 'file://' + f.name, 'universe', 'src'))
+        self.assertTrue(install_pool(self.archivedir, f.name, 'universe', 'src'))
         self.assertFalse(install_pool(self.archivedir, f.name, 'universe', 'src'))
 
         self.assertEqual(_ls(os.path.join(self.archivedir, 'pool')), 'main universe')
@@ -827,9 +834,9 @@
         self.assertEqual(_ls(os.path.join(self.archivedir, 'pool', 'main', 'libs')), 'libsrc')
         self.assertEqual(_ls(os.path.join(self.archivedir, 'pool', 'main', 'libs', 'libsrc')),
                          os.path.basename(f.name))
-        self.assertEqual(open(os.path.join(
-            self.archivedir, 'pool', 'main', 'libs', 'libsrc', os.path.basename(f.name))).read(),
-            'hello\n')
+        with open(os.path.join(self.archivedir, 'pool', 'main', 'libs',
+                               'libsrc', os.path.basename(f.name))) as f:
+            self.assertEqual(f.read(), 'hello\n')
 
         f.close()
 
@@ -876,20 +883,21 @@
             self.archivedir, 'dists', 'edgy', 'main', 'binary-powerpc', 'Packages.xz'))
         self.assertEqual(map, xzmap)
 
-        self.assertEqual(open(os.path.join(
-            self.archivedir, 'dists', 'edgy', 'main', 'binary-i386', 'Packages')).read(),
-            '')
-        self.assertEqual(open(os.path.join(
-            self.archivedir, 'dists', 'edgy', 'universe', 'binary-powerpc', 'Packages')).read(),
-            '')
-        self.assertEqual(open(os.path.join(
-            self.archivedir, 'dists', 'edgy', 'main', 'binary-amd64', 'Packages')).read(),
-            '')
-        self.assertEqual(open(os.path.join(
-            self.archivedir, 'dists', 'edgy', 'universe', 'binary-amd64', 'Packages')).read(),
-            '')
+        with open(os.path.join(self.archivedir, 'dists', 'edgy', 'main',
+                               'binary-i386', 'Packages')) as f:
+            self.assertEqual(f.read(), '')
+        with open(os.path.join(self.archivedir, 'dists', 'edgy', 'universe',
+                               'binary-powerpc', 'Packages')) as f:
+            self.assertEqual(f.read(), '')
+        with open(os.path.join(self.archivedir, 'dists', 'edgy', 'main',
+                               'binary-amd64', 'Packages')) as f:
+            self.assertEqual(f.read(), '')
+        with open(os.path.join(self.archivedir, 'dists', 'edgy', 'universe',
+                               'binary-amd64', 'Packages')) as f:
+            self.assertEqual(f.read(), '')
 
-        rel = open(os.path.join(self.archivedir, 'dists', 'edgy', 'Release')).read()
+        with open(os.path.join(self.archivedir, 'dists', 'edgy', 'Release')) as f:
+            rel = f.read()
         self.assertIn('\nSuite: edgy\n', rel)
         self.assertIn('Architectures: amd64 i386 powerpc\n', rel)
         self.assertIn('\nComponents: main restricted universe\n', rel)
@@ -924,8 +932,9 @@
         self.assertEqual(_ls(os.path.join(
             self.archivedir, 'dists', 'edgy', 'main', 'source')),
             'Sources Sources.gz Sources.xz')
-        self.assertEqual(open(os.path.join(
-            self.archivedir, 'dists', 'edgy', 'main', 'binary-i386', 'Packages')).read(), '')
+        with open(os.path.join(self.archivedir, 'dists', 'edgy', 'main',
+                               'binary-i386', 'Packages')) as f:
+            self.assertEqual(f.read(), '')
         map = debcontrol_map(os.path.join(
             self.archivedir, 'dists', 'edgy', 'main', 'source', 'Sources'))
 
@@ -987,9 +996,9 @@
             self.archivedir, 'dists', 'edgy', 'universe', 'binary-i386', 'Packages.xz'))
         self.assertEqual(map, xzmap)
 
-        self.assertEqual(open(os.path.join(self.archivedir, 'dists', 'edgy',
-                                           'main', 'binary-i386', 'Packages')).read(),
-                         '')
+        with open(os.path.join(self.archivedir, 'dists', 'edgy',
+                               'main', 'binary-i386', 'Packages')) as f:
+            self.assertEqual(f.read(),'')
 
         self.assertGreater(os.path.getsize(
             os.path.join(self.archivedir, 'dists', 'edgy', 'Release')), 0)
@@ -998,11 +1007,11 @@
         '''performance of create_indexes()'''
 
         # install some test debs
-        for i in xrange(1000):
+        for i in range(1000):
             install_pool(self.archivedir, TestArchive.create_deb(
                 str(i), '1-1', 'i386', self.testfiles_dir), 'main', str(i))
 
-        print '[setup done]',
+        print('[setup done]')
         sys.stdout.flush()
 
         # call create_indexes()
@@ -1011,8 +1020,8 @@
         t2 = time.time()
 
         pkg = os.path.join(self.archivedir, 'dists', 'edgy', 'main', 'binary-i386', 'Packages')
-        print 'Packages:', os.path.getsize(pkg), 'Packages.gz:', os.path.getsize(pkg + '.gz'),
-        print '%.1f seconds ' % (t2 - t1),
+        print('Packages:', os.path.getsize(pkg), 'Packages.gz:', os.path.getsize(pkg + '.gz'))
+        print('%.1f seconds ' % (t2 - t1))
         sys.stdout.flush()
 
     def test_create_indexes_refarchive(self):
@@ -1088,25 +1097,25 @@
             map = debcontrol_map(os.path.join(
                 self.archivedir, 'dists', 'dapper', 'universe', 'binary-i386', 'Packages.gz'),
                 'Version')
-            self.assertEqual(map.keys(), ['1-1'])
+            self.assertEqual(list(map.keys()), ['1-1'])
             self.assertEqual(map['1-1']['Package'], 'foo-dbgsym')
 
             map = debcontrol_map(os.path.join(
                 self.archivedir, 'dists', 'dapper', 'main', 'binary-powerpc', 'Packages.gz'),
                 'Version')
-            self.assertEqual(map.keys(), ['2-2'])
+            self.assertEqual(list(map.keys()), ['2-2'])
             self.assertEqual(map['2-2']['Package'], 'bar1-dbgsym')
 
             map = debcontrol_map(os.path.join(
                 self.archivedir, 'dists', 'edgy', 'universe', 'binary-i386', 'Packages.gz'),
                 'Version')
-            self.assertEqual(map.keys(), ['1-2'])
+            self.assertEqual(list(map.keys()), ['1-2'])
             self.assertEqual(map['1-2']['Package'], 'foo-dbgsym')
 
             map = debcontrol_map(os.path.join(
                 self.archivedir, 'dists', 'edgy', 'main', 'binary-powerpc', 'Packages.gz'),
                 'Version')
-            self.assertEqual(map.keys(), ['2-2ubuntu1'])
+            self.assertEqual(list(map.keys()), ['2-2ubuntu1'])
             self.assertEqual(map['2-2ubuntu1']['Package'], 'bar1-dbgsym')
 
         finally:
@@ -1115,7 +1124,7 @@
     def test_debcontrol_dominate(self):
         '''debcontrol_dominate()'''
 
-        i = tempfile.NamedTemporaryFile()
+        i = tempfile.NamedTemporaryFile(mode='w')
         i.write('''Package: foo
 Version: 1-0
 Description: oldest foo
@@ -1202,11 +1211,16 @@
     def test_archive_cleanup_customlayout(self):
         '''archive_cleanup() with a non-standard archive structure'''
 
-        open(os.path.join(self.archivedir, 'foo_1_i386.deb'), 'w').write('foo_1')
-        open(os.path.join(self.archivedir, 'foo_2_i386.deb'), 'w').write('foo_2')
-        open(os.path.join(self.archivedir, 'bar_1_i386.deb'), 'w').write('bar_1')
-        open(os.path.join(self.archivedir, 'cruft_3_i386.deb'), 'w').write('cruft')
-        open(os.path.join(self.archivedir, 'Packages'), 'w').write('''Package: foo
+        with open(os.path.join(self.archivedir, 'foo_1_i386.deb'), 'w') as f:
+            f.write('foo_1')
+        with open(os.path.join(self.archivedir, 'foo_2_i386.deb'), 'w') as f:
+            f.write('foo_2')
+        with open(os.path.join(self.archivedir, 'bar_1_i386.deb'), 'w') as f:
+            f.write('bar_1')
+        with open(os.path.join(self.archivedir, 'cruft_3_i386.deb'), 'w') as f:
+            f.write('cruft')
+        with open(os.path.join(self.archivedir, 'Packages'), 'w') as f:
+            f.write('''Package: foo
 Version: 2
 Filename: foo_2_i386.deb
 
@@ -1280,16 +1294,20 @@
         # check that Packages.gz and Sources.gz are re-created properly
         s = os.path.join(self.archivedir, 'dists', 'dapper', 'main',
                          'source', 'Sources')
-        self.assertEqual(open(s).read(), gzip.open(s + '.gz').read())
+        with open(s, mode='rb') as f:
+            self.assertEqual(f.read(), gzip.open(s + '.gz').read())
         s = os.path.join(self.archivedir, 'dists', 'dapper', 'main',
                          'binary-i386', 'Packages')
-        self.assertEqual(open(s).read(), gzip.open(s + '.gz').read())
+        with open(s, mode='rb') as f:
+            self.assertEqual(f.read(), gzip.open(s + '.gz').read())
         s = os.path.join(self.archivedir, 'dists', 'edgy', 'universe',
                          'source', 'Sources')
-        self.assertEqual(open(s).read(), gzip.open(s + '.gz').read())
+        with open(s, mode='rb') as f:
+            self.assertEqual(f.read(), gzip.open(s + '.gz').read())
         s = os.path.join(self.archivedir, 'dists', 'edgy', 'universe',
                          'binary-i386', 'Packages')
-        self.assertEqual(open(s).read(), gzip.open(s + '.gz').read())
+        with open(s, mode='rb') as f:
+            self.assertEqual(f.read(), gzip.open(s + '.gz').read())
 
     def test_archive_dominate_flat(self):
         '''archive_dominate() with a flat tree'''

=== modified file 'ddeb-retriever'
--- ddeb-retriever	2014-07-17 07:40:31 +0000
+++ ddeb-retriever	2017-07-10 17:23:38 +0000
@@ -1,4 +1,4 @@
-#!/usr/bin/python
+#!/usr/bin/python3
 import ddeb_retriever
 
 ddeb_retriever.main()

=== modified file 'ddeb_retriever.py'
--- ddeb_retriever.py	2017-04-22 23:07:59 +0000
+++ ddeb_retriever.py	2017-07-10 17:23:38 +0000
@@ -1,5 +1,5 @@
-#!/usr/bin/python
-import os.path
+#!/usr/bin/python3
+import os
 import logging
 import urllib
 import argparse
@@ -76,7 +76,7 @@
     Return True if it was installed successfully or already present,
     otherwise False.
     '''
-    ddeb = urllib.unquote(os.path.basename(url))
+    ddeb = urllib.parse.unquote(os.path.basename(url))
     try:
         (dbgsymname, version, arch) = ddeb.split('_')
         assert arch.endswith('.ddeb')
@@ -107,7 +107,15 @@
             os.makedirs(destdir)
         except OSError:
             pass
-        urllib.urlretrieve(url, dest)
+        with urllib.request.urlopen(url) as req:
+            if req.getcode() != 200:
+                logging.error('URL %s failed with code %u', req.geturl(), code)
+                return False
+            response = req.read()
+        with open(dest + '.new', 'wb') as handle:
+            handle.write(response)
+        os.rename(dest + '.new', dest)
+
         logging.debug('Downloaded %s into %s', ddeb, os.path.dirname(dest))
 
     return True