On 7 Nov 2023, at 15:12, Andrey Popov via lists.openembedded.org 
<andrey.popov=yadro....@lists.openembedded.org> wrote:
> @@ -29,8 +29,10 @@ do_bootimg[depends] += 
> "dosfstools-native:do_populate_sysroot \
>                         mtools-native:do_populate_sysroot \
>                         cdrtools-native:do_populate_sysroot \
>                         virtual/kernel:do_deploy \
> -                        ${MLPREFIX}syslinux:do_populate_sysroot \
> -                        syslinux-native:do_populate_sysroot \
> +                        ${@d.getVar('MLPREFIX') + 
> 'syslinux:do_populate_sysroot \
> +                           syslinux-native:do_populate_sysroot' if 
> __import__('re').match('i.86', d.getVar('TARGET_ARCH')) or \
> +                                                                   
> __import__('re').match('x86_64', d.getVar('TARGET_ARCH')) else \
> +                           'xorriso-native:do_populate_sysroot'} \
>                         ${@'%s:do_image_%s' % (d.getVar('PN'), 
> d.getVar('LIVE_ROOTFS_TYPE').replace('-', '_')) if d.getVar('ROOTFS') else 
> ''} \
>                         "

Instead of looking at the TARGET_ARCH, please use the perfectly good machine 
features that already exist: pcbios and efi. If the target is EFI-only there’s 
no point in supporting syslinux, and it would be nice to be able to drop it 
entirely one day in the future.  This class shouldn’t be checking the 
architecture at all, simply check for the efi and pcbios features.

This also pulls in a hard-dependency on xorriso which isn’t in oe-core, so that 
will break the users of the class that just use core.  xorriso also doesn’t 
exist in any layer anymore as I removed it from meta-oe in nanbield, so I know 
that you’ve not tested this code with master or nanbield.

> @@ -71,14 +73,92 @@ MKISOFS_OPTIONS = "-no-emul-boot -boot-load-size 4 
> -boot-info-table"
> BOOTIMG_VOLUME_ID   ?= "boot"
> BOOTIMG_EXTRA_SPACE ?= "512"
> 
> +def compute_chs(sector_z):
> +    C = int(sector_z / (63 * 255))
> +    H = int((sector_z % (63 * 255)) / 63)
> +    # convert zero-based sector to CHS format
> +    S = int(sector_z % 63) + 1
> +    # munge accord to partition table format
> +    S = (S & 0x3f) | (((C >> 8) & 0x3) << 6)
> +    C = (C & 0xFF)
> +    return (C, H, S)
> +
> +def mk_efi_part_table(iso, start, length):
> +    from struct import pack
> +
> +    # Compute starting and ending CHS addresses for the partition entry.
> +    (s_C, s_H, s_S) = compute_chs(start)
> +    (e_C, e_H, e_S) = compute_chs(start + length - 1)
> +
> +    # Write the 66 byte partition table to bytes 0x1BE through 0x1FF in
> +    # sector 0 of the .ISO.
> +    #
> +    # See the partition table format here:
> +    # http://en.wikipedia.org/wiki/Master_boot_record#Sector_layout
> +    f = open(iso, 'r+b')
> +    f.seek(0x1BE)
> +    f.write(pack("<8BLL48xH", 0x80, s_H, s_S, s_C,
> +                 0xEF, e_H, e_S, e_C, start, length, 0xAA55))
> +    f.close()

Why not continue to use syslinux-native to provide isohybrid? My understanding 
is that isohybrid simply messes with partition tables and doesn’t depend on a 
target syslinux. Of course if I’m wrong then this needs to be explained in the 
class: we’ve a problem with complicated classes being written without any 
explanation of what is happening so lets try to improve the situation instead 
of making it worse.

> +def runtool(cmdln_or_args):
> +    import subprocess
> +
> +    if isinstance(cmdln_or_args, list):
> +        cmd = cmdln_or_args[0]
> +        shell = False
> +    else:
> +        import shlex
> +        cmd = shlex.split(cmdln_or_args)[0]
> +        shell = True
> +
> +    sout = subprocess.PIPE
> +    serr = subprocess.STDOUT
> +
> +    try:
> +        process = subprocess.Popen(cmdln_or_args, stdout=sout,
> +                                   stderr=serr, shell=shell)
> +        sout, serr = process.communicate()
> +        # combine stdout and stderr, filter None out and decode
> +        out = ''.join([out.decode('utf-8') for out in [sout, serr] if out])
> +    except OSError as err:
> +        bb.fatal("Cannot run command %s: %s" % (cmd, err))
> +
> +    return process.returncode, out
> +
> +def exec_cmd(cmd_and_args):
> +    args = cmd_and_args.split()
> +    ret, out = runtool(args)
> +    out = out.strip()
> +    if ret != 0:
> +        bb.fatal("exec_cmd: %s returned '%s' instead of 0\noutput: %s" % \
> +                 (cmd_and_args, ret, out))
> +    return ret, out

These two functions are basically a bad reimplementation of subproccess.run(). 
Don’t copy-paste, just use modern API.

Ross

-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#191445): 
https://lists.openembedded.org/g/openembedded-core/message/191445
Mute This Topic: https://lists.openembedded.org/mt/102444305/21656
Group Owner: openembedded-core+ow...@lists.openembedded.org
Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub 
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Reply via email to