Merge lp:~james-w/pkgme/careful-tar-exclude into lp:pkgme

[James Westby]

Be more careful in excluding/transforming when making a tarfile.

Only exclude ./debian in case the application contains a 'something/debian'
dir.

Don't transform symlink targets. If they are absolute they shouldn't be
changed. If they are relative and contain the current dir name then they
are broken anyway.

Hi,

This fixes the two (I think) causes of the "unrepresentable changes to source"
errors we are seeing. Both are mistakes in the tar command we use to create
the tarball that change the contents.

Thanks,

James

[Michael Vogt (mvo)]

This is a drive-by review. The fix and the tests look fine!

The only remark is that it took me some seconds to figure out what:

  def get_all_paths(path):
    """Get all the paths that are needed for `path`.

is doing. Maybe something like:
"""Get all the paths that are needed to build/mkdir `path`.
"

review: Approve

[Jonathan Lange (jml)]

On 26 September 2012 07:53, Michael Vogt <email address hidden> wrote:
> Review: Approve
>
> This is a drive-by review. The fix and the tests look fine!
>
>
> The only remark is that it took me some seconds to figure out what:
>
> def get_all_paths(path):
> """Get all the paths that are needed for `path`.
>
> is doing. Maybe something like:

Renaming to get_all_parents would probably clarify.

Other than that, wonderful, thanks!

jml

[lp:~james-w/pkgme/careful-tar-exclude]

151. By James Westby on 2012-09-26

Rename get_all_paths to try and make the intent clearer.

=== modified file 'pkgme/debuild.py'
--- pkgme/debuild.py	2012-07-27 07:49:18 +0000
+++ pkgme/debuild.py	2012-09-26 14:45:26 +0000
@@ -24,7 +24,7 @@
     bin_files = []
     for dirpath, dirnames, filenames in os.walk(directory):
         for filename in filenames:
-            # test if its a binary file, 
+            # test if its a binary file,
             # this is what grep is doing to identify binary files
             if "\0" in open(os.path.join(dirpath, filename)).read(4*1024):
                 bin_files.append(
@@ -40,7 +40,7 @@
     to include binary data like icons.
     """
     debian_dir = os.path.join(directory, "debian")
-    bin_files = [os.path.join("debian", p) 
+    bin_files = [os.path.join("debian", p)
                  for p in _find_binary_files_in_dir(debian_dir)]
     with open(os.path.join(
             directory, "debian/source/include-binaries"), "w") as f:
@@ -49,13 +49,13 @@
 
 def build_source_package(directory, sign=True):
     """Build a source package using debuild"""
-    # prepare the debian/dir by adding binary files to 
+    # prepare the debian/dir by adding binary files to
     #    debian/source/include-binaries
-    # 
+    #
     # this is essentially what the following is doing:
-    #    "debuild --source-option=--include-binaries" 
+    #    "debuild --source-option=--include-binaries"
     # but that option is only available on natty and later (not lucid)
-    # 
+    #
     # once we do no longer support lucid this can go
     build_debian_source_include_binaries_content(directory)
     # now build the orig.tar.gz
@@ -71,7 +71,7 @@
 
 def build_orig_tar_gz(target_dir):
     """Build a pkgname_version.orig.tar.gz from the target_dir.
-    
+
     This is usually run as part of build_source_package and does
     rarely need to be run on its own.
 
@@ -85,11 +85,12 @@
     version = changelog.get_version()
     target_file = os.path.join(
         target_dir, "..", "%s_%s.orig.tar.gz" % (pkgname, version))
-    cmd = ["tar", 
-           "-c", 
+    cmd = ["tar",
+           "-c",
            "-z",
-           "--exclude", "debian",
-           "--transform", "s,.,%s-%s," % (pkgname, version),
+           "--exclude", "./debian",
+           # "S" flag means "don't transform symlink targets"
+           "--transform", "s,.,%s-%s,S" % (pkgname, version),
            "-f", target_file,
            ".",
            ]

=== modified file 'pkgme/tests/__init__.py'
--- pkgme/tests/__init__.py	2012-08-31 10:13:33 +0000
+++ pkgme/tests/__init__.py	2012-09-26 14:45:26 +0000
@@ -8,6 +8,7 @@
     module_names = [
         'pkgme.tests.test_api',
         'pkgme.tests.test_backend',
+        'pkgme.tests.test_debuild',
         'pkgme.tests.test_distutils_command',
         'pkgme.tests.test_info_elements',
         'pkgme.tests.test_main',

=== modified file 'pkgme/tests/test_debuild.py'
--- pkgme/tests/test_debuild.py	2012-09-03 15:13:23 +0000
+++ pkgme/tests/test_debuild.py	2012-09-26 14:45:26 +0000
@@ -11,6 +11,21 @@
     )
 
 
+def expand_path_components(path):
+    """Get all the path components that are needed to mkdir `path`.
+
+        get_all_paths('foo') == ['foo']
+        get_all_paths('foo/bar') == ['foo', 'foo/bar']
+        get_all_paths('foo/bar/baz') == ['foo', 'foo/bar', 'foo/bar/baz']
+        get_all_paths('') == []
+    """
+    parts = []
+    while path != '':
+        parts.insert(0, path)
+        path = os.path.dirname(path)
+    return parts
+
+
 def DebianTempDirFixture():
     return FileTree(
         {'debian/icons/': {},
@@ -19,7 +34,7 @@
 
 class BuildTarTestCase(TestCase):
 
-    def test_build_orig_tar_gz(self):
+    def make_debian_tempdir(self):
         tempdir = self.useFixture(DebianTempDirFixture())
         changelog_path = tempdir.join("debian", "changelog")
         with open(changelog_path, "w") as f:
@@ -30,8 +45,12 @@
 
  -- Some Guy <[e-mail]>  Thu, 19 Apr 2012 10:53:30 +0200
 """)
+        return tempdir
+
+    def test_build_orig_tar_gz(self):
+        tempdir = self.make_debian_tempdir()
         with open(tempdir.join("canary"), "w") as f:
-            f.write("pieep")
+            f.write("peep")
         # build it
         result_path = build_orig_tar_gz(tempdir.path)
         # verify
@@ -42,6 +61,30 @@
                 ["testpkg-0.1", "testpkg-0.1/canary"],
                 [m.name for m in f.getmembers()])
 
+    def test_build_orig_tar_gz_not_exclude_other_debian(self):
+        tempdir = self.make_debian_tempdir()
+        canary_filename = "something/debian/canary"
+        os.makedirs(tempdir.join(os.path.dirname(canary_filename)))
+        with open(tempdir.join(canary_filename), "w") as f:
+            f.write("pieep")
+        result_path = build_orig_tar_gz(tempdir.path)
+        canary_paths = expand_path_components(canary_filename)
+        expected_paths = ["testpkg-0.1"]
+        expected_paths += ["testpkg-0.1/" + path for path in canary_paths]
+        with tarfile.open(result_path) as f:
+            self.assertEqual(expected_paths,
+                [m.name for m in f.getmembers()])
+
+    def test_build_orig_tar_gz_preserves_absolute_symlinks(self):
+        tempdir = self.make_debian_tempdir()
+        source = 'source'
+        target = '/a/path'
+        os.symlink(target, tempdir.join(source))
+        result_path = build_orig_tar_gz(tempdir.path)
+        with tarfile.open(result_path) as f:
+            info = f.getmember("testpkg-0.1/" + source)
+            self.assertEqual(target, info.linkname)
+
 
 class BuildIncludeBinariesTestCase(TestCase):