Merge ~laney/germinate:master into germinate:master

[Iain Lane]

As discussed on a few occasions, here's an initial merge proposal to support seeding of snaps in germinate.

This is basically a passthrough of "snap:" entries to .snap output files. I know we had said that we'd try to avoid using a colon, but I didn't find anything that was suitable and didn't look contrived. It requires some branches in the parser which I don't think are too bad. If it is, though, we can try harder to find another way of specifying seeded snaps.

When implementing this I've tried to keep snaps separated from debs as much as possible. They don't require any dependency expansion, we don't perform availability checks on them*, and if they appear intermingled with debs in the pre-existing output then consumers are likely to be confused by their presence.

Feedback welcome. I'll be proposing a companion livecd-rootfs MP soon.

* as discussed at the Ubuntu Rally in NYC

[Colin Watson (cjwatson)]

I'm still not wild about the "snap:" prefix, but I guess if nothing else works then we'll have to live with it. Could you please ensure that the new syntax is documented in man/germinate.1, though? That's the official reference for seed syntax.

review: Approve

[Colin Watson (cjwatson)]

Also, have you thought about how we're going to annotate classic snaps? I gather that we may want to install both classic and non-classic snaps on images ...

[Iain Lane (laney)]

Re-pushed, thanks for the review. This time I have added a section to the manpage - please review that.

> Also, have you thought about how we're going to annotate classic snaps? I
> gather that we may want to install both classic and non-classic snaps on
> images ...

Um, no, good point. Do those have to have their classicness specified in seed.yaml somehow? (I've asked #snappy for clarification as there's no documentation I can find. Currently waiting for an answer.)

Do you have a preference on syntax? Maybe "snap:foo/classic"? AFAICS "/" isn't used apart from in _filter_packages to specify a regex & that wouldn't apply here. For outputting I can imagine either passing that straight through, transforming it to "foo (classic)", or adding a new column in increasing order of difficulty on the livecd-rootfs side. Not that difficulty there matters too much.

[Iain Lane (laney)]

> Um, no, good point. Do those have to have their classicness specified in
> seed.yaml somehow? (I've asked #snappy for clarification as there's no
> documentation I can find. Currently waiting for an answer.)
>
> Do you have a preference on syntax? Maybe "snap:foo/classic"? AFAICS "/" isn't
> used apart from in _filter_packages to specify a regex & that wouldn't apply
> here. For outputting I can imagine either passing that straight through,
> transforming it to "foo (classic)", or adding a new column in increasing order
> of difficulty on the livecd-rootfs side. Not that difficulty there matters too
> much.

OK, I did this. I went for the second option. Classic snaps should be annotated with "/classic", and that is transformed to "(classic)" in the first column of the output.

(In livecd-rootfs I translate that back to "/classic" to make it easier to pass to the function which installs snaps, which is slightly ugly but it seems worth it to me to have a more human-readable output from germinate.)

[Iain Lane (laney)]

gentle ping for a review please

[Colin Watson (cjwatson)]

I would not be very surprised to find extra requirements popping up here later that need changes to the syntax, but I guess this will do for now and we can see where the wind takes us. Thanks!

review: Approve

diff --git a/germinate/germinator.py b/germinate/germinator.py
index a86c25d..310ad44 100644
--- a/germinate/germinator.py
+++ b/germinate/germinator.py
@@ -128,6 +128,7 @@ class GerminatedSeed(object):
         self._entries = []
         self._features = set()
         self._recommends_entries = []
+        self._snaps = set()
         self._close_seeds = set()
         self._depends = set()
         self._build_depends = set()
@@ -139,6 +140,7 @@ class GerminatedSeed(object):
         self._build_srcs = set()
         self._not_build_srcs = set()
         self._reasons = {}
+        self._snap_reasons = {}
         self._blacklist = set()
         self._blacklist_seen = False
         # Note that this relates to the vestigial global blacklist file, not
@@ -194,6 +196,7 @@ class GerminatedSeed(object):
         self._entries = copy._entries
         self._recommends_entries = copy._recommends_entries
         self._depends = copy._depends
+        self._snaps = copy._snaps
         self._build_depends = copy._build_depends
         self._sourcepkgs = copy._sourcepkgs
         self._build_sourcepkgs = copy._build_sourcepkgs
@@ -202,6 +205,7 @@ class GerminatedSeed(object):
         self._build_srcs = copy._build_srcs
         self._not_build_srcs = copy._not_build_srcs
         self._reasons = copy._reasons
+        self._snap_reasons = copy._snap_reasons
         self._blacklist_seen = False
         self._blacklisted = copy._blacklisted
         self._grown = True
@@ -234,6 +238,10 @@ class GerminatedSeed(object):
         return set(self._depends)
 
     @property
+    def snaps(self):
+        return self._snaps
+
+    @property
     def build_depends(self):
         return set(self._build_depends)
 
@@ -328,6 +336,7 @@ class GerminatedSeedStructure(object):
         self._all = set()
         self._all_srcs = set()
         self._all_reasons = {}
+        self._all_snap_reasons = {}
 
         self._blacklist = {}
 
@@ -765,6 +774,7 @@ class Germinator(object):
 
         seedpkgs = []
         seedrecommends = []
+        seedsnaps = []
         substvars = {}
 
         for line in raw_seed:
@@ -781,7 +791,7 @@ class Germinator(object):
                 pkg = pkg[:pkg.find("#")]
 
             colon = pkg.find(":")
-            if colon != -1:
+            if colon != -1 and not pkg.startswith("snap:"):
                 # Special header
                 name = pkg[:colon]
                 name = name.lower()
@@ -846,10 +856,22 @@ class Germinator(object):
             else:
                 is_blacklist = False
 
+            # this is a seeded snap
+            if pkg.startswith("snap:"):
+                is_snap = True
+                pkg = pkg[5:]
+                if pkg.endswith("/classic"):
+                    pkg = "%s (classic)" % pkg[:-8]
+            else:
+                is_snap = False
+
             # a (pkgname) indicates that this is a recommend
             # and not a depends
             if pkg.startswith('(') and pkg.endswith(')'):
                 pkg = pkg[1:-1]
+                if is_snap or pkg.startswith("snap:"):
+                    _logger.warning("Recommends entries cannot be used with snap packages, ignoring %s", pkg)
+                    continue
                 pkgs = self._filter_packages(self._packages, pkg)
                 if not pkgs:
                     pkgs = [pkg]  # virtual or expanded; check again later
@@ -857,6 +879,10 @@ class Germinator(object):
                     seedrecommends.extend(self._substitute_seed_vars(
                         substvars, pkg))
 
+            if is_snap and pkg.startswith('%'):
+                _logger.warning("%% entries cannot be used with snap packages, ignoring %s", pkg)
+                continue
+
             if pkg.startswith('%'):
                 pkg = pkg[1:]
                 if pkg in self._sources:
@@ -865,6 +891,8 @@ class Germinator(object):
                 else:
                     _logger.warning("Unknown source package: %s", pkg)
                     pkgs = []
+            elif is_snap:
+                pkgs = [pkg]
             else:
                 pkgs = self._filter_packages(self._packages, pkg)
                 if not pkgs:
@@ -876,8 +904,12 @@ class Germinator(object):
                     seed._blacklist.update(self._substitute_seed_vars(
                         substvars, pkg))
             else:
-                for pkg in pkgs:
-                    seedpkgs.extend(self._substitute_seed_vars(substvars, pkg))
+                if is_snap:
+                    for pkg in pkgs:
+                        seedsnaps.extend(self._substitute_seed_vars(substvars, pkg))
+                else:
+                    for pkg in pkgs:
+                        seedpkgs.extend(self._substitute_seed_vars(substvars, pkg))
 
         di_kernel_versions = None
 
@@ -921,6 +953,9 @@ class Germinator(object):
                 # No idea
                 _logger.error("Unknown %s package: %s", seed, pkg)
 
+        for pkg in seedsnaps:
+            seed._snaps.add(pkg)
+
         for pkg in self._hints:
             if (self._hints[pkg] == seed.name and
                 not self._already_seeded(seed, pkg)):
@@ -1023,6 +1058,10 @@ class Germinator(object):
                     self._di_kernel_versions = pkg
                 else:
                     self._add_package(seed, pkg, seed._seed_reason)
+            for pkg in self.get_seed_snap_entries(structure, seedname):
+                # avoid collisions with deb packages with the same name
+                self._remember_why(seed._snap_reasons, pkg, seed._seed_reason, False, False)
+                self._remember_why(output._all_snap_reasons, pkg, seed._seed_reason, False, False)
 
             self._di_kernel_versions = None
 
@@ -1736,6 +1775,23 @@ class Germinator(object):
             if not isinstance(e, SeedKernelVersions)]
         return ret
 
+    def get_seed_snap_entries(self, structure, seedname):
+        """Return the explicitly seeded snap entries for this seed."""
+        seed = self._get_seed(structure, seedname)
+        output = set(seed._snaps)
+        for innerseed in self._inner_seeds(seed):
+            if innerseed.name == seed.name:
+                continue
+            output -= innerseed._depends
+        # Take care to preserve the original ordering.
+        # Use a temporary variable to work around a pychecker bug.
+        ret = [e for e in seed._snaps if e in output]
+        return ret
+
+    def get_snaps(self, structure, seedname):
+        """ Return the explicitly seeded snap entries for this seed."""
+        return self._get_seed(structure, seedname).snaps
+
     def get_depends(self, structure, seedname):
         """Return the dependencies of this seed."""
         return self._get_seed(structure, seedname).depends
@@ -1842,6 +1898,34 @@ class Germinator(object):
                 print(fmt % (src_len, src, mnt_len,
                              self._sources[src]["Maintainer"]), file=f)
 
+    def _write_snap_list(self, reasons, filename, snapset):
+        snaplist = sorted(snapset)
+
+        pkg_len = len("Package")
+        why_len = len("Why")
+
+        for pkg in snaplist:
+            _pkg_len = len(pkg)
+            if _pkg_len > pkg_len:
+                pkg_len = _pkg_len
+
+            why = reasons[pkg][0] if pkg in reasons else ""
+            _why_len = len(str(why))
+            if _why_len > why_len:
+                why_len = _why_len
+
+        with AtomicFile(filename) as f:
+            print("%-*s | %-*s" %
+                  (pkg_len, "Package",
+                   why_len, "Why"), file=f)
+            print(("-" * pkg_len) + "-+-" + ("-" * why_len), file=f)
+            for pkg in snaplist:
+                why = reasons[pkg][0] if pkg in reasons else ""
+                print("%-*s | %-*s" %
+                      (pkg_len, pkg,
+                       why_len, why), file=f)
+            print(("-" * (pkg_len + why_len + 3)), file=f)
+
     def write_full_list(self, structure, filename, seedname):
         """Write the full (run-time) dependency expansion of this seed."""
         seed = self._get_seed(structure, seedname)
@@ -1865,6 +1949,11 @@ class Germinator(object):
         seed = self._get_seed(structure, seedname)
         self._write_list(seed._reasons, filename, seed._depends)
 
+    def write_snap_list(self, structure, filename, seedname):
+        """Write the snaps seeded in this seed."""
+        seed = self._get_seed(structure, seedname)
+        self._write_snap_list(seed._snap_reasons, filename, seed._snaps)
+
     def write_build_depends_list(self, structure, filename, seedname):
         """Write the build-dependencies of this seed."""
         seed = self._get_seed(structure, seedname)
@@ -1896,6 +1985,16 @@ class Germinator(object):
         self._write_list(self._output[structure]._all_reasons, filename,
                          all_bins)
 
+    def write_all_snap_list(self, structure, filename):
+        """Write all the snap packages in this structure."""
+        all_snaps = set()
+
+        for seedname in structure.names:
+            all_snaps |= self.get_snaps(structure, seedname)
+
+        self._write_snap_list(self._output[structure]._all_snap_reasons, filename,
+                              all_snaps)
+
     def write_all_source_list(self, structure, filename):
         """Write all the source packages for this structure."""
         all_srcs = set()
diff --git a/germinate/log.py b/germinate/log.py
index dc7c0f2..558779f 100644
--- a/germinate/log.py
+++ b/germinate/log.py
@@ -57,3 +57,4 @@ def germinate_logging(level):
         handler.setFormatter(GerminateFormatter())
         logger.addHandler(handler)
         logger.propagate = False
+    return logger
diff --git a/germinate/scripts/germinate_main.py b/germinate/scripts/germinate_main.py
index 094e27e..0b9f29d 100644
--- a/germinate/scripts/germinate_main.py
+++ b/germinate/scripts/germinate_main.py
@@ -187,6 +187,7 @@ def main(argv):
         g.write_depends_list(structure, seedname + ".depends", seedname)
         g.write_build_depends_list(structure,
                                    seedname + ".build-depends", seedname)
+        g.write_snap_list (structure, seedname + ".snaps", seedname)
 
         if seedname != "extra" and seedname in structure:
             structure.write_seed_text(seedname + ".seedtext", seedname)
@@ -196,6 +197,7 @@ def main(argv):
 
     g.write_all_list(structure, "all")
     g.write_all_source_list(structure, "all.sources")
+    g.write_all_snap_list(structure, "all.snaps")
 
     g.write_supported_list(structure, "%s+build-depends" % structure.supported)
     g.write_supported_source_list(
diff --git a/germinate/tests/helpers.py b/germinate/tests/helpers.py
index b1ad38c..180ae48 100644
--- a/germinate/tests/helpers.py
+++ b/germinate/tests/helpers.py
@@ -130,5 +130,12 @@ class TestCase(unittest.TestCase):
         with io.open(seed_path, "a", encoding="UTF-8") as seed:
             print(u(" * %s") % pkg, file=seed)
 
+    def addSeedSnap(self, seed_dist, seed_name, pkg):
+        self.setUpDirs()
+        seed_path = os.path.join(self.seeds_dir, seed_dist, seed_name)
+        self.ensureParentDir(seed_path)
+        with io.open(seed_path, "a", encoding="UTF-8") as seed:
+            print(u(" * snap:%s") % pkg, file=seed)
+
     def openSeedStructure(self, branch):
         return SeedStructure(branch, seed_bases=["file://%s" % self.seeds_dir])
diff --git a/germinate/tests/test_germinator.py b/germinate/tests/test_germinator.py
index 79956dc..2cb0d52 100644
--- a/germinate/tests/test_germinator.py
+++ b/germinate/tests/test_germinator.py
@@ -358,4 +358,38 @@ class TestGerminator(TestCase):
             set(["gettext", "debhelper"]),
             germinator.get_build_depends(structure, "base"))
 
+    def test_snap(self):
+        import logging
+        from germinate.log import germinate_logging
+        germinate_logging(logging.INFO)
+        branch = "collection.precise"
+        self.addSeed(branch, "base")
+        self.addSeedSnap(branch, "base", "hello")
+        germinator = Germinator("i386")
+        structure = self.openSeedStructure(branch)
+        germinator.plant_seeds(structure)
+        germinator.grow(structure)
+
+        self.assertEqual(
+            set(["hello"]),
+            germinator.get_snaps(structure, "base"))
+
+    def test_snap_recommends(self):
+        import logging
+        from germinate.log import germinate_logging
+        logger = germinate_logging(logging.INFO)
+        branch = "collection.precise"
+        self.addSeed(branch, "base")
+        self.addSeedSnap(branch, "base", "(hello)")
+        with self.assertLogs(logger, level=logging.WARNING) as logs:
+            germinator = Germinator("i386")
+            structure = self.openSeedStructure(branch)
+            germinator.plant_seeds(structure)
+            germinator.grow(structure)
+        self.assertIn('ignoring hello', logs.output[0])
+
+        self.assertEqual(
+            set([]),
+            germinator.get_snaps(structure, "base"))
+
     # TODO: Germinator needs many more unit tests.
diff --git a/germinate/tests/test_integration.py b/germinate/tests/test_integration.py
index 6e16802..756f077 100644
--- a/germinate/tests/test_integration.py
+++ b/germinate/tests/test_integration.py
@@ -67,6 +67,44 @@ class TestGerminate(TestCase):
         self.assertTrue("hello" in supported)
         self.assertTrue("hello-dependency" in supported)
 
-        all_ = self.parseOutput("supported")
+        all_ = self.parseOutput("all")
         self.assertTrue("hello" in all_)
         self.assertTrue("hello-dependency" in all_)
+
+    def test_snap(self):
+        # Need Packages
+        self.addSource("warty", "main", "hello", "1.0-1",
+                       ["hello", "hello-dependency"])
+        self.addPackage("warty", "main", "i386", "hello-dependency", "1.0-1",
+                        fields={"Source": "hello"})
+        self.addSeed("ubuntu.warty", "supported")
+        self.addSeedSnap("ubuntu.warty", "supported", "mycoolsnap")
+        self.runGerminate("-s", "ubuntu.warty", "-d", "warty", "-c", "main")
+
+        supported = self.parseOutput("supported.snaps")
+        self.assertIn("mycoolsnap", supported)
+
+        all_ = self.parseOutput("all.snaps")
+        self.assertIn("mycoolsnap", all_)
+
+        debs = self.parseOutput("supported")
+        self.assertNotIn("mycoolsnap", debs)
+
+    def test_classic_snap(self):
+        # Need Packages
+        self.addSource("warty", "main", "hello", "1.0-1",
+                       ["hello", "hello-dependency"])
+        self.addPackage("warty", "main", "i386", "hello-dependency", "1.0-1",
+                        fields={"Source": "hello"})
+        self.addSeed("ubuntu.warty", "supported")
+        self.addSeedSnap("ubuntu.warty", "supported", "mycoolsnap/classic")
+        self.runGerminate("-s", "ubuntu.warty", "-d", "warty", "-c", "main")
+
+        supported = self.parseOutput("supported.snaps")
+        self.assertIn("mycoolsnap (classic)", supported)
+
+        all_ = self.parseOutput("all.snaps")
+        self.assertIn("mycoolsnap (classic)", all_)
+
+        debs = self.parseOutput("supported")
+        self.assertNotIn("mycoolsnap (classic)", debs)
diff --git a/germinate/tests/test_seeds.py b/germinate/tests/test_seeds.py
index f7815ba..cc81b3b 100644
--- a/germinate/tests/test_seeds.py
+++ b/germinate/tests/test_seeds.py
@@ -55,6 +55,7 @@ class TestSeed(TestCase):
         self.addSeedPackage("collection.dist", "test2", "foo")
         self.addSeed("collection.dist", "test3")
         self.addSeedPackage("collection.dist", "test3", "bar")
+        self.addSeedSnap("collection.dist", "test3", "quux")
 
     def test_init_no_vcs(self):
         """__init__ can open a seed from a collection without a VCS."""
@@ -65,6 +66,15 @@ class TestSeed(TestCase):
         self.assertEqual("collection.dist", seed.branch)
         self.assertEqual(" * foo\n", seed.text)
 
+    def test_init_no_vcs_snap(self):
+        """__init__ can open a seed from a collection without a VCS."""
+        seed = Seed(
+            ["file://%s" % self.seeds_dir], ["collection.dist"], "test3")
+        self.assertEqual("test3", seed.name)
+        self.assertEqual("file://%s" % self.seeds_dir, seed.base)
+        self.assertEqual("collection.dist", seed.branch)
+        self.assertEqual(" * bar\n * snap:quux\n", seed.text)
+
     def test_behaves_as_file(self):
         """A Seed context can be read from as a file object."""
         seed = Seed(
diff --git a/man/germinate.1 b/man/germinate.1
index 1b8edf3..68682f2 100644
--- a/man/germinate.1
+++ b/man/germinate.1
@@ -108,6 +108,27 @@ It is not intended for the purpose of working around buggy package
 relationships, and attempts to do so will not work because
 .Ic apt
 has no way to know about blacklist entries in seeds.
+.It snap:name
+Seed entries beginning with
+.Sq \&snap:
+are
+.Em snap
+packages. These are different from
+.Em deb
+packages in that they do not have (build-)dependencies, can not be
+recommended, and do not end up in any resulting metapackages. (If you try to
+recommend a snap package, it will be ignored completely.) Snaps specified in
+seeds will be output in a
+.Em .snaps
+file named after the corresponding seed, as software processing the output of
+.Nm
+will typically need to treat snaps differently from debs.
+.Nm
+will not check remotely to see if a given snap is available, therefore seeds
+are expected to explicitly list all architectures a snap is to be seeded on.
+.Sq \&snap:
+entries can also be suffixed with "/classic" to indicate that the snaps need to
+be installed with classic confinement on end-user systems.
 .It key: value
 Some seeds also contain headers at the top of the file, in
 .Dq key: value