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

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

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

PV is a basic bitbake variable and i think it should not be dropped for
images. In fact you could imagine people having a v2 or v3 of their
images in their layers.
A second version of the image could have another meaning than "the way
we build has changed" it could also be "the content/configuration has
changed".

i.e.:

my-image_2.0.bb:
require recipes-core/images/isar-image-base.bb
IMAGE_INSTALL += "example-custom"

my_image_3.0.bb:
require recipes-core/images/isar-image-base.bb
IMAGE_INSTALL += "example-custom hello nano parted"
ROOTFS_EXTRA = "2048"

Henning

> 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. :-(
> 
> Alex
> 
> >   
> >> # Multistrap config use relative paths, so
> >> ensure that we are in the right folder cd ${TOPDIR}  
> >   



-- 
Siemens AG
Corporate Technology
CT RDA ITP SES-DE
Otto-Hahn-Ring 6
81739 Muenchen, Germany
Mobile: +49 172 8378927
mailto: henning.schild@siemens.com