curtin-dev team mailing list archive

[Simon Glass <mp+484176@xxxxxxxxxxxxxxxxxx>]

Ooops I didn't save my comments

Diff comments:

> diff --git a/HACKING.rst b/HACKING.rst
> index f2b618d..f723906 100644
> --- a/HACKING.rst
> +++ b/HACKING.rst
> @@ -46,6 +46,12 @@ Do these things once
>      git remote add LP_USER ssh://LP_USER@xxxxxxxxxxxxxxxxx/~LP_USER/curtin
>      git push LP_USER master
>  
> +* Install `libapt-pkg-dev` as it is needed by tox:

Thanks, done

> +
> +  .. code:: sh
> +
> +    sudo apt install libapt-pkg-dev
> +
>  .. _repository: https://git.launchpad.net/curtin
>  .. _contributor license agreement: https://ubuntu.com/legal/contributors
>  .. _contributor-agreement-canonical: https://launchpad.net/%7Econtributor-agreement-canonical/+members
> diff --git a/curtin/commands/curthooks.py b/curtin/commands/curthooks.py
> index 0c4d7fd..34c4a5d 100644
> --- a/curtin/commands/curthooks.py
> +++ b/curtin/commands/curthooks.py
> @@ -873,6 +873,38 @@ def translate_old_grub_schema(cfg):
>      cfg['boot'] = grub_cfg
>  
>  
> +def setup_extlinux(
> +        cfg: dict,
> +        target: str):
> +    """Set up an extlinux.conf file
> +
> +    :param: cfg: A config dict containing config.BootCfg in cfg['boot'].
> +    # :param: target: A string specifying the path to the chroot mountpoint.

done

> +    """
> +    bootcfg = config.fromdict(config.BootCfg, cfg.get('boot', {}))
> +
> +    # If there is a separate boot partition, set fw_boot_dir to empty
> +    storage_cfg = cfg.get('storage', {}).get('config', {})
> +    fw_boot_dir = '/boot'
> +    root = None
> +    for item in storage_cfg:

OK done, but I had to get the storage dict, i.e. call extract_storage_ordered_dict()

> +        if item['type'] == 'mount':
> +            if item['path'] == '/boot':
> +                fw_boot_dir = ''
> +            elif item['path'] == '/':
> +                root = item
> +
> +    if not root:
> +        raise ValueError("Storage configuration has no root directory")
> +
> +    storage_config_dict = block_meta.extract_storage_ordered_dict(cfg)

Hmmm, well it seems that block_meta.mount_data() needs it. I get an error like this if I try to pass just 'cfg:

TestInstallExtlinux::test_single_partition_cfg:

ValueError: mount entry refers to non-existant device vdb-part1_format: ({'id': 'vdb-part1_mount', 'type': 'mount', 'path': '/', 'device': 'vdb-part1_format', 'spec': '/dev/sda1'})

Since I am now using it above, perhaps this is OK? 

Here is what def block_meta.mount_data(info, storage_config) is passed for the storage_config in each case:

bad (passing just 'cfg'):
storage_config = {'config': [{'id': 'vdb', 'name': 'vdb', 'path': '/dev/vdb', 'ptable': 'gpt', ...}, {'device': 'vdb', 'id': 'vdb-part1...part1'}, {'device': 'vdb-part1_format', 'id': 'vdb-part1_mount', 'path': '/', 'spec': '/dev/sda1', ...}], 'version': 1}

good (passing storage_cfg):
storage_config OrderedDict({'vdb': {'id': 'vdb', 'type': 'disk', 'name': 'vdb', 'path': '/dev/vdb', 'ptable': 'gpt'}, 'vdb-part1': {'id': 'vdb-part1', 'type': 'partition', 'device': 'vdb', 'number': 1}, 'vdb-part1_format': {'id': 'vdb-part1_format', 'type': 'format', 'volume': 'vdb-part1', 'fstype': 'ext4'}, 'vdb-part1_mount': {'id': 'vdb-part1_mount', 'type': 'mount', 'path': '/', 'device': 'vdb-part1_format', 'spec': '/dev/sda1'}})

The line:

        if info['device'] not in storage_config:

in mount_data() seems to require a dict?

> +
> +    fdata = block_meta.mount_data(root, storage_config_dict)
> +    spec = block_meta.resolve_fdata_spec(fdata)
> +
> +    install_extlinux(bootcfg, target, fw_boot_dir, spec)
> +
> +
>  def setup_boot(
>          cfg: dict,
>          target: str,
> diff --git a/curtin/commands/install_extlinux.py b/curtin/commands/install_extlinux.py
> index b52b591..dc1bd64 100644
> --- a/curtin/commands/install_extlinux.py
> +++ b/curtin/commands/install_extlinux.py
> @@ -21,18 +23,25 @@ def build_content(bootcfg: config.BootCfg, target: str):
>      associated with fdt and fdtdir options.
>  
>      We assume that the boot directory is available as /boot in the target.
> +
> +    :param: bootcfg: A boot-config dict
> +    :param: target: A string specifying the path to the chroot mountpoint.
> +    :param: fw_boot_dir: Firmware's view of the /boot directory; when there is
> +        a separate /boot partition, firmware will access that as the root
> +        directory of the filesystem, so '' should be passed here. When the boot
> +        directory is just a subdirectory of '/', then '/boot' should be passed
> +    :param: root_spec: Root device to pass to kernel
>      """
>      def get_entry(label, params, menu_label_append=''):
>          return f'''\
>  label {label}
>  \tmenu label {menu_label} {version}{menu_label_append}
> -\tlinux /{kernel_path}
> -\tinitrd /{initrd_path}
> +\tlinux {fw_boot_dir}/{kernel_path}
> +\tinitrd {fw_boot_dir}/{initrd_path}
>  \tappend {params}'''
>  
>      buf = io.StringIO()
> -    params = 'ro quiet'
> -    alternatives = ['default', 'recovery']
> +    params = f'{root_spec} ro quiet'

Oh thank you! That fills in a missing gap for me.

However I added this in the original MP - this is just making it selectable. In other words, the single/quite stuff is already there.

Would it be able to sort this out in a future MP, along the lines of get_carryover_params()?

>      menu_label = 'Linux'
>  
>      # For the recovery option, remove 'quiet' and add 'single'


-- 
https://code.launchpad.net/~sjg1/curtin/+git/curtin/+merge/484176
Your team curtin developers is subscribed to branch curtin:master.