Re: [PATCH 3/5] Set up target image fstab prior to installing any packages

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

Hi,

On 2018-06-06 13:58, [ext] Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> Package postinst hooks may need the fstab or may want to modify it,
> including customization packages or the upcoming u-boot-script
> generator. So extract the already identical setup pattern from the
> distro config scripts, generate the fstab prior to calling
> setup_root_file_system and let the latter inject the file before
> starting the package installation. By specifying "-" as fstab file, we
> can skip the injection for the buildchroot which does not need it.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>   .../recipes-core/images/files/debian-configscript.sh   | 14 --------------
>   .../recipes-core/images/files/raspbian-configscript.sh | 14 --------------
>   meta-isar/recipes-core/images/isar-image-base.bb       | 18 ++++++++++++++----
>   meta/classes/isar-bootstrap-helper.bbclass             |  5 +++--
>   meta/recipes-devtools/buildchroot/buildchroot.bb       |  2 +-
>   5 files changed, 18 insertions(+), 35 deletions(-)
> 
> diff --git a/meta-isar/recipes-core/images/files/debian-configscript.sh b/meta-isar/recipes-core/images/files/debian-configscript.sh
> index 136918c..bcd5507 100755
> --- a/meta-isar/recipes-core/images/files/debian-configscript.sh
> +++ b/meta-isar/recipes-core/images/files/debian-configscript.sh
> @@ -7,20 +7,6 @@ set -e
>   
>   readonly MACHINE_SERIAL="$1"
>   readonly BAUDRATE_TTY="$2"
> -readonly ROOTFS_DEV="$3"
> -readonly ROOTFS_TYPE="$4"
> -
> -cat > /etc/fstab << EOF
> -# Begin /etc/fstab
> -/dev/$ROOTFS_DEV	/		$ROOTFS_TYPE		defaults		1	1
> -proc		/proc		proc		nosuid,noexec,nodev	0	0
> -sysfs		/sys		sysfs		nosuid,noexec,nodev	0	0
> -devpts		/dev/pts	devpts		gid=5,mode=620		0	0
> -tmpfs		/run		tmpfs		defaults		0	0
> -devtmpfs	/dev		devtmpfs	mode=0755,nosuid	0	0
> -
> -# End /etc/fstab
> -EOF
>   
>   # Create console device
>   if [ ! -e /dev/console ]; then
> diff --git a/meta-isar/recipes-core/images/files/raspbian-configscript.sh b/meta-isar/recipes-core/images/files/raspbian-configscript.sh
> index 04ea571..3631c26 100644
> --- a/meta-isar/recipes-core/images/files/raspbian-configscript.sh
> +++ b/meta-isar/recipes-core/images/files/raspbian-configscript.sh
> @@ -7,20 +7,6 @@ set -e
>   
>   readonly MACHINE_SERIAL="$1"
>   readonly BAUDRATE_TTY="$2"
> -readonly ROOTFS_DEV="$3"
> -readonly ROOTFS_TYPE="$4"
> -
> -cat > /etc/fstab << EOF
> -# Begin /etc/fstab
> -/dev/$ROOTFS_DEV	/		$ROOTFS_TYPE		defaults		1	1
> -proc		/proc		proc		nosuid,noexec,nodev	0	0
> -sysfs		/sys		sysfs		nosuid,noexec,nodev	0	0
> -devpts		/dev/pts	devpts		gid=5,mode=620		0	0
> -tmpfs		/run		tmpfs		defaults		0	0
> -devtmpfs	/dev		devtmpfs	mode=0755,nosuid	0	0
> -
> -# End /etc/fstab
> -EOF
>   
>   # Enable tty conditionally, systemd does not have the file but its own magic
>   if [ -f /etc/inittab ]; then
> diff --git a/meta-isar/recipes-core/images/isar-image-base.bb b/meta-isar/recipes-core/images/isar-image-base.bb
> index f91dfb0..bf90e30 100644
> --- a/meta-isar/recipes-core/images/isar-image-base.bb
> +++ b/meta-isar/recipes-core/images/isar-image-base.bb
> @@ -28,15 +28,25 @@ do_rootfs[root_cleandirs] = "${IMAGE_ROOTFS} \
>                                ${IMAGE_ROOTFS}/isar-apt"
>   
>   do_rootfs() {
> -    setup_root_file_system "${IMAGE_ROOTFS}" "clean" \
> +    cat > ${WORKDIR}/fstab << EOF
> +# Begin /etc/fstab
> +/dev/${ROOTFS_DEV}	/		${ROOTFS_TYPE}		defaults		1	1
> +proc		/proc		proc		nosuid,noexec,nodev	0	0
> +sysfs		/sys		sysfs		nosuid,noexec,nodev	0	0
> +devpts		/dev/pts	devpts		gid=5,mode=620		0	0
> +tmpfs		/run		tmpfs		defaults		0	0
> +devtmpfs	/dev		devtmpfs	mode=0755,nosuid	0	0
> +
> +# End /etc/fstab
> +EOF
> +
> +    setup_root_file_system "${IMAGE_ROOTFS}" "clean" "${WORKDIR}/fstab" \
>           ${IMAGE_PREINSTALL} ${IMAGE_INSTALL}
>   
>       # Configure root filesystem
>       sudo install -m 755 "${WORKDIR}/${DISTRO_CONFIG_SCRIPT}" "${IMAGE_ROOTFS}"
>       sudo chroot ${IMAGE_ROOTFS} /${DISTRO_CONFIG_SCRIPT} ${MACHINE_SERIAL} \
> -                                                         ${BAUDRATE_TTY} \
> -                                                         ${ROOTFS_DEV} \
> -                                                         ${ROOTFS_TYPE}
> +                                                         ${BAUDRATE_TTY}
>   
>       # Cleanup
>       sudo rm "${IMAGE_ROOTFS}/${DISTRO_CONFIG_SCRIPT}"
> diff --git a/meta/classes/isar-bootstrap-helper.bbclass b/meta/classes/isar-bootstrap-helper.bbclass
> index a3f1686..5d4dca0 100644
> --- a/meta/classes/isar-bootstrap-helper.bbclass
> +++ b/meta/classes/isar-bootstrap-helper.bbclass
> @@ -17,8 +17,8 @@ def reverse_bb_array(d, varname):
>   setup_root_file_system() {
>       ROOTFSDIR="$1"
>       CLEAN="$2"
> -    shift
> -    shift
> +    FSTAB="$3"
> +    shift 3

If that starts getting this complex with the second optional parameter, 
maybe we should look into using getopt or just the normal shell 'case' 
loop over the options. I would prefer that instead of introducing more 
and more '-' positional parameter.

With 'clean' and 'noclean' we could sort of get away with it because we 
always specify 'noclean' in those places instead of just '-'. It wasn't 
ideal, but since it was the only one it was sort of ok. Now since we are 
expanding this we should really switch to something easier to use and 
understand.

Claudius

>       PACKAGES="$@"
>       APT_ARGS="install --yes -o Debug::pkgProblemResolver=yes"
>       CLEAN_FILES="${ROOTFSDIR}/etc/hostname ${ROOTFSDIR}/etc/resolv.conf"
> @@ -26,6 +26,7 @@ setup_root_file_system() {
>       sudo cp -Trpfx \
>           "${DEPLOY_DIR_IMAGE}/isar-bootstrap-${DISTRO}-${DISTRO_ARCH}/" \
>           "$ROOTFSDIR"
> +    [ "${FSTAB}" != "-" ] && cat ${FSTAB} | sudo tee "$ROOTFSDIR/etc/fstab"
>   
>       echo "deb [trusted=yes] file:///isar-apt ${DEBDISTRONAME} main" | \
>           sudo tee "$ROOTFSDIR/etc/apt/sources.list.d/isar-apt.list" >/dev/null
> diff --git a/meta/recipes-devtools/buildchroot/buildchroot.bb b/meta/recipes-devtools/buildchroot/buildchroot.bb
> index e405509..10142b7 100644
> --- a/meta/recipes-devtools/buildchroot/buildchroot.bb
> +++ b/meta/recipes-devtools/buildchroot/buildchroot.bb
> @@ -69,7 +69,7 @@ do_build[root_cleandirs] = "${BUILDCHROOT_DIR} \
>   do_build[depends] = "isar-apt:do_cache_config isar-bootstrap:do_deploy"
>   
>   do_build() {
> -    setup_root_file_system "${BUILDCHROOT_DIR}" "noclean" \
> +    setup_root_file_system "${BUILDCHROOT_DIR}" "noclean" "-" \
>           ${BUILDCHROOT_PREINSTALL}
>   
>       # Install package builder script
> 

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