Re: [PATCH 2/2] isar-bootstrap: exit if command fails under flock

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

Am Sun, 12 Aug 2018 11:46:30 +0300
schrieb "Maxim Yu. Osipov" <mosipov@ilbers.de>:

> Hi Jan,
> 
> On 08/11/2018 06:52 PM, Jan Kiszka wrote:
> > On 2018-08-10 12:31, Maxim Yu. Osipov wrote:  
> >> If command under 'flock' fails, isar_bootstrap continues to
> >> execute. This is wrong behaviour, which leads that the
> >> do_bootstrap task is considered successfully done.
> >>
> >> Signed-off-by: Maxim Yu. Osipov <mosipov@ilbers.de>
> >> ---
> >>   .../recipes-core/isar-bootstrap/isar-bootstrap.inc | 28 
> >> +++++++++++-----------
> >>   1 file changed, 14 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc 
> >> b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
> >> index 5c90713..60e5d10 100644
> >> --- a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
> >> +++ b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
> >> @@ -187,7 +187,7 @@ isar_bootstrap() {
> >>                                            ${DEBOOTSTRAP_KEYRING} \
> >>                                            "${@get_distro_suite(d, 
> >> True)}" \
> >>                                            "${ROOTFSDIR}" \
> >> -                                         "${@get_distro_source(d, 
> >> True)}"
> >> +                                         "${@get_distro_source(d, 
> >> True)}" || exit 1
> >>               else
> >>                    sudo -E "${DEBOOTSTRAP}" --verbose \
> >> @@ -198,45 +198,45 @@ isar_bootstrap() {
> >>                                             ${DEBOOTSTRAP_KEYRING}
> >> \ "${@get_distro_suite(d, 
> >> False)}" \
> >>                                            "${ROOTFSDIR}" \
> >> -                                         "${@get_distro_source(d, 
> >> False)}"
> >> +                                         "${@get_distro_source(d, 
> >> False)}" || exit 1
> >>               fi
> >>               # Install apt config
> >>               sudo mkdir -p "${ROOTFSDIR}/etc/apt/preferences.d"
> >>               sudo install -v -m644 "${APTPREFS}" \
> >> -                                  
> >> "${ROOTFSDIR}/etc/apt/preferences.d/bootstrap"
> >> +                                  
> >> "${ROOTFSDIR}/etc/apt/preferences.d/bootstrap" || exit 1
> >>               sudo mkdir -p "${ROOTFSDIR}/etc/apt/sources.list.d"
> >>               sudo install -v -m644 "${APTSRCS}" \
> >> -                                  
> >> "${ROOTFSDIR}/etc/apt/sources.list.d/bootstrap.list"
> >> +                                  
> >> "${ROOTFSDIR}/etc/apt/sources.list.d/bootstrap.list" || exit 1
> >>               sudo rm -f "${ROOTFSDIR}/etc/apt/sources.list"
> >>               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"
> >> +                                  
> >> "${ROOTFSDIR}/etc/apt/apt.conf.d/50isar.conf" || exit 1
> >>               if [ "${@get_distro_suite(d, True)}" = "stretch" ]
> >> && [ "${@get_host_release().split('.')[0]}" -lt "4" ]; then
> >>                   sudo install -v -m644 
> >> "${WORKDIR}/isar-apt-fallback.conf" \
> >> -                                      
> >> "${ROOTFSDIR}/etc/apt/apt.conf.d/55isar-fallback.conf"
> >> +                                      
> >> "${ROOTFSDIR}/etc/apt/apt.conf.d/55isar-fallback.conf" || exit 1
> >>               fi
> >>               # Set locale
> >> -            sudo install -v -m644 "${WORKDIR}/locale" 
> >> "${ROOTFSDIR}/etc/locale"
> >> +            sudo install -v -m644 "${WORKDIR}/locale" 
> >> "${ROOTFSDIR}/etc/locale" || exit 1
> >>               sudo sed -i '/en_US.UTF-8 UTF-8/s/^#//g' 
> >> "${ROOTFSDIR}/etc/locale.gen"
> >> -            sudo -E chroot "${ROOTFSDIR}" /usr/sbin/locale-gen
> >> +            sudo -E chroot "${ROOTFSDIR}" /usr/sbin/locale-gen ||
> >> exit 1 # setup chroot
> >> -            sudo install -v -m755 "${WORKDIR}/chroot-setup.sh" 
> >> "${ROOTFSDIR}/chroot-setup.sh"
> >> -            sudo "${ROOTFSDIR}/chroot-setup.sh" "setup"
> >> "${ROOTFSDIR}"
> >> +            sudo install -v -m755 "${WORKDIR}/chroot-setup.sh" 
> >> "${ROOTFSDIR}/chroot-setup.sh" || exit 1
> >> +            sudo "${ROOTFSDIR}/chroot-setup.sh" "setup" 
> >> "${ROOTFSDIR}" || exit 1
> >>               # update APT
> >> -            sudo mount -t devtmpfs -o mode=0755,nosuid devtmpfs 
> >> ${ROOTFSDIR}/dev
> >> -            sudo mount -t proc none ${ROOTFSDIR}/proc
> >> +            sudo mount -t devtmpfs -o mode=0755,nosuid devtmpfs 
> >> ${ROOTFSDIR}/dev || exit 1
> >> +            sudo mount -t proc none ${ROOTFSDIR}/proc || exit 1
> >>               E="${@bb.utils.export_proxies(d)}"
> >>               export DEBIAN_FRONTEND=noninteractive
> >> -            sudo -E chroot "${ROOTFSDIR}" /usr/bin/apt-get update
> >> -y
> >> +            sudo -E chroot "${ROOTFSDIR}" /usr/bin/apt-get update
> >> -y || exit 1
> >>               sudo -E chroot "${ROOTFSDIR}" /usr/bin/apt-get 
> >> dist-upgrade -y \
> >> -                                      -o
> >> Debug::pkgProblemResolver=yes
> >> +                                      -o 
> >> Debug::pkgProblemResolver=yes || exit 1
> >>               # Finalize debootstrap by setting the link in deploy
> >>               ln -Tfsr "${ROOTFSDIR}" "${DEPLOY_ISAR_BOOTSTRAP}"
> >>  
> > 
> > This is a bit ugly and leaves out a number of commands. Why can't
> > we do "set -e" inside that section?  
> 
> Actually, 'set -e' is put automatically in corresponding 
> run.do_bootstrap script but it doesn't work for flock case.

The problem is that all these steps are "the command" for flock. That
is pretty ugly to begin with. If one needs to do that, all those
commands should be chained with &&.
What would be nicer would be a bitbake lock for isar_bootstrap or the
two "do_bootstrap" calls and get rid of the flock.

Your patch fails to deal with all the errors the other commands could
trigger. And i wonder why we need sudo inside "sudo flock". (commit
ab0a1c8c)

Henning

> Maxim.
>