curtin-dev team mailing list archive

[Dan Bungert <mp+484176@xxxxxxxxxxxxxxxxxx>]

Thanks for this Simon.  I have some minor feedback items.
The VM test runner is known flaky right now, I have run this vm test set locally and confirmed that it is just fine so that is not blocking to a merge.

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:

First off, I'm seeing some impressively out-of-date content in this file so I'll give it a look.

Secondly thanks for improving the HACKING doc.  For the immediate thing you're trying to solve, I'd rather point people to the tools/vmtest-system-setup script as a single source of truth - there is a bunch more needed and it handles this.  Would you update this to have people run tools/vmtest-system-setup instead?

> +
> +  .. 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.

stray '#' in docstring

> +    """
> +    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:

nit: could use select_configs(storage_cfg, type='mount') here

> +        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)

I don't object to use of extract_storage_ordered_dict here, but does anything need it?  If you aren't concerned about order by id, then cfg should be equivalent here.   Note also that the id strings are entirely arbitrary, so it just really guarantees that the order of iteration in one spot handling curtin storage config will be in the same order as iteration in another spot handling curtin storage config.  Again, I'm fine with using it, but I'm curious what depends on that.

> +
> +    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,
> @@ -1931,7 +1963,14 @@ def uses_grub(machine):
>      return True
>  
>  
> -def builtin_curthooks(cfg, target, state):
> +def builtin_curthooks(cfg: dict, target: str, state: dict):
> +    """Run the built-in curthooks

Thanks for doing this!

> +
> +    :param: cfg: Config dict
> +    :param: target: Target directory for the new installation
> +    :param: state: State information obtained from environment variables. See
> +        load_command_environment()
> +    """
>      LOG.info('Running curtin builtin curthooks')
>      stack_prefix = state.get('report_stack_prefix', '')
>      state_etcd = os.path.split(state['fstab'])[0]
> 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'

Suggest to pursue this in a future MP: some ISOs (desktop) will want to produce a target system with "quiet splash", some (server) will no.  We do this today by reading the kernel command line we booted with, which I'm not 100% certain is the best choice but is current behavior and should probably match here.  See install_grub.py `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.