launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad-buildd/snapception into lp:launchpad-buildd.

Commit message:
Allow optionally installing snapcraft as a snap.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1737994 in launchpad-buildd: "build.snapcraft.io uses snapcraft from the deb archive"
  https://bugs.launchpad.net/launchpad-buildd/+bug/1737994

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad-buildd/snapception/+merge/337126

This is controlled by an optional "channels" key in the build arguments, which takes a dict mapping snap names to channel names.  At present anything other than "core" and "snapcraft" will cause a warning but otherwise be ignored, but I chose this structure so that it's reasonably easy to extend in future (e.g. for base snaps).

In the process, I noticed that snapd wasn't actually being started in LXD-based builds due to details of how the systemd services are started in snapd.postinst, which definitely wasn't intended and is probably a regression, so I fixed that as otherwise "snap install" obviously doesn't work.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad-buildd/snapception into lp:launchpad-buildd.

=== modified file 'debian/changelog'
--- debian/changelog	2018-01-15 10:08:55 +0000
+++ debian/changelog	2018-02-04 00:20:53 +0000
@@ -1,3 +1,13 @@
+launchpad-buildd (159) UNRELEASED; urgency=medium
+
+  * Allow all snapd services in the policy-rc.d we install in LXD
+    containers; for better or worse, snapd.postinst (via deb-systemd-invoke)
+    won't start any of them, including snapd itself, if any of them is
+    forbidden.  Mask snapd.refresh.timer so that it doesn't cause trouble.
+  * Allow optionally installing snapcraft as a snap (LP: #1737994).
+
+ -- Colin Watson <cjwatson@xxxxxxxxxx>  Fri, 02 Feb 2018 16:14:46 +0000
+
 launchpad-buildd (158) xenial; urgency=medium
 
   [ Steve Langasek ]

=== modified file 'lpbuildd/snap.py'
--- lpbuildd/snap.py	2017-08-25 16:05:49 +0000
+++ lpbuildd/snap.py	2018-02-04 00:20:53 +0000
@@ -38,6 +38,7 @@
     def initiate(self, files, chroot, extra_args):
         """Initiate a build with a given set of files and chroot."""
         self.name = extra_args["name"]
+        self.channels = extra_args.get("channels", {})
         self.branch = extra_args.get("branch")
         self.git_repository = extra_args.get("git_repository")
         self.git_path = extra_args.get("git_path")
@@ -62,6 +63,16 @@
     def doRunBuild(self):
         """Run the process to build the snap."""
         args = []
+        known_snaps = ("core", "snapcraft")
+        for snap in known_snaps:
+            if snap in self.channels:
+                args.extend(["--channel-%s" % snap, self.channels[snap]])
+        unknown_snaps = set(self.channels) - set(known_snaps)
+        if unknown_snaps:
+            print(
+                "Channels requested for unknown snaps: %s" %
+                " ".join(sorted(unknown_snaps)),
+                file=sys.stderr)
         if self.proxy_url:
             args.extend(["--proxy-url", self.proxy_url])
         if self.revocation_endpoint:

=== modified file 'lpbuildd/target/build_snap.py'
--- lpbuildd/target/build_snap.py	2017-11-24 16:08:17 +0000
+++ lpbuildd/target/build_snap.py	2018-02-04 00:20:53 +0000
@@ -48,6 +48,14 @@
     @classmethod
     def add_arguments(cls, parser):
         super(BuildSnap, cls).add_arguments(parser)
+        parser.add_argument(
+            "--channel-core", metavar="CHANNEL",
+            help="install core snap from CHANNEL")
+        parser.add_argument(
+            "--channel-snapcraft", metavar="CHANNEL",
+            help=(
+                "install snapcraft as a snap from CHANNEL rather than as a "
+                ".deb"))
         build_from_group = parser.add_mutually_exclusive_group(required=True)
         build_from_group.add_argument(
             "--branch", metavar="BRANCH", help="build from this Bazaar branch")
@@ -100,7 +108,7 @@
 
     def install(self):
         logger.info("Running install phase...")
-        deps = ["snapcraft"]
+        deps = []
         if self.args.backend == "lxd":
             # udev is installed explicitly to work around
             # https://bugs.launchpad.net/snapd/+bug/1731519.
@@ -113,7 +121,17 @@
             deps.append("git")
         if self.args.proxy_url:
             deps.extend(["python3", "socat"])
+        if not self.args.channel_snapcraft:
+            deps.append("snapcraft")
         self.backend.run(["apt-get", "-y", "install"] + deps)
+        if self.args.channel_core:
+            self.backend.run(
+                ["snap", "install",
+                 "--channel=%s" % self.args.channel_core, "core"])
+        if self.args.channel_snapcraft:
+            self.backend.run(
+                ["snap", "install", "--classic",
+                 "--channel=%s" % self.args.channel_snapcraft, "snapcraft"])
         if self.args.proxy_url:
             self.backend.copy_in(
                 os.path.join(self.slavebin, "snap-git-proxy"),

=== modified file 'lpbuildd/target/lxd.py'
--- lpbuildd/target/lxd.py	2017-11-13 15:18:07 +0000
+++ lpbuildd/target/lxd.py	2018-02-04 00:20:53 +0000
@@ -51,7 +51,7 @@
             -*) shift ;;
             systemd-udevd|systemd-udevd.service|udev|udev.service)
                 exit 0 ;;
-            snapd|snapd.socket|snapd.service)
+            snapd|snapd.*)
                 exit 0 ;;
             *)
                 echo "Not running services in chroot."
@@ -419,6 +419,14 @@
                 no_cdn_file.name,
                 "/etc/systemd/system/snapd.service.d/no-cdn.conf")
 
+        # Refreshing snaps from a timer unit during a build isn't
+        # appropriate.  Mask this, but manually so that we don't depend on
+        # systemctl existing.  This relies on /etc/systemd/system/ having
+        # been created above.
+        self.run(
+            ["ln", "-s", "/dev/null",
+             "/etc/systemd/system/snapd.refresh.timer"])
+
     def run(self, args, cwd=None, env=None, input_text=None, get_output=False,
             echo=False, **kwargs):
         """See `Backend`."""

=== modified file 'lpbuildd/target/tests/test_build_snap.py'
--- lpbuildd/target/tests/test_build_snap.py	2017-11-24 16:08:17 +0000
+++ lpbuildd/target/tests/test_build_snap.py	2018-02-04 00:20:53 +0000
@@ -54,6 +54,12 @@
         super(RanAptGet, self).__init__(["apt-get", "-y"] + list(args))
 
 
+class RanSnap(RanCommand):
+
+    def __init__(self, *args):
+        super(RanSnap, self).__init__(["snap"] + list(args))
+
+
 class RanBuildCommand(RanCommand):
 
     def __init__(self, args, cwd="/build", **kwargs):
@@ -111,7 +117,7 @@
         build_snap = parse_args(args=args).operation
         build_snap.install()
         self.assertThat(build_snap.backend.run.calls, MatchesListwise([
-            RanAptGet("install", "snapcraft", "bzr"),
+            RanAptGet("install", "bzr", "snapcraft"),
             ]))
 
     def test_install_git(self):
@@ -123,7 +129,7 @@
         build_snap = parse_args(args=args).operation
         build_snap.install()
         self.assertThat(build_snap.backend.run.calls, MatchesListwise([
-            RanAptGet("install", "snapcraft", "git"),
+            RanAptGet("install", "git", "snapcraft"),
             ]))
 
     def test_install_proxy(self):
@@ -143,12 +149,27 @@
             os.fchmod(proxy_script.fileno(), 0o755)
         build_snap.install()
         self.assertThat(build_snap.backend.run.calls, MatchesListwise([
-            RanAptGet("install", "snapcraft", "git", "python3", "socat"),
+            RanAptGet("install", "git", "python3", "socat", "snapcraft"),
             ]))
         self.assertEqual(
             (b"proxy script\n", stat.S_IFREG | 0o755),
             build_snap.backend.backend_fs["/usr/local/bin/snap-git-proxy"])
 
+    def test_install_channels(self):
+        args = [
+            "buildsnap",
+            "--backend=fake", "--series=xenial", "--arch=amd64", "1",
+            "--channel-core=candidate", "--channel-snapcraft=edge",
+            "--branch", "lp:foo", "test-snap",
+            ]
+        build_snap = parse_args(args=args).operation
+        build_snap.install()
+        self.assertThat(build_snap.backend.run.calls, MatchesListwise([
+            RanAptGet("install", "bzr"),
+            RanSnap("install", "--channel=candidate", "core"),
+            RanSnap("install", "--classic", "--channel=edge", "snapcraft"),
+            ]))
+
     def test_repo_bzr(self):
         args = [
             "buildsnap",
@@ -332,7 +353,7 @@
         build_snap.backend.run = FakeRevisionID("42")
         self.assertEqual(0, build_snap.run())
         self.assertThat(build_snap.backend.run.calls, MatchesAll(
-            AnyMatch(RanAptGet("install", "snapcraft", "bzr")),
+            AnyMatch(RanAptGet("install", "bzr", "snapcraft")),
             AnyMatch(RanBuildCommand(
                 ["bzr", "branch", "lp:foo", "test-snap"])),
             AnyMatch(RanBuildCommand(

=== modified file 'lpbuildd/target/tests/test_lxd.py'
--- lpbuildd/target/tests/test_lxd.py	2017-11-13 15:18:07 +0000
+++ lpbuildd/target/tests/test_lxd.py	2018-02-04 00:20:53 +0000
@@ -254,9 +254,14 @@
                     lxc +
                     ["mknod", "-m", "0660", "/dev/loop%d" % minor,
                      "b", "7", str(minor)]))
-        expected_args.append(
-            Equals(
-                lxc + ["mkdir", "-p", "/etc/systemd/system/snapd.service.d"]))
+        expected_args.extend([
+            Equals(
+                lxc + ["mkdir", "-p", "/etc/systemd/system/snapd.service.d"]),
+            Equals(
+                lxc +
+                ["ln", "-s", "/dev/null",
+                 "/etc/systemd/system/snapd.refresh.timer"]),
+            ])
         self.assertThat(
             [proc._args["args"] for proc in processes_fixture.procs],
             MatchesListwise(expected_args))