Merge lp:~sergiusens/snapcraft/collisions into lp:~snappy-dev/snapcraft/core

[Sergio Schvezov]

Collision logic updates with an introduction of _BUILTIN_OPTIONS so all parts have a certain set of options by default even if not declared.

This basically changes collisions to take into account filesets and also check during staging AND snapping.

[Michael Vogt (mvo)]

Thanks, this looks good.

review: Approve

[lp:~sergiusens/snapcraft/collisions]

187. By Sergio Schvezov on 2015-09-14

keys with default values are better

[Michael Vogt (mvo)]

Nice!

review: Approve

=== modified file 'snapcraft/cmds.py'
--- snapcraft/cmds.py	2015-09-14 16:21:34 +0000
+++ snapcraft/cmds.py	2015-09-14 16:21:34 +0000
@@ -223,24 +223,22 @@
             qemu.kill()
 
 
-def check_for_collisions(parts):
-    partsFiles = {}
+def _check_for_collisions(parts, stage):
+    parts_files = {}
     for part in parts:
         # Gather our own files up
-        partFiles = set()
-        for root, dirs, files in os.walk(part.installdir):
-            partFiles |= set([os.path.join(root, f) for f in files])
-        partFiles = set([os.path.relpath(x, part.installdir) for x in partFiles])
+        fileset = getattr(part.code.options, stage, ['*']) or ['*']
+        part_files, _ = snapcraft.plugin.migratable_filesets(fileset, part.installdir)
 
         # Scan previous parts for collisions
-        for otherPartName in partsFiles:
-            common = partFiles & partsFiles[otherPartName]
+        for other_part_name in parts_files:
+            common = part_files & parts_files[other_part_name]
             if common:
-                logger.error('Error: parts %s and %s have the following files in common:\n  %s', otherPartName, part.names()[0], '\n  '.join(sorted(common)))
+                logger.error('Error: parts %s and %s have the following files in common:\n  %s', other_part_name, part.names()[0], '\n  '.join(sorted(common)))
                 return False
 
         # And add our files to the list
-        partsFiles[part.names()[0]] = partFiles
+        parts_files[part.names()[0]] = part_files
 
     return True
 
@@ -269,14 +267,14 @@
 
     for part in config.all_parts:
         for cmd in cmds:
-            if cmd == 'stage':
+            if cmd is 'stage' or cmd is 'snap':
                 # This ends up running multiple times, as each part gets to its
                 # staging cmd.  That's inefficient, but largely OK.
                 # FIXME: fix the above by iterating over cmds before iterating
                 # all_parts.  But then we need to make sure we continue to handle
                 # cases like go, where you want go built before trying to pull
                 # a go project.
-                if not check_for_collisions(config.all_parts):
+                if not _check_for_collisions(config.all_parts, cmd):
                     sys.exit(1)
 
             common.env = config.build_env_for_part(part)

=== modified file 'snapcraft/plugin.py'
--- snapcraft/plugin.py	2015-09-14 16:21:34 +0000
+++ snapcraft/plugin.py	2015-09-14 16:21:34 +0000
@@ -30,6 +30,14 @@
 logger = logging.getLogger(__name__)
 
 
+_BUILTIN_OPTIONS = {
+    'filesets': {},
+    'snap': [],
+    'stage': [],
+    'stage-packages': [],
+}
+
+
 def is_local_plugin(name):
     return name.startswith("x-")
 
@@ -88,9 +96,16 @@
             pass
         options = Options()
 
-        for opt in self.config.get('options', []):
+        plugin_options = self.config.get('options', {})
+        # Let's append some mandatory options, but not .update() to not lose
+        # original content
+        for key in _BUILTIN_OPTIONS:
+            if key not in plugin_options:
+                plugin_options[key] = _BUILTIN_OPTIONS[key]
+
+        for opt in plugin_options:
             attrname = opt.replace('-', '_')
-            opt_parameters = self.config['options'][opt] or {}
+            opt_parameters = plugin_options[opt] or {}
             if opt in properties:
                 setattr(options, attrname, properties[opt])
             else:
@@ -240,8 +255,7 @@
     return part
 
 
-def _migrate_files(fileset, srcdir, dstdir):
-    # validate
+def migratable_filesets(fileset, srcdir):
     includes, excludes = _get_file_list(fileset)
 
     include_files = _generate_include_set(srcdir, includes)
@@ -256,6 +270,12 @@
     snap_dirs = set([x for x in snap_files if os.path.isdir(os.path.join(srcdir, x)) and not os.path.islink(os.path.join(srcdir, x))])
     snap_files = snap_files - snap_dirs
 
+    return snap_files, snap_dirs
+
+
+def _migrate_files(fileset, srcdir, dstdir):
+    snap_files, snap_dirs = migratable_filesets(fileset, srcdir)
+
     if snap_dirs:
         common.run(['mkdir', '-p'] + list(snap_dirs), cwd=dstdir)
     if snap_files:

=== modified file 'snapcraft/tests/test_cmds.py'
--- snapcraft/tests/test_cmds.py	2015-08-24 18:27:50 +0000
+++ snapcraft/tests/test_cmds.py	2015-09-14 16:21:34 +0000
@@ -39,12 +39,14 @@
 
         part1 = mock.Mock()
         part1.names.return_value = ['part1']
+        part1.code.options.stage = ['*']
         part1.installdir = tmpdir + '/install1'
         os.makedirs(part1.installdir + '/a')
         open(part1.installdir + '/a/1', mode='w').close()
 
         part2 = mock.Mock()
         part2.names.return_value = ['part2']
+        part2.code.options.stage = ['*']
         part2.installdir = tmpdir + '/install2'
         os.makedirs(part2.installdir + '/a')
         open(part2.installdir + '/1', mode='w').close()
@@ -53,16 +55,17 @@
 
         part3 = mock.Mock()
         part3.names.return_value = ['part3']
+        part3.code.options.stage = ['*']
         part3.installdir = tmpdir + '/install3'
         os.makedirs(part3.installdir + '/a')
         os.makedirs(part3.installdir + '/b')
         open(part3.installdir + '/1', mode='w').close()
         open(part3.installdir + '/a/2', mode='w').close()
 
-        self.assertTrue(cmds.check_for_collisions([part1, part2]))
+        self.assertTrue(cmds._check_for_collisions([part1, part2], 'stage'))
         self.assertEqual('', fake_logger.output)
 
-        self.assertFalse(cmds.check_for_collisions([part1, part2, part3]))
+        self.assertFalse(cmds._check_for_collisions([part1, part2, part3], 'stage'))
         self.assertEqual(
             'Error: parts part2 and part3 have the following files in common:\n'
             '  1\n'