Re: [Buildroot] [PATCH v3 2/3] package/dracut: new host package

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

Thierry, All,

On 2021-12-23 12:13 +0100, Thierry Bultel spake thusly:
> dracut is the tool used by desktop distributions to
> build initrds. In the embedded world, it can be
> very usefull, too, for instance when wanting to
> create an initramfs for a system recovery mode.
> Whereas it is definitively possible to achieve
> this with buildroot, the process is to have a
> dedicated buildroot configuration for that,
> and perform a full build. Instead of doing that,
> the idea is to use dracut to pick the needed
> binaries/shared libraries, configuration files,
> or kernel modules from the 'target' directory.
> The advantage is to save build time, and also
> to have a consistency between the packages versions
> taken for the recovery and the production filesystem.

Please, wrap your commit log to ~72 chars.

> Signed-off-by: Thierry Bultel <thierry.bultel@linatsea.fr>
> ---
[--SNIP--]
> diff --git a/package/dracut/05busybox-buildroot/module-setup.sh b/package/dracut/05busybox-buildroot/module-setup.sh
> new file mode 100755
> index 0000000000..b227571db4
> --- /dev/null
> +++ b/package/dracut/05busybox-buildroot/module-setup.sh

You will have to (briefly, maybe) explain how those 'modules' are meant
to be used...

> @@ -0,0 +1,69 @@
> +#!/bin/bash

... here you make it an executable (0755) shell script, but it only
defines functions, it actually calls nothing. Are moduels supposed to be
source-d from dracut itself?

> +check() {
> +	require_binaries \
> +		busybox | return 1

This can't possibly work: it will always fail (there is a single pipe,
so the whole command will fail).

Then what happens depends on whether the shell is set -e or not. If it
is not set -e, the execution continues, and the check() function will
always return 0 (success). If the shell is set -e, then execution of
check() ends right there, and presumably check() is called as an
if-block condition, and thus the condition will never be true.

So, it was not tested? ;-)

(also: commands that fit on a line should be on a single line, like so:
    require_binaries busybox || return 1
)

> +	return 0
> +}
> +
> +depends() {
> +	return 0
> +}
> +
> +
> +install_busybox_links() {
> +	dir=$1
> +	linkname=$2
> +
> +	(cd "$dracutsysrootdir"$dir &&

    $ shellcheck package/dracut/05busybox-buildroot/module-setup.sh
        (cd "$dracutsysrootdir"$dir &&
                               ^--^ SC2086: Double quote to prevent globbing and word splitting.

Also, please do variable expansion with curly braces (everywhere):

    "${dracutsysrootdir}${dir}"

> +	for x in *; do
> +	    if [ "$(readlink "$x")" = $linkname ]; then
> +	    	ln -sf $linkname $initdir/$dir/$x
> +	    fi
> +	done
> +	)
> +
> +}
> +
> +install() {
> +

No initial empty line at begining of function.

[--SNIP--]
> +#	mkdir -p $initdir/etc/init.d
> +#	cp -a $dracutsysrootdir/etc/init.d/* $initdir/etc/init.d

Commented-out debug code or non-function code should just be removed.

> +	

No empty line at the end of function.

> +}
> +
> diff --git a/package/dracut/06uclibc/module-setup.sh b/package/dracut/06uclibc/module-setup.sh
> new file mode 100644
> index 0000000000..77e9bd864e
> --- /dev/null
> +++ b/package/dracut/06uclibc/module-setup.sh
> @@ -0,0 +1,20 @@
> +#!/bin/bash
> +
> +check() {
> +	return 0
> +}
> +
> +depends() {
> +	return 0
> +}
> +
> +
> +
> +install() {
> +
> +	# Despite of the fact that the listed dependency (reported by readelf -d)
> +	# is purely ld-uClibc.so.1, the loader needs the ld-uClibc.so.0, too
> +
> +	ln -s ld-uClibc.so.1 $initdir/lib/ld-uClibc.so.0
> +

Extra empty lines...

But still, a little explanations (in the commit log) on how those
scripts are supposed to be used would help further review.

[--SNIP--]
> diff --git a/package/dracut/dracut.mk b/package/dracut/dracut.mk
> new file mode 100644
> index 0000000000..6c82b4ba75
> --- /dev/null
> +++ b/package/dracut/dracut.mk
> @@ -0,0 +1,45 @@
> +################################################################################
> +#
> +# dracut
> +#
> +################################################################################
> +
> +DRACUT_VERSION = 055
> +DRACUT_SOURCE = dracut-$(DRACUT_VERSION).tar.xz
> +DRACUT_SITE = $(BR2_KERNEL_MIRROR)/linux/utils/boot/dracut
> +DRACUT_LICENSE = GPL-2.0
> +DRACUT_LICENSE_FILES = COPYING
> +
> +HOST_DRACUT_DEPENDENCIES += host-pkgconf host-kmod host-cross-ldd

First assignment should not be +=, but just =.

> +define HOST_DRACUT_POST_INSTALL_ENABLE_FAKEROOT
> +	$(SED) '/unset LD_LIBRARY_PATH/d' $(HOST_DIR)/bin/dracut
> +	$(SED) '/unset LD_PRELOAD/d' $(HOST_DIR)/bin/dracut

As for cross-ldd, we prefer that patching be done with actual patches
rather than sed expressions.

> +endef
> +
> +define HOST_DRACUT_POST_INSTALL_WRAPPER_SCRIPT
> +	$(INSTALL) -D -m 0755 package/dracut/dracut_wrapper.sh \
> +		$(HOST_DIR)/usr/bin/dracut_wrapper.sh

Your commit log does not explain the reason why we need to have a
wrapper, or how it is going to be used...

Usually, when we introduce a wrapper, it replace sthe original
executable in name, and the original is renamed, with the wrapper
eventually calling into the renamed original...

So having a wrapper installed as you do it is slightly off-road...

> +endef
> +
> +ifeq ($(BR2_TOOLCHAIN_USES_UCLIBC),y)
> +define HOST_DRACUT_POST_INSTALL_UCLIBC_MODULE
> +	$(INSTALL) -D -m 0755 package/dracut/06uclibc/module-setup.sh \
> +		$(HOST_DIR)/lib/dracut/modules.d/06uclibc/module-setup.sh
> +endef
> +HOST_DRACUT_POST_INSTALL_HOOKS+=HOST_DRACUT_POST_INSTALL_UCLIBC_MODULE
> +endif

And what about glibc? musl?

Can we unconditionally install the modules and change their check()
functions to decide whther the moduels is usable or not? Or something
else dynamic?

> +ifeq ($(BR2_INIT_BUSYBOX),y)
> +define HOST_DRACUT_POST_INSTALL_BUSYBOX_MODULE
> +	$(INSTALL) -D -m 0755 package/dracut/05busybox-buildroot/module-setup.sh \
> +		$(HOST_DIR)/lib/dracut/modules.d/05busybox-buildroot/module-setup.sh
> +endef
> +HOST_DRACUT_POST_INSTALL_HOOKS+=HOST_DRACUT_POST_INSTALL_BUSYBOX_MODULE

And what about systemd as an init system? openrc? sysv-init? Others?

> +endif
> +
> +HOST_DRACUT_POST_INSTALL_HOOKS+=HOST_DRACUT_POST_INSTALL_ENABLE_FAKEROOT
> +HOST_DRACUT_POST_INSTALL_HOOKS+=HOST_DRACUT_POST_INSTALL_WRAPPER_SCRIPT

Spaces around the operator. Also, since this is the same hook, a single
assignment is better:

    HOST_DRACUT_POST_INSTALL_HOOKS += \
        HOST_DRACUT_POST_INSTALL_ENABLE_FAKEROOT \
        HOST_DRACUT_POST_INSTALL_WRAPPER_SCRIPT

Note: we prefer to have uncondtional asignemnts come before the
conditional ones, and we also prefer to have ythe hook assignment
come right after the hook definitionm like so:


    define HOST_DRACUT_POST_INSTALL_ENABLE_FAKEROOT
        blabla
    endef
    HOST_DRACUT_POST_INSTALL_HOOKS = HOST_DRACUT_POST_INSTALL_ENABLE_FAKEROOT

    define HOST_DRACUT_POST_INSTALL_WRAPPER_SCRIPT
        blabla
    endef
    HOST_DRACUT_POST_INSTALL_HOOKS += HOST_DRACUT_POST_INSTALL_WRAPPER_SCRIPT

    ifeq ($(BR2_INIT_BUSYBOX),y)
    define HOST_DRACUT_POST_INSTALL_BUSYBOX_MODULE
        blabla
    endef
    HOST_DRACUT_POST_INSTALL_HOOKS += HOST_DRACUT_POST_INSTALL_BUSYBOX_MODULE
    endif

and so on...

Regards,
Yann E. MORIN.

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