Re: [Buildroot] [PATCH next v5 1/3] package/dracut: new host package

["Yann E. MORIN" <yann.morin.1998@free.fr>]

Arnout, All,

On 2022-08-15 13:17 +0200, Arnout Vandecappelle (Essensium/Mind) spake thusly:
> From: Thierry Bultel <thierry.bultel@linatsea.fr>
> Dracut is the tool used by desktop distributions to build initrds.
[--SNIP--]
> diff --git a/package/dracut/0001-dracut.sh-don-t-unset-LD_PRELOAD.patch b/package/dracut/0001-dracut.sh-don-t-unset-LD_PRELOAD.patch
> new file mode 100644
> index 0000000000..87083d2cef
> --- /dev/null
> +++ b/package/dracut/0001-dracut.sh-don-t-unset-LD_PRELOAD.patch
> @@ -0,0 +1,29 @@
> +From bb12f15856911d8532b569116da7dab4cbf107be Mon Sep 17 00:00:00 2001
> +From: Thierry Bultel <thierry.bultel@linatsea.fr>
> +Date: Mon, 10 Jan 2022 09:09:43 +0100
> +Subject: [PATCH] dracut.sh: don't unset LD_PRELOAD
> +
> +LD_PRELOAD is needed to run under fakeroot.

We also need LD_LIBRARY_PATH to actually find our lifakeroot.so (as
discussed on IRC).

[--SNIP--]
> diff --git a/package/dracut/busybox-init-module-setup.sh b/package/dracut/busybox-init-module-setup.sh
> new file mode 100644
> index 0000000000..2667f866d1
> --- /dev/null
> +++ b/package/dracut/busybox-init-module-setup.sh
> @@ -0,0 +1,62 @@
> +#!/bin/bash
> +
> +check() {
> +	require_binaries busybox || return 1
> +	return 0

return 0 is not needed: a shell function returns with the exit code from
the last command it ran. In this case, the previous command succeeded,
or we'd have already returned with 1, so the return code would be 0.

Still, we can keep it if that makes it consistent with the rest of the
dracut modules.

> +}
> +
> +depends() {
> +	return 0

Here, a colon should be enough:  depends() { :; }
Yet, consistency? ;-)

> +}
> +
> +install_busybox_links() {
> +	dir=$1
> +	linkname=$2
> +
> +	(cd "${dracutsysrootdir?}${dir}" &&
> +	for x in *; do
> +		if [ "$(readlink "$x")" = "${linkname}" ]; then

Always expand between curly braces, even single-char variables.

> +			ln -sf "${linkname}" "${initdir?}/${dir}/$x"

Ditto.

> +		fi
> +	done
> +	)
> +}
> +
> +install() {
> +	inst_multiple /bin/busybox
> +
> +	# wrapper script for early console; will launch /sbin/init
> +	# after having mounted devtmpfs
> +	inst_multiple /init
> +
> +	ln -s ../bin/busybox "${initdir?}"/sbin/init

Isn't that already covered by the second install_busybox_links call,
below?

> +	if [ -e "${dracutsysrootdir?}/lib64" ]; then
> +		ln -sf lib "${initdir?}/lib64"
> +		ln -sf lib "${initdir?}/usr/lib64"
> +	fi
> +
> +	if [ -e "${dracutsysrootdir?}/lib32" ]; then
> +		ln -sf lib "${initdir?}/lib32"
> +		ln -sf lib "${initdir?}/usr/lib32"
> +	fi
> +
> +	install_busybox_links "/bin" "busybox"
> +	install_busybox_links "/sbin" "../bin/busybox"

This one should catch the /sbin/init -> /bin/busybox symlink, no?

> +	if [ ! -L /bin ]; then
> +		install_busybox_links "/usr/bin" "../../bin/busybox"
> +		install_busybox_links "/usr/sbin" "../../bin/busybox"
> +	fi
> +
> +	inst_multiple \
> +		/etc/inittab	\
> +		/etc/init.d/rcS \
> +		/etc/init.d/rcK \
> +		/etc/issue	\
> +		/etc/fstab	\
> +		/etc/group	\
> +		/etc/passwd	\
> +		/etc/shadow	\
> +		/etc/hostname
> +}
> +

Spurious empty line at EOF.

> diff --git a/package/dracut/dracut.mk b/package/dracut/dracut.mk
> new file mode 100644
> index 0000000000..7afec80c0e
> --- /dev/null
> +++ b/package/dracut/dracut.mk
> @@ -0,0 +1,47 @@
> +################################################################################
> +#
> +# dracut
> +#
> +################################################################################
> +
> +DRACUT_VERSION = 055

056 is tarball-released, and 057 is github-tagged now. We can bump in a
later commit, of course.

> +DRACUT_SOURCE = dracut-$(DRACUT_VERSION).tar.xz
> +DRACUT_SITE = $(BR2_KERNEL_MIRROR)/linux/utils/boot/dracut

So, what is the canonical source? Kernel.org has 056, but not 057. Their
README.md hints that github is the official source:

    Currently dracut is developed on [github.com](https://github.com/dracutdevs/dracut).

    The release tarballs are [here](https://github.com/dracutdevs/dracut/releases).

> +DRACUT_LICENSE = GPL-2.0
> +DRACUT_LICENSE_FILES = COPYING
> +
> +HOST_DRACUT_DEPENDENCIES = host-pkgconf host-kmod host-prelink-cross
> +
> +define HOST_DRACUT_POST_INSTALL_WRAPPER_SCRIPT
> +	mv $(HOST_DIR)/bin/dracut $(HOST_DIR)/bin/dracut.real
> +	install -D -m 0755 $(HOST_DRACUT_PKGDIR)/dracut_wrapper.sh $(HOST_DIR)/bin/dracut

There is no reason to add the '.sh' suffix to any executable shell
script, since what matters is the shebang line, especially since it
is eventually installed as a non-suffixed name.

> +endef
> +HOST_DRACUT_POST_INSTALL_HOOKS += HOST_DRACUT_POST_INSTALL_WRAPPER_SCRIPT
> +
> +# When using uClibc or musl, there must be "ls-uClibc.so.1" or
                                              ,^^
s/ls/ld/ ------------------------------------'

[--SNIP--]
> diff --git a/package/dracut/dracut_wrapper.sh b/package/dracut/dracut_wrapper.sh
> new file mode 100644
> index 0000000000..3f58b0907e
> --- /dev/null
> +++ b/package/dracut/dracut_wrapper.sh
> @@ -0,0 +1,33 @@
> +#!/bin/bash
> +set -e
> +
> +# Find the --sysroot argument
> +sysroot=
> +next_arg=
> +for arg; do
> +	if [ "$next_arg" = 1 ]; then

For such a situation, there is a construct that I started to like and
use, is to use 'true' and 'false' instead of markers in variables, so
that we can actually test the variable directly:

    sysroot
    next_arg=false
    for arg; do
        if ${next_arg}; then
            next_arg=false
            sysroot="${arg}"
            continue  # not break, in case there are more than one
        fi
        case "${arg}" in
          (--sysroot)
            next_arg=true
            continue
            ;;
          (--sysroot=*)
            sysroot="${arg#*=}"
            continue  # not break, in case there are more than one
            ;;
        esac
    done

> +		next_arg=
> +		sysroot="$arg"
> +		continue
> +	fi
> +
> +	case "$arg" in
> +	--sysroot=*)
> +		sysroot="${arg#*=}"
> +		;;
> +	--sysroot)
> +		next_arg=1
> +		;;
> +	esac
> +done
> +if [ -z "$sysroot" ]; then

Curly-braces for expansion.

> +	echo "$0: --sysroot argument must be given." 1>&2
> +	exit 1
> +fi
> +
> +topdir="$(dirname "$(realpath "$(dirname "$0")")")"

You can avoid a call to dirname, with just: ${0%/*}

Also, always expand variables with curly braces, even positional
arguments.

> +export DRACUT_LDD="$topdir/sbin/prelink-rtld --root='${sysroot}'"
                       ^^^^^^
Always expand variables between curly braces, especially since there is
already such an expansion on the same line (also valid below).

> +export DRACUT_INSTALL="$topdir/lib/dracut/dracut-install"
> +export DRACUT_LDCONFIG=/bin/true
> +export dracutbasedir="$topdir/lib/dracut"
> +exec "$topdir/bin/dracut.real" "$@"
> diff --git a/package/dracut/libc-links-module-setup.sh b/package/dracut/libc-links-module-setup.sh
> new file mode 100755
> index 0000000000..e15b216e6e
> --- /dev/null
> +++ b/package/dracut/libc-links-module-setup.sh
> @@ -0,0 +1,27 @@
> +#!/bin/bash
> +
> +# Adds the missing links for uClibc or musl, if needed
> +
> +check() {
> +	return 0
> +}
> +
> +depends() {
> +	return 0
> +}
> +
> +install() {
> +	# Despite of the fact that the listed dependency (reported by readelf -d)
> +	# is purely /lib/libc.so, the musl symlink is needed anyway.
> +	musl_link="$(find "${dracutsysrootdir?}/lib" -name "ld-musl-*.so*")"
> +	if [ -n "$musl_link" ] ; then
> +		ln -s libc.so "${initdir?}/lib/$(basename "${musl_link}")"

Besides the usual curly-braces expansion comment, you don't need to use
basename here: ${musl_link##*/}

All of those are minor and can be fixed when applying, but there are a
few where a reply/confirmation would still be welcome.

> +	fi
> +
> +	# Same for uClibc, the listed dependency
> +	# is ld-uClibc.so.1, the loader needs the ld-uClibc.so.0, too
> +	uclibc_link="$(find "${dracutsysrootdir?}/lib" -name "ld-uClibc-*.so*")"
> +	if [ -n "$uclibc_link" ] ; then
> +		ln -s ld-uClibc.so.1 "${initdir?s}/lib/ld-uClibc.so.0"
> +	fi
> +}
> -- 
> 2.37.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot