Merge lp:~jml/pkgme-service/add-download-file into lp:pkgme-service

[Jonathan Lange]

Add download_file from pkgme-devportal

I wrote a useful message here before, but the Internet ate it.

pkgme-service uses download_file, but pkgme-devportal does not. This branch
copies download_file into pkgme-service so it can be removed from devportalbinary.

[James Westby (james-w)]

review: Approve

[ISD Branch Mangler (isd-branches-mangler)]

The attempt to merge lp:~jml/pkgme-service/add-download-file into lp:pkgme-service failed. Below is the output from the failed tests.

python2.7 bootstrap.py --distribute --version 1.5.1 \
    --download-base=download-cache/dist --eggs=eggs \
    --setup-source distribute_setup.py
touch --no-create bin/buildout
./bin/buildout
Develop: '/tmp/tmpHkM6lC/.'
Updating scripts.
Updating filetemplates.
touch --no-create bin/py
./bin/py django_project/manage.py test djpkgme
Creating test database for alias 'default'...
Destroying test database for alias 'default'...

Tree is up to date at revision 44 of branch bzr+ssh://bazaar.launchpad.net/+branch/ca-download-cache
..............................E...............................
======================================================================
ERROR: test_gtk (djpkgme.tests.test_integration.TestEndToEnd)
djpkgme.tests.test_integration.TestEndToEnd.test_gtk
----------------------------------------------------------------------
_StringException: Empty attachments:
  twisted-log

celery-log: {{{
[2012-11-13 22:31:00,675: WARNING/MainProcess] -------------- <email address hidden> v2.5.0
---- **** -----
--- * *** * -- [Configuration]
-- * - **** --- . broker: djkombu.transport.DatabaseTransport:////
- ** ---------- . loader: djcelery.loaders.DjangoLoader
- ** ---------- . logfile: <email address hidden>@WARNING
- ** ---------- . concurrency: 1
- ** ---------- . events: ON
- *** --- * --- . beat: OFF
-- ******* ----
--- ***** ----- [Queues]
 -------------- . celery: exchange:celery (direct) binding:celery
[2012-11-13 22:31:00,687: WARNING/MainProcess] <email address hidden> has started.
[2012-11-13 22:31:06,258: INFO/PoolWorker-1] ['tar', '--force-local', '--no-same-owner', '--no-same-permissions', '-xf', u'/tmp/tmp7kivhZ/download/package-name4.tar.gz', '-C', '/tmp/tmp7kivhZ/working'] finished in 0.445s
[2012-11-13 22:31:07,815: INFO/PoolWorker-1] ['/tmp/eggs/pkgme_devportal-0.4.10-py2.7.egg/devportalbinary/backends/python-stub/want'] finished in 1.555s
[2012-11-13 22:31:09,035: INFO/PoolWorker-1] ['/tmp/eggs/pkgme_devportal-0.4.10-py2.7.egg/devportalbinary/backends/diff/want'] finished in 1.220s
[2012-11-13 22:31:09,937: INFO/PoolWorker-1] ['/tmp/eggs/pkgme_devportal-0.4.10-py2.7.egg/devportalbinary/backends/pdf/want'] finished in 0.900s
[2012-11-13 22:31:11,504: INFO/PoolWorker-1] ['/tmp/eggs/pkgme_devportal-0.4.10-py2.7.egg/devportalbinary/backends/deb-bin/want'] finished in 1.567s
[2012-11-13 22:31:13,235: INFO/PoolWorker-1] ['/tmp/eggs/pkgme_devportal-0.4.10-py2.7.egg/devportalbinary/backends/exe/want'] finished in 1.730s
[2012-11-13 22:31:14,021: INFO/PoolWorker-1] ['/tmp/eggs/pkgme_devportal-0.4.10-py2.7.egg/devportalbinary/backends/deb-src/want'] finished in 0.785s
[2012-11-13 22:31:15,127: INFO/PoolWorker-1] ['/tmp/eggs/pkgme_devportal-0.4.10-py2.7.egg/devportalbinary/backends/text/want'] finished in 1.106s
[2012-11-13 22:31:17,969: INFO/PoolWorker-1] ['/tmp/eggs/pkgme_devportal-0.4.10-py2.7.egg/devportalbinary/backends/image/want'] finished in 2.840s
[2012-11-13 ...

=== modified file 'src/djpkgme/osutils.py'
--- src/djpkgme/osutils.py	2012-09-20 16:11:44 +0000
+++ src/djpkgme/osutils.py	2012-11-13 15:21:25 +0000
@@ -10,8 +10,16 @@
 import os
 import shutil
 import tarfile
+import tempfile
+from urllib2 import (
+    HTTPError,
+    Request,
+    URLError,
+    urlopen,
+    )
 import zipfile
 
+from bzrlib import urlutils
 from pkgme.run_script import (
     run_subprocess,
     ScriptFailed,
@@ -130,3 +138,75 @@
     finally:
         t.close()
     return destination
+
+
+def _open_file_for_writing(url, directory=None, name=None, working_dir=None):
+    """Open a file for writing.
+
+    If neither 'directory' nor 'name' are specified then make a secure
+    tempfile with 'mkstemp'.  If directory is specified, then opens a file in
+    that directory, either with 'name' or the trailing segment of the URL.  If
+    directory is not specified and name is specified, then opens the file in a
+    temporary directory.
+
+    If 'working_dir' is specified, then all temporary files or directories
+    will be created inside that directory.
+    """
+    if directory is None:
+        if name is None:
+            fd, path = tempfile.mkstemp(dir=working_dir)
+        else:
+            return _open_file_for_writing(
+                url, tempfile.mkdtemp(dir=working_dir), name, working_dir)
+    else:
+        if name is None:
+            name = urlutils.unescape(urlutils.basename(url))
+        path = os.path.join(directory, name)
+        fd = os.open(path, os.O_RDWR | os.O_CREAT | os.O_EXCL)
+    return fd, path
+
+
+def _wrap_urlopen(request):
+    """urlopen with better error messages."""
+    url = request.get_full_url()
+    try:
+        return urlopen(request)
+    except HTTPError, original:
+        raise HTTPError(
+            original.url,
+            original.code,
+            '%s: <%s>' % (original.msg, url),
+            original.hdrs,
+            original.fp,
+            )
+    except URLError, original:
+        if original.args:
+            message = '%s: %s' % (str(original.args[0]), url)
+        else:
+            message = url
+        e = URLError(message)
+        e.original = original
+        raise e
+
+
+def download_file(url, directory=None, name=None, working_dir=None,
+                  bufsize=4 * 2 ** 10, headers=None):
+    """Download 'url' into 'directory'."""
+    # XXX: download_file is no longer used within pkgme-devportal
+    request = Request(url)
+    if headers is not None:
+        for h in headers:
+            request.add_header(h, headers[h])
+    download = _wrap_urlopen(request)
+    try:
+        fd, path = _open_file_for_writing(url, directory, name, working_dir)
+        try:
+            while True:
+                data = download.read(bufsize)
+                if not data:
+                    return path
+                os.write(fd, data)
+        finally:
+            os.close(fd)
+    finally:
+        download.close()
 
=== modified file 'src/djpkgme/tasks.py'
--- src/djpkgme/tasks.py	2012-09-18 12:34:40 +0000
+++ src/djpkgme/tasks.py	2012-11-13 15:21:25 +0000
@@ -12,9 +12,6 @@
 from oops_dictconfig import config_from_dict
 
 from devportalbinary.metadata import MetadataBackend
-from devportalbinary.database import (
-    download_file,
-    )
 from fixtures import (
     TempDir,
     )
@@ -33,6 +30,7 @@
     submit_pkgme_info,
     )
 from .osutils import (
+    download_file,
     make_tarball,
     prepare_file,
     )
 
=== modified file 'src/djpkgme/tests/test_osutils.py'
--- src/djpkgme/tests/test_osutils.py	2012-09-20 16:55:57 +0000
+++ src/djpkgme/tests/test_osutils.py	2012-11-13 15:21:25 +0000
@@ -1,19 +1,27 @@
 import errno
 import os
+from StringIO import StringIO
 import subprocess
 import tarfile
+from urllib2 import (
+    HTTPError,
+    URLError,
+    )
 import zipfile
 
 from fixtures import TempDir
 from testtools import TestCase
 from testtools.matchers import (
     DirExists,
+    Equals,
     FileContains,
     HasPermissions,
+    Matcher,
     Not,
     TarballContains,
     )
 from treeshape import (
+    CONTENT,
     from_rough_spec,
     HasFileTree,
     FileTree,
@@ -21,7 +29,9 @@
     )
 
 from ..osutils import (
+    _open_file_for_writing,
     DEFAULT_WORKING_DIRECTORY_NAME,
+    download_file,
     make_tarball,
     prepare_file,
     try_extract_with_tar_command,
@@ -223,3 +233,100 @@
         self.assertEqual(
             destination,
             os.path.join(destination_dir, '%s-2.tar.gz' % (package_name,)))
+
+
+class IsChildPath(Matcher):
+
+    def __init__(self, parent_path):
+        super(IsChildPath, self).__init__()
+        self._parent_path = parent_path
+
+    def match(self, child_path):
+        parent_segments = os.path.abspath(self._parent_path).split(os.sep)
+        child_segments = os.path.abspath(child_path).split(os.sep)
+        return Equals(parent_segments).match(
+            child_segments[:len(parent_segments)])
+
+
+class TestOpenForWriting(TestCase):
+
+    def assertIsWriteable(self, fd, path):
+        contents = self.getUniqueString()
+        os.write(fd, contents)
+        os.close(fd)
+        self.assertThat(path, FileContains(contents))
+
+    def test_default(self):
+        fd, path = _open_file_for_writing('http://example.org/foo')
+        self.addCleanup(os.unlink, path)
+        self.assertIsWriteable(fd, path)
+
+    def test_directory_provided(self):
+        temp_path = self.useFixture(TempDir()).path
+        fd, path = _open_file_for_writing(
+            'http://example.org/foo', temp_path)
+        self.assertIsWriteable(fd, path)
+        self.assertEqual(os.path.join(temp_path, 'foo'), path)
+
+    def test_name_provided(self):
+        fd, path = _open_file_for_writing(
+            'http://example.org/foo', name='bar')
+        self.addCleanup(os.unlink, path)
+        self.assertIsWriteable(fd, path)
+        self.assertEqual('bar', os.path.basename(path))
+
+    def test_directory_and_name_provided(self):
+        temp_path = self.useFixture(TempDir()).path
+        fd, path = _open_file_for_writing(
+            'http://example.org/foo', temp_path, name='bar')
+        self.assertIsWriteable(fd, path)
+        self.assertEqual(os.path.join(temp_path, 'bar'), path)
+
+    def test_working_directory_no_name(self):
+        working_dir = self.useFixture(TempDir()).path
+        fd, path = _open_file_for_writing(
+            'http://example.org/foo', working_dir=working_dir)
+        self.assertIsWriteable(fd, path)
+        self.assertThat(path, IsChildPath(working_dir))
+
+    def test_working_directory_name(self):
+        working_dir = self.useFixture(TempDir()).path
+        fd, path = _open_file_for_writing(
+            'http://example.org/foo', name='bar', working_dir=working_dir)
+        self.assertIsWriteable(fd, path)
+        self.assertEqual('bar', os.path.basename(path))
+        self.assertThat(path, IsChildPath(working_dir))
+
+
+class TestDownloadFile(TestCase):
+
+    def test_saves_file(self):
+        from djpkgme import osutils
+        contents = self.getUniqueString()
+        def urlopen(url):
+            return StringIO(contents)
+        self.patch(osutils, 'urlopen', urlopen)
+        path = download_file('http://example.org/foo')
+        self.assertThat(path, FileContains(contents))
+
+    def test_url_error_includes_url(self):
+        from djpkgme import osutils
+        def urlopen(url):
+            raise URLError("whatever")
+        self.patch(osutils, 'urlopen', urlopen)
+        url = 'http://example.org/foo'
+        e = self.assertRaises(URLError, download_file, url)
+        self.assertEqual("<urlopen error whatever: %s>" % (url,), str(e))
+
+    def test_http_error_includes_url(self):
+        from djpkgme import osutils
+        url = 'http://example.org/foo'
+        # HTTPError must get a valid fp, otherwise it won't store the URL.
+        tree = self.useFixture(FileTree({'thing.txt': {CONTENT: 'whatever'}}))
+        fp = open(os.path.join(tree.path, 'thing.txt'), 'r')
+        def urlopen(url):
+            raise HTTPError(url, 404, 'whatever dude', {}, fp)
+        self.patch(osutils, 'urlopen', urlopen)
+        e = self.assertRaises(HTTPError, download_file, url)
+        self.assertEqual(
+            "HTTP Error 404: whatever dude: <%s>" % (url,), str(e))