Re: [PATCH v4] meta/classes: generate bill of material from image

[Claudius Heine <claudius.heine.ext@siemens.com>]

Hi Quirin,

On 13/08/2019 10.18, [ext] Q. Gylstorff wrote:
> From: Quirin Gylstorff <quirin.gylstorff@siemens.com>
> 
> To create products it is necessary to have a list
> of used packages for clearance and to security monitoring.
> To get a simple list of packages use dpkg-query and generate
> a list with the following pattern:
> 
> source name| source version | binary package name | binary version
> 
> The list is stored in ${IMAGE_FULLNAME}.rootfs.manifest
> 
> Remove the feature with:
> ROOTFS_FEATURES_remove = "generate-manifest"
> 
> Signed-off-by: Quirin Gylstorff <quirin.gylstorff@siemens.com>
> ---
> Changes:
> v4:
> Add sdk rootfs to manifest
> Avoid duplicated code and move gen_accounts_array and gen_manifest_array to
>    shell-list-processing-helper
> call dpkg-query from $PATH
> 
> v3:
> Add list of manifest for buildchroot manifest
> This list can be exdent to add additional output generators
> 
> v2:
> use FEATURE instead of own variable
> 
>   meta/classes/image-account-extension.bbclass  | 28 ++--------
>   .../image-package-list-extension.bbclass      | 54 +++++++++++++++++++
>   meta/classes/image.bbclass                    |  3 +-
>   .../shell-list-processing-helper.bbclass      | 30 +++++++++++
>   4 files changed, 89 insertions(+), 26 deletions(-)
>   create mode 100644 meta/classes/image-package-list-extension.bbclass
>   create mode 100644 meta/classes/shell-list-processing-helper.bbclass
> 
> diff --git a/meta/classes/image-account-extension.bbclass b/meta/classes/image-account-extension.bbclass
> index 22754da..df44c49 100644
> --- a/meta/classes/image-account-extension.bbclass
> +++ b/meta/classes/image-account-extension.bbclass
> @@ -25,36 +25,14 @@ GROUPS ??= ""
>   #GROUP_root[gid] = ""
>   #GROUP_root[flags] = "system"
>   
> -def gen_accounts_array(d, listname, entryname, flags, verb_flags=None):
> -    from itertools import chain
> -
> -    entries = (d.getVar(listname, True) or "").split()
> -    return " ".join(
> -        ":".join(
> -            chain(
> -                (entry,),
> -                (
> -                    (",".join(
> -                        (
> -                            d.getVarFlag(entryname + "_" + entry, flag, True) or ""
> -                        ).split()
> -                    ) if flag not in (verb_flags or []) else (
> -                        d.getVarFlag(entryname + "_" + entry, flag, True) or ""
> -                    )).replace(":","=")
> -                    for flag in flags
> -                ),
> -            )
> -        )
> -        for entry in entries
> -    )
> -
> +inherit shell-list-processing-helper
>   # List of space separated entries, where each entry has the format:
>   # username:encryptedpassword:expiredate:inactivenumber:userid:groupid:comment:homedir:shell:group1,group2:flag1,flag2
> -IMAGE_ACCOUNTS_USERS =+ "${@gen_accounts_array(d, 'USERS', 'USER', ['password',  'expire', 'inactive', 'uid', 'gid', 'comment', 'home', 'shell', 'groups', 'flags'], ['password', 'comment', 'home', 'shell'])}"
> +IMAGE_ACCOUNTS_USERS =+ "${@gen_shell_list(d, 'USERS', 'USER', ['password',  'expire', 'inactive', 'uid', 'gid', 'comment', 'home', 'shell', 'groups', 'flags'], ['password', 'comment', 'home', 'shell'])}"
>   
>   # List of space separated entries, where each entry has the format:
>   # groupname:groupid:flag1,flag2
> -IMAGE_ACCOUNTS_GROUPS =+ "${@gen_accounts_array(d, 'GROUPS', 'GROUP', ['gid', 'flags'])}"
> +IMAGE_ACCOUNTS_GROUPS =+ "${@gen_shell_list(d, 'GROUPS', 'GROUP', ['gid', 'flags'])}"
>   
>   ROOTFS_CONFIGURE_COMMAND += "image_configure_accounts"
>   image_configure_accounts[weight] = "3"
> diff --git a/meta/classes/image-package-list-extension.bbclass b/meta/classes/image-package-list-extension.bbclass
> new file mode 100644
> index 0000000..0aa3015
> --- /dev/null
> +++ b/meta/classes/image-package-list-extension.bbclass
> @@ -0,0 +1,54 @@
> +# This software is a part of ISAR.
> +# Copyright (C) Siemens AG, 2019
> +#
> +# SPDX-License-Identifier: MIT
> +MANIFESTS ?= "target build sdk"
> +DPKG_DIR ?= "/var/lib/dpkg"
> +# rootfs needs to be mounted inside of buildchroot
> +MANIFEST_build[rootfs] ?= "${DPKG_DIR}"
> +MANIFEST_target[rootfs] ?= "${PP_ROOTFS}${DPKG_DIR}"
> +MANIFEST_sdk[rootfs] ?= "/work/${DISTRO}-${DISTRO_ARCH}/sdkchroot-${HOST_DISTRO}-${HOST_ARCH}-${DISTRO_ARCH}/rootfs${DPKG_DIR}"

That is a long and pretty explicit path. Can you change this to use more 
commonly used variables and if those are missing define them somewhere 
global where sdkchroot and others can use them?

Otherwise if those paths are changed at some point in the future, we 
have to hunt down every of those magic variables to fix them as well, 
instead of just at one global point.

You should also probably write something about the user facing interface 
of this class in the documentation.

> +
> +inherit shell-list-processing-helper
> +IMAGE_MANIFESTS =+ "${@gen_shell_list(d, 'MANIFESTS', 'MANIFEST', ['rootfs'])}"
> +
> +do_image_generate_manifest[dirs] = "${DEPLOY_DIR_IMAGE}"
> +image_generate_manifest() {
> +    image_do_mounts
> +    # mount working directory to access sdk rootfs
> +    sudo -s <<'EOSUDO'
> +        ( flock 9
> +            mkdir -p ${BUILDCHROOT_DIR}/work
> +            if ! mountpoint ${BUILDCHROOT_DIR}/work >/dev/null 2>&1; then
> +                mount --bind --make-private ${TMPDIR}/work ${BUILDCHROOT_DIR}/work
> +            fi

Does that mean that 'do_image_generate_manifest' now depends on 
sdkchroot? Is that task dependency missing?

Personally I would have the manifest for sdkchroot 'opt-in', since I 
don't want to create a sdkchroot just to build a image everytime.

> +        ) 9>${MOUNT_LOCKFILE}
> +EOSUDO
> +    list='${@" ".join(d.getVar('IMAGE_MANIFESTS', True).split())} '
> +    while true; do
> +        list_rest="${list#*:* }"
> +        entry="${list%%${list_rest}}"
> +        list="${list_rest}"
> +
> +        if [ -z "${entry}" ]; then
> +            break
> +        fi
> +        # Add colon to the end of the entry and remove trailing space:
> +        entry="${entry% }:"
> +
> +        # Decode entries:
> +        name="${entry%%:*}"
> +        entry="${entry#${name}:}"
> +
> +        rootfs="${entry%%:*}"
> +        entry="${entry#${rootfs}:}"

Maybe put a empty line here. You had 3 here before, now none, just 
settle with one, maybe two, if you are feeling generous today. :)

> +        if sudo -E chroot ${BUILDCHROOT_DIR} test -d "$rootfs"; then
> +        sudo -E chroot ${BUILDCHROOT_DIR} \
> +        dpkg-query --admindir="$rootfs" \
> +        -f '${source:Package}|${source:Version}|${binary:Package}|${Version}\n' -W > \
> +        ${DEPLOY_DIR_IMAGE}/${IMAGE_FULLNAME}."$name".manifest
> +        fi
> +    done
> +}
> +ROOTFS_POSTPROCESS_COMMAND += "${@bb.utils.contains('ROOTFS_FEATURES', 'generate-manifest', 'image_generate_manifest', '', d)}"
> +
> diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass
> index ec6bd39..60dd9fb 100644
> --- a/meta/classes/image.bbclass
> +++ b/meta/classes/image.bbclass
> @@ -58,7 +58,7 @@ image_do_mounts() {
>   }
>   
>   ROOTFSDIR = "${IMAGE_ROOTFS}"
> -ROOTFS_FEATURES += "copy-package-cache clean-package-cache finalize-rootfs"
> +ROOTFS_FEATURES += "copy-package-cache clean-package-cache finalize-rootfs generate-manifest"
>   ROOTFS_PACKAGES += "${IMAGE_PREINSTALL} ${IMAGE_INSTALL}"
>   
>   inherit rootfs
> @@ -68,6 +68,7 @@ inherit image-tools-extension
>   inherit image-postproc-extension
>   inherit image-locales-extension
>   inherit image-account-extension
> +inherit image-package-list-extension
>   
>   # Extra space for rootfs in MB
>   ROOTFS_EXTRA ?= "64"
> diff --git a/meta/classes/shell-list-processing-helper.bbclass b/meta/classes/shell-list-processing-helper.bbclass
> new file mode 100644
> index 0000000..105066b
> --- /dev/null
> +++ b/meta/classes/shell-list-processing-helper.bbclass
> @@ -0,0 +1,30 @@
> +# This software is a part of ISAR.
> +# Copyright (C) Siemens AG, 2019
> +#
> +# SPDX-License-Identifier: MIT
> +#
> +# This class extends the image.bbclass for creating user accounts and groups.

To much copy pasta.

> +
> +def gen_shell_list(d, listname, entryname, flags, verb_flags=None):
> +    from itertools import chain
> +
> +    entries = (d.getVar(listname, True) or "").split()
> +    return " ".join(
> +        ":".join(
> +            chain(
> +                (entry,),
> +                (
> +                    (",".join(
> +                        (
> +                            d.getVarFlag(entryname + "_" + entry, flag, True) or ""
> +                        ).split()
> +                    ) if flag not in (verb_flags or []) else (
> +                        d.getVarFlag(entryname + "_" + entry, flag, True) or ""
> +                    )).replace(":","=")
> +                    for flag in flags
> +                ),
> +            )
> +        )
> +        for entry in entries
> +    )
> +

I a still a bit unconvinced that we need the list feature here. Since 
that feature is more for the usecase that there needs to be a flexible 
number of configuration items each with multiple parameters. Like users 
and groups. That needs to be customized by the end user.

Here we now have 3, with each just one setting each.

Instead I would think that this could be a 'rootfs' extension, where 
each rootfs (every image, buildchroot and sdk) can create their own 
package listing.

regards,
Claudius

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-54 Fax: (+49)-8142-66989-80 Email: ch@denx.de