launchpad-reviewers team mailing list archive

[Colin Watson <mp+437624@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:snap-arch-all into launchpad:master.

Commit message:
Handle "all" in snapcraft architectures declaration

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/437624

This has apparently been supported by snapcraft for quite some time, but we overlooked it while implementing `architectures` in Launchpad.  Reported by James Henstridge.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:snap-arch-all into launchpad:master.

diff --git a/lib/lp/snappy/adapters/buildarch.py b/lib/lp/snappy/adapters/buildarch.py
index eb87a7d..d2dc8fd 100644
--- a/lib/lp/snappy/adapters/buildarch.py
+++ b/lib/lp/snappy/adapters/buildarch.py
@@ -39,6 +39,24 @@ class IncompatibleArchitecturesStyleError(SnapArchitecturesParserError):
         )
 
 
+class AllConflictInBuildForError(SnapArchitecturesParserError):
+    """Error for when `build-for` contains `all` and another architecture."""
+
+    def __init__(self):
+        super().__init__(
+            "'build-for' contains both 'all' and another architecture name"
+        )
+
+
+class AllConflictInBuildOnError(SnapArchitecturesParserError):
+    """Error for when `build-on` contains `all` and another architecture."""
+
+    def __init__(self):
+        super().__init__(
+            "'build-on' contains both 'all' and another architecture name"
+        )
+
+
 class DuplicateBuildOnError(SnapArchitecturesParserError):
     """Error for when multiple `build-on`s include the same architecture."""
 
@@ -134,14 +152,21 @@ class SnapBuildInstance:
         :param supported_architectures: List of supported architectures,
             sorted by priority.
         """
+        build_on = architecture.build_on
+        # "all" indicates that the architecture doesn't matter.  Try to pick
+        # an appropriate architecture in this case.
+        # `Snap.requestBuildsFromJob` orders `supported_architectures` such
+        # that we can reasonably pick the first one if all else fails.
+        if "all" in build_on:
+            build_on = architecture.build_for
+            if "all" in build_on:
+                build_on = supported_architectures[0]
         try:
             self.architecture = next(
-                arch
-                for arch in supported_architectures
-                if arch in architecture.build_on
+                arch for arch in supported_architectures if arch in build_on
             )
         except StopIteration:
-            raise UnsupportedBuildOnError(architecture.build_on)
+            raise UnsupportedBuildOnError(build_on)
 
         self.target_architectures = architecture.build_for
         self.required = architecture.build_error != "ignore"
@@ -186,6 +211,12 @@ def determine_architectures_to_build(
             SnapArchitecture(build_on=a) for a in supported_arches
         ]
 
+    for arch in architectures:
+        if "all" in arch.build_on and len(arch.build_on) > 1:
+            raise AllConflictInBuildOnError()
+        if "all" in arch.build_for and len(arch.build_for) > 1:
+            raise AllConflictInBuildForError()
+
     allow_duplicate_build_on = (
         snap_base
         and snap_base.features.get(SnapBaseFeature.ALLOW_DUPLICATE_BUILD_ON)
diff --git a/lib/lp/snappy/adapters/tests/test_buildarch.py b/lib/lp/snappy/adapters/tests/test_buildarch.py
index 8a9ef2e..399ea76 100644
--- a/lib/lp/snappy/adapters/tests/test_buildarch.py
+++ b/lib/lp/snappy/adapters/tests/test_buildarch.py
@@ -5,6 +5,8 @@ from testscenarios import WithScenarios, load_tests_apply_scenarios
 from testtools.matchers import HasLength
 
 from lp.snappy.adapters.buildarch import (
+    AllConflictInBuildForError,
+    AllConflictInBuildOnError,
     DuplicateBuildOnError,
     SnapArchitecture,
     SnapBuildInstance,
@@ -154,6 +156,28 @@ class TestSnapBuildInstance(WithScenarios, TestCase):
                 "expected_required": False,
             },
         ),
+        (
+            "build on all",
+            {
+                "architecture": SnapArchitecture(build_on=["all"]),
+                "supported_architectures": ["amd64", "i386", "armhf"],
+                "expected_architecture": "amd64",
+                "expected_target_architectures": ["all"],
+                "expected_required": True,
+            },
+        ),
+        (
+            "build on all, build for i386",
+            {
+                "architecture": SnapArchitecture(
+                    build_on=["all"], build_for="i386"
+                ),
+                "supported_architectures": ["amd64", "i386", "armhf"],
+                "expected_architecture": "i386",
+                "expected_target_architectures": ["i386"],
+                "expected_required": True,
+            },
+        ),
     ]
 
     def test_build_instance(self):
@@ -397,6 +421,24 @@ class TestDetermineArchitecturesToBuild(WithScenarios, TestCaseWithFactory):
             },
         ),
         (
+            "build-on contains both all and another architecture",
+            {
+                "architectures": [{"build-on": ["all", "amd64"]}],
+                "supported_architectures": ["amd64"],
+                "expected_exception": AllConflictInBuildOnError,
+            },
+        ),
+        (
+            "build-for contains both all and another architecture",
+            {
+                "architectures": [
+                    {"build-on": "amd64", "build-for": ["amd64", "all"]}
+                ],
+                "supported_architectures": ["amd64"],
+                "expected_exception": AllConflictInBuildForError,
+            },
+        ),
+        (
             "multiple build-for for the same build-on",
             {
                 "snap_base_features": {