Re: [PATCH 1/2] isar: Change build folders tree

[Alexander Smirnov <asmirnov@ilbers.de>]

On 09/08/2017 04:07 PM, Henning Schild wrote:
> Am Fri, 8 Sep 2017 15:36:09 +0300
> schrieb Alexander Smirnov <asmirnov@ilbers.de>:
> 
>>> This one seems pretty big and is doing multiple things at a time. I
>>> suggest to split it into smaller units.
>>>
>>> As far as i can tell it is doing at least 3 things at a time.
>>> - rename S to IMAGE_ROOTFS
>>> - change multiconf stamps from "${MACHINE}-${DISTRO}" to
>>>     "${DISTRO}-${MACHINE}"
>>> - change folder structure "work/${PN}/${MACHINE}/${DISTRO}" to
>>>     "work/${DISTRO}-${DISTRO_ARCH}/${PF}"
>>>    
>>
>> No problem, if general approach is Ok, I'm going to split it into
>> pieces.
>>
>>> More inline.
>>>
>>> Am Fri, 8 Sep 2017 12:48:33 +0300
>>> schrieb Alexander Smirnov <asmirnov@ilbers.de>:
>>>    
>>>> Differentiate folders tree, that are generated during build on
>>>> the highest possible level. So the overal tree now looks like the
>>>> following:
>>>>
>>>>    tmp/${DISTRO}-${DISTRO_ARCH}/${PF}
>>>>
>>>> This approach eliminates lots of subfolders to diffirentiate
>>>> buildchroot and image for multiconfig builds. Now each
>>>> configuration has private tree.
>>>>
>>>> Signed-off-by: Alexander Smirnov <asmirnov@ilbers.de>
>>>> ---
>>>>    meta-isar/classes/rpi-sdimg.bbclass              |  2 +-
>>>>    meta-isar/recipes-core/images/isar-image-base.bb | 19
>>>> ++++++++----------- meta/classes/ext4-img.bbclass
>>>> | 10 +++++----- meta/classes/image.bbclass                       |
>>>> 12 +++++++----- meta/conf/isar-bitbake.conf
>>>> |  3 ++- meta/recipes-devtools/buildchroot/buildchroot.bb |  9
>>>> ++++----- 6 files changed, 27 insertions(+), 28 deletions(-)
>>>>
>>>> diff --git a/meta-isar/classes/rpi-sdimg.bbclass
>>>> b/meta-isar/classes/rpi-sdimg.bbclass index 2614c41..91b09cd 100644
>>>> --- a/meta-isar/classes/rpi-sdimg.bbclass
>>>> +++ b/meta-isar/classes/rpi-sdimg.bbclass
>>>> @@ -47,7 +47,7 @@ do_rpi_sdimg () {
>>>>        BOOT_BLOCKS=$(LC_ALL=C parted -s ${SDIMG} unit b print | awk
>>>> '/ 1 / { print substr($4, 1, length($4 -1)) / 512 /2 }') rm -f
>>>> ${WORKDIR}/boot.img mkfs.vfat -n "${BOOTDD_VOLUME_ID}" -S 512 -C
>>>> ${WORKDIR}/boot.img $BOOT_BLOCKS
>>>> -    mcopy -i ${WORKDIR}/boot.img -s ${S}/boot/* ::/
>>>> +    mcopy -i ${WORKDIR}/boot.img -s ${IMAGE_ROOTFS}/boot/* ::/
>>>
>>> This could be one patch that replaces S with IMAGE_ROOTFS.
>>>    
>>
>> Ok.
>>
>>>>    
>>>>        # Burn Partitions
>>>>        dd if=${WORKDIR}/boot.img of=${SDIMG} conv=notrunc seek=1
>>>> bs=$(expr ${IMAGE_ROOTFS_ALIGNMENT} \* 1024) && sync && sync diff
>>>> --git a/meta-isar/recipes-core/images/isar-image-base.bb
>>>> b/meta-isar/recipes-core/images/isar-image-base.bb index
>>>> b679d97..178ac05 100644 ---
>>>> a/meta-isar/recipes-core/images/isar-image-base.bb +++
>>>> b/meta-isar/recipes-core/images/isar-image-base.bb @@ -9,6 +9,7 @@
>>>> LICENSE = "gpl-2.0" LIC_FILES_CHKSUM =
>>>> "file://${LAYERDIR_isar}/licenses/COPYING.GPLv2;md5=751419260aa954499f7abaabaa882bbe"
>>>> PV = "1.0" +PF = "${PN}"
>>>>    
>>>>    inherit image
>>>>    
>>>> @@ -17,11 +18,7 @@ DEPENDS += "${IMAGE_INSTALL}"
>>>>    IMAGE_PREINSTALL += "apt \
>>>>                         dbus"
>>>>    
>>>> -WORKDIR = "${TMPDIR}/work/${PN}/${MACHINE}/${DISTRO}"
>>>    
>>>> -S = "${WORKDIR}/rootfs"
>>>> -IMAGE_ROOTFS = "${S}"
>>>
>>> Part of the S -> IMAGE_ROOTFS
>>>    
>>
>> Ok.
>>
>>>> -
>>>> -do_rootfs[stamp-extra-info] = "${MACHINE}-${DISTRO}"
>>>> +do_rootfs[stamp-extra-info] = "${DISTRO}-${MACHINE}"
>>>
>>> This is also a change that should be in a seperate smaller patch.
>>
>> Ok.
>>
>>>       
>>>>    do_rootfs() {
>>>>        install -d -m 755 ${WORKDIR}/hooks_multistrap
>>>> @@ -38,20 +35,20 @@ do_rootfs() {
>>>>        sed -i 's|##DISTRO_APT_SOURCE##|${DISTRO_APT_SOURCE}|'
>>>> ${WORKDIR}/multistrap.conf sed -i
>>>> 's|##DISTRO_SUITE##|${DISTRO_SUITE}|' ${WORKDIR}/multistrap.conf
>>>> sed -i 's|##DISTRO_COMPONENTS##|${DISTRO_COMPONENTS}|'
>>>> ${WORKDIR}/multistrap.conf
>>>> -    sed -i
>>>> 's|##CONFIG_SCRIPT##|./tmp/work/${PN}/${MACHINE}/${DISTRO}/configscript.sh|'
>>>> ${WORKDIR}/multistrap.conf
>>>> -    sed -i
>>>> 's|##SETUP_SCRIPT##|./tmp/work/${PN}/${MACHINE}/${DISTRO}/setup.sh|'
>>>> ${WORKDIR}/multistrap.conf
>>>> -    sed -i
>>>> 's|##DIR_HOOKS##|./tmp/work/${PN}/${MACHINE}/${DISTRO}/hooks_multistrap|'
>>>> ${WORKDIR}/multistrap.conf
>>>> +    sed -i
>>>> 's|##CONFIG_SCRIPT##|./tmp/work/${DISTRO}-${DISTRO_ARCH}/${PF}/configscript.sh|'
>>>> ${WORKDIR}/multistrap.conf
>>>> +    sed -i
>>>> 's|##SETUP_SCRIPT##|./tmp/work/${DISTRO}-${DISTRO_ARCH}/${PF}/setup.sh|'
>>>> ${WORKDIR}/multistrap.conf
>>>> +    sed -i
>>>> 's|##DIR_HOOKS##|./tmp/work/${DISTRO}-${DISTRO_ARCH}/${PF}/hooks_multistrap|'
>>>> ${WORKDIR}/multistrap.conf # Multistrap config use relative paths,
>>>> so ensure that we are in the right folder cd ${TOPDIR}
>>>>    
>>>>        # Create root filesystem
>>>> -    sudo multistrap -a ${DISTRO_ARCH} -d "${S}" -f
>>>> "${WORKDIR}/multistrap.conf" || true
>>>> +    sudo multistrap -a ${DISTRO_ARCH} -d "${IMAGE_ROOTFS}" -f
>>>> "${WORKDIR}/multistrap.conf" || true
>>>>        # Configure root filesystem
>>>> -    sudo chroot ${S} /configscript.sh ${MACHINE_SERIAL}
>>>> ${BAUDRATE_TTY} \
>>>> +    sudo chroot ${IMAGE_ROOTFS} /configscript.sh ${MACHINE_SERIAL}
>>>> ${BAUDRATE_TTY} \ ${ROOTFS_DEV}
>>>> -    sudo rm ${S}/configscript.sh
>>>> +    sudo rm ${IMAGE_ROOTFS}/configscript.sh
>>>>    }
>>>>    
>>>>    addtask rootfs before do_populate
>>>> diff --git a/meta/classes/ext4-img.bbclass
>>>> b/meta/classes/ext4-img.bbclass index 8588626..9af781f 100644
>>>> --- a/meta/classes/ext4-img.bbclass
>>>> +++ b/meta/classes/ext4-img.bbclass
>>>> @@ -6,7 +6,7 @@ ROOTFS_EXTRA ?= "64"
>>>>    
>>>>    EXT4_IMAGE_FILE =
>>>> "${DEPLOY_DIR_IMAGE}/${PN}-${MACHINE}-${DISTRO}.ext4.img"
>>>> -do_ext4_image[stamp-extra-info] = "${MACHINE}-${DISTRO}"
>>>> +do_ext4_image[stamp-extra-info] = "${DISTRO}-${MACHINE}"
>>>>    
>>>>    # Generate ext4 filesystem image
>>>>    do_ext4_image() {
>>>> @@ -14,23 +14,23 @@ do_ext4_image() {
>>>>    
>>>>        rm -f ${EXT4_IMAGE_FILE}
>>>>    
>>>> -    ROOTFS_SIZE=`sudo du -sm ${S} |  awk '{print $1 +
>>>> ${ROOTFS_EXTRA};}'`
>>>> +    ROOTFS_SIZE=`sudo du -sm ${IMAGE_ROOTFS} |  awk '{print $1 +
>>>> ${ROOTFS_EXTRA};}'` dd if=/dev/zero of=${EXT4_IMAGE_FILE} bs=1M
>>>> count=${ROOTFS_SIZE}
>>>>
>>>>        sudo mkfs.ext4 -F ${EXT4_IMAGE_FILE}
>>>>    
>>>>        mkdir -p ${WORKDIR}/mnt
>>>>        sudo mount -o loop ${EXT4_IMAGE_FILE} ${WORKDIR}/mnt
>>>> -    sudo cp -r ${S}/* ${WORKDIR}/mnt
>>>> +    sudo cp -r ${IMAGE_ROOTFS}/* ${WORKDIR}/mnt
>>>>        sudo umount ${WORKDIR}/mnt
>>>>        rm -r ${WORKDIR}/mnt
>>>>    
>>>>        if [ -n "${KERNEL_IMAGE}" ]; then
>>>> -        cp ${S}/boot/${KERNEL_IMAGE} ${DEPLOY_DIR_IMAGE}
>>>> +        cp ${IMAGE_ROOTFS}/boot/${KERNEL_IMAGE}
>>>> ${DEPLOY_DIR_IMAGE} fi
>>>>    
>>>>        if [ -n "${INITRD_IMAGE}" ]; then
>>>> -        cp ${S}/boot/${INITRD_IMAGE} ${DEPLOY_DIR_IMAGE}
>>>> +        cp ${IMAGE_ROOTFS}/boot/${INITRD_IMAGE}
>>>> ${DEPLOY_DIR_IMAGE} fi
>>>>    }
>>>>    
>>>> diff --git a/meta/classes/image.bbclass
>>>> b/meta/classes/image.bbclass index a7f0d74..fe06352 100644
>>>> --- a/meta/classes/image.bbclass
>>>> +++ b/meta/classes/image.bbclass
>>>> @@ -9,20 +9,22 @@ IMAGE_TYPE    ?= "ext4-img"
>>>>    
>>>>    inherit ${IMAGE_TYPE}
>>>>    
>>>> -do_populate[stamp-extra-info] = "${MACHINE}-${DISTRO}"
>>>> +IMAGE_ROOTFS = "${WORKDIR}/rootfs"
>>>> +
>>>> +do_populate[stamp-extra-info] = "${DISTRO}-${MACHINE}"
>>>>    
>>>>    # Install Debian packages, that were built from sources
>>>>    do_populate() {
>>>>        if [ -n "${IMAGE_INSTALL}" ]; then
>>>> -        sudo mkdir -p ${S}/deb
>>>> +        sudo mkdir -p ${IMAGE_ROOTFS}/deb
>>>>    
>>>>            for p in ${IMAGE_INSTALL}; do
>>>> -            sudo cp ${DEPLOY_DIR_DEB}/${p}_*.deb ${S}/deb
>>>> +            sudo cp ${DEPLOY_DIR_DEB}/${p}_*.deb
>>>> ${IMAGE_ROOTFS}/deb done
>>>>    
>>>> -        sudo chroot ${S} /usr/bin/dpkg -i -R /deb
>>>> +        sudo chroot ${IMAGE_ROOTFS} /usr/bin/dpkg -i -R /deb
>>>>    
>>>> -        sudo rm -rf ${S}/deb
>>>> +        sudo rm -rf ${IMAGE_ROOTFS}/deb
>>>>        fi
>>>>    }
>>>>    
>>>> diff --git a/meta/conf/isar-bitbake.conf
>>>> b/meta/conf/isar-bitbake.conf index f85f5cc..5a26743 100644
>>>> --- a/meta/conf/isar-bitbake.conf
>>>> +++ b/meta/conf/isar-bitbake.conf
>>>> @@ -18,9 +18,10 @@
>>>>    # ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
>>>> USE OR # OTHER DEALINGS IN THE SOFTWARE.
>>>>    
>>>> +WORKDIR = "${TMPDIR}/work/${DISTRO}-${DISTRO_ARCH}/${PF}"
>>>>    DEPLOY_DIR_DEB = "${TMPDIR}/deploy/deb/${MACHINE}"
>>>>    SSTATE_DIR ?= "${TMPDIR}/sstate-cache"
>>>> -BUILDCHROOT_DIR =
>>>> "${TOPDIR}/tmp/work/buildchroot/${DISTRO}-${DISTRO_ARCH}/rootfs"
>>>> +BUILDCHROOT_DIR =
>>>> "${TMPDIR}/work/${DISTRO}-${DISTRO_ARCH}/buildchroot/rootfs" #
>>>> Setup our default hash policy BB_SIGNATURE_HANDLER ?= "noop"
>>>> diff --git a/meta/recipes-devtools/buildchroot/buildchroot.bb
>>>> b/meta/recipes-devtools/buildchroot/buildchroot.bb index
>>>> ccba683..155bd3f 100644 ---
>>>> a/meta/recipes-devtools/buildchroot/buildchroot.bb +++
>>>> b/meta/recipes-devtools/buildchroot/buildchroot.bb @@ -9,6 +9,7 @@
>>>> LICENSE = "gpl-2.0" LIC_FILES_CHKSUM =
>>>> "file://${LAYERDIR_isar}/licenses/COPYING.GPLv2;md5=751419260aa954499f7abaabaa882bbe"
>>>>    PV = "1.0"
>>>> +PF = "${PN}"
>>>
>>> I think this is to remove the package version from WORKDIR, i
>>> suggest to do WORKDIR= instead of overwriting such an essential
>>> variable. Writing to PF will i.e. also affect FILESPATH and STAMP.
>>> This line deserves a comment!
>>
>> I'm still thinking about this...
>>
>> My idea is that buildchroot and image recipes don't need version at
>> all, because there is no source code provider, that could track the
>> code revision. The line:
>>
>>    PV = "1.0"
>>
>> is confusing and makes really no sense, I can't imagine by which
>> event it should be updated. Any changes in this recipe should be
>> reflected in whole Isar version update. So the proposal is to drop
>> it, but by dropping it we will get broken ${PF} variable, it will
>> look like: "buildchroot-".
>>
>> So, the desired goal - to have buildchroot and image WORKDIRs without
>> version suffixes:
>>
>> 1. If drop PV, the PF should be updated
>> 2. If do not drop PV, WORKDIR should be updated
>>
>>>    
>>>>    
>>>>    BUILDCHROOT_PREINSTALL ?= "gcc \
>>>>                               make \
>>>> @@ -21,8 +22,6 @@ BUILDCHROOT_PREINSTALL ?= "gcc \
>>>>                               apt \
>>>>                               automake"
>>>>    
>>>> -WORKDIR = "${TMPDIR}/work/${PF}/${DISTRO}"
>>>> -
>>>>    do_build[stamp-extra-info] = "${DISTRO}-${DISTRO_ARCH}"
>>>>    
>>>>    do_build() {
>>>> @@ -40,9 +39,9 @@ do_build() {
>>>>        sed -i 's|##DISTRO_APT_SOURCE##|${DISTRO_APT_SOURCE}|'
>>>> ${WORKDIR}/multistrap.conf sed -i
>>>> 's|##DISTRO_SUITE##|${DISTRO_SUITE}|' ${WORKDIR}/multistrap.conf
>>>> sed -i 's|##DISTRO_COMPONENTS##|${DISTRO_COMPONENTS}|'
>>>> ${WORKDIR}/multistrap.conf
>>>> -    sed -i
>>>> 's|##CONFIG_SCRIPT##|./tmp/work/${PF}/${DISTRO}/configscript.sh|'
>>>> ${WORKDIR}/multistrap.conf
>>>> -    sed -i
>>>> 's|##SETUP_SCRIPT##|./tmp/work/${PF}/${DISTRO}/setup.sh|'
>>>> ${WORKDIR}/multistrap.conf
>>>> -    sed -i
>>>> 's|##DIR_HOOKS##|./tmp/work/${PF}/${DISTRO}/hooks_multistrap|'
>>>> ${WORKDIR}/multistrap.conf
>>>> +    sed -i
>>>> 's|##CONFIG_SCRIPT##|./tmp/work/${DISTRO}-${DISTRO_ARCH}/${PF}/configscript.sh|'
>>>> ${WORKDIR}/multistrap.conf
>>>> +    sed -i
>>>> 's|##SETUP_SCRIPT##|./tmp/work/${DISTRO}-${DISTRO_ARCH}/${PF}/setup.sh|'
>>>> ${WORKDIR}/multistrap.conf
>>>> +    sed -i
>>>> 's|##DIR_HOOKS##|./tmp/work/${DISTRO}-${DISTRO_ARCH}/${PF}/hooks_multistrap|'
>>>> ${WORKDIR}/multistrap.conf
>>>
>>> The pattern tmp/work/${DISTRO}-${DISTRO_ARCH}/${PF}/ shows up all
>>> the time, can that be derived from $WORKDIR instead of writing it
>>> down again and again?
>>
>> That's mad, but mulstistrap understands relative paths only. Any
>> attempt to replace it by absolute path lead to errors. That's why it
>> was done in this way, because WORKDIR is absolute path. :-(
> 
> Ok. Could this not be fixed with something like the following in
> isar-bitbake.conf:
> 
> WORKDIR_SUFFIX=${DISTRO}-${DISTRO_ARCH}/${PF}
> WORKDIR=${WORKDIR}/${WORKDIR_SUFFIX}

Does this line correct? After concatenation there will be line:

   ${TMPDIR}/work/${PF}/${DISTRO}-${DISTRO_ARCH}/${PF}

I assume it's not the one you want...

Due to we need eventually whole path:

   ./tmp/work/${DISTRO}-${DISTRO_ARCH}/${PF}

we could a bit change your proposal:

WORKDIR_RELATIVE=tmp/work/${DISTRO}-${DISTRO_ARCH}/${PF}
WORKDIR=${TOPDIR}/${WORKDIR_RELATIVE}

so the pattern for multistrap will be: ./${WORKDIR_RELATIVE}

what do you think?

Alex

> and now you can use WORKDIR_SUFFIX in all these lines. Such a change
> should probably be its own patch.
> 
> Henning
> 
>> Alex
>>
>>>    
>>>> # Multistrap config use relative paths, so
>>>> ensure that we are in the right folder cd ${TOPDIR}
>>>    
> 

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