Merge lp:~cjwatson/launchpad/snap-request-build-explicit-arch-consistency into lp:launchpad

[Colin Watson]

Use build request jobs for all snap build requests in the web UI.

Previously build request jobs were only used when people didn't explicitly select architectures, which was very confusing since it meant bases weren't honoured properly.

[William Grant (wgrant)]

review: Approve (code)

=== modified file 'lib/lp/snappy/browser/snap.py'
--- lib/lp/snappy/browser/snap.py	2019-05-16 10:21:14 +0000
+++ lib/lp/snappy/browser/snap.py	2019-06-21 11:37:21 +0000
@@ -57,7 +57,6 @@
 from lp.registry.enums import VCSType
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.services.features import getFeatureFlag
-from lp.services.helpers import english_list
 from lp.services.propertycache import cachedproperty
 from lp.services.scripts import log
 from lp.services.utils import seconds_since_epoch
@@ -92,7 +91,6 @@
     MissingSnapcraftYaml,
     NoSuchSnap,
     SNAP_PRIVATE_FEATURE_FLAG,
-    SnapBuildAlreadyPending,
     SnapPrivateFeatureDisabled,
     )
 from lp.snappy.interfaces.snapbuild import ISnapBuildSet
@@ -256,20 +254,6 @@
     return items
 
 
-def new_builds_notification_text(builds, already_pending=None):
-    nr_builds = len(builds)
-    if not nr_builds:
-        builds_text = "All requested builds are already queued."
-    elif nr_builds == 1:
-        builds_text = "1 new build has been queued."
-    else:
-        builds_text = "%d new builds have been queued." % nr_builds
-    if nr_builds and already_pending:
-        return structured("<p>%s</p><p>%s</p>", builds_text, already_pending)
-    else:
-        return builds_text
-
-
 class SnapRequestBuildsView(LaunchpadFormView):
     """A view for requesting builds of a snap package."""
 
@@ -328,45 +312,18 @@
             'channels': self.context.auto_build_channels,
             }
 
-    def requestBuild(self, data):
-        """User action for requesting a number of builds.
-
-        We raise exceptions for most errors, but if there's already a
-        pending build for a particular architecture, we simply record that
-        so that other builds can be queued and a message displayed to the
-        caller.
-        """
-        informational = {}
-        builds = []
-        already_pending = []
-        for arch in data['distro_arch_series']:
-            try:
-                build = self.context.requestBuild(
-                    self.user, data['archive'], arch, data['pocket'],
-                    channels=data['channels'])
-                builds.append(build)
-            except SnapBuildAlreadyPending:
-                already_pending.append(arch)
-        if already_pending:
-            informational['already_pending'] = (
-                "An identical build is already pending for %s." %
-                english_list(arch.architecturetag for arch in already_pending))
-        return builds, informational
-
     @action('Request builds', name='request')
     def request_action(self, action, data):
         if data.get('distro_arch_series', []):
-            builds, informational = self.requestBuild(data)
-            already_pending = informational.get('already_pending')
-            notification_text = new_builds_notification_text(
-                builds, already_pending)
-            self.request.response.addNotification(notification_text)
+            architectures = [
+                arch.architecturetag for arch in data['distro_arch_series']]
         else:
-            self.context.requestBuilds(
-                self.user, data['archive'], data['pocket'],
-                channels=data['channels'])
-            self.request.response.addNotification(
-                _('Builds will be dispatched soon.'))
+            architectures = None
+        self.context.requestBuilds(
+            self.user, data['archive'], data['pocket'],
+            architectures=architectures, channels=data['channels'])
+        self.request.response.addNotification(
+            _('Builds will be dispatched soon.'))
         self.next_url = self.cancel_url
 
 

=== modified file 'lib/lp/snappy/browser/tests/test_snap.py'
--- lib/lp/snappy/browser/tests/test_snap.py	2019-06-11 09:39:29 +0000
+++ lib/lp/snappy/browser/tests/test_snap.py	2019-06-21 11:37:21 +0000
@@ -29,6 +29,7 @@
     AfterPreprocessing,
     Equals,
     Is,
+    MatchesDict,
     MatchesListwise,
     MatchesSetwise,
     MatchesStructure,
@@ -1690,8 +1691,8 @@
             Unauthorized, self.getViewBrowser, self.snap, "+request-builds")
 
     def test_request_builds_with_architectures_action(self):
-        # Requesting a build with architectures selected creates pending
-        # builds.
+        # Requesting a build with architectures selected creates a pending
+        # build request limited to those architectures.
         browser = self.getViewBrowser(
             self.snap, "+request-builds", user=self.person)
         browser.getControl("amd64").selected = True
@@ -1699,21 +1700,24 @@
         browser.getControl("Request builds").click()
 
         login_person(self.person)
-        builds = self.snap.pending_builds
-        self.assertContentEqual(
-            [self.ubuntu.main_archive], set(build.archive for build in builds))
-        self.assertContentEqual(
-            ["amd64", "i386"],
-            [build.distro_arch_series.architecturetag for build in builds])
-        self.assertContentEqual(
-            [PackagePublishingPocket.UPDATES],
-            set(build.pocket for build in builds))
-        self.assertContentEqual(
-            [2510], set(build.buildqueue_record.lastscore for build in builds))
+        [request] = self.snap.pending_build_requests
+        self.assertThat(removeSecurityProxy(request), MatchesStructure(
+            snap=Equals(self.snap),
+            status=Equals(SnapBuildRequestStatus.PENDING),
+            error_message=Is(None),
+            builds=AfterPreprocessing(list, Equals([])),
+            archive=Equals(self.ubuntu.main_archive),
+            _job=MatchesStructure(
+                requester=Equals(self.person),
+                pocket=Equals(PackagePublishingPocket.UPDATES),
+                channels=Equals({}),
+                architectures=MatchesSetwise(Equals("amd64"), Equals("i386")),
+                )))
 
     def test_request_builds_with_architectures_ppa(self):
         # Selecting a different archive with architectures selected creates
-        # builds in that archive.
+        # a build request targeting that archive and limited to those
+        # architectures.
         ppa = self.factory.makeArchive(
             distribution=self.ubuntu, owner=self.person, name="snap-ppa")
         browser = self.getViewBrowser(
@@ -1725,12 +1729,15 @@
         browser.getControl("Request builds").click()
 
         login_person(self.person)
-        builds = self.snap.pending_builds
-        self.assertEqual([ppa], [build.archive for build in builds])
+        [request] = self.snap.pending_build_requests
+        self.assertThat(request, MatchesStructure(
+            archive=Equals(ppa),
+            architectures=MatchesSetwise(Equals("amd64"))))
 
     def test_request_builds_with_architectures_channels(self):
-        # Selecting different channels with architectures selected creates
-        # builds using those channels.
+        # Selecting different channels with architectures selected creates a
+        # build request using those channels and limited to those
+        # architectures.
         browser = self.getViewBrowser(
             self.snap, "+request-builds", user=self.person)
         browser.getControl(name="field.channels.core").value = "edge"
@@ -1739,25 +1746,10 @@
         browser.getControl("Request builds").click()
 
         login_person(self.person)
-        builds = self.snap.pending_builds
-        self.assertEqual(
-            [{"core": "edge"}], [build.channels for build in builds])
-
-    def test_request_builds_with_architectures_rejects_duplicate(self):
-        # A duplicate build request with architectures selected causes a
-        # notification.
-        self.snap.requestBuild(
-            self.person, self.ubuntu.main_archive, self.distroseries["amd64"],
-            PackagePublishingPocket.UPDATES)
-        browser = self.getViewBrowser(
-            self.snap, "+request-builds", user=self.person)
-        browser.getControl("amd64").selected = True
-        browser.getControl("i386").selected = True
-        browser.getControl("Request builds").click()
-        main_text = extract_text(find_main_content(browser.contents))
-        self.assertIn("1 new build has been queued.", main_text)
-        self.assertIn(
-            "An identical build is already pending for amd64.", main_text)
+        [request] = self.snap.pending_build_requests
+        self.assertThat(request, MatchesStructure(
+            channels=MatchesDict({"core": Equals("edge")}),
+            architectures=MatchesSetwise(Equals("amd64"))))
 
     def test_request_builds_no_architectures_action(self):
         # Requesting a build with no architectures selected creates a
@@ -1779,7 +1771,8 @@
             _job=MatchesStructure(
                 requester=Equals(self.person),
                 pocket=Equals(PackagePublishingPocket.UPDATES),
-                channels=Equals({}))))
+                channels=Equals({}),
+                architectures=Is(None))))
 
     def test_request_builds_no_architectures_ppa(self):
         # Selecting a different archive with no architectures selected
@@ -1796,7 +1789,9 @@
 
         login_person(self.person)
         [request] = self.snap.pending_build_requests
-        self.assertEqual(ppa, request.archive)
+        self.assertThat(request, MatchesStructure(
+            archive=Equals(ppa),
+            architectures=Is(None)))
 
     def test_request_builds_no_architectures_channels(self):
         # Selecting different channels with no architectures selected

=== modified file 'lib/lp/snappy/interfaces/snap.py'
--- lib/lp/snappy/interfaces/snap.py	2019-05-22 18:33:10 +0000
+++ lib/lp/snappy/interfaces/snap.py	2019-06-21 11:37:21 +0000
@@ -76,6 +76,7 @@
     Dict,
     Int,
     List,
+    Set,
     Text,
     TextLine,
     )
@@ -336,6 +337,10 @@
         title=_("Source snap channels for builds produced by this request"),
         key_type=TextLine(), required=False, readonly=True)
 
+    architectures = Set(
+        title=_("If set, this request is limited to these architecture tags"),
+        value_type=TextLine(), required=False, readonly=True)
+
 
 class ISnapView(Interface):
     """`ISnap` attributes that require launchpad.View permission."""
@@ -437,8 +442,9 @@
         """
 
     def requestBuildsFromJob(requester, archive, pocket, channels=None,
-                             allow_failures=False, fetch_snapcraft_yaml=True,
-                             build_request=None, logger=None):
+                             architectures=None, allow_failures=False,
+                             fetch_snapcraft_yaml=True, build_request=None,
+                             logger=None):
         """Synchronous part of `Snap.requestBuilds`.
 
         Request that the snap package be built for relevant architectures.
@@ -448,6 +454,9 @@
         :param pocket: The pocket that should be targeted.
         :param channels: A dictionary mapping snap names to channels to use
             for these builds.
+        :param architectures: If not None, limit builds to architectures
+            with these architecture tags (in addition to any other
+            applicable constraints).
         :param allow_failures: If True, log exceptions other than "already
             pending" from individual build requests; if False, raise them to
             the caller.

=== modified file 'lib/lp/snappy/interfaces/snapjob.py'
--- lib/lp/snappy/interfaces/snapjob.py	2019-05-22 18:33:10 +0000
+++ lib/lp/snappy/interfaces/snapjob.py	2019-06-21 11:37:21 +0000
@@ -22,6 +22,7 @@
     Datetime,
     Dict,
     List,
+    Set,
     TextLine,
     )
 
@@ -78,6 +79,10 @@
             "are supported."),
         key_type=TextLine(), required=False, readonly=True)
 
+    architectures = Set(
+        title=_("If set, limit builds to these architecture tags."),
+        value_type=TextLine(), required=False, readonly=True)
+
     date_created = Datetime(
         title=_("Time when this job was created."),
         required=True, readonly=True)
@@ -101,7 +106,7 @@
 
 class ISnapRequestBuildsJobSource(IJobSource):
 
-    def create(snap, requester, archive, pocket, channels):
+    def create(snap, requester, archive, pocket, channels, architectures=None):
         """Request builds of a snap package.
 
         :param snap: The snap package to build.
@@ -110,6 +115,9 @@
         :param pocket: The pocket that should be targeted.
         :param channels: A dictionary mapping snap names to channels to use
             for these builds.
+        :param architectures: If not None, limit builds to architectures
+            with these architecture tags (in addition to any other
+            applicable constraints).
         """
 
     def findBySnap(snap, statuses=None, job_ids=None):

=== modified file 'lib/lp/snappy/model/snap.py'
--- lib/lp/snappy/model/snap.py	2019-05-16 10:21:14 +0000
+++ lib/lp/snappy/model/snap.py	2019-06-21 11:37:21 +0000
@@ -249,6 +249,11 @@
         """See `ISnapBuildRequest`."""
         return self._job.channels
 
+    @property
+    def architectures(self):
+        """See `ISnapBuildRequest`."""
+        return self._job.architectures
+
 
 @implementer(ISnap, IHasOwner)
 class Snap(Storm, WebhookTargetMixin):
@@ -655,11 +660,13 @@
         notify(ObjectCreatedEvent(build, user=requester))
         return build
 
-    def requestBuilds(self, requester, archive, pocket, channels=None):
+    def requestBuilds(self, requester, archive, pocket, channels=None,
+                      architectures=None):
         """See `ISnap`."""
         self._checkRequestBuild(requester, archive)
         job = getUtility(ISnapRequestBuildsJobSource).create(
-            self, requester, archive, pocket, channels)
+            self, requester, archive, pocket, channels,
+            architectures=architectures)
         return self.getBuildRequest(job.job_id)
 
     @staticmethod
@@ -694,7 +701,8 @@
         else:
             return channels
 
-    def requestBuildsFromJob(self, requester, archive, pocket, channels=None,
+    def requestBuildsFromJob(self, requester, archive, pocket,
+                             channels=None, architectures=None,
                              allow_failures=False, fetch_snapcraft_yaml=True,
                              build_request=None, logger=None):
         """See `ISnap`."""
@@ -731,7 +739,9 @@
             supported_arches = OrderedDict(
                 (das.architecturetag, das) for das in sorted(
                     self.getAllowedArchitectures(distro_series),
-                    key=attrgetter("processor.id")))
+                    key=attrgetter("processor.id"))
+                if (architectures is None or
+                    das.architecturetag in architectures))
             architectures_to_build = determine_architectures_to_build(
                 snapcraft_data, supported_arches.keys())
         except Exception as e:

=== modified file 'lib/lp/snappy/model/snapjob.py'
--- lib/lp/snappy/model/snapjob.py	2018-10-09 09:25:19 +0000
+++ lib/lp/snappy/model/snapjob.py	2019-06-21 11:37:21 +0000
@@ -1,4 +1,4 @@
-# Copyright 2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2018-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Snap package jobs."""
@@ -184,13 +184,18 @@
     config = config.ISnapRequestBuildsJobSource
 
     @classmethod
-    def create(cls, snap, requester, archive, pocket, channels):
+    def create(cls, snap, requester, archive, pocket, channels,
+               architectures=None):
         """See `ISnapRequestBuildsJobSource`."""
         metadata = {
             "requester": requester.id,
             "archive": archive.id,
             "pocket": pocket.value,
             "channels": channels,
+            # Really a set or None, but sets aren't directly
+            # JSON-serialisable.
+            "architectures": (
+                list(architectures) if architectures is not None else None),
             }
         snap_job = SnapJob(snap, cls.class_job_type, metadata)
         job = cls(snap_job)
@@ -292,6 +297,12 @@
         return self.metadata["channels"]
 
     @property
+    def architectures(self):
+        """See `ISnapRequestBuildsJob`."""
+        architectures = self.metadata["architectures"]
+        return set(architectures) if architectures is not None else None
+
+    @property
     def date_created(self):
         """See `ISnapRequestBuildsJob`."""
         return self.context.job.date_created
@@ -346,6 +357,7 @@
         try:
             self.builds = self.snap.requestBuildsFromJob(
                 requester, archive, self.pocket, channels=self.channels,
+                architectures=self.architectures,
                 build_request=self.build_request, logger=log)
             self.error_message = None
         except self.retry_error_types:

=== modified file 'lib/lp/snappy/tests/test_snap.py'
--- lib/lp/snappy/tests/test_snap.py	2019-06-20 13:16:35 +0000
+++ lib/lp/snappy/tests/test_snap.py	2019-06-21 11:37:21 +0000
@@ -475,7 +475,9 @@
             status=Equals(SnapBuildRequestStatus.PENDING),
             error_message=Is(None),
             builds=AfterPreprocessing(set, MatchesSetwise()),
-            archive=Equals(snap.distro_series.main_archive)))
+            archive=Equals(snap.distro_series.main_archive),
+            channels=MatchesDict({"snapcraft": Equals("edge")}),
+            architectures=Is(None)))
         [job] = getUtility(ISnapRequestBuildsJobSource).iterReady()
         self.assertThat(job, MatchesStructure(
             job_id=Equals(request.id),
@@ -484,7 +486,8 @@
             requester=Equals(snap.owner.teamowner),
             archive=Equals(snap.distro_series.main_archive),
             pocket=Equals(PackagePublishingPocket.UPDATES),
-            channels=Equals({"snapcraft": "edge"})))
+            channels=Equals({"snapcraft": "edge"}),
+            architectures=Is(None)))
 
     def test_requestBuilds_without_distroseries(self):
         # requestBuilds schedules a job for a snap without a distroseries.
@@ -502,16 +505,51 @@
             status=Equals(SnapBuildRequestStatus.PENDING),
             error_message=Is(None),
             builds=AfterPreprocessing(set, MatchesSetwise()),
-            archive=Equals(archive)))
-        [job] = getUtility(ISnapRequestBuildsJobSource).iterReady()
-        self.assertThat(job, MatchesStructure(
-            job_id=Equals(request.id),
-            job=MatchesStructure.byEquality(status=JobStatus.WAITING),
-            snap=Equals(snap),
-            requester=Equals(snap.owner.teamowner),
-            archive=Equals(archive),
-            pocket=Equals(PackagePublishingPocket.UPDATES),
-            channels=Equals({"snapcraft": "edge"})))
+            archive=Equals(archive),
+            channels=MatchesDict({"snapcraft": Equals("edge")}),
+            architectures=Is(None)))
+        [job] = getUtility(ISnapRequestBuildsJobSource).iterReady()
+        self.assertThat(job, MatchesStructure(
+            job_id=Equals(request.id),
+            job=MatchesStructure.byEquality(status=JobStatus.WAITING),
+            snap=Equals(snap),
+            requester=Equals(snap.owner.teamowner),
+            archive=Equals(archive),
+            pocket=Equals(PackagePublishingPocket.UPDATES),
+            channels=Equals({"snapcraft": "edge"}),
+            architectures=Is(None)))
+
+    def test_requestBuilds_with_architectures(self):
+        # If asked to build for particular architectures, requestBuilds
+        # passes those through to the job.
+        snap = self.factory.makeSnap()
+        now = get_transaction_timestamp(IStore(snap))
+        with person_logged_in(snap.owner.teamowner):
+            request = snap.requestBuilds(
+                snap.owner.teamowner, snap.distro_series.main_archive,
+                PackagePublishingPocket.UPDATES,
+                channels={"snapcraft": "edge"},
+                architectures={"amd64", "i386"})
+        self.assertThat(request, MatchesStructure(
+            date_requested=Equals(now),
+            date_finished=Is(None),
+            snap=Equals(snap),
+            status=Equals(SnapBuildRequestStatus.PENDING),
+            error_message=Is(None),
+            builds=AfterPreprocessing(set, MatchesSetwise()),
+            archive=Equals(snap.distro_series.main_archive),
+            channels=MatchesDict({"snapcraft": Equals("edge")}),
+            architectures=MatchesSetwise(Equals("amd64"), Equals("i386"))))
+        [job] = getUtility(ISnapRequestBuildsJobSource).iterReady()
+        self.assertThat(job, MatchesStructure(
+            job_id=Equals(request.id),
+            job=MatchesStructure.byEquality(status=JobStatus.WAITING),
+            snap=Equals(snap),
+            requester=Equals(snap.owner.teamowner),
+            archive=Equals(snap.distro_series.main_archive),
+            pocket=Equals(PackagePublishingPocket.UPDATES),
+            channels=Equals({"snapcraft": "edge"}),
+            architectures=MatchesSetwise(Equals("amd64"), Equals("i386"))))
 
     def test__findBase(self):
         snap_base_set = getUtility(ISnapBaseSet)
@@ -585,7 +623,7 @@
         with person_logged_in(job.requester):
             builds = job.snap.requestBuildsFromJob(
                 job.requester, job.archive, job.pocket,
-                removeSecurityProxy(job.channels),
+                channels=removeSecurityProxy(job.channels),
                 build_request=job.build_request)
         self.assertRequestedBuildsMatch(builds, job, ["sparc"], job.channels)
 
@@ -601,11 +639,29 @@
         with person_logged_in(job.requester):
             builds = job.snap.requestBuildsFromJob(
                 job.requester, job.archive, job.pocket,
-                removeSecurityProxy(job.channels),
+                channels=removeSecurityProxy(job.channels),
                 build_request=job.build_request)
         self.assertRequestedBuildsMatch(
             builds, job, ["mips64el", "riscv64"], job.channels)
 
+    def test_requestBuildsFromJob_architectures_parameter(self):
+        # If an explicit set of architectures was given as a parameter,
+        # requestBuildsFromJob intersects those with any other constraints
+        # when requesting builds.
+        self.useFixture(GitHostingFixture(blob="name: foo\n"))
+        job = self.makeRequestBuildsJob(["avr", "mips64el", "riscv64"])
+        self.assertEqual(
+            get_transaction_timestamp(IStore(job.snap)), job.date_created)
+        transaction.commit()
+        with person_logged_in(job.requester):
+            builds = job.snap.requestBuildsFromJob(
+                job.requester, job.archive, job.pocket,
+                channels=removeSecurityProxy(job.channels),
+                architectures={"avr", "riscv64"},
+                build_request=job.build_request)
+        self.assertRequestedBuildsMatch(
+            builds, job, ["avr", "riscv64"], job.channels)
+
     def test_requestBuildsFromJob_no_distroseries_explicit_base(self):
         # If the snap doesn't specify a distroseries but has an explicit
         # base, requestBuildsFromJob requests builds for the appropriate

=== modified file 'lib/lp/snappy/tests/test_snapjob.py'
--- lib/lp/snappy/tests/test_snapjob.py	2018-09-10 11:18:42 +0000
+++ lib/lp/snappy/tests/test_snapjob.py	2019-06-21 11:37:21 +0000
@@ -1,4 +1,4 @@
-# Copyright 2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2018-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for snap package jobs."""
@@ -133,6 +133,46 @@
                     channels=Equals({"core": "stable"}))
                 for arch in ("avr2001", "x32")]))))
 
+    def test_run_with_architectures(self):
+        # If the user explicitly requested architectures, the job passes
+        # those through when requesting builds, intersecting them with other
+        # constraints.
+        distroseries, processors = self.makeSeriesAndProcessors(
+            ["avr2001", "sparc64", "x32"])
+        [git_ref] = self.factory.makeGitRefs()
+        snap = self.factory.makeSnap(
+            git_ref=git_ref, distroseries=distroseries, processors=processors)
+        expected_date_created = get_transaction_timestamp(IStore(snap))
+        job = SnapRequestBuildsJob.create(
+            snap, snap.registrant, distroseries.main_archive,
+            PackagePublishingPocket.RELEASE, {"core": "stable"},
+            architectures=["sparc64", "x32"])
+        snapcraft_yaml = dedent("""\
+            architectures:
+              - build-on: avr2001
+              - build-on: x32
+            """)
+        self.useFixture(GitHostingFixture(blob=snapcraft_yaml))
+        with dbuser(config.ISnapRequestBuildsJobSource.dbuser):
+            JobRunner([job]).runAll()
+        now = get_transaction_timestamp(IStore(snap))
+        self.assertEmailQueueLength(0)
+        self.assertThat(job, MatchesStructure(
+            job=MatchesStructure.byEquality(status=JobStatus.COMPLETED),
+            date_created=Equals(expected_date_created),
+            date_finished=MatchesAll(
+                GreaterThan(expected_date_created), LessThan(now)),
+            error_message=Is(None),
+            builds=AfterPreprocessing(set, MatchesSetwise(
+                MatchesStructure(
+                    build_request=MatchesStructure.byEquality(id=job.job.id),
+                    requester=Equals(snap.registrant),
+                    snap=Equals(snap),
+                    archive=Equals(distroseries.main_archive),
+                    distro_arch_series=Equals(distroseries["x32"]),
+                    pocket=Equals(PackagePublishingPocket.RELEASE),
+                    channels=Equals({"core": "stable"}))))))
+
     def test_run_failed(self):
         # A failed run sets the job status to FAILED and records the error
         # message.