Merge lp:~cjwatson/launchpad/snap-daily-builds-request-builds into lp:launchpad

[Colin Watson]

Make automatic builds of snaps honour build-on architectures.

[William Grant (wgrant)]

review: Approve (code)

=== modified file 'lib/lp/snappy/adapters/buildarch.py'
--- lib/lp/snappy/adapters/buildarch.py	2018-06-14 17:11:56 +0000
+++ lib/lp/snappy/adapters/buildarch.py	2018-07-30 09:08:46 +0000
@@ -122,12 +122,12 @@
 
 
 def determine_architectures_to_build(snapcraft_data, supported_arches):
-    """Return a set of architectures to build based on snapcraft.yaml.
+    """Return a list of architectures to build based on snapcraft.yaml.
 
     :param snapcraft_data: A parsed snapcraft.yaml.
     :param supported_arches: An ordered list of all architecture tags that
         we can create builds for.
-    :return: a set of `SnapBuildInstance`s.
+    :return: a list of `SnapBuildInstance`s.
     """
     architectures_list = snapcraft_data.get("architectures")
 
@@ -162,10 +162,10 @@
     if duplicates:
         raise DuplicateBuildOnError(duplicates)
 
-    architectures_to_build = set()
+    architectures_to_build = []
     for arch in architectures:
         try:
-            architectures_to_build.add(
+            architectures_to_build.append(
                 SnapBuildInstance(arch, supported_arches))
         except UnsupportedBuildOnError:
             # Snaps are allowed to declare that they build on architectures
 
=== modified file 'lib/lp/snappy/interfaces/snap.py'
--- lib/lp/snappy/interfaces/snap.py	2018-06-18 16:42:43 +0000
+++ lib/lp/snappy/interfaces/snap.py	2018-07-30 09:08:46 +0000
@@ -406,6 +406,7 @@
         """
 
     def requestBuildsFromJob(requester, archive, pocket, channels=None,
+                             allow_failures=False, fetch_snapcraft_yaml=True,
                              logger=None):
         """Synchronous part of `Snap.requestBuilds`.
 
@@ -416,6 +417,13 @@
         :param pocket: The pocket that should be targeted.
         :param channels: A dictionary mapping snap names to channels to use
             for these builds.
+        :param allow_failures: If True, log exceptions other than "already
+            pending" from individual build requests; if False, raise them to
+            the caller.
+        :param fetch_snapcraft_yaml: If True, fetch snapcraft.yaml from the
+            appropriate branch or repository and use it to decide which
+            builds to request; if False, fall back to building for all
+            supported architectures.
         :param logger: An optional logger.
         :return: A sequence of `ISnapBuild` instances.
         """
@@ -474,12 +482,17 @@
     @operation_returns_collection_of(Interface)
     @export_write_operation()
     @operation_for_version("devel")
-    def requestAutoBuilds(allow_failures=False, logger=None):
+    def requestAutoBuilds(allow_failures=False, fetch_snapcraft_yaml=False,
+                          logger=None):
         """Create and return automatic builds for this snap package.
 
         :param allow_failures: If True, log exceptions other than "already
             pending" from individual build requests; if False, raise them to
             the caller.
+        :param fetch_snapcraft_yaml: If True, fetch snapcraft.yaml from the
+            appropriate branch or repository and use it to decide which
+            builds to request; if False, fall back to building for all
+            supported architectures.
         :param logger: An optional logger.
         :raises CannotRequestAutoBuilds: if no auto_build_archive or
             auto_build_pocket is set.
 
=== modified file 'lib/lp/snappy/model/snap.py'
--- lib/lp/snappy/model/snap.py	2018-07-13 09:17:40 +0000
+++ lib/lp/snappy/model/snap.py	2018-07-30 09:08:46 +0000
@@ -552,18 +552,22 @@
         return self.getBuildRequest(job.job_id)
 
     def requestBuildsFromJob(self, requester, archive, pocket, channels=None,
+                             allow_failures=False, fetch_snapcraft_yaml=True,
                              logger=None):
         """See `ISnap`."""
-        try:
-            snapcraft_data = removeSecurityProxy(
-                getUtility(ISnapSet).getSnapcraftYaml(self))
-        except CannotFetchSnapcraftYaml as e:
-            if not e.unsupported_remote:
-                raise
-            # The only reason we can't fetch the file is because we don't
-            # support fetching from this repository's host.  In this case
-            # the best thing is to fall back to building for all supported
-            # architectures.
+        if fetch_snapcraft_yaml:
+            try:
+                snapcraft_data = removeSecurityProxy(
+                    getUtility(ISnapSet).getSnapcraftYaml(self))
+            except CannotFetchSnapcraftYaml as e:
+                if not e.unsupported_remote:
+                    raise
+                # The only reason we can't fetch the file is because we
+                # don't support fetching from this repository's host.  In
+                # this case the best thing is to fall back to building for
+                # all supported architectures.
+                snapcraft_data = {}
+        else:
             snapcraft_data = {}
         # Sort by Processor.id for determinism.  This is chosen to be the
         # same order as in BinaryPackageBuildSet.createForSource, to
@@ -589,11 +593,18 @@
                 builds.append(build)
             except SnapBuildAlreadyPending as e:
                 pass
+            except Exception as e:
+                if not allow_failures:
+                    raise
+                elif logger is not None:
+                    logger.exception(
+                        " - %s/%s/%s: %s",
+                        self.owner.name, self.name, arch, e)
         return builds
 
-    def requestAutoBuilds(self, allow_failures=False, logger=None):
+    def requestAutoBuilds(self, allow_failures=False,
+                          fetch_snapcraft_yaml=False, logger=None):
         """See `ISnap`."""
-        builds = []
         if self.auto_build_archive is None:
             raise CannotRequestAutoBuilds("auto_build_archive")
         if self.auto_build_pocket is None:
@@ -603,29 +614,10 @@
             logger.debug(
                 "Scheduling builds of snap package %s/%s",
                 self.owner.name, self.name)
-        for arch in self.getAllowedArchitectures():
-            try:
-                build = self.requestBuild(
-                    self.owner, self.auto_build_archive, arch,
-                    self.auto_build_pocket, self.auto_build_channels)
-                if logger is not None:
-                    logger.debug(
-                        " - %s/%s/%s: Build requested.",
-                        self.owner.name, self.name, arch.architecturetag)
-                builds.append(build)
-            except SnapBuildAlreadyPending as e:
-                if logger is not None:
-                    logger.warning(
-                        " - %s/%s/%s: %s",
-                        self.owner.name, self.name, arch.architecturetag, e)
-            except Exception as e:
-                if not allow_failures:
-                    raise
-                elif logger is not None:
-                    logger.exception(
-                        " - %s/%s/%s: %s",
-                        self.owner.name, self.name, arch.architecturetag, e)
-        return builds
+        return self.requestBuildsFromJob(
+            self.owner, self.auto_build_archive, self.auto_build_pocket,
+            channels=self.auto_build_channels, allow_failures=allow_failures,
+            fetch_snapcraft_yaml=fetch_snapcraft_yaml, logger=logger)
 
     def getBuildRequest(self, job_id):
         """See `ISnap`."""
@@ -1113,7 +1105,7 @@
         builds = []
         for snap in snaps:
             builds.extend(snap.requestAutoBuilds(
-                allow_failures=True, logger=logger))
+                allow_failures=True, fetch_snapcraft_yaml=True, logger=logger))
         return builds
 
     def detachFromBranch(self, branch):
 
=== modified file 'lib/lp/snappy/tests/test_snap.py'
--- lib/lp/snappy/tests/test_snap.py	2018-06-18 16:42:43 +0000
+++ lib/lp/snappy/tests/test_snap.py	2018-07-30 09:08:46 +0000
@@ -1285,9 +1285,10 @@
     def makeAutoBuildableSnap(self, **kwargs):
         processor = self.factory.makeProcessor(supports_virtualized=True)
         das = self.makeBuildableDistroArchSeries(processor=processor)
+        [git_ref] = self.factory.makeGitRefs()
         snap = self.factory.makeSnap(
             distroseries=das.distroseries, processors=[das.processor],
-            auto_build=True, **kwargs)
+            git_ref=git_ref, auto_build=True, **kwargs)
         return das, snap
 
     def test_makeAutoBuilds(self):
@@ -1296,7 +1297,11 @@
         self.assertEqual([], getUtility(ISnapSet).makeAutoBuilds())
         das, snap = self.makeAutoBuildableSnap(is_stale=True)
         logger = BufferLogger()
-        [build] = getUtility(ISnapSet).makeAutoBuilds(logger=logger)
+        with GitHostingFixture(blob=dedent("""\
+                architectures:
+                  - build-on: %s
+                """) % das.architecturetag):
+            [build] = getUtility(ISnapSet).makeAutoBuilds(logger=logger)
         self.assertThat(build, MatchesStructure.byEquality(
             requester=snap.owner, snap=snap, distro_arch_series=das,
             status=BuildStatus.NEEDSBUILD,
@@ -1319,7 +1324,11 @@
             requester=snap.owner, snap=snap, archive=snap.auto_build_archive,
             distroarchseries=das)
         logger = BufferLogger()
-        builds = getUtility(ISnapSet).makeAutoBuilds(logger=logger)
+        with GitHostingFixture(blob=dedent("""\
+                architectures:
+                  - build-on: %s
+                """) % das.architecturetag):
+            builds = getUtility(ISnapSet).makeAutoBuilds(logger=logger)
         self.assertEqual([], builds)
         self.assertEqual([], logger.getLogBuffer().splitlines())
 
@@ -1340,7 +1349,13 @@
             channels={"snapcraft": "stable"})
 
         logger = BufferLogger()
-        builds = getUtility(ISnapSet).makeAutoBuilds(logger=logger)
+        snapcraft_yaml = dedent("""\
+            architectures:
+              - build-on: %s
+              - build-on: %s
+            """) % (das1.architecturetag, das2.architecturetag)
+        with GitHostingFixture(blob=snapcraft_yaml):
+            builds = getUtility(ISnapSet).makeAutoBuilds(logger=logger)
         self.assertThat(builds, MatchesSetwise(
             MatchesStructure(
                 requester=Equals(snap1.owner), snap=Equals(snap1),
@@ -1369,14 +1384,19 @@
             removeSecurityProxy(snap).is_stale = True
             IStore(snap).flush()
         logger = BufferLogger()
-        builds = getUtility(ISnapSet).makeAutoBuilds(logger=logger)
+        with GitHostingFixture(blob=snapcraft_yaml):
+            builds = getUtility(ISnapSet).makeAutoBuilds(logger=logger)
         self.assertEqual([], builds)
         self.assertEqual([], logger.getLogBuffer().splitlines())
 
     def test_makeAutoBuilds_skips_non_stale_snaps(self):
         # ISnapSet.makeAutoBuilds skips snap packages that are not stale.
         das, snap = self.makeAutoBuildableSnap(is_stale=False)
-        self.assertEqual([], getUtility(ISnapSet).makeAutoBuilds())
+        with GitHostingFixture(blob=dedent("""\
+                architectures:
+                  - build-on: %s
+                """) % das.architecturetag):
+            self.assertEqual([], getUtility(ISnapSet).makeAutoBuilds())
 
     def test_makeAutoBuilds_skips_pending(self):
         # ISnapSet.makeAutoBuilds skips snap packages that already have
@@ -1389,14 +1409,15 @@
             distroarchseries=das,
             date_created=datetime.now(pytz.UTC) - timedelta(days=1))
         logger = BufferLogger()
-        builds = getUtility(ISnapSet).makeAutoBuilds(logger=logger)
+        with GitHostingFixture(blob=dedent("""\
+                architectures:
+                  - build-on: %s
+                """) % das.architecturetag):
+            builds = getUtility(ISnapSet).makeAutoBuilds(logger=logger)
         self.assertEqual([], builds)
         expected_log_entries = [
             "DEBUG Scheduling builds of snap package %s/%s" % (
                 snap.owner.name, snap.name),
-            "WARNING  - %s/%s/%s: An identical build of this snap package "
-            "is already pending." % (
-                snap.owner.name, snap.name, das.architecturetag),
             ]
         self.assertEqual(
             expected_log_entries, logger.getLogBuffer().splitlines())
@@ -1410,7 +1431,11 @@
             distroarchseries=das,
             date_created=datetime.now(pytz.UTC) - timedelta(days=1),
             status=BuildStatus.FULLYBUILT, duration=timedelta(minutes=1))
-        builds = getUtility(ISnapSet).makeAutoBuilds()
+        with GitHostingFixture(blob=dedent("""\
+                architectures:
+                  - build-on: %s
+                """) % das.architecturetag):
+            builds = getUtility(ISnapSet).makeAutoBuilds()
         self.assertEqual(1, len(builds))
 
     def test_makeAutoBuilds_with_older_and_newer_builds(self):
@@ -1423,7 +1448,11 @@
                 archive=snap.auto_build_archive, distroarchseries=das,
                 date_created=datetime.now(pytz.UTC) - timediff,
                 status=BuildStatus.FULLYBUILT, duration=timedelta(minutes=1))
-        self.assertEqual([], getUtility(ISnapSet).makeAutoBuilds())
+        with GitHostingFixture(blob=dedent("""\
+                architectures:
+                  - build-on: %s
+                """) % das.architecturetag):
+            self.assertEqual([], getUtility(ISnapSet).makeAutoBuilds())
 
     def test_makeAutoBuilds_with_recent_build_from_different_archive(self):
         # If a snap package has been built recently but from an archive
@@ -1434,9 +1463,49 @@
             requester=snap.owner, snap=snap, distroarchseries=das,
             date_created=datetime.now(pytz.UTC) - timedelta(minutes=30),
             status=BuildStatus.FULLYBUILT, duration=timedelta(minutes=1))
-        builds = getUtility(ISnapSet).makeAutoBuilds()
+        with GitHostingFixture(blob=dedent("""\
+                architectures:
+                  - build-on: %s
+                """) % das.architecturetag):
+            builds = getUtility(ISnapSet).makeAutoBuilds()
         self.assertEqual(1, len(builds))
 
+    def test_makeAutoBuilds_honours_explicit_architectures(self):
+        # ISnapSet.makeAutoBuilds honours an explicit list of architectures
+        # in the snap's snapcraft.yaml.
+        das1, snap = self.makeAutoBuildableSnap(is_stale=True)
+        das2 = self.makeBuildableDistroArchSeries(
+            distroseries=das1.distroseries,
+            processor=self.factory.makeProcessor(supports_virtualized=True))
+        das3 = self.makeBuildableDistroArchSeries(
+            distroseries=das1.distroseries,
+            processor=self.factory.makeProcessor(supports_virtualized=True))
+        snap.setProcessors([das1.processor, das2.processor, das3.processor])
+        logger = BufferLogger()
+        with GitHostingFixture(blob=dedent("""\
+                architectures:
+                  - build-on: %s
+                  - build-on: %s
+                """) % (das1.architecturetag, das2.architecturetag)):
+            builds = getUtility(ISnapSet).makeAutoBuilds(logger=logger)
+        self.assertThat(set(builds), MatchesSetwise(*[
+            MatchesStructure.byEquality(
+                requester=snap.owner, snap=snap, distro_arch_series=das,
+                status=BuildStatus.NEEDSBUILD)
+            for das in (das1, das2)
+            ]))
+        expected_log_entries = [
+            "DEBUG Scheduling builds of snap package %s/%s" % (
+                snap.owner.name, snap.name),
+            "DEBUG  - %s/%s/%s: Build requested." % (
+                snap.owner.name, snap.name, das1.architecturetag),
+            "DEBUG  - %s/%s/%s: Build requested." % (
+                snap.owner.name, snap.name, das2.architecturetag),
+            ]
+        self.assertEqual(
+            expected_log_entries, logger.getLogBuffer().splitlines())
+        self.assertFalse(snap.is_stale)
+
     def test_detachFromBranch(self):
         # ISnapSet.detachFromBranch clears the given Bazaar branch from all
         # Snaps.