[Merge] ~adam-retter/uvtool:emu-riscv64 into uvtool:main

[Robie Basak]

Review: Needs Fixing

> it's hard to navigate

I appreciate the difficulty. Sorry about that! In time this will get better, but the details of making it simpler is quite complicated because of the history of our infrastructure predating git itself by many years.

This looks good to me. Thanks! I have a few minor requests for changes inline, from a general maintainability perspective. You can update by (force) pushing to the newer URL I gave you - this MP uses that now, and the old one isn't required (that was the wrong push URL to use).

I don't have any riscv64-specific knowledge and will ask a colleague to take a look from that perspective. You might want to wait for that review.

Diff comments:

> diff --git a/template-emu-riscv64.xml b/template-emu-riscv64.xml
> new file mode 100644
> index 0000000..871d516
> --- /dev/null
> +++ b/template-emu-riscv64.xml
> @@ -0,0 +1,21 @@
> +<domain type='qemu'>
> +    <os>
> +        <type arch='riscv64' machine='virt'>hvm</type>
> +        <kernel>/usr/lib/u-boot/qemu-riscv64_smode/uboot.elf</kernel>

This is more of a question for a colleague who is familiar with qemu-related packaging.

This file looks like it is shipped by the u-boot-qemu package. Do we need to and/or should we Recommend it, or similar, in packaging? Is there an established pattern we could follow? I don't see any of the other existing domain templates having a dependency like this.

> +        <cmdline>root=/dev/vda1</cmdline>
> +        <boot dev='hd'/>
> +    </os>
> +    <features>
> +        <acpi/>
> +    </features>
> +    <devices>
> +        <emulator>/usr/bin/qemu-system-riscv64</emulator>
> +        <interface type='network'>
> +            <source network='default'/>
> +            <target dev='vnet0'/>
> +        </interface>
> +        <console type='pty'>
> +            <target type='serial' port='0'/>
> +        </console>
> +    </devices>
> +</domain>
> diff --git a/uvtool/libvirt/__init__.py b/uvtool/libvirt/__init__.py
> index 3559a03..106f6f3 100644
> --- a/uvtool/libvirt/__init__.py
> +++ b/uvtool/libvirt/__init__.py
> @@ -90,7 +91,7 @@ def is_emulation_target(target_arch):
>      returns True if emulation is needed and supported.
>      """
>      # early exit on not supported emulations
> -    if target_arch != 'armhf':
> +    if target_arch != 'armhf' and target_arch != 'riscv64':

Please use `if target_arch not in ['armhf', 'riscv64']:` instead. This is easier to read since target_arch isn't duplicated and the reader doesn't have to resolve the question of whether the two names are exactly the same.

>          return False
>  
>      local_arch = platform_arch_to_image_arch(platform.machine())
> diff --git a/uvtool/libvirt/kvm.py b/uvtool/libvirt/kvm.py
> index 1bc0d63..68fb1d3 100755
> --- a/uvtool/libvirt/kvm.py
> +++ b/uvtool/libvirt/kvm.py
> @@ -75,6 +75,8 @@ def get_template_path(target_arch):
>      if uvtool.libvirt.is_emulation_target(target_arch):
>          if target_arch == 'armhf':
>              return '/usr/share/uvtool/libvirt/template-emu-armhf.xml'
> +        if target_arch == 'riscv64':

Please follow the existing pattern (eg. a few lines below) and use elif. I appreciate it makes no functional difference, but in that case following the existing nearby patterns is preferable to avoid style-soup.

> +            return '/usr/share/uvtool/libvirt/template-emu-riscv64.xml'
>  
>      if target_arch == 'aarch64':
>          return '/usr/share/uvtool/libvirt/template-aarch64.xml'


-- 
https://code.launchpad.net/~adam-retter/uvtool/+git/uvtool/+merge/457345
Your team Ubuntu Sponsors is requested to review the proposed merge of ~adam-retter/uvtool:emu-riscv64 into uvtool:main.