Re: [PATCH 1/6] meta-isar-bin: Enable caching of deb packages

[Henning Schild <henning.schild@siemens.com>]

This patch is very big, doing a lot of things at a time.
The commit message on the other hand is not very verbose.

creating the repo is one step
 - i think using reprepro is a good idea
moving do_populate to that repo is another one
 - i think this task should go away and the repo should be given to
   multistrap
and there are probably more ways to split that into reviewable pieces

So this review is not too verbose, we need smaller pieces.

More inline

Am Sun, 27 Aug 2017 18:13:34 +0300
schrieb Alexander Smirnov <asmirnov@ilbers.de>:

> From: Frank Lenormand <lenormf@gmail.com>
> 
> Adds the possibility to create an apt repository for newly built
> packages and then use that repo for populating the target filesystem.
> 
> Signed-off-by: Frank Lenormand <lenormf@gmail.com>
> Signed-off-by: Alexander Smirnov <asmirnov@ilbers.de>
> ---
>  meta-isar-bin/conf/layer.conf                      |  18 +++
>  meta-isar-bin/files/distributions.in               |   3 +
>  meta-isar/conf/bblayers.conf.sample                |   1 +
>  .../images/files/debian-configscript.sh            |   8 ++
>  .../images/files/raspbian-configscript.sh          |   8 ++
>  meta-isar/recipes-core/images/isar-image-base.bb   |   8 +-
>  meta/classes/dpkg.bbclass                          | 126
> +++++++++++++++++++--
> meta/classes/image.bbclass                         |  30 +++-- 8
> files changed, 186 insertions(+), 16 deletions(-) create mode 100644
> meta-isar-bin/conf/layer.conf create mode 100644
> meta-isar-bin/files/distributions.in
> 
> diff --git a/meta-isar-bin/conf/layer.conf
> b/meta-isar-bin/conf/layer.conf new file mode 100644
> index 0000000..24534e1
> --- /dev/null
> +++ b/meta-isar-bin/conf/layer.conf
> @@ -0,0 +1,18 @@
> +# Enable package caching with '1'
> +DEBCACHE_ENABLED ?= "0"
> +
> +# Codename of the repository created by the caching class
> +DEBDISTRONAME = "isar"
> +
> +# Path to the caching repository
> +DEBCACHEDIR ?= "${LAYERDIR}/apt"
> +
> +# Path to the mount point of the repository within the target
> rootfs, during +# population
> +DEBCACHEMNT ?= "/opt/cache/apt"
> +
> +# Path to the databases used by `reprepro`
> +DEBDBDIR ?= "${LAYERDIR}/db"
> +
> +# Path to the configuration files templates used by `reprepro`
> +DEBFILESDIR ?= "${LAYERDIR}/files"
> diff --git a/meta-isar-bin/files/distributions.in
> b/meta-isar-bin/files/distributions.in new file mode 100644
> index 0000000..59f4429
> --- /dev/null
> +++ b/meta-isar-bin/files/distributions.in
> @@ -0,0 +1,3 @@
> +Codename: {DISTRO_NAME}
> +Architectures: i386 armhf source
> +Components: main
> diff --git a/meta-isar/conf/bblayers.conf.sample
> b/meta-isar/conf/bblayers.conf.sample index 80867e7..53a362b 100644
> --- a/meta-isar/conf/bblayers.conf.sample
> +++ b/meta-isar/conf/bblayers.conf.sample
> @@ -8,6 +8,7 @@ BBFILES ?= ""
>  BBLAYERS ?= " \
>    ##ISARROOT##/meta \
>    ##ISARROOT##/meta-isar \
> +  ##ISARROOT##/meta-isar-bin \
>    "
>  BBLAYERS_NON_REMOVABLE ?= " \
>    ##ISARROOT##/meta \
> diff --git
> a/meta-isar/recipes-core/images/files/debian-configscript.sh
> b/meta-isar/recipes-core/images/files/debian-configscript.sh index
> 4ac37d0..b05babb 100644 ---
> a/meta-isar/recipes-core/images/files/debian-configscript.sh +++
> b/meta-isar/recipes-core/images/files/debian-configscript.sh @@ -8,6
> +8,8 @@ set -e readonly MACHINE_SERIAL="$1" readonly BAUDRATE_TTY="$2"
>  readonly ROOTFS_DEV="$3"
> +readonly DEBCACHEMNT="$4"
> +readonly DEBDISTRONAME="$5"
>  
>  cat >> /etc/default/locale << EOF
>  LANG=en_US.UTF-8
> @@ -83,3 +85,9 @@ fi
>  if [ -x "$TARGET/sbin/init" -a -x "$TARGET/usr/sbin/policy-rc.d" ];
> then rm -f $TARGET/usr/sbin/policy-rc.d
>  fi
> +
> +mkdir -p /etc/apt/sources.list.d/
> +cat <<EOF >/etc/apt/sources.list.d/${DEBDISTRONAME}.list
> +deb file:${DEBCACHEMNT}/ ${DEBDISTRONAME} main
> +deb-src file:${DEBCACHEMNT}/ ${DEBDISTRONAME} main
> +EOF
> diff --git
> a/meta-isar/recipes-core/images/files/raspbian-configscript.sh
> b/meta-isar/recipes-core/images/files/raspbian-configscript.sh index
> 2454481..f995f4d 100644 ---
> a/meta-isar/recipes-core/images/files/raspbian-configscript.sh +++
> b/meta-isar/recipes-core/images/files/raspbian-configscript.sh @@
> -8,6 +8,8 @@ set -e readonly MACHINE_SERIAL="$1" readonly
> BAUDRATE_TTY="$2" readonly ROOTFS_DEV="$3"
> +readonly DEBCACHEMNT="$4"
> +readonly DEBDISTRONAME="$5"
>  
>  cat >> /etc/default/locale << EOF
>  LANG=en_US.UTF-8
> @@ -89,3 +91,9 @@ KERNEL_IMAGE=`ls /boot | grep vmlinuz`
>  cat > /boot/config.txt << EOF
>  kernel=$KERNEL_IMAGE
>  EOF
> +
> +mkdir -p /etc/apt/sources.list.d/
> +cat <<EOF >/etc/apt/sources.list.d/${DEBDISTRONAME}.list
> +deb file:${DEBCACHEMNT}/ ${DEBDISTRONAME} main
> +deb-src file:${DEBCACHEMNT}/ ${DEBDISTRONAME} main
> +EOF
> diff --git a/meta-isar/recipes-core/images/isar-image-base.bb
> b/meta-isar/recipes-core/images/isar-image-base.bb index
> b679d97..85e6b27 100644 ---
> a/meta-isar/recipes-core/images/isar-image-base.bb +++
> b/meta-isar/recipes-core/images/isar-image-base.bb @@ -49,8 +49,12 @@
> do_rootfs() { sudo multistrap -a ${DISTRO_ARCH} -d "${S}" -f
> "${WORKDIR}/multistrap.conf" || true 
>      # Configure root filesystem
> -    sudo chroot ${S} /configscript.sh ${MACHINE_SERIAL}
> ${BAUDRATE_TTY} \
> -        ${ROOTFS_DEV}
> +    sudo chroot ${S} /configscript.sh \
> +        ${MACHINE_SERIAL}             \
> +        ${BAUDRATE_TTY}               \
> +        ${ROOTFS_DEV}                 \
> +        ${DEBCACHEMNT}                \
> +        ${DEBDISTRONAME}
>      sudo rm ${S}/configscript.sh
>  }
>  
> diff --git a/meta/classes/dpkg.bbclass b/meta/classes/dpkg.bbclass
> index 360a95c..b1e201d 100644
> --- a/meta/classes/dpkg.bbclass
> +++ b/meta/classes/dpkg.bbclass
> @@ -1,5 +1,5 @@
>  # This software is a part of ISAR.
> -# Copyright (C) 2015-2016 ilbers GmbH
> +# Copyright (C) 2015-2017 ilbers GmbH
>  
>  # Add dependency from buildchroot creation
>  DEPENDS += "buildchroot"
> @@ -55,12 +55,124 @@ do_build() {
>      sudo chroot ${BUILDCHROOT_DIR} /build.sh ${PP}/${SRC_DIR}
>  }
>  
> -
>  # Install package to dedicated deploy directory
> -do_install() {
> -    install -m 644 ${BUILDROOT}/*.deb ${DEPLOY_DIR_DEB}/
> +do_binary_deb_install() {
> +    readonly DIR_CACHE="${DEBCACHEDIR}/${DISTRO}"
> +    readonly DIR_DB="${DEBDBDIR}/${DISTRO}"
> +
> +    if [ "${DEBCACHE_ENABLED}" != "0" ]; then
> +        # If `bitbake` is running for the first time, the cache
> doesn't exist
> +        # yet and needs to be configured using a `distributions`
> file.
> +        # A template stored in the layer directory is pre-processed
> to
> +        # generate the configuration file, which is then placed in
> the
> +        # appropriate directory.
> +        if [ ! -e "${DIR_CACHE}/conf/distributions" ]; then
> +            mkdir -p "${DIR_CACHE}/conf"
> +            sed -e "s#{DISTRO_NAME}#${DEBDISTRONAME}#g" \
> +                "${DEBFILESDIR}/distributions.in" \
> +                > "${DIR_CACHE}/conf/distributions"
> +        fi

This step should be in the image and not in every package. I guess it
has a race with multiple packages running this init routine.

> +        # Add binary and source packages to the deb cache
> +        # If the cache doesn't exist yet, it will be created using
> the
> +        # `distributions` file generated above.
> +        ls -1 "${BUILDROOT}"/*.deb "${BUILDROOT}"/*.dsc | while read
> -r p; do
> +            reprepro --waitforlock 3 -b "${DIR_CACHE}" --dbdir
> "${DIR_DB}" \
> +                -C main "include${p##*.}" "${DEBDISTRONAME}" "${p}"
> +        done
> +    else
> +        # Deb caching is disabled, simply copy all binary packages
> to the
> +        # deploy directory
> +        mkdir -p "${DEPLOY_DIR_DEB}"
> +        install -m 644 "${BUILDROOT}"/*.deb "${DEPLOY_DIR_DEB}/"
> +    fi
>  }
>  
> -addtask install after do_build
> -do_install[dirs] = "${DEPLOY_DIR_DEB}"
> -do_install[stamp-extra-info] = "${MACHINE}"
> +addtask binary_deb_install after do_build
> +do_binary_deb_install[stamp-extra-info] = "${DISTRO}-${DISTRO_ARCH}"

Here a task got renamed. This should be another patch ... one that i
already tried to get in.

> +
> +# Deb caching lambda run during the parsing phase that checks
> whether the +# current package has to be rebuilt, or taken from the
> cache +python __anonymous () {
> +    if d.getVar("DEBCACHE_ENABLED", True) == "0":
> +        # Deb caching is disabled, do nothing
> +        return True
> +
> +    PN = d.getVar("PN", True)
> +    PV = d.getVar("PV", True)
> +    DISTRO_ARCH = d.getVar("DISTRO_ARCH", True)
> +    DEBCACHEDIR = d.getVar("DEBCACHEDIR", True)
> +    DEBDISTRONAME = d.getVar("DEBDISTRONAME", True)
> +    DEBDBDIR = d.getVar("DEBDBDIR", True)
> +    DISTRO = d.getVar("DISTRO", True)
> +    path_cache = os.path.join(DEBCACHEDIR, DISTRO)
> +    path_databases = os.path.join(DEBDBDIR, DISTRO)
> +    path_distributions = os.path.join(path_cache, "conf",
> "distributions") +
> +    # The distributions file is needed by `reprepro` to know what
> types
> +    # of packages are supported, what the distribution name is, etc.
> +    # If it doesn't exist, we have nothing in the cache, do nothing.
> +    if not os.path.exists(path_distributions):
> +        return
> +
> +    # Anonymous functions are run several times under different
> contexts
> +    # during the parsing phase, which would let the code that
> follows be run
> +    # as many times for the same package.
> +    # In order to guarantee that our subroutine only runs once per
> package, we
> +    # use bitbake's "persist" API in order to have reliable
> persistent storage
> +    # accross calls of the lambda (using a simple variable in the
> class won't
> +    # work, as several contexts won't allow fetching its value).
> +    pd = bb.persist_data.persist("DEBCACHE_PACKAGES", d)
> +    if PN in pd and pd[PN] == PV:
> +        return
> +
> +    import subprocess
> +    try:
> +        # The databases used by `reprepro` are not stored within the
> cache in
> +        # order to make versioning of only the files needed to use
> the cache
> +        # as an official Debian repository simpler.
> +        # As such, if a developer uses a peer's cache to speed up
> their build
> +        # time but have never run bitbake, the database will not
> have been
> +        # created, so we regenerate them here.
> +        if not os.path.exists(path_databases) and
> os.path.exists(path_cache):
> +            bb.note("Regenerating the cache databases...")
> +            subprocess.check_call([
> +                "reprepro",
> +                "--waitforlock", "3",
> +                "-b", path_cache,
> +                "--dbdir", path_databases,
> +                "export", DEBDISTRONAME,
> +            ])
> +
> +        # Get a list of the versions of all the packages named after
> the
> +        # current bitbake package, and check whether the current
> package
> +        # version is returned.
> +        # As `reprepro` always returns zero with this particular
> operation, we
> +        # have to use this workaround to check for a package in the
> cache.
> +        package_version = subprocess.check_output([
> +            "reprepro",
> +            "--waitforlock", "3",
> +            "-b", path_cache,
> +            "--dbdir", path_databases,
> +            "-C", "main",
> +            "-A", DISTRO_ARCH,
> +            "--list-format", "${version}",
> +            "list", DEBDISTRONAME, PN,
> +        ])
> +        package_version = package_version.decode("utf-8")
> +        if package_version == PV:
> +            # The below list contains the names of all the tasks are
> in charge
> +            # of building the package when the cache isn't enabled
> or if the
> +            # package hasn't been placed in it already.
> +            # As all tasks are enabled by default, we prevent their
> execution
> +            # by setting the `noexec` flag, which will prevent a
> rebuild of
> +            # the package when it's cached.
> +            for task in ["fetch", "unpack", "build", "install"]:
> +                d.setVarFlag("do_{}".format(task), "noexec", "1")
> +
> +            # Cache the results of this command so that subsequent
> executions
> +            # of this anonymous functions don't run the same code
> again.
> +            pd[PN] = PV
> +    except subprocess.CalledProcessError as e:
> +        bb.fatal("Unable to check for a candidate for package {0}
> (errorcode: {1})".format(PN, e.returncode)) +}
> diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass
> index a7f0d74..f42aa48 100644
> --- a/meta/classes/image.bbclass
> +++ b/meta/classes/image.bbclass
> @@ -11,18 +11,34 @@ inherit ${IMAGE_TYPE}
>  
>  do_populate[stamp-extra-info] = "${MACHINE}-${DISTRO}"
>  
> -# Install Debian packages, that were built from sources
> +# Install Debian packages from the cache
>  do_populate() {
> +    readonly DIR_CACHE="${DEBCACHEDIR}/${DISTRO}"
> +
>      if [ -n "${IMAGE_INSTALL}" ]; then
> -        sudo mkdir -p ${S}/deb
> +        if [ "${DEBCACHE_ENABLED}" != "0" ]; then
> +            sudo mkdir -p "${S}/${DEBCACHEMNT}"
> +            sudo mount -o bind "${DIR_CACHE}" "${S}/${DEBCACHEMNT}"
> +
> +            sudo chroot "${S}" apt-get update -y

This will fail if you are behind a proxy. It makes a known issue worse.

> +            for package in ${IMAGE_INSTALL}; do
> +                sudo chroot "${S}" apt-get install -t
> "${DEBDISTRONAME}" -y \
> +                    --allow-unauthenticated "${package}"
> +            done
> +
> +            sudo umount "${S}/${DEBCACHEMNT}"

A whole lot of sudo. I do not mind but am still wondering ...

Can we replace the bind-mount with a symlink? If anything between the
mount and umount fails and you need to run again ... will the mount
fail if the old one is still there?

> +        else
> +            sudo mkdir -p ${S}/deb
>  
> -        for p in ${IMAGE_INSTALL}; do
> -            sudo cp ${DEPLOY_DIR_DEB}/${p}_*.deb ${S}/deb
> -        done
> +            for p in ${IMAGE_INSTALL}; do
> +                find "${DEPLOY_DIR_DEB}" -type f -name '*.deb' -exec
> \
> +                    sudo cp '{}' "${S}/deb/" \;
> +            done
>  
> -        sudo chroot ${S} /usr/bin/dpkg -i -R /deb
> +            sudo chroot ${S} /usr/bin/dpkg -i -R /deb
>  
> -        sudo rm -rf ${S}/deb
> +            sudo rm -rf ${S}/deb
> +        fi
>      fi
>  }
>