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

[Alexander Smirnov <asmirnov@ilbers.de>]

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?

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

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

-- 
With best regards,
Alexander Smirnov

ilbers GmbH
Baierbrunner Str. 28c
D-81379 Munich
+49 (89) 122 67 24-0
http://ilbers.de/
Commercial register Munich, HRB 214197
General manager: Baurzhan Ismagulov