[Merge] ~waveform/ubuntu-release-upgrader:remove-uboot into ubuntu-release-upgrader:ubuntu/master
Brian Murray
mp+406663 at code.launchpad.net
Mon Aug 9 16:58:09 UTC 2021
Review: Needs Fixing
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.
Diff comments:
> diff --git a/DistUpgrade/DistUpgradeQuirks.py b/DistUpgrade/DistUpgradeQuirks.py
> index 712c98e..40f9f97 100644
> --- a/DistUpgrade/DistUpgradeQuirks.py
> +++ b/DistUpgrade/DistUpgradeQuirks.py
> @@ -1205,3 +1208,133 @@ 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'):
> + failure_action = (
> + "you may need to replace u_boot_* with vmlinuz, and add "
If a user is going to see this message the 'y' in 'you' should be capitalized.
> + "'initramfs initrd.img followkernel' to config.txt on your boot "
> + "partition; see LP: #1936401 for further details")
Reading LP: #1936401 I don't think it really has any further details that would help a person understand why these initramfs options are necessary.
> + 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=vmlinuz')
> + result.append('initramfs initrd.img followkernel')
> + 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
Should 'place' be 'replace'?
> + # 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=vmlinuz')
> + result.append('initramfs initrd.img followkernel')
This is the 3rd time I've seen 'initramfs initrd.img followkernel' - perhaps it should be a variable in case it needs updating.
> + 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
Why are the initial comments skipped? Is there any information that would be valuable to users?
> + # 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)
Could boot_dir already be a part of 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 = merge_includes(replace_uboot(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/tests/test_quirks.py b/tests/test_quirks.py
> index cc4c8f1..0e92275 100644
> --- a/tests/test_quirks.py
> +++ b/tests/test_quirks.py
> @@ -519,6 +522,203 @@ 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, 'custom.txt')))
There is no assertTrue for config.txt, while this will fail later when trying to read the file it's a bit easier to read / keep count of all the files if we test for that.
> + 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):
>
--
https://code.launchpad.net/~waveform/ubuntu-release-upgrader/+git/ubuntu-release-upgrader/+merge/406663
Your team Ubuntu Core Development Team is subscribed to branch ubuntu-release-upgrader:ubuntu/master.
More information about the Ubuntu-reviews
mailing list