Merge lp:~james-w/pkgme-devportal/executable-executables into lp:pkgme-devportal

[James Westby]

Make executables executable.

The executable may not be marked exectuable (e.g. if it was delivered
in a .zip), so we also consider any ELF executables as candidate exectuables.

That will make it more likely we find the right thing, but it won't be
enough to actually make that file exectuable. Therefore we override
dh_fixperms to chmod the file at build time so it will be installed correctly.

We also tell dh_fixperms to not adjust anything in /opt so that it doesn't
remove any needed exectuable bits.

Hi,

This tries to make executable handling more robust and lenient.

It should now work for things delivered as zip files, and will also
do a better job at making sure that any executables stay executable.

It also fixes another instance of the package name bug.

The branch is a bit messy, sorry. I couldn't think of any changes to
make the code more sensible though.

Thanks,

James

[Jonathan Lange (jml)]

Looks good. The only thing I'd consider changing is making the "special casing" a bit less special by making it a loop or something similar.

review: Approve

[lp:~james-w/pkgme-devportal/executable-executables]

126. By James Westby on 2012-09-18

Use a loop for the elements requiring package name. Thanks jml.

=== modified file 'devportalbinary/binary.py'
--- devportalbinary/binary.py	2012-09-11 17:19:18 +0000
+++ devportalbinary/binary.py	2012-09-18 14:36:18 +0000
@@ -59,9 +59,14 @@
 # the overrides template for a dh_shlibdeps
 OVERRIDE_DH_SHLIBDEPS_TEMPLATE = """
 override_dh_shlibdeps:
-\tdh_shlibdeps -l%s
+\tdh_shlibdeps -l{ld_search_path}
 """
 
+OVERRIDE_DH_FIXPERMS_TEMPLATE = """
+override_dh_fixperms:
+\tchmod u+x debian/{package_name}/{executable_path}
+\tdh_fixperms -X/opt/
+"""
 
 # the default extra debian/rules targets
 DEFAULT_EXTRA_RULES_TARGETS = OVERRIDE_DH_STRIP_TEMPLATE
@@ -160,16 +165,26 @@
         return None
 
 
+def is_elf_exectuable(file_type):
+    """Whether `file_type` indicates an ELF executable."""
+    if file_type.startswith('ELF'):
+        if "executable" in file_type.split(",")[0]:
+            return True
+    return False
+
+
 def iter_executables(path):
     """Iterate through all executable files under 'path'.
 
     Paths yielded will be relative to 'path'. Directories will *not* be
-    yielded. "Executable" is determined by filesystem permissions.
+    yielded. A file is considered "executable" if it has the executable
+    bit set, or if it is an ELF executable.
     """
     for root, dirs, files in os.walk(path):
         for filename in files:
             file_path = os.path.join(root, filename)
-            if os.access(file_path, os.X_OK):
+            if (os.access(file_path, os.X_OK)
+                    or is_elf_exectuable(get_file_type(file_path))):
                 yield os.path.relpath(file_path, path)
 
 
@@ -189,7 +204,7 @@
 OBJDUMP = '/usr/bin/objdump'
 # Map the objdump "architecture" value to the dpkg architecture,
 # objdump report i386 as "i386" but amd64 as "i386:x86-64" 
-OBJDUMP_MAPPING = { 
+OBJDUMP_MAPPING = {
     "i386:x86-64" : "amd64",
 }
 
@@ -319,15 +334,21 @@
 def guess_extra_debian_rules_targets(
     package_name,
     path,
-    embedded_libs_finder=guess_embedded_libs_search_paths):
+    embedded_libs_finder=guess_embedded_libs_search_paths,
+    executable_path=None):
     embedded_paths = embedded_libs_finder(path)
     extra_targets = DEFAULT_EXTRA_RULES_TARGETS
     if embedded_paths:
-        base_dir = "$(CURDIR)/debian/%(package_name)s/opt/" \
-            "%(package_name)s/" % { 'package_name' : package_name }
+        base_dir = "$(CURDIR)/debian/{package_name}/opt/" \
+            "{package_name}/".format(package_name=package_name)
         ld_search_path = build_ld_library_search_path(
             base_dir, embedded_paths)
-        extra_targets += OVERRIDE_DH_SHLIBDEPS_TEMPLATE % ld_search_path
+        extra_targets += OVERRIDE_DH_SHLIBDEPS_TEMPLATE.format(
+            ld_search_path=ld_search_path)
+    if executable_path:
+        extra_targets += OVERRIDE_DH_FIXPERMS_TEMPLATE.format(
+                executable_path=executable_path,
+                    package_name=package_name)
     return extra_targets
 
 
@@ -401,12 +422,14 @@
 This file was automatically generated.
 """ % (maintainer_suffix,)
 
-    def get_extra_targets(self):
-        package_name = self.get_package_name()
-        return guess_extra_debian_rules_targets(package_name, self.path)
+    def get_extra_targets(self, package_name):
+        executable_path = self.get_executable(package_name)
+        return guess_extra_debian_rules_targets(package_name, self.path,
+                executable_path=executable_path)
 
     @classmethod
     def want_with_metadata(cls, path, metadata):
+        # XXX: should it also check that there is an executable?
         if list(iter_binaries(path)):
             return {
                 'score': 10,

=== modified file 'devportalbinary/metadata.py'
--- devportalbinary/metadata.py	2012-09-17 16:05:32 +0000
+++ devportalbinary/metadata.py	2012-09-18 14:36:18 +0000
@@ -353,7 +353,7 @@
     def get_extra_control_binary_fields(self):
         return None
 
-    def get_extra_targets(self):
+    def get_extra_targets(self, package_name):
         """Return any extra debian/rules targets. """
         return ""
 
@@ -537,12 +537,16 @@
             Distribution,
             ExplicitCopyright,
             ExtraControlBinaryFields,
-            ExtraTargets,
             Maintainer,
             Section,
             Version,
             Homepage,
             ]
+        PACKAGE_NAME_ELEMENTS = [
+            ExtraFiles,
+            ExtraFilesFromPaths,
+            ExtraTargets,
+            ]
         info = {}
         for element in COMPULSORY_ELEMENTS:
             info[element] = self._calculate_info_element(element)
@@ -550,15 +554,12 @@
             value = self._calculate_info_element(element)
             if value:
                 info[element] = value
-        # Special-case ExtraFiles and ExtraFilesFromPaths since they need
-        # PackageName.
+        # Special-case those elements that need the package name
         package_name = PackageName.clean(info[PackageName])
-        info[ExtraFiles] = self._calculate_info_element(
-            ExtraFiles, package_name)
-        extra_files_from_paths = self._calculate_info_element(
-            ExtraFilesFromPaths, package_name)
-        if extra_files_from_paths:
-            info[ExtraFilesFromPaths] = extra_files_from_paths
+        for element in PACKAGE_NAME_ELEMENTS:
+            value = self._calculate_info_element(element, package_name)
+            if value:
+                info[element] = value
         return info
 
     @classmethod

=== modified file 'devportalbinary/tests/test_binary.py'
--- devportalbinary/tests/test_binary.py	2012-09-11 17:19:18 +0000
+++ devportalbinary/tests/test_binary.py	2012-09-18 14:36:18 +0000
@@ -30,6 +30,7 @@
     guess_embedded_libs_search_paths,
     guess_executable,
     guess_extra_debian_rules_targets,
+    is_elf_exectuable,
     iter_binaries,
     iter_executables,
     needed_libraries_from_objdump,
@@ -69,6 +70,16 @@
         executables = list(iter_executables(path))
         self.assertEqual(['some-file'], executables)
 
+    def test_finds_elf_executable(self):
+        # ELF executables are considered executables.
+        executable = get_test_data_file_path('hello', 'hello')
+        with open(executable, 'rb') as f:
+            content = f.read()
+        tree = FileTree({'some-file': {PERMISSIONS: 0644, CONTENT: content}})
+        path = self.useFixture(tree).path
+        executables = list(iter_executables(path))
+        self.assertEqual(['some-file'], executables)
+
     def test_no_files_at_all(self):
         # iter_executables finds no executables if there are no files at all.
         path = self.useFixture(FileTree({})).path
@@ -383,21 +394,36 @@
         conf = load_configuration()
         self.assertEqual(backend.get_architecture(), "i386")
 
+    def test_get_extra_targets_makes_executable_executable(self):
+        package_name = 'package'
+        executable_path = 'a/binary'
+        path = self.getUniqueString()
+        extra_targets = guess_extra_debian_rules_targets(package_name, path,
+                lambda l: [], executable_path=executable_path)
+        self.assertIn(
+                "override_dh_fixperms:"
+                "\n\tchmod u+x debian/{1}/{0}"
+                "\n\tdh_fixperms -X/opt/".format(executable_path,
+                    package_name),
+                extra_targets)
+
     def test_get_extra_targets_no_bundled_libs(self):
         package_name = self.getUniqueString()
         path = self.getUniqueString()
         self.assertEqual(
-            guess_extra_debian_rules_targets(package_name, path, lambda l: []),
-            DEFAULT_EXTRA_RULES_TARGETS)
+            DEFAULT_EXTRA_RULES_TARGETS,
+            guess_extra_debian_rules_targets(package_name, path, lambda l: []))
 
     def test_get_extra_targets_bundled_libs(self):
         package_name = "a-pkgname"
         path = self.getUniqueString()
         ld_search_path = ["x86/lib"]
+        expected_ld_search_path = ("$(CURDIR)/debian/{package_name}"
+                "/opt/{package_name}/{search_path}").format(package_name=package_name,
+                        search_path=ld_search_path[0])
         self.assertEqual(
-            DEFAULT_EXTRA_RULES_TARGETS+OVERRIDE_DH_SHLIBDEPS_TEMPLATE % (
-                "$(CURDIR)/debian/%s/opt/%s/%s" % (
-                    package_name, package_name, ld_search_path[0])),
+            DEFAULT_EXTRA_RULES_TARGETS+OVERRIDE_DH_SHLIBDEPS_TEMPLATE.format(
+                ld_search_path=expected_ld_search_path),
             guess_extra_debian_rules_targets(
                 package_name, path, lambda l: ld_search_path))
 
@@ -422,3 +448,26 @@
 This file was automatically generated.
 """ % (submitter,)
         self.assertEqual(expected, backend.get_explicit_copyright())
+
+
+class IsElfExecutableTests(TestCase):
+
+    def test_not_elf(self):
+        self.assertEqual(False, is_elf_exectuable('blah'))
+
+    def test_not_executable(self):
+        self.assertEqual(False,
+                is_elf_exectuable('ELF 32-bit LSB shared object'))
+
+    def test_executable(self):
+        self.assertEqual(True,
+                is_elf_exectuable('ELF 32-bit LSB executable'))
+
+    def test_executable_with_extra_junk(self):
+        self.assertEqual(True,
+                is_elf_exectuable('ELF 32-bit LSB executable, blah'))
+
+    def test_not_executable_with_extra_junk(self):
+        self.assertEqual(False,
+                is_elf_exectuable(
+                    'ELF 32-bit LSB shared object, blah executable'))