launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:generalise-snap-build-base into launchpad:master.

Commit message:
Implement build-base for snaps without base: bare

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1835532 in Launchpad itself: "snapcraft behavior for type: base"
  https://bugs.launchpad.net/launchpad/+bug/1835532

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

Sync up our base selection for snap builds with snapcraft:

  https://github.com/snapcore/snapcraft/blob/4.0/snapcraft/internal/meta/snap.py#L161-L181

This makes two significant changes to base selection:

 * `build-base` now wins in all cases, even if the snap does not have `base: bare`;

 * if `build-base` is not set and the snap has `type: base`, then the snap's `name` is used as the base.

If neither of those apply, then the value of `base` is used, as before.

`base: bare` without `build-base` is now an error, while previously the default base would have been used.  However, this combination is forbidden by snapcraft at the schema-validation change, so this change shouldn't matter in practice.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:generalise-snap-build-base into launchpad:master.

diff --git a/lib/lp/snappy/model/snap.py b/lib/lp/snappy/model/snap.py
index 229eaa4..4860fad 100644
--- a/lib/lp/snappy/model/snap.py
+++ b/lib/lp/snappy/model/snap.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2019 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 from __future__ import absolute_import, print_function, unicode_literals
@@ -689,18 +689,27 @@ class Snap(Storm, WebhookTargetMixin):
     @staticmethod
     def _findBase(snapcraft_data):
         """Find a suitable base for a build."""
+        base = snapcraft_data.get("base")
+        build_base = snapcraft_data.get("build-base")
+        name = snapcraft_data.get("name")
+        snap_type = snapcraft_data.get("type")
+
+        # Keep this in sync with
+        # snapcraft.internal.meta.snap.Snap.get_build_base.
         snap_base_set = getUtility(ISnapBaseSet)
-        snap_base_name = snapcraft_data.get("base")
+        if build_base is not None:
+            snap_base_name = build_base
+        elif name is not None and snap_type == "base":
+            snap_base_name = name
+        else:
+            snap_base_name = base
+
         if isinstance(snap_base_name, bytes):
             snap_base_name = snap_base_name.decode("UTF-8")
-        if snap_base_name == "bare":
-            snap_base_name = snapcraft_data.get("build-base")
-            if isinstance(snap_base_name, bytes):
-                snap_base_name = snap_base_name.decode("UTF-8")
         if snap_base_name is not None:
             return snap_base_set.getByName(snap_base_name), snap_base_name
         else:
-            return snap_base_set.getDefault(), snap_base_name
+            return snap_base_set.getDefault(), None
 
     def _pickDistroSeries(self, snap_base, snap_base_name):
         """Pick a suitable `IDistroSeries` for a build."""
diff --git a/lib/lp/snappy/tests/test_snap.py b/lib/lp/snappy/tests/test_snap.py
index 809b617..0b712fe 100644
--- a/lib/lp/snappy/tests/test_snap.py
+++ b/lib/lp/snappy/tests/test_snap.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2019 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test snap packages."""
@@ -564,14 +564,27 @@ class TestSnap(TestCaseWithFactory):
             self.assertEqual(
                 (snap_base, snap_base.name),
                 Snap._findBase({"base": "bare", "build-base": snap_base.name}))
+            self.assertEqual(
+                (snap_base, snap_base.name),
+                Snap._findBase({"build-base": snap_base.name}))
+            self.assertEqual(
+                (snap_base, snap_base.name),
+                Snap._findBase({"type": "base", "name": snap_base.name}))
         self.assertRaises(
             NoSuchSnapBase, Snap._findBase,
             {"base": "nonexistent"})
         self.assertRaises(
             NoSuchSnapBase, Snap._findBase,
+            {"base": "bare"})
+        self.assertRaises(
+            NoSuchSnapBase, Snap._findBase,
             {"base": "bare", "build-base": "nonexistent"})
+        self.assertRaises(
+            NoSuchSnapBase, Snap._findBase,
+            {"build-base": "nonexistent"})
         self.assertEqual((None, None), Snap._findBase({}))
-        self.assertEqual((None, None), Snap._findBase({"base": "bare"}))
+        self.assertEqual(
+            (None, None), Snap._findBase({"name": snap_bases[0].name}))
 
     def test__findBase_with_default(self):
         with admin_logged_in():
@@ -585,15 +598,28 @@ class TestSnap(TestCaseWithFactory):
             self.assertEqual(
                 (snap_base, snap_base.name),
                 Snap._findBase({"base": "bare", "build-base": snap_base.name}))
+            self.assertEqual(
+                (snap_base, snap_base.name),
+                Snap._findBase({"build-base": snap_base.name}))
+            self.assertEqual(
+                (snap_base, snap_base.name),
+                Snap._findBase({"type": "base", "name": snap_base.name}))
         self.assertRaises(
             NoSuchSnapBase, Snap._findBase,
             {"base": "nonexistent"})
         self.assertRaises(
             NoSuchSnapBase, Snap._findBase,
+            {"base": "bare"})
+        self.assertRaises(
+            NoSuchSnapBase, Snap._findBase,
             {"base": "bare", "build-base": "nonexistent"})
+        self.assertRaises(
+            NoSuchSnapBase, Snap._findBase,
+            {"build-base": "nonexistent"})
         self.assertEqual((snap_bases[0], None), Snap._findBase({}))
         self.assertEqual(
-            (snap_bases[0], None), Snap._findBase({"base": "bare"}))
+            (snap_bases[0], None),
+            Snap._findBase({"name": snap_bases[0].name}))
 
     def makeRequestBuildsJob(self, arch_tags, git_ref=None):
         distro = self.factory.makeDistribution()