Merge lp:~jelmer/bzr-builddeb/fetch-tarball into lp:bzr-builddeb

[Jelmer Vernooij]

Another refactoring branch. The main change this makes is to rename get_specific_version to fetch_tarball and to make the new method return the path to the tarball (instead of not specifying the behaviour).

[lp:~jelmer/bzr-builddeb/fetch-tarball]

501. By Jelmer Vernooij on 2011-01-24

Tests.

[James Westby (james-w)]

review: Approve

=== modified file 'cmds.py'
--- cmds.py	2011-01-24 03:08:28 +0000
+++ cmds.py	2011-01-24 03:08:28 +0000
@@ -563,7 +563,7 @@
         tarball_filename = os.path.join(orig_dir, dest_name)
         upstream = UpstreamBranchSource(upstream_branch,
                 {version: upstream_revision})
-        upstream.get_specific_version(package, version, orig_dir)
+        upstream.fetch_tarball(package, version, orig_dir)
         return tarball_filename
 
     def _fetch_tarball(self, package, version, orig_dir, location, v3):

=== modified file 'merge_upstream.py'
--- merge_upstream.py	2011-01-17 22:18:44 +0000
+++ merge_upstream.py	2011-01-24 03:08:28 +0000
@@ -26,7 +26,6 @@
 #    Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
 #
 
-import itertools
 import subprocess
 
 try:

=== modified file 'tests/test_upstream.py'
--- tests/test_upstream.py	2010-03-04 14:26:13 +0000
+++ tests/test_upstream.py	2011-01-24 03:08:28 +0000
@@ -20,18 +20,24 @@
 # We have a bit of a problem with testing the actual uscan etc. integration,
 # so just mock them.
 
+"""Tests for the upstream module."""
+
+
+import os
+
 from bzrlib.tests import (
-        TestCase,
-        TestCaseWithTransport,
-        )
+    TestCase,
+    TestCaseWithTransport,
+    )
 from bzrlib.plugins.builddeb.errors import (
-        PackageVersionNotPresent,
-        )
+    PackageVersionNotPresent,
+    )
 from bzrlib.plugins.builddeb.upstream import (
-        AptSource,
-        StackedUpstreamSource,
-        UScanSource,
-        )
+    AptSource,
+    StackedUpstreamSource,
+    UpstreamBranchSource,
+    UScanSource,
+    )
 
 
 class MockSources(object):
@@ -103,7 +109,7 @@
         apt_pkg = MockAptPkg(sources)
         src = AptSource()
         src._run_apt_source = caller.call
-        self.assertRaises(PackageVersionNotPresent, src.get_specific_version,
+        self.assertRaises(PackageVersionNotPresent, src.fetch_tarball,
             "apackage", "0.2", "target", _apt_pkg=apt_pkg)
         self.assertEqual(1, apt_pkg.init_called_times)
         self.assertEqual(1, apt_pkg.get_pkg_source_records_called_times)
@@ -118,7 +124,7 @@
         apt_pkg = MockAptPkg(sources)
         src = AptSource()
         src._run_apt_source = caller.call
-        self.assertRaises(PackageVersionNotPresent, src.get_specific_version,
+        self.assertRaises(PackageVersionNotPresent, src.fetch_tarball,
             "apackage", "0.2", "target", _apt_pkg=apt_pkg)
         self.assertEqual(1, apt_pkg.init_called_times)
         self.assertEqual(1, apt_pkg.get_pkg_source_records_called_times)
@@ -133,7 +139,7 @@
         apt_pkg = MockAptPkg(sources)
         src = AptSource()
         src._run_apt_source = caller.call
-        src.get_specific_version("apackage", "0.2", "target", 
+        src.fetch_tarball("apackage", "0.2", "target", 
             _apt_pkg=apt_pkg)
         self.assertEqual(1, apt_pkg.init_called_times)
         self.assertEqual(1, apt_pkg.get_pkg_source_records_called_times)
@@ -152,7 +158,7 @@
         apt_pkg = MockAptPkg(sources)
         src = AptSource()
         src._run_apt_source = caller.call
-        self.assertRaises(PackageVersionNotPresent, src.get_specific_version,
+        self.assertRaises(PackageVersionNotPresent, src.fetch_tarball,
             "apackage", "0.2", "target", 
             _apt_pkg=apt_pkg)
         self.assertEqual(1, apt_pkg.init_called_times)
@@ -173,7 +179,7 @@
         self._succeed = succeed
         self._specific_versions = []
 
-    def get_specific_version(self, package, version, target_dir):
+    def fetch_tarball(self, package, version, target_dir):
         self._specific_versions.append((package, version, target_dir))
         if not self._succeed:
             raise PackageVersionNotPresent(package, version, self)
@@ -189,7 +195,7 @@
         b = RecordingSource(True)
         c = RecordingSource(False)
         stack = StackedUpstreamSource([a, b, c])
-        stack.get_specific_version("mypkg", "1.0", "bla")
+        stack.fetch_tarball("mypkg", "1.0", "bla")
         self.assertEquals([("mypkg", "1.0", "bla")], b._specific_versions)
         self.assertEquals([("mypkg", "1.0", "bla")], a._specific_versions)
         self.assertEquals([], c._specific_versions)
@@ -205,7 +211,7 @@
         b = RecordingSource(False)
         stack = StackedUpstreamSource([a, b])
         self.assertRaises(PackageVersionNotPresent, 
-                stack.get_specific_version, "pkg", "1.0", "bla")
+                stack.fetch_tarball, "pkg", "1.0", "bla")
         self.assertEquals([("pkg", "1.0", "bla")], b._specific_versions)
         self.assertEquals([("pkg", "1.0", "bla")], a._specific_versions)
 
@@ -233,3 +239,26 @@
         self.assertEquals(None, src._export_watchfile())
         self.tree.smart_add(['debian/watch'])
         self.assertTrue(src._export_watchfile() is not None)
+
+
+class UpstreamBranchSourceTests(TestCaseWithTransport):
+
+    def setUp(self):
+        super(UpstreamBranchSourceTests, self).setUp()
+        self.tree = self.make_branch_and_tree('.')
+
+    def test_fetch_tarball(self):
+        self.tree.commit("msg")
+        self.tree.branch.tags.set_tag("1.0", self.tree.branch.last_revision())
+        source = UpstreamBranchSource(self.tree.branch,
+            {"1.0": self.tree.branch.last_revision()})
+        os.mkdir("mydir")
+        self.assertEquals("mydir/foo_1.0.orig.tar.gz",
+            source.fetch_tarball("foo", "1.0", "mydir"))
+        self.failUnlessExists("mydir/foo_1.0.orig.tar.gz")
+
+    def test_fetch_tarball_not_found(self):
+        source = UpstreamBranchSource(self.tree.branch)
+        self.tree.commit("msg")
+        self.assertRaises(PackageVersionNotPresent,
+            source.fetch_tarball, "foo", "1.0", "mydir")

=== modified file 'upstream.py'
--- upstream.py	2010-05-28 20:45:50 +0000
+++ upstream.py	2011-01-24 03:08:28 +0000
@@ -62,14 +62,15 @@
         """
         raise NotImplementedError(self.get_latest_version)
 
-    def get_specific_version(self, package, version, target_dir):
+    def fetch_tarball(self, package, version, target_dir):
         """Fetch the source tarball for a particular version.
 
         :param package: Name of the package
         :param version: Version string of the version to fetch
         :param target_dir: Directory in which to store the tarball
+        :return: Path of the fetched tarball
         """
-        raise NotImplementedError(self.get_specific_version)
+        raise NotImplementedError(self.fetch_tarball)
 
     def _tarball_path(self, package, version, target_dir, format=None):
         return os.path.join(target_dir, tarball_name(package, version,
@@ -83,7 +84,7 @@
         self.branch = branch
         self.tree = tree
 
-    def get_specific_version(self, package, version, target_dir):
+    def fetch_tarball(self, package, version, target_dir):
         db = DistributionBranch(self.branch, None, tree=self.tree)
         if not db.has_upstream_version_in_packaging_branch(version):
             raise PackageVersionNotPresent(package, version, self)
@@ -102,12 +103,13 @@
             raise PackageVersionNotPresent(package, version, self)
         except PerFileTimestampsNotSupported:
             raise PackageVersionNotPresent(package, version, self)
+        return target_filename
 
 
 class AptSource(UpstreamSource):
     """Upstream source that uses apt-source."""
 
-    def get_specific_version(self, package, upstream_version, target_dir, 
+    def fetch_tarball(self, package, upstream_version, target_dir, 
             _apt_pkg=None):
         if _apt_pkg is None:
             import apt_pkg
@@ -138,7 +140,7 @@
             version = get_fn(sources, 'version', 'Version')
             if upstream_version == Version(version).upstream_version:
                 if self._run_apt_source(package, version, target_dir):
-                    return
+                    return self._tarball_path(package, version, target_dir)
                 break
         note("apt could not find the needed tarball.")
         raise PackageVersionNotPresent(package, upstream_version, self)
@@ -158,7 +160,7 @@
 
 class UpstreamBranchSource(UpstreamSource):
     """Upstream source that uses the upstream branch.
-    
+
     :ivar upstream_branch: Branch with upstream sources
     :ivar upstream_version_map: Map from version strings to revids
     """
@@ -179,7 +181,7 @@
                 revspec).as_revision_id(self.upstream_branch)
         return None
 
-    def get_specific_version(self, package, version, target_dir):
+    def fetch_tarball(self, package, version, target_dir):
         self.upstream_branch.lock_read()
         try:
             revid = self._get_revision_id(version)
@@ -193,6 +195,11 @@
             export(rev_tree, target_filename, 'tgz', tarball_base)
         finally:
             self.upstream_branch.unlock()
+        return target_filename
+
+    def __repr__(self):
+        return "<%s for %r>" % (self.__class__.__name__,
+            self.upstream_branch.base)
 
 
 class GetOrigSourceSource(UpstreamSource):
@@ -216,11 +223,11 @@
             if os.path.exists(fetched_tarball):
                 repack_tarball(fetched_tarball, desired_tarball_name,
                                target_dir=target_dir, force_gz=False)
-                return True
+                return fetched_tarball
         note("get-orig-source did not create %s", desired_tarball_name)
-        return False
+        return None
 
-    def get_specific_version(self, package, version, target_dir):
+    def fetch_tarball(self, package, version, target_dir):
         if self.larstiq:
             rules_name = 'rules'
         else:
@@ -238,10 +245,11 @@
                     os.mkdir(export_dir)
                     export_dir = os.path.join(export_dir, "debian")
                 export(self.tree, export_dir, format="dir")
-                if not self._get_orig_source(base_export_dir,
-                        desired_tarball_names, target_dir):
+                tarball_path = self._get_orig_source(base_export_dir,
+                        desired_tarball_names, target_dir)
+                if tarball_path is None:
                     raise PackageVersionNotPresent(package, version, self)
-                return
+                return tarball_path
             finally:
                 shutil.rmtree(tmpdir)
         note("No debian/rules file to try and use for a get-orig-source rule")
@@ -285,7 +293,7 @@
             tmp.close()
         return tempfilename
 
-    def get_specific_version(self, package, version, target_dir):
+    def fetch_tarball(self, package, version, target_dir):
         tempfilename = self._export_watchfile()
         if tempfilename is None:
             raise PackageVersionNotPresent(package, version, self)
@@ -295,9 +303,7 @@
                 raise PackageVersionNotPresent(package, version, self)
         finally:
             os.unlink(tempfilename)
-
-    def get_latest_version(self, package, version, target_dir):
-        pass
+        return self._tarball_path(package, version, target_dir)
 
 
 class SelfSplitSource(UpstreamSource):
@@ -320,16 +326,17 @@
         finally:
             shutil.rmtree(tmpdir)
 
-    def get_specific_version(self, package, version, target_dir):
+    def fetch_tarball(self, package, version, target_dir):
         note("Using the current branch without the 'debian' directory "
                 "to create the tarball")
-        self._split(package, version, 
-                    self._tarball_path(package, version, target_dir))
+        tarball_path = self._tarball_path(package, version, target_dir)
+        self._split(package, version, tarball_path)
+        return tarball_path
 
 
 class StackedUpstreamSource(UpstreamSource):
     """An upstream source that checks a list of other upstream sources.
-    
+
     The first source that can provide a tarball, wins. 
     """
 
@@ -339,10 +346,10 @@
     def __repr__(self):
         return "%s(%r)" % (self.__class__.__name__, self._sources)
 
-    def get_specific_version(self, package, version, target_dir):
+    def fetch_tarball(self, package, version, target_dir):
         for source in self._sources:
             try:
-                return source.get_specific_version(package, version, target_dir)
+                return source.fetch_tarball(package, version, target_dir)
             except PackageVersionNotPresent:
                 pass
         raise PackageVersionNotPresent(package, version, self)
@@ -410,13 +417,13 @@
             if not os.path.exists(self.store_dir):
                 os.makedirs(self.store_dir)
             try:
-                self.source.get_specific_version(self.package,
+                self.source.fetch_tarball(self.package,
                     self.version.upstream_version, self.store_dir)
             except PackageVersionNotPresent:
                 raise MissingUpstreamTarball(self._tarball_names()[0])
         else:
-             note("Using the upstream tarball that is present in "
-                     "%s" % self.store_dir)
+             note("Using the upstream tarball that is present in %s" %
+                 self.store_dir)
         path = self.provide_from_store_dir(target_dir)
         assert path is not None
         return path