Merge ~waveform/ubuntu-release-upgrader:remove-uboot into ubuntu-release-upgrader:ubuntu/master

[Dave Jones]

Adds a quirk to remote u-boot from the boot sequence of Raspberry Pi installations on impish. This doesn't remove the u-boot binaries themselves, and leaves the actual configuration commented out in the config.txt in case user's wish to revert easily.

LP: #1936401

[Brian Murray (brian-murray)]

I found the function _remove_uboot_on_rpi a bit hard to read and had to scroll up and down multiple times because the file is read, then some other functions are defined, and then the file is passed to those functions. It might be easier for others to read if that was restructured a bit.

I can't really comment on the correctness of the boot config contents but as a release upgrader quirk this seems good to me.

I've some additional comments in-line.

review: Needs Fixing

[Dave Jones (waveform)]

Added responses to each review comment. Will upload a commit to address the outstanding points.

diff --git a/DistUpgrade/DistUpgradeQuirks.py b/DistUpgrade/DistUpgradeQuirks.py
index 712c98e..9d6400b 100644
--- a/DistUpgrade/DistUpgradeQuirks.py
+++ b/DistUpgrade/DistUpgradeQuirks.py
@@ -145,6 +145,9 @@ class DistUpgradeQuirks(object):
         if 'ubuntu-desktop-raspi' in cache:
             if cache['ubuntu-desktop-raspi'].is_installed:
                 self._replace_fkms_overlay()
+        if 'linux-firmware-raspi2' in cache:
+            if cache['linux-firmware-raspi2'].is_installed:
+                self._remove_uboot_on_rpi()
 
     # individual quirks handler when the dpkg run is finished ---------
     def PostCleanup(self):
@@ -1157,7 +1160,7 @@ class DistUpgradeQuirks(object):
 
     def _replace_fkms_overlay(self, boot_dir='/boot/firmware'):
         failure_action = (
-            "you may need to replace the vc4-fkms-v3d overlay with "
+            "You may need to replace the vc4-fkms-v3d overlay with "
             "vc4-kms-v3d in config.txt on your boot partition")
 
         try:
@@ -1205,3 +1208,136 @@ class DistUpgradeQuirks(object):
         except IOError as exc:
             logging.error("unable to write new boot config to %s: %s; %s",
                           boot_config_filename, exc, failure_action)
+
+    def _remove_uboot_on_rpi(self, boot_dir='/boot/firmware'):
+        kernel_line = 'kernel=vmlinuz'
+        initramfs_line = 'initramfs initrd.img followkernel' 
+        failure_action = (
+            "You may need to replace u_boot_* with vmlinuz, and add "
+            "{initramfs_line!r} to config.txt on your boot "
+            "partition; see LP: #1936401 for further details")
+        failure_action = failure_action.format(initramfs_line=initramfs_line)
+        change_prefix = '# commented by do-release-upgrade (LP: #1936401)'
+        added_prefix = '# added by do-release-upgrade (LP: #1936401)'
+        merge_prefix = '# merged from {} by do-release-upgrade (LP: #1936401)'
+
+        try:
+            boot_config_filename = os.path.join(boot_dir, 'config.txt')
+            with open(boot_config_filename, 'r', encoding='utf-8') as f:
+                boot_config = f.read()
+        except FileNotFoundError:
+            logging.error("failed to open boot configuration in %s; %s",
+                          boot_config_filename, failure_action)
+            return
+
+        def replace_uboot(lines):
+            result = []
+            removed_uboot = added_kernel = False
+            for line in lines:
+                if line == '[all]':
+                    # Add the vmlinuz kernel and initrd.img to the first
+                    # encountered [all] section, just in case the user has any
+                    # custom kernel overrides later in the sequence
+                    result.append(line)
+                    if not added_kernel:
+                        result.append(added_prefix)
+                        result.append(kernel_line)
+                        result.append(initramfs_line)
+                        added_kernel = True
+                elif line.startswith('device_tree_address='):
+                    # Disable any device_tree_address= line (leave the boot
+                    # firmware to place this rather than risk trampling over it
+                    # with kernel/initrd)
+                    result.append(change_prefix)
+                    result.append('#' + line)
+                elif line.startswith('kernel=uboot_rpi_'):
+                    # Disable all kernel=uboot_rpi_* lines found in the
+                    # configuration
+                    removed_uboot = True
+                    result.append(change_prefix)
+                    result.append('#' + line)
+                else:
+                    result.append(line)
+            # We don't *want* to touch the config unless we absolutely have to,
+            # and the user may have already performed this conversion (e.g. to
+            # take advantage of USB MSD boot), or otherwise customized their
+            # boot configuration. Hence, if we didn't find (and remove) any
+            # u-boot entries, assume the kernel/initrd config was already fine
+            # and return the verbatim config
+            if removed_uboot:
+                if not added_kernel:
+                    result.append(added_prefix)
+                    result.append('[all]')
+                    result.append(kernel_line)
+                    result.append(initramfs_line)
+                return result
+            else:
+                return lines
+
+        def merge_includes(lines):
+            result = []
+            skip_comments = True
+            found_includes = False
+            for line in lines:
+                # Skip the initial comment block warning that config.txt is not
+                # to be edited (the usercfg.txt and syscfg.txt files were
+                # merged in the groovy release, but not for upgraders)
+                if line.startswith('#') and skip_comments:
+                    continue
+                skip_comments = False
+                if line in ('include syscfg.txt', 'include usercfg.txt'):
+                    # Merge the usercfg.txt and syscfg.txt configuration files
+                    # into config.txt, skipping the initial comments. Note that
+                    # we don't bother with other includes the user has added
+                    # themselves (as they presumably want to keep those)
+                    found_includes = True
+                    included_filename = line.split(maxsplit=1)[1]
+                    result.append(merge_prefix.format(included_filename))
+                    included_filename = os.path.join(boot_dir,
+                                                     included_filename)
+                    skip_comments = True
+                    with open(included_filename, 'r', encoding='utf-8') as f:
+                        for line in f:
+                            if line.startswith('#') and skip_comments:
+                                continue
+                            skip_comments = False
+                            result.append(line.rstrip())
+                    target_filename = included_filename + '.distUpgrade'
+                    try:
+                        os.rename(included_filename, target_filename)
+                    except IOError as exc:
+                        logging.error("failed to move included configuration "
+                                      "from %s to %s; %s", included_filename,
+                                      target_filename, exc)
+                else:
+                    result.append(line)
+            # Again, if we found no includes, return the original verbatim
+            # (i.e. with initial comments intact)
+            if found_includes:
+                return result
+            else:
+                return lines
+
+        lines = [line.rstrip() for line in boot_config.splitlines()]
+        lines = replace_uboot(merge_includes(lines))
+        new_config = ''.join(line + '\n' for line in lines)
+
+        if new_config == boot_config:
+            logging.warning("no u-boot removal performed in %s",
+                            boot_config_filename)
+            return
+
+        try:
+            boot_backup_filename = boot_config_filename + '.distUpgrade'
+            with open(boot_backup_filename, 'w', encoding='utf-8') as f:
+                f.write(boot_config)
+        except IOError as exc:
+            logging.error("unable to write boot config backup to %s: %s; %s",
+                          boot_backup_filename, exc, failure_action)
+            return
+        try:
+            with open(boot_config_filename, 'w', encoding='utf-8') as f:
+                f.write(new_config)
+        except IOError as exc:
+            logging.error("unable to write new boot config to %s: %s; %s",
+                          boot_config_filename, exc, failure_action)
diff --git a/debian/changelog b/debian/changelog
index 762e5e9..15d3ca0 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,10 @@
+ubuntu-release-upgrader (1:21.10.7) impish; urgency=medium
+
+  * DistUpgrade/DistUpgradeQuirks.py: Add quirk to merge raspi boot config
+    into one file, and remove u-boot references (LP: #1936401)
+
+ -- Dave Jones <[e-mail]>  Tue, 03 Aug 2021 14:50:36 +0100
+
 ubuntu-release-upgrader (1:21.10.6) impish; urgency=medium
 
   * DistUpgrade/ReleaseAnnouncement, DistUpgrade/EOLReleaseAnnouncement:
diff --git a/tests/test_quirks.py b/tests/test_quirks.py
index cc4c8f1..2e96087 100644
--- a/tests/test_quirks.py
+++ b/tests/test_quirks.py
@@ -458,13 +458,14 @@ class TestQuirks(unittest.TestCase):
 
     def test_replace_fkms_overlay_no_changes(self):
         with tempfile.TemporaryDirectory() as boot_dir:
-            demo_config = ("# This is a demo boot config\n"
-                           "[pi4\n"
-                           "max_framebuffers=2\n"
-                           "[all]\n"
-                           "arm_64bit=1\n"
-                           "kernel=vmlinuz\n"
-                           "initramfs initrd.img followkernel\n")
+            demo_config = """\
+# This is a demo boot config
+[pi4]
+max_framebuffers=2
+[all]
+arm_64bit=1
+kernel=vmlinuz
+initramfs initrd.img followkernel"""
             with open(os.path.join(boot_dir, 'config.txt'), 'w') as f:
                 f.write(demo_config)
 
@@ -480,32 +481,34 @@ class TestQuirks(unittest.TestCase):
 
     def test_replace_fkms_overlay_with_changes(self):
         with tempfile.TemporaryDirectory() as boot_dir:
-            demo_config = (
-                "# This is a demo boot config\n"
-                "[pi4\n"
-                "max_framebuffers=2\n"
-                "[all]\n"
-                "arm_64bit=1\n"
-                "kernel=vmlinuz\n"
-                "initramfs initrd.img followkernel\n"
-                "dtoverlay=vc4-fkms-v3d,cma-256\n"
-                "start_x=1\n"
-                "gpu_mem=256\n")
-            expected_config = (
-                "# This is a demo boot config\n"
-                "[pi4\n"
-                "max_framebuffers=2\n"
-                "[all]\n"
-                "arm_64bit=1\n"
-                "kernel=vmlinuz\n"
-                "initramfs initrd.img followkernel\n"
-                "# changed by do-release-upgrade (LP: #1923673)\n"
-                "#dtoverlay=vc4-fkms-v3d,cma-256\n"
-                "dtoverlay=vc4-kms-v3d,cma-256\n"
-                "# disabled by do-release-upgrade (LP: #1923673)\n"
-                "#start_x=1\n"
-                "# disabled by do-release-upgrade (LP: #1923673)\n"
-                "#gpu_mem=256\n")
+            demo_config = """\
+# This is a demo boot config
+[pi4]
+max_framebuffers=2
+[all]
+arm_64bit=1
+kernel=vmlinuz
+initramfs initrd.img followkernel
+dtoverlay=vc4-fkms-v3d,cma-256
+start_x=1
+gpu_mem=256
+"""
+            expected_config = """\
+# This is a demo boot config
+[pi4]
+max_framebuffers=2
+[all]
+arm_64bit=1
+kernel=vmlinuz
+initramfs initrd.img followkernel
+# changed by do-release-upgrade (LP: #1923673)
+#dtoverlay=vc4-fkms-v3d,cma-256
+dtoverlay=vc4-kms-v3d,cma-256
+# disabled by do-release-upgrade (LP: #1923673)
+#start_x=1
+# disabled by do-release-upgrade (LP: #1923673)
+#gpu_mem=256
+"""
             with open(os.path.join(boot_dir, 'config.txt'), 'w') as f:
                 f.write(demo_config)
 
@@ -519,6 +522,205 @@ class TestQuirks(unittest.TestCase):
             with open(os.path.join(boot_dir, 'config.txt')) as f:
                 self.assertTrue(f.read() == expected_config)
 
+    def test_remove_uboot_no_config(self):
+        with tempfile.TemporaryDirectory() as boot_dir:
+            mock_controller = mock.Mock()
+            q = DistUpgradeQuirks(mock_controller, mock.Mock())
+            q._remove_uboot_on_rpi(boot_dir)
+
+            self.assertFalse(os.path.exists(os.path.join(
+                boot_dir, 'config.txt.distUpgrade')))
+
+    def test_remove_uboot_no_changes(self):
+        with tempfile.TemporaryDirectory() as boot_dir:
+            native_config = """\
+# This is a demo boot config with a comment at the start that should not
+# be removed
+
+[pi4]
+max_framebuffers=2
+
+[all]
+arm_64bit=1
+kernel=vmlinuz
+initramfs initrd.img followkernel
+
+# This is a user-added include that should not be merged
+include custom.txt
+"""
+            custom_config = """\
+# This is the custom included configuration file
+
+hdmi_group=1
+hdmi_mode=4
+"""
+            with open(os.path.join(boot_dir, 'config.txt'), 'w') as f:
+                f.write(native_config)
+            with open(os.path.join(boot_dir, 'custom.txt'), 'w') as f:
+                f.write(custom_config)
+
+            mock_controller = mock.Mock()
+            q = DistUpgradeQuirks(mock_controller, mock.Mock())
+            q._remove_uboot_on_rpi(boot_dir)
+
+            self.assertFalse(os.path.exists(os.path.join(
+                boot_dir, 'config.txt.distUpgrade')))
+            self.assertFalse(os.path.exists(os.path.join(
+                boot_dir, 'custom.txt.distUpgrade')))
+            with open(os.path.join(boot_dir, 'config.txt')) as f:
+                self.assertTrue(f.read() == native_config)
+            with open(os.path.join(boot_dir, 'custom.txt')) as f:
+                self.assertTrue(f.read() == custom_config)
+
+    def test_remove_uboot_with_changes(self):
+        with tempfile.TemporaryDirectory() as boot_dir:
+            config_txt = """\
+# This is a warning that you should not edit this file. The upgrade should
+# remove this comment
+
+[pi4]
+# This is a comment that should be included
+kernel=uboot_rpi_4.bin
+max_framebuffers=2
+
+[pi2]
+kernel=uboot_rpi_2.bin
+
+[pi3]
+kernel=uboot_rpi_3.bin
+
+[all]
+arm_64bit=1
+device_tree_address=0x3000000
+include syscfg.txt
+include usercfg.txt
+dtoverlay=vc4-fkms-v3d,cma-256
+include custom.txt
+"""
+            usercfg_txt = """\
+# Another chunk of warning text that should be skipped
+"""
+            syscfg_txt = """\
+# Yet more warnings to exclude
+dtparam=audio=on
+dtparam=spi=on
+enable_uart=1
+"""
+            custom_txt = """\
+# This is a user-added file that should be left alone by the upgrade
+[gpio4=1]
+kernel=custom
+"""
+            expected_config_txt = """\
+
+[pi4]
+# This is a comment that should be included
+# commented by do-release-upgrade (LP: #1936401)
+#kernel=uboot_rpi_4.bin
+max_framebuffers=2
+
+[pi2]
+# commented by do-release-upgrade (LP: #1936401)
+#kernel=uboot_rpi_2.bin
+
+[pi3]
+# commented by do-release-upgrade (LP: #1936401)
+#kernel=uboot_rpi_3.bin
+
+[all]
+# added by do-release-upgrade (LP: #1936401)
+kernel=vmlinuz
+initramfs initrd.img followkernel
+arm_64bit=1
+# commented by do-release-upgrade (LP: #1936401)
+#device_tree_address=0x3000000
+# merged from syscfg.txt by do-release-upgrade (LP: #1936401)
+dtparam=audio=on
+dtparam=spi=on
+enable_uart=1
+# merged from usercfg.txt by do-release-upgrade (LP: #1936401)
+dtoverlay=vc4-fkms-v3d,cma-256
+include custom.txt
+"""
+            with open(os.path.join(boot_dir, 'config.txt'), 'w') as f:
+                f.write(config_txt)
+            with open(os.path.join(boot_dir, 'syscfg.txt'), 'w') as f:
+                f.write(syscfg_txt)
+            with open(os.path.join(boot_dir, 'usercfg.txt'), 'w') as f:
+                f.write(usercfg_txt)
+            with open(os.path.join(boot_dir, 'custom.txt'), 'w') as f:
+                f.write(custom_txt)
+
+            mock_controller = mock.Mock()
+            q = DistUpgradeQuirks(mock_controller, mock.Mock())
+            q._remove_uboot_on_rpi(boot_dir)
+
+            self.assertTrue(os.path.exists(os.path.join(
+                boot_dir, 'config.txt.distUpgrade')))
+            self.assertTrue(os.path.exists(os.path.join(
+                boot_dir, 'syscfg.txt.distUpgrade')))
+            self.assertTrue(os.path.exists(os.path.join(
+                boot_dir, 'usercfg.txt.distUpgrade')))
+            self.assertTrue(os.path.exists(os.path.join(
+                boot_dir, 'config.txt')))
+            self.assertTrue(os.path.exists(os.path.join(
+                boot_dir, 'custom.txt')))
+            self.assertFalse(os.path.exists(os.path.join(
+                boot_dir, 'syscfg.txt')))
+            self.assertFalse(os.path.exists(os.path.join(
+                boot_dir, 'usercfg.txt')))
+            self.assertFalse(os.path.exists(os.path.join(
+                boot_dir, 'custom.txt.distUpgrade')))
+            with open(os.path.join(boot_dir, 'config.txt')) as f:
+                self.assertTrue(f.read() == expected_config_txt)
+            with open(os.path.join(boot_dir, 'custom.txt')) as f:
+                self.assertTrue(f.read() == custom_txt)
+
+    def test_remove_uboot_no_all_section(self):
+        with tempfile.TemporaryDirectory() as boot_dir:
+            config_txt = """\
+arm_64bit=1
+device_tree_address=0x3000000
+
+[pi4]
+# This is a comment that should be included
+kernel=uboot_rpi_4.bin
+max_framebuffers=2
+
+[pi3]
+kernel=uboot_rpi_3.bin
+"""
+            expected_config_txt = """\
+arm_64bit=1
+# commented by do-release-upgrade (LP: #1936401)
+#device_tree_address=0x3000000
+
+[pi4]
+# This is a comment that should be included
+# commented by do-release-upgrade (LP: #1936401)
+#kernel=uboot_rpi_4.bin
+max_framebuffers=2
+
+[pi3]
+# commented by do-release-upgrade (LP: #1936401)
+#kernel=uboot_rpi_3.bin
+# added by do-release-upgrade (LP: #1936401)
+[all]
+kernel=vmlinuz
+initramfs initrd.img followkernel
+"""
+            with open(os.path.join(boot_dir, 'config.txt'), 'w') as f:
+                f.write(config_txt)
+
+            mock_controller = mock.Mock()
+            q = DistUpgradeQuirks(mock_controller, mock.Mock())
+            q._remove_uboot_on_rpi(boot_dir)
+
+            self.assertTrue(os.path.exists(os.path.join(
+                boot_dir, 'config.txt.distUpgrade')))
+            with open(os.path.join(boot_dir, 'config.txt')) as f:
+                self.assertTrue(f.read() == expected_config_txt)
+
 
 class TestSnapQuirks(unittest.TestCase):