Merge lp:~sil2100/ubuntu-cdimage/cdimage-to-livefs-map into lp:ubuntu-cdimage

[Łukasz Zemczak]

Add a etc/cdimage-to-livefs-map mapping config file to streamline all the various cdimage->livefs mappings done implicitly in code. We also add the mapping bits for generic arm64 and amd64 images while at it.

Add a etc/cdimage-to-livefs-map mapping config file to streamline all the various cdimage->livefs mappings done implicitly in code. We also add the mapping bits for generic arm64 and amd64 images while at it.

This was proposed by xnox when we originally discussed the generic image changes. I personally HATE that right now we have all those weird differences between ubuntu-cdimage and live-build SUBARCH, PROJECT and SUBPROJECT definitions. In my mind, since we own both projects, we should simply be consistent across both projects and always plan in such a way to not confuse people by doing some translations from like one subarch to another. But I know all this requires refactoring of both cdimage and livecd-rootfs.

What I hate even more is implicitness of such mappings. I am a fan of code that's consistent and easy to read, without having to know specific hidden 'quirks'. This is why today I decided to go on with Dimitri's idea of having an explicit cdimage -> live mapping file where any custom mappings like these can go. This way new developers will know about what differences are and why (as we can leave comments for those there).

I added some unit tests for the new functionality which now passes and made sure existing test also pass. Well, the ones that should pass. There are some unrelated unit test failures which I'd like to fix with some other MP.

[Łukasz Zemczak (sil2100)]

Please note that this was written in a heat of the moment in a very fast pace, so it might not be perfect. So a review is more than welcome!

[Dimitri John Ledkov (xnox)]

Please add "SERIES" filter column. Such that we can have correct per-series mappings defined.

Not sure about "cdimage-to-livefs-map" name, maybe it is "cdimage-to-livecd-rootfs-map".

[Łukasz Zemczak (sil2100)]

Thanks for the review! I'll change the name and add a SERIES column, since it seems like we need it for impish right now at least.

[lp:~sil2100/ubuntu-cdimage/cdimage-to-livefs-map]

1948. By Łukasz Zemczak on 2021-06-25

Rename the mapping file to something that makes more sense, handle '-' in the target subarch name and add 'series' (DIST) column to the mapping file.

[Dimitri John Ledkov (xnox)]

review: Approve

=== added file 'etc/cdimage-to-livecd-rootfs-map'
--- etc/cdimage-to-livecd-rootfs-map	1970-01-01 00:00:00 +0000
+++ etc/cdimage-to-livecd-rootfs-map	2021-06-25 10:12:21 +0000
@@ -0,0 +1,13 @@
+# PROJECT			IMAGE_TYPE			DIST		ARCH		SUBARCH		LIVEFS_PROJECT		LIVEFS_CPUARCH	LIVEFS_SUBARCH
+# Put more specific matches first.
+# Like etc/default-arches, "PROJECT" is in fact PROJECT[-SUBPROJECT][-LOCALE]. "-" means 'empty'.
+
+ubuntu-server		daily-preinstalled	*			riscv64		unleashed	ubuntu-cpc			*				sifive_fu540
+ubuntu-server		daily-preinstalled	*			riscv64		unmatched	ubuntu-cpc			*				sifive_hifive_unmatched_fu740
+# XXX: We should probably get rid of this once we start building unleased+unmatched image pairs for focal, as this is here
+#  only for focal legacy reasons.
+ubuntu-server		daily-preinstalled	focal		riscv64		-			ubuntu-cpc			*				hifive
+ubuntu-server		daily-preinstalled	*			*			-			ubuntu-cpc			*				generic
+ubuntu-server		daily-preinstalled	*			*			*			ubuntu-cpc			*				*
+ubuntu-appliance	*					*			*			*			ubuntu-core			*				*
+livecd-base			*					*			*			*			base				*				*

=== modified file 'lib/cdimage/config.py'
--- lib/cdimage/config.py	2021-04-23 10:48:40 +0000
+++ lib/cdimage/config.py	2021-06-25 10:12:21 +0000
@@ -285,6 +285,7 @@
         for key, value in kwargs.items():
             self[key] = value
         config_path = os.path.join(self.root, "etc", "config")
+        self.livefs_arch_mapping = {}
         if read:
             if os.path.exists(config_path):
                 self.read(config_path)
@@ -305,6 +306,7 @@
             self.set_default_arches()
         if "CPUARCHES" not in self:
             self.set_default_cpuarches()
+        self.set_livefs_mapping()
 
     def __setitem__(self, key, value):
         config_value = value
@@ -393,6 +395,58 @@
                 return arches
         return None
 
+    def set_livefs_mapping(self):
+        self.livefs_arch_mapping = {}
+        mapping = os.path.join(self.root, "etc", "cdimage-to-livecd-rootfs-map")
+        if not os.path.exists(mapping):
+            return
+        want_project_bits = [self.project]
+        if self.subproject:
+            want_project_bits.append(self.subproject)
+        if self["UBUNTU_DEFAULTS_LOCALE"]:
+            want_project_bits.append(self["UBUNTU_DEFAULTS_LOCALE"])
+        want_project = "-".join(want_project_bits)
+        with open(mapping) as f:
+            mapping_file = f.readlines()
+        for arch in self.arches:
+            (want_cpuarch, _, want_subarch) = arch.partition("+")
+            for line in mapping_file:
+                line = line.strip()
+                if not line or line.startswith("#"):
+                    continue
+                try:
+                    (project, image_type, series, cpuarch, subarch,
+                     livefs_project, livefs_cpuarch, livefs_subarch) = \
+                        line.split(None, 7)
+                except ValueError:
+                    continue
+                if not fnmatch.fnmatchcase(want_project, project):
+                    continue
+                if not fnmatch.fnmatchcase(self.image_type, image_type):
+                    continue
+                if not self.match_series(series):
+                    continue
+                if not fnmatch.fnmatchcase(want_cpuarch, cpuarch):
+                    continue
+                # - means 'no subarch'
+                if subarch == "-":
+                    if want_subarch:
+                        continue
+                elif not fnmatch.fnmatchcase(want_subarch, subarch):
+                    continue
+                # * means 'no change'
+                if livefs_project == "*":
+                    livefs_project = self.project
+                if livefs_cpuarch == "*":
+                    livefs_cpuarch = want_cpuarch
+                if livefs_subarch == "*":
+                    livefs_subarch = want_subarch
+                elif livefs_subarch == "-":
+                    livefs_subarch = None
+                livefs_arch = ("%s+%s" % (livefs_cpuarch, livefs_subarch) 
+                               if livefs_subarch else livefs_cpuarch)
+                self.livefs_arch_mapping[arch] = (livefs_project, livefs_arch)
+
     def set_default_cpuarches(self):
         self["CPUARCHES"] = " ".join(
             sorted(set(arch.split("+")[0] for arch in self.arches)))
@@ -457,6 +511,16 @@
             return '22'
         return None
 
+    def livefs_project_for_arch(self, arch):
+        if arch not in self.livefs_arch_mapping:
+            return self.project
+        return self.livefs_arch_mapping[arch][0]
+
+    def livefs_arch_for_arch(self, arch):
+        if arch not in self.livefs_arch_mapping:
+            return arch
+        return self.livefs_arch_mapping[arch][1]
+
     def export(self):
         ret = dict(os.environ)
         for key, value in self.items():

=== modified file 'lib/cdimage/livefs.py'
--- lib/cdimage/livefs.py	2021-03-11 10:51:35 +0000
+++ lib/cdimage/livefs.py	2021-06-25 10:12:21 +0000
@@ -67,34 +67,20 @@
     pass
 
 
-def split_arch(arch):
-    arch_bits = arch.split("+", 1)
+def split_arch(config, arch):
+    # To make sure we're compatible with everything before, we need to do the
+    # arch -> livefs_arch mapping here. This way we're consistent with how it
+    # worked previously
+    live_arch = config.livefs_arch_for_arch(arch)
+    arch_bits = live_arch.split("+", 1)
     if len(arch_bits) == 1:
         arch_bits.append("")
     cpuarch, subarch = arch_bits
-
-    if cpuarch == "amd64" and subarch == "mac":
-        # Use normal amd64 live image on amd64+mac.
-        subarch = ""
-
-    if cpuarch == "riscv64":
-        # need to specify subarch to get raw.xz instead of qcow2
-        # originally, no subarch was the unleashed image. Add support
-        # for user-friendly subarch names and translate them to u-boot
-        # target names for livecd-rootfs maybe the two will be able to
-        # merge but at the moment, they are very different SOCs.
-        if subarch == "":
-            subarch = "hifive"  # for focal
-        elif subarch == "unleashed":
-            subarch = "sifive_fu540"
-        elif subarch == "unmatched":
-            subarch = "sifive_hifive_unmatched_fu740"
-
     return cpuarch, subarch
 
 
 def live_builder(config, arch):
-    cpuarch, subarch = split_arch(arch)
+    cpuarch, subarch = split_arch(config, arch)
     project = config.project
 
     path = os.path.join(config.root, "production", "livefs-builders")
@@ -126,7 +112,7 @@
 def live_build_options(config, arch):
     options = []
 
-    cpuarch, subarch = split_arch(arch)
+    cpuarch, subarch = split_arch(config, arch)
     if (cpuarch in ("armel", "armhf") and
             config.image_type == "daily-preinstalled"):
         if subarch in ("mx5", "omap", "omap4"):
@@ -149,22 +135,10 @@
 
 
 def live_project(config, arch):
-    project = config.project
-
-    if project == "livecd-base":
-        liveproject = "base"
-    elif (project == "ubuntu-server" and
-          config.image_type == "daily-preinstalled"):
-        liveproject = "ubuntu-cpc"
-    elif project == "ubuntu-appliance":
-        liveproject = "ubuntu-core"
-    else:
-        liveproject = project
-
-    cpuarch, subarch = split_arch(arch)
+    liveproject = config.livefs_project_for_arch(arch)
 
     if config["CDIMAGE_DVD"]:
-        if project in ("ubuntu", "kubuntu", "edubuntu", "ubuntustudio"):
+        if config.project in ("ubuntu", "kubuntu", "edubuntu", "ubuntustudio"):
             liveproject += "-dvd"
 
     return liveproject
@@ -183,7 +157,7 @@
 
     command.extend(live_build_options(config, arch))
 
-    cpuarch, subarch = split_arch(arch)
+    cpuarch, subarch = split_arch(config, arch)
     if cpuarch:
         command.extend(["-A", cpuarch])
     if subarch:
@@ -202,7 +176,7 @@
 
 
 def live_build_lp_kwargs(config, lp, lp_livefs, arch):
-    cpuarch, subarch = split_arch(arch)
+    cpuarch, subarch = split_arch(config, arch)
     kwargs = {}
     metadata_override = {}
 
@@ -254,7 +228,7 @@
     bits = [config.project]
     if config.subproject:
         bits.append(config.subproject)
-    cpuarch, subarch = split_arch(arch)
+    cpuarch, subarch = split_arch(config, arch)
     bits.append(cpuarch)
     if subarch:
         bits.append(subarch)
@@ -273,7 +247,7 @@
     livefs_id_bits = [project]
     if config.subproject:
         livefs_id_bits.append(config.subproject)
-    cpuarch, subarch = split_arch(arch)
+    cpuarch, subarch = split_arch(config, arch)
     if subarch:
         livefs_id_bits.append(subarch)
     if config["UBUNTU_DEFAULTS_LOCALE"]:
@@ -304,7 +278,7 @@
 
 
 def live_lp_info(config, arch):
-    cpuarch, subarch = split_arch(arch)
+    cpuarch, subarch = split_arch(config, arch)
     want_project_bits = [config.project]
     if config.subproject:
         want_project_bits.append(config.subproject)
@@ -462,7 +436,7 @@
     if config["LIVECD_BASE"]:
         return config["LIVECD_BASE"]
 
-    cpuarch, subarch = split_arch(arch)
+    cpuarch, subarch = split_arch(config, arch)
     series = config["DIST"]
 
     if config["LIVECD"]:
@@ -482,7 +456,7 @@
 
 
 def flavours(config, arch):
-    cpuarch, subarch = split_arch(arch)
+    cpuarch, subarch = split_arch(config, arch)
     project = config.project
     series = config["DIST"]
 
@@ -550,7 +524,7 @@
     if item == "ltsp-squashfs" and arch == "amd64":
         # use i386 LTSP image on amd64 too
         arch = "i386"
-    cpuarch, subarch = split_arch(arch)
+    cpuarch, subarch = split_arch(config, arch)
     project = config.project
     series = config["DIST"]
     liveproject = live_project(config, arch)

=== modified file 'lib/cdimage/tests/test_config.py'
--- lib/cdimage/tests/test_config.py	2020-12-02 17:05:14 +0000
+++ lib/cdimage/tests/test_config.py	2021-06-25 10:12:21 +0000
@@ -164,6 +164,91 @@
         config = Config(SUBPROJECT="wubi", IMAGE_TYPE="wubi")
         self.assertEqual("amd64 i386", config["ARCHES"])
 
+    def test_livefs_mapping(self):
+        os.environ["CDIMAGE_ROOT"] = self.use_temp_dir()
+        os.environ["ARCHES"] = "arm64 arm64+unleashed"
+        etc_dir = os.path.join(self.temp_dir, "etc")
+        with mkfile(os.path.join(etc_dir, "config")) as f:
+            print(dedent("""\
+                #! /bin/sh
+                PROJECT=ubuntu-server
+                DIST=focal
+                """), file=f)
+        with mkfile(os.path.join(etc_dir, "cdimage-to-livecd-rootfs-map")) as f:
+            print("ubuntu-server\tdaily-preinstalled\t*\tarm64\tunleashed\t"
+                  "ubuntu-cpc\t*\tsifive_fu540", file=f)
+        config = Config(IMAGE_TYPE="daily-preinstalled")
+        # Check if project and arch got overriden
+        self.assertEqual(
+            "ubuntu-cpc", config.livefs_project_for_arch("arm64+unleashed"))
+        self.assertEqual(
+            "arm64+sifive_fu540",
+            config.livefs_arch_for_arch("arm64+unleashed"))
+        # And if no mapping entries present, no changes should be made
+        self.assertEqual(
+            "ubuntu-server", config.livefs_project_for_arch("arm64"))
+        self.assertEqual(
+            "arm64", config.livefs_arch_for_arch("arm64"))
+
+    def test_livefs_mapping_empty_subarch(self):
+        os.environ["CDIMAGE_ROOT"] = self.use_temp_dir()
+        os.environ["ARCHES"] = "arm64 arm64+unleashed"
+        etc_dir = os.path.join(self.temp_dir, "etc")
+        with mkfile(os.path.join(etc_dir, "config")) as f:
+            print(dedent("""\
+                #! /bin/sh
+                PROJECT=ubuntu-server
+                DIST=focal
+                """), file=f)
+        with mkfile(os.path.join(etc_dir, "cdimage-to-livecd-rootfs-map")) as f:
+            print("ubuntu-server\tdaily-preinstalled\t*\tarm64\t-\t"
+                  "ubuntu-cpc\t*\tgeneric\n"
+                  "ubuntu-server\tdaily-preinstalled\t*\tarm64\tunleashed\t"
+                  "ubuntu-cpc\t*\t-\n", file=f)
+        config = Config(IMAGE_TYPE="daily-preinstalled")
+        # Check if no subarch is converted to a subarch
+        self.assertEqual(
+            "ubuntu-cpc", config.livefs_project_for_arch("arm64"))
+        self.assertEqual(
+            "arm64+generic",
+            config.livefs_arch_for_arch("arm64"))
+        # ...and other way around, subarch case to no subarch
+        self.assertEqual(
+            "ubuntu-cpc", config.livefs_project_for_arch("arm64+unleashed"))
+        self.assertEqual(
+            "arm64",
+            config.livefs_arch_for_arch("arm64+unleashed"))
+
+    def test_livefs_mapping_series(self):
+        os.environ["CDIMAGE_ROOT"] = self.use_temp_dir()
+        os.environ["ARCHES"] = "arm64 arm64+unleashed"
+        etc_dir = os.path.join(self.temp_dir, "etc")
+        with mkfile(os.path.join(etc_dir, "config")) as f:
+            print(dedent("""\
+                #! /bin/sh
+                PROJECT=ubuntu-server
+                DIST=groovy
+                """), file=f)
+        with mkfile(os.path.join(etc_dir, 
+                                 "cdimage-to-livecd-rootfs-map")) as f:
+            print("ubuntu-server\tdaily-preinstalled\tfocal-\tarm64\t-\t"
+                  "ubuntu-cpc\t*\tgeneric\n"
+                  "ubuntu-server\tdaily-preinstalled\impish\tarm64\t"
+                  "unleashed\tubuntu-cpc\t*\tsifive_unleashed\n", file=f)
+        config = Config(IMAGE_TYPE="daily-preinstalled")
+        # This should be matched by series
+        self.assertEqual(
+            "ubuntu-cpc", config.livefs_project_for_arch("arm64"))
+        self.assertEqual(
+            "arm64+generic",
+            config.livefs_arch_for_arch("arm64"))
+        # ...this not
+        self.assertEqual(
+            "ubuntu-server", config.livefs_project_for_arch("arm64+unleashed"))
+        self.assertEqual(
+            "arm64+unleashed",
+            config.livefs_arch_for_arch("arm64+unleashed"))
+
     def test_read_shell(self):
         os.environ["CDIMAGE_ROOT"] = self.use_temp_dir()
         with mkfile(os.path.join(self.temp_dir, "etc", "config")) as f:
@@ -329,6 +414,21 @@
         config["IMAGE_TYPE"] = "daily-live"
         self.assertEqual("daily-live", config.image_type)
 
+    def test_livefs_project_for_arch(self):
+        config = Config(read=False)
+        config["PROJECT"] = "ubuntu-server"
+        config.livefs_arch_mapping = {"amd64+generic": ("ubuntu-cpc", "amd64")}
+        self.assertEqual("ubuntu-cpc",
+                         config.livefs_project_for_arch("amd64+generic"))
+        self.assertEqual("ubuntu-server",
+                         config.livefs_project_for_arch("amd64"))
+
+    def test_livefs_arch_for_arch(self):
+        config = Config(read=False)
+        config.livefs_arch_mapping = {"amd64": ("ubuntu-cpc", "amd64+generic")}
+        self.assertEqual("amd64+generic", config.livefs_arch_for_arch("amd64"))
+        self.assertEqual("arm64", config.livefs_arch_for_arch("arm64"))
+
     def test_all_projects(self):
         config = Config(read=False)
         self.assertEqual([], config.all_projects)

=== modified file 'lib/cdimage/tests/test_livefs.py'
--- lib/cdimage/tests/test_livefs.py	2020-12-10 17:32:42 +0000
+++ lib/cdimage/tests/test_livefs.py	2021-06-25 10:12:21 +0000
@@ -148,27 +148,38 @@
 
 class TestSplitArch(TestCase):
     def test_amd64(self):
-        self.assertEqual(("amd64", ""), split_arch("amd64"))
-
-    def test_amd64_mac(self):
-        self.assertEqual(("amd64", ""), split_arch("amd64+mac"))
+        config = Config(read=False)
+        self.assertEqual(("amd64", ""), split_arch(config, "amd64"))
 
     def test_arm64_raspi(self):
-        self.assertEqual(("arm64", "raspi"), split_arch("arm64+raspi"))
+        config = Config(read=False)
+        self.assertEqual(("arm64", "raspi"), split_arch(config, "arm64+raspi"))
 
     def test_armhf_omap4(self):
-        self.assertEqual(("armhf", "omap4"), split_arch("armhf+omap4"))
+        config = Config(read=False)
+        self.assertEqual(("armhf", "omap4"), split_arch(config, "armhf+omap4"))
 
     def test_i386(self):
-        self.assertEqual(("i386", ""), split_arch("i386"))
+        config = Config(read=False)
+        self.assertEqual(("i386", ""), split_arch(config, "i386"))
 
 
 class TestLiveProject(TestCase):
     def assertProjectEqual(self, expected, project, series, arch="i386",
                            **kwargs):
-        config = Config(read=False)
-        config["PROJECT"] = project
-        config["DIST"] = series
+        os.environ["CDIMAGE_ROOT"] = self.use_temp_dir()
+        os.environ["ARCHES"] = arch
+        etc_dir = os.path.join(self.temp_dir, "etc")
+        with mkfile(os.path.join(etc_dir, "config")) as f:
+            print(dedent("""\
+                #! /bin/sh
+                PROJECT=%s
+                DIST=%s
+                """ % (project, series)), file=f)
+        with mkfile(os.path.join(etc_dir, "cdimage-to-livecd-rootfs-map")) as f:
+            print("ubuntu-appliance\t*\t*\t*\t*\tubuntu-core\t*\t*\n"
+                  "livecd-base\t*\t*\t*\t*\tbase\t*\t*", file=f)
+        config = Config()
         for key, value in kwargs.items():
             config[key.upper()] = value
         self.assertEqual(expected, live_project(config, arch))
@@ -179,22 +190,23 @@
     def test_ubuntu_dvd(self):
         for series in all_series[7:]:
             self.assertProjectEqual(
-                "ubuntu-dvd", "ubuntu", series, cdimage_dvd="1")
+                "ubuntu-dvd", "ubuntu", series.full_name, cdimage_dvd="1")
 
     def test_kubuntu_dvd(self):
         for series in all_series[7:]:
             self.assertProjectEqual(
-                "kubuntu-dvd", "kubuntu", series, cdimage_dvd="1")
+                "kubuntu-dvd", "kubuntu", series.full_name, cdimage_dvd="1")
 
     def test_edubuntu_dvd(self):
         for series in all_series[10:]:
             self.assertProjectEqual(
-                "edubuntu-dvd", "edubuntu", series, cdimage_dvd="1")
+                "edubuntu-dvd", "edubuntu", series.full_name, cdimage_dvd="1")
 
     def test_ubuntustudio_dvd(self):
         for series in all_series[15:]:
             self.assertProjectEqual(
-                "ubuntustudio-dvd", "ubuntustudio", series, cdimage_dvd="1")
+                "ubuntustudio-dvd", "ubuntustudio", series.full_name,
+                cdimage_dvd="1")
 
     def test_ubuntu_appliance(self):
         # We currently only support ubuntu-appliances for bionic (UC18)
@@ -1103,6 +1115,9 @@
         self.config["PROJECT"] = "ubuntu-server"
         self.config["DIST"] = "xenial"
         self.config["IMAGE_TYPE"] = "daily-preinstalled"
+        self.config.livefs_arch_mapping = {
+            "armhf+raspi2": ("ubuntu-cpc", "armhf+raspi2")
+            }
         self.assertTrue(download_live_items(self.config, "armhf+raspi2",
                                             "disk1.img.xz"))
         mock_fetch.assert_called_once_with(