Merge lp:~zyga/checkbox/fix-1300263 into lp:checkbox

[Zygmunt Krynicki]

e075587 plainbox:provider_manager: silence warnings if optional files are missing
9e7fd3a plainbox:provider_manager: install built executables

[Daniel Manrique (roadmr)]

Looks good!

26 + try:
27 + os.makedirs(dest_bin_dir, exist_ok=True)
28 + except IOError:
29 + pass

Why are you ignoring IOError? if creation failed, won't things fail later?

exist_ok=True avoids OSError if components already exist (though "OSError will also be raised if the directory creation fails."), but maybe IOError signals something else that would warrant our attention.

review: Needs Information

[Zygmunt Krynicki (zyga)]

os.makedirs is notoriously odd, it doesn't work as if exists_ok=True was passed. I think it's a python bug but for now I apply the same tactics everywhere. I agree that this should be cleaned up but I would rather fix all instances (and reference any bugs in os.makedirs(exists_ok=True)

[Daniel Manrique (roadmr)]

ok, let's approve then, just don't forget to handle this. Also, it should raise OSError, not IOError, which is what tripped me up. A repro case of when you saw it raise IOError would be useful.

review: Approve

=== modified file 'plainbox/plainbox/provider_manager.py'
--- plainbox/plainbox/provider_manager.py	2014-03-31 17:55:04 +0000
+++ plainbox/plainbox/provider_manager.py	2014-03-31 18:56:23 +0000
@@ -254,20 +254,35 @@
         associated with this commands have been parsed and are ready for
         execution.
         """
-        self._write_provider_file(ns.root, ns.prefix, ns.layout)
+        provider = self.get_provider()
+        self._write_provider_file(ns.root, ns.prefix, ns.layout, provider)
+        self._copy_all_executables(ns.root, ns.prefix, ns.layout, provider)
         self._copy_all_data(ns.root, ns.prefix, ns.layout)
 
-    def _write_provider_file(self, root, prefix, layout):
+    def _write_provider_file(self, root, prefix, layout, provider):
         self._write_to_file(
             root, self._PROVIDER_TEMPLATE.format(
                 prefix=prefix, provider=self.definition),
             lambda stream: self._get_provider_config_obj(
-                layout, prefix).write(stream))
+                layout, prefix, provider).write(stream))
+
+    def _copy_all_executables(self, root, prefix, layout, provider):
+        dest_map = self._get_dest_map(layout, prefix)
+        dest_bin_dir = root + dest_map['bin']
+        try:
+            os.makedirs(dest_bin_dir, exist_ok=True)
+        except IOError:
+            pass
+        for executable in provider.get_all_executables():
+            shutil.copy(executable, dest_bin_dir)
 
     def _copy_all_data(self, root, prefix, layout):
         dest_map = self._get_dest_map(layout, prefix)
         assert os.path.isabs(self.definition.location)
         for src_name, dst_name in dest_map.items():
+            # the bin/ directory is handled by _copy_all_executables()
+            if src_name == 'bin':
+                continue
             assert not os.path.isabs(src_name)
             assert os.path.isabs(dst_name)
             src_name = os.path.join(self.definition.location, src_name)
@@ -279,9 +294,6 @@
                     pass
                 _logger.info(_("copying: %s => %s"), src_name, dst_name)
                 shutil.copytree(src_name, dst_name)
-            else:
-                _logger.warning(_("cannot copy %s, no such file or directory"),
-                                src_name)
 
     def _get_dest_map(self, layout, prefix):
         # Compute directory layout
@@ -293,7 +305,7 @@
             if dest_name_template is not None
         }
 
-    def _get_provider_config_obj(self, layout, prefix):
+    def _get_provider_config_obj(self, layout, prefix, provider):
         dest_map = self._get_dest_map(layout, prefix)
         # Create the .provider file config object
         config_obj = self.definition.get_parser_obj()
@@ -310,9 +322,18 @@
             # explicit
             config_obj.remove_option(section, 'location')
             for src_name, key_id in self._DEF_MAP.items():
-                if os.path.exists(os.path.join(self.definition.location,
-                                               src_name)):
+                # We should emit each foo_dir key only if the corresponding
+                # directory actually exists
+                should_emit_key = os.path.exists(
+                    os.path.join(self.definition.location, src_name))
+                if should_emit_key:
                     config_obj.set(section, key_id, dest_map[src_name])
+            # NOTE: Handle bin_dir specially:
+            # For bin_dir do the exception that any executables (also
+            # counting those from build/bin) should trigger bin_dir to be
+            # listed since we will create/copy files there anyway.
+            if provider.get_all_executables() != []:
+                config_obj.set(section, 'bin_dir', dest_map['bin'])
         return config_obj
 
     def _write_to_file(self, root, pathname, callback):