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

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

Am Tue, 29 Aug 2017 09:40:46 +0300
schrieb Alexander Smirnov <asmirnov@ilbers.de>:

> On 08/28/2017 06:18 PM, Henning Schild wrote:
> > This patch is very big, doing a lot of things at a time.  
> 
> Ok, I'll think how it can be split.
> 
> > The commit message on the other hand is not very verbose.  
> 
> Half of the patch is the comments that describe in details what
> happens in the code, so please questions and proposals.

I will comment on that once the patch got split. In general readable
good does not require comments. Comments add redundancy and if comments
are required it is usually a sign that the code is too complicated.

> > 
> > 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  
> 
> Ok
> 
> > and there are probably more ways to split that into reviewable
> > pieces  
> 
> Please questions and proposals.

Yes, for v2.

> > 
> > 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.  
> 
> Ok.
> 
> >   
> >> +        # 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.
> >   
> 
> This implemented in the next patch (to keep this patch in original 
> format), but yes, it should be done here.
> 
> >> +
> >> +# 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 ...  
> 
> Please questions and proposals instead of negative tone.

That was maybe slightly negative. Why? Because that patch was sent at a
time where the policy was still "do not add more sudos". And the patch
violates more of the groundrules we have agreed on.
You posting the patch signals that our groundrules seem to apply
to !ilbers-contributions only.

> > 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?  
> 
> If we switch to multistrap-based populating, I think this will go.

I know, but we have not decided that yet.

Henning
 
> >   
> >> +        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
> >>   }
> >>     
> >