Re: [PATCH 2/2] integrate ubifs image type

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

Am Mon, 28 Jan 2019 14:40:36 +0100
schrieb Claudius Heine <claudius.heine.ext@siemens.com>:

> Hi Henning,
> 
> On 28/01/2019 14.03, Henning Schild wrote:
> > Am Mon, 28 Jan 2019 13:28:21 +0100
> > schrieb "[ext] claudius.heine.ext@siemens.com"
> > <claudius.heine.ext@siemens.com>:
> >   
> >> From: Claudius Heine <ch@denx.de>
> >>
> >> Signed-off-by: Claudius Heine <ch@denx.de>
> >> ---
> >>   doc/user_manual.md                            |  1 +
> >>   meta-isar/conf/local.conf.sample              |  1 +
> >>   .../multiconfig/qemuamd64-buster-ubifs.conf   | 16 +++++++
> >>   meta/classes/ubifs-img.bbclass                | 42
> >> +++++++++++++++++++ scripts/ci_build.sh                           |
> >> 1 + 5 files changed, 61 insertions(+)
> >>   create mode 100644
> >> meta-isar/conf/multiconfig/qemuamd64-buster-ubifs.conf create mode
> >> 100644 meta/classes/ubifs-img.bbclass
> >>
> >> diff --git a/doc/user_manual.md b/doc/user_manual.md
> >> index c4fe42a..c9924ad 100644
> >> --- a/doc/user_manual.md
> >> +++ b/doc/user_manual.md
> >> @@ -476,6 +476,7 @@ Isar contains additional image type classes
> >> that can be used as reference:
> >>    - `ext4-img`
> >>    - `rpi-sdimg`
> >>    - `targz-img`
> >> + - `ubifs-img`
> >>    - `wic-img`
> >>   
> >>   ---
> >> diff --git a/meta-isar/conf/local.conf.sample
> >> b/meta-isar/conf/local.conf.sample index a671b20..9ea366c 100644
> >> --- a/meta-isar/conf/local.conf.sample
> >> +++ b/meta-isar/conf/local.conf.sample
> >> @@ -53,6 +53,7 @@ BBMULTICONFIG = " \
> >>       hikey-stretch \
> >>       qemuamd64-buster \
> >>       qemuamd64-buster-tgz \
> >> +    qemuamd64-buster-ubifs \
> >>       rpi-jessie \
> >>   "
> >>   
> >> diff --git a/meta-isar/conf/multiconfig/qemuamd64-buster-ubifs.conf
> >> b/meta-isar/conf/multiconfig/qemuamd64-buster-ubifs.conf new file
> >> mode 100644 index 0000000..7c638b9
> >> --- /dev/null
> >> +++ b/meta-isar/conf/multiconfig/qemuamd64-buster-ubifs.conf
> >> @@ -0,0 +1,16 @@
> >> +# This software is a part of ISAR.
> >> +# Copyright (c) Siemens AG, 2018
> >> +#
> >> +# SPDX-License-Identifier: MIT
> >> +
> >> +MACHINE ?= "qemuamd64"
> >> +
> >> +DISTRO ?= "debian-buster"
> >> +DISTRO_ARCH ?= "amd64"
> >> +
> >> +KERNEL_NAME ?= "amd64"
> >> +
> >> +MKUBIFS_ARGS ?= "-m 0x1000 -e 0x3e000 -c 1500"
> >> +IMAGE_TYPE ?= "ubifs-img"
> >> +
> >> +IMAGE_INSTALL += "sshd-regen-keys"
> >> diff --git a/meta/classes/ubifs-img.bbclass
> >> b/meta/classes/ubifs-img.bbclass new file mode 100644
> >> index 0000000..f5e17d3
> >> --- /dev/null
> >> +++ b/meta/classes/ubifs-img.bbclass
> >> @@ -0,0 +1,42 @@
> >> +# This software is a part of ISAR.
> >> +# Copyright (c) Siemens AG, 2018
> >> +
> >> +python() {
> >> +    if not d.getVar("MKUBIFS_ARGS"):
> >> +        bb.fatal("MKUBIFS_ARGS must be set")
> >> +}
> >> +
> >> +inherit image  
> > 
> > I dislike all this variable setting and additional mounting.
> >   
> >> +UBIFS_IMAGE_FILE ?= "${IMAGE_FULLNAME}.ubifs.img"
> >> +
> >> +IMAGER_INSTALL += "mtd-utils"
> >> +
> >> +PP = "/home/builder/${PN}"  
> > 
> > That is already defined in another file in Isar. We should rather
> > find a common location and refactor.
> >   
> >> +PP_DEPLOY = "${PP}/deploy"
> >> +PP_ROOTFS = "${PP}/rootfs"  
> > 
> > Similar comments might apply here.  
> 
> Do you think that refactoring should be part of this patchset?

I am not sure, just wanted to write it down. On the one hand it would
be nice to solve all drive-by issues as we go. On the other hand that
will slow down development and impose on contributors.
So feel free to ignore my comments, the inconsistencies are not your
fault and asking you to fix them would be too much.

> >> +BUILDROOT = "${BUILDCHROOT_DIR}${PP}"
> >> +BUILDROOT_DEPLOY = "${BUILDCHROOT_DIR}${PP_DEPLOY}"
> >> +BUILDROOT_ROOTFS = "${BUILDCHROOT_DIR}${PP_ROOTFS}"  
> > 
> > Same here, and "buildroot" to me is something else ;).  
>
> That is part of my plot to slowly merge bitbake, isar and buildroot ;)

Good first step. We actually have buildroot isar recipes in
jailhouse-images, so this name can potentially get confusing for real.

> >   
> >> +do_ubifs_image[stamp-extra-info] = "${DISTRO}-${MACHINE}"
> >> +
> >> +# Generate ubifs filesystem image
> >> +do_ubifs_image() {
> >> +    rm -f ${DEPLOY_DIR_IMAGE}/${UBIFS_IMAGE_FILE}
> >> +
> >> +    buildchroot_do_mounts
> >> +
> >> +    sudo flock ${MOUNT_LOCKFILE} -c ' \
> >> +        mkdir -p ${BUILDROOT_DEPLOY} ${BUILDROOT_ROOTFS}
> >> +        mount --bind ${DEPLOY_DIR_IMAGE} ${BUILDROOT_DEPLOY}
> >> +        mount --bind ${IMAGE_ROOTFS} ${BUILDROOT_ROOTFS}
> >> +    '  
> > 
> > I think this mounting should also be required by a proper ext4
> > class. On the other hand the wic one moves the image into the final
> > deploy folder.
> > Not sure what is best but maybe a good idea to go for "always move"
> > or "always mount".  
> 
> Hmm.. In OE this task would not to touch the deploy directory and
> *copy* all artifacts in a dedicated task AFAIK. But OE has a
> completely different image creation pipeline and infrastructure than
> isar, so who knows how things should be done here.

We currently generate and deploy in one task. The fun thing is that we
need multiple commands and can probably end up in weird states if we
get interrupted or fail.

Changing that general pattern is a discussion of its own, since it
would change the API.

Staying in the current API i think the mount and generate directly into
deploy ... no more chmod/chown mv ... whatever. Would be the cleanest
way. We already have a reliable umount mechanism to clean up if things
go wrong.

If you agree: Please include that deploydir/rootfs mount into the common
mounts and update your imager. I will do the same for wic and maybe
ext4.

> I am open to discussion about implementing some rules and
> documentation about how things should be done in isar.

I assume that consistent references are equally efficient as
documentation, maybe more.

Henning

> Claudius
> 
> > 
> > Henning
> >   
> >> +    # Create ubifs image using buildchroot tools
> >> +    sudo chroot ${BUILDCHROOT_DIR} /usr/sbin/mkfs.ubifs
> >> ${MKUBIFS_ARGS} \
> >> +                -r "${PP_ROOTFS}"
> >> "${PP_DEPLOY}/${UBIFS_IMAGE_FILE}" +}
> >> +
> >> +addtask ubifs_image before do_build after do_copy_boot_files
> >> do_install_imager_deps diff --git a/scripts/ci_build.sh
> >> b/scripts/ci_build.sh index f3523e8..dcde0b4 100755
> >> --- a/scripts/ci_build.sh
> >> +++ b/scripts/ci_build.sh
> >> @@ -115,6 +115,7 @@ else
> >>           multiconfig:qemuamd64-stretch:isar-image-base \
> >>           multiconfig:qemuamd64-buster:isar-image-base \
> >>           multiconfig:qemuamd64-buster-tgz:isar-image-base \
> >> +        multiconfig:qemuamd64-buster-ubifs:isar-image-base \
> >>           multiconfig:rpi-jessie:isar-image-base
> >>       # qemu-user-static of <= buster too old to build that
> >>       #multiconfig:qemuarm64-buster:isar-image-base  
> >   
>