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

[Alexander Smirnov <asmirnov@ilbers.de>]

On 08/29/2017 10:51 AM, Henning Schild wrote:
> 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.

I think it's a matter of taste, but Ok, I got your position.

> 
>>>
>>> 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.

Ok.

> 
>>>
>>> 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.

That is actually not true, because the policy still is "do not add more 
sudo if you have no use-case for them".

I'd like to cover this topic again to try to explain the issue. The 
thread was started about your comment:

 > Some security enhancing packages can cause our initrd to be not
 > readable by a normal user. So we need to copy with sudo.
 > Also regular cp would destroy ownership and other attributes of files,
 > possibly creating problems in the future.

and in the patch you added three sudo instead of single one for initrd. 
No problem with sudo for initrd - you provided use-case. But "possibly 
creating problems in the future" doesn't provide use-case for the 
remaining two entries.

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