Re: [PATCH] debootstrap: Add fallback apt cache support

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

On 2018-05-14 13:11, Alexander Smirnov wrote:
> 
> 
> On 05/14/2018 01:33 PM, Claudius Heine wrote:
>> Hi Alex,
>>
>> On 2018-05-14 11:40, Alexander Smirnov wrote:
>>>
>>> On 05/14/2018 12:27 PM, Claudius Heine wrote:
>>>> Hi Alex,
>>>>
>>>> so I guess that this patch replaces the 'isar-bootstrap: Increase 
>>>> cache room' one right?
>>>>
>>>> On 2018-05-11 17:35, Alexander Smirnov wrote:
>>>>> Add apt config file to switch caching mechanism from MMap to 
>>>>> array-based.
>>>>> This needed to provide possibility to build fresh Isar suites on 
>>>>> old hosts.
>>>>> For example, building Isar stretch on jessie host fails with the 
>>>>> following
>>>>> error:
>>>>>
>>>>>    Hit:1 http://security.debian.org stretch/updates InRelease
>>>>>    Ign:2 http://ftp.de.debian.org/debian stretch InRelease
>>>>>    Hit:3 http://ftp.de.debian.org/debian stretch-updates InRelease
>>>>>    Hit:4 http://ftp.de.debian.org/debian stretch Release
>>>>>    E: Dynamic MMap ran out of room. Please increase the size of 
>>>>> APT::Cache-Start. Current value: 25165824. (man 5 apt.conf)
>>>>>    qemu: uncaught target signal 11 (Segmentation fault) - core dumped
>>>>>    Segmentation fault
>>>>>
>>>>> Now Isar performs the following:
>>>>> 1. Checking if user attempts to build stretch on old kernels
>>>>> 2. Temporarly installing this apt config file.
>>>>> 3. Removing this file after image rootfs creation
>>>>>
>>>>> So this file will not go to target rootfs.
>>>>>
>>>>> Signed-off-by: Alexander Smirnov <asmirnov@ilbers.de>
>>>>> ---
>>>>>   meta-isar/recipes-core/images/isar-image-base.bb        |  2 ++
>>>>>   .../isar-bootstrap/files/isar-apt-fallback.conf         |  4 ++++
>>>>>   meta/recipes-core/isar-bootstrap/isar-bootstrap.bb      | 17 
>>>>> ++++++++++++++++-
>>>>>   3 files changed, 22 insertions(+), 1 deletion(-)
>>>>>   create mode 100644 
>>>>> meta/recipes-core/isar-bootstrap/files/isar-apt-fallback.conf
>>>>>
>>>>> diff --git a/meta-isar/recipes-core/images/isar-image-base.bb 
>>>>> b/meta-isar/recipes-core/images/isar-image-base.bb
>>>>> index 00e926a..d65b1f1 100644
>>>>> --- a/meta-isar/recipes-core/images/isar-image-base.bb
>>>>> +++ b/meta-isar/recipes-core/images/isar-image-base.bb
>>>>> @@ -53,4 +53,6 @@ do_rootfs() {
>>>>>       sudo rmdir ${IMAGE_ROOTFS}/isar-apt
>>>>>       sudo umount -l ${IMAGE_ROOTFS}/dev
>>>>>       sudo umount -l ${IMAGE_ROOTFS}/proc
>>>>> +    [ -e "${IMAGE_ROOTFS}/etc/apt/apt.conf.d/55isar-fallback.conf" 
>>>>> ] && \
>>>>> +        sudo rm 
>>>>> "${IMAGE_ROOTFS}/etc/apt/apt.conf.d/55isar-fallback.conf" || true
>>>>
>>>> Why not just 'sudo rm -f 
>>>> "${IMAGE_ROOTFS}/etc/apt/apt.conf.d/55isar-fallback.conf"' ?
>>>>
>>>
>>> Matter of taste, I don't like to remove things that don't exist.
>>
>> I guess it is a matter of taste. I do think '|| true' is ugly and 
> 
> If you do think so, why then you use it in your isar-debootstrap 
> implementation?

You mean like this?

    sudo umount -l "${ROOTFSDIR}/dev" || true

I still find that ugly, but in may cases unavoidable. Bash/Shell is just 
a ugly language IMO. Just because something is ugly doesn't mean it 
should or can be avoided all the time.

> 
>> testing the existence of the file before removing it redundant.
>>
> 
> For me it's not too valuable issue, so I could drop this check in v3.

Ok, thanks.

Claudius

> 
> Alex
> 
>> Cheers,
>> Claudius
>>
>>>
>>>>
>>>>>   }
>>>>> diff --git 
>>>>> a/meta/recipes-core/isar-bootstrap/files/isar-apt-fallback.conf 
>>>>> b/meta/recipes-core/isar-bootstrap/files/isar-apt-fallback.conf
>>>>> new file mode 100644
>>>>> index 0000000..12ddbea
>>>>> --- /dev/null
>>>>> +++ b/meta/recipes-core/isar-bootstrap/files/isar-apt-fallback.conf
>>>>> @@ -0,0 +1,4 @@
>>>>> +# Switch apt caching mechanism from mmap to array-based.
>>>>> +# This helps to build fresh Isar suites on old hosts.
>>>>> +
>>>>> +APT::Cache-Fallback "1";
>>>>> diff --git a/meta/recipes-core/isar-bootstrap/isar-bootstrap.bb 
>>>>> b/meta/recipes-core/isar-bootstrap/isar-bootstrap.bb
>>>>> index a38dd88..a3a3a2b 100644
>>>>> --- a/meta/recipes-core/isar-bootstrap/isar-bootstrap.bb
>>>>> +++ b/meta/recipes-core/isar-bootstrap/isar-bootstrap.bb
>>>>> @@ -10,7 +10,9 @@ Description = "Minimal debian root file system"
>>>>>   LICENSE = "gpl-2.0"
>>>>>   LIC_FILES_CHKSUM = 
>>>>> "file://${LAYERDIR_isar}/licenses/COPYING.GPLv2;md5=751419260aa954499f7abaabaa882bbe" 
>>>>>
>>>>>   FILESPATH_prepend := "${THISDIR}/files:"
>>>>> -SRC_URI = "file://isar-apt.conf"
>>>>> +SRC_URI = " \
>>>>> +    file://isar-apt.conf \
>>>>> +    file://isar-apt-fallback.conf"
>>>>>   PV = "1.0"
>>>>>   WORKDIR = "${TMPDIR}/work/${DISTRO}-${DISTRO_ARCH}/${PN}"
>>>>> @@ -165,6 +167,13 @@ do_bootstrap() {
>>>>>   }
>>>>>   addtask bootstrap before do_build after do_apt_config_prepare
>>>>> +def get_host_release():
>>>>> +    import platform
>>>>> +    rel = platform.release()
>>>>> +    return rel
>>>>> +
>>>>> +HOST_KERN_VER = "${@get_host_release().split('.')[0]}"
>>>> If you need this variable in just one place. It might be useful to 
>>>> just add it there.
>>>>
>>>
>>> I've added the separate variable, because probably some additional 
>>> version specification could be needed later, like HOST_KERN_MAJOR 
>>> etc., in this case python inlining could be difficult for reading.
>>>
>>> But for today I have only one case, so could drop it in v3.
>>>
>>> Alex
>>>
>>>>> +
>>>>>   do_apt_config_install[stamp-extra-info] = "${DISTRO}-${DISTRO_ARCH}"
>>>>>   do_apt_config_install() {
>>>>>       sudo mkdir -p "${ROOTFSDIR}/etc/apt/preferences.d"
>>>>> @@ -177,6 +186,12 @@ do_apt_config_install() {
>>>>>       sudo mkdir -p "${ROOTFSDIR}/etc/apt/apt.conf.d"
>>>>>       sudo install -v -m644 "${WORKDIR}/isar-apt.conf" \
>>>>> "${ROOTFSDIR}/etc/apt/apt.conf.d/50isar.conf"
>>>>> +
>>>>> +    if [ "${@get_distro_suite(d)}" = "stretch" ] && [ 
>>>>> "${HOST_KERN_VER}" -lt "4" ]; then
>>>>
>>>> Like this:
>>>>
>>>>      if [ "${@get_distro_suite(d)}" = "stretch" ] && [ 
>>>> "${@get_host_release().split('.')[0]}" -lt "4" ]; then
>>>>
>>>> Or is it useful to overwrite this variable somewhere?
>>>>
>>>>> +        sudo install -v -m644 "${WORKDIR}/isar-apt-fallback.conf" \
>>>>> + "${ROOTFSDIR}/etc/apt/apt.conf.d/55isar-fallback.conf"
>>>>> +    fi
>>>>> +
>>>>>   }
>>>>>   addtask apt_config_install before do_build after do_bootstrap
>>>>
>>>> Apart from those small nitpicks it looks good,
>>>> 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